Re: [PATCH 1/7] Assume O_DIRECTORY for opendir

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 02/03/2018 15:59, Adhemerval Zanella wrote:
> This patch assumes O_DIRECTORY works as defined by POSIX on opendir
> implementation (aligning with other glibc code, for instance pwd).  This
> allows remove both the fallback code to handle system with missing or
> broken O_DIRECTORY along with the Linux specific opendir.c which just
> advertise the working flag.
> 
> Checked on aarch64-linux-gnu, x86_64-linux-gnu, i686-linux-gnu,
> sparcv9-linux-gnu, sparc64-linux-gnu, powerpc-linux-gnu, and
> powerpc64le-linux-gnu.
> 
> 	* sysdeps/posix/opendir.c (o_directory_works, tryopen_o_directory):
> 	Remove definitions.
> 	(opendir_oflags): Use O_DIRECTORY regardless.
> 	(__opendir, __opendirat): Remove need_isdir_precheck usage.
> 	* sysdeps/unix/sysv/linux/opendir.c: Remove file.

If no one opposes it I will commit this shortly.

> ---
>  ChangeLog                         |   8 +++
>  sysdeps/posix/opendir.c           | 108 ++------------------------------------
>  sysdeps/unix/sysv/linux/opendir.c |  20 -------
>  3 files changed, 13 insertions(+), 123 deletions(-)
>  delete mode 100644 sysdeps/unix/sysv/linux/opendir.c
> 
> diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
> index d925804..dea23b2 100644
> --- a/sysdeps/posix/opendir.c
> +++ b/sysdeps/posix/opendir.c
> @@ -15,24 +15,13 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#include <assert.h>
> -#include <errno.h>
> -#include <limits.h>
> -#include <stdbool.h>
> -#include <stddef.h>
> -#include <stdlib.h>
>  #include <dirent.h>
>  #include <fcntl.h>
> -#include <sys/param.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -#include <unistd.h>
> -#include <stdio.h>
> -#include <string.h>
> -
> -#include <dirstream.h>
> +#include <errno.h>
> +#include <stdio.h>	/* For BUFSIZ.  */
> +#include <sys/param.h>	/* For MIN and MAX.  */
> +
>  #include <not-cancel.h>
> -#include <kernel-features.h>
>  
>  /* The st_blksize value of the directory is used as a hint for the
>     size of the buffer which receives struct dirent values from the
> @@ -40,49 +29,8 @@
>     file system provides a bogus value.  */
>  #define MAX_DIR_BUFFER_SIZE 1048576U
>  
> -/* opendir() must not accidentally open something other than a directory.
> -   Some OS's have kernel support for that, some don't.  In the worst
> -   case we have to stat() before the open() AND fstat() after.
> -
> -   We have to test at runtime for kernel support since libc may have
> -   been compiled with different headers to the kernel it's running on.
> -   This test can't be done reliably in the general case.  We'll use
> -   /dev/null, which if it's not a device lots of stuff will break, as
> -   a guinea pig.  It may be missing in chroot environments, so we
> -   make sure to fail safe. */
> -#ifdef O_DIRECTORY
> -# ifdef O_DIRECTORY_WORKS
> -#  define o_directory_works 1
> -#  define tryopen_o_directory() while (1) /* This must not be called.  */
> -# else
> -static int o_directory_works;
> -
> -static void
> -tryopen_o_directory (void)
> -{
> -  int serrno = errno;
> -  int x = __open_nocancel ("/dev/null", O_RDONLY|O_NDELAY|O_DIRECTORY);
> -
> -  if (x >= 0)
> -    {
> -      __close_nocancel_nostatus (x);
> -      o_directory_works = -1;
> -    }
> -  else if (errno != ENOTDIR)
> -    o_directory_works = -1;
> -  else
> -    o_directory_works = 1;
> -
> -  __set_errno (serrno);
> -}
> -# endif
> -# define EXTRA_FLAGS O_DIRECTORY
> -#else
> -# define EXTRA_FLAGS 0
> -#endif
> -
>  enum {
> -  opendir_oflags = O_RDONLY|O_NDELAY|EXTRA_FLAGS|O_LARGEFILE|O_CLOEXEC
> +  opendir_oflags = O_RDONLY|O_NDELAY|O_DIRECTORY|O_LARGEFILE|O_CLOEXEC
>  };
>  
>  static bool
> @@ -98,21 +46,6 @@ invalid_name (const char *name)
>    return false;
>  }
>  
> -
> -static bool
> -need_isdir_precheck (void)
> -{
> -#ifdef O_DIRECTORY
> -  /* Test whether O_DIRECTORY works.  */
> -  if (o_directory_works == 0)
> -    tryopen_o_directory ();
> -
> -  /* We can skip the expensive `stat' call if O_DIRECTORY works.  */
> -  return o_directory_works < 0;
> -#endif
> -  return true;
> -}
> -
>  static DIR *
>  opendir_tail (int fd)
>  {
> @@ -144,22 +77,6 @@ __opendirat (int dfd, const char *name)
>    if (__glibc_unlikely (invalid_name (name)))
>      return NULL;
>  
> -  if (need_isdir_precheck ())
> -    {
> -      /* We first have to check whether the name is for a directory.  We
> -	 cannot do this after the open() call since the open/close operation
> -	 performed on, say, a tape device might have undesirable effects.  */
> -      struct stat64 statbuf;
> -      if (__glibc_unlikely (__fxstatat64 (_STAT_VER, dfd, name,
> -					  &statbuf, 0) < 0))
> -	return NULL;
> -      if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode)))
> -	{
> -	  __set_errno (ENOTDIR);
> -	  return NULL;
> -	}
> -    }
> -
>    return opendir_tail (__openat_nocancel (dfd, name, opendir_oflags));
>  }
>  #endif
> @@ -172,21 +89,6 @@ __opendir (const char *name)
>    if (__glibc_unlikely (invalid_name (name)))
>      return NULL;
>  
> -  if (need_isdir_precheck ())
> -    {
> -      /* We first have to check whether the name is for a directory.  We
> -	 cannot do this after the open() call since the open/close operation
> -	 performed on, say, a tape device might have undesirable effects.  */
> -      struct stat64 statbuf;
> -      if (__glibc_unlikely (__xstat64 (_STAT_VER, name, &statbuf) < 0))
> -	return NULL;
> -      if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode)))
> -	{
> -	  __set_errno (ENOTDIR);
> -	  return NULL;
> -	}
> -    }
> -
>    return opendir_tail (__open_nocancel (name, opendir_oflags));
>  }
>  weak_alias (__opendir, opendir)
> diff --git a/sysdeps/unix/sysv/linux/opendir.c b/sysdeps/unix/sysv/linux/opendir.c
> deleted file mode 100644
> index 2677253..0000000
> --- a/sysdeps/unix/sysv/linux/opendir.c
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -/* Copyright (C) 2000-2018 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library; if not, see
> -   <http://www.gnu.org/licenses/>.  */
> -
> -#define O_DIRECTORY_WORKS	1
> -
> -#include <sysdeps/posix/opendir.c>
>