[PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

Introducing a new struct makes it unnecessary to copy the name out of
the dirent structure.   The name is subsequently allocated on the heap
anyway.

I extended the existing test with a long name.  The test now crashes
before the fix.

We should split sysdeps/unix/sysv/linux/i386/glob64.c into multiple
files, rather than including posix/glob.c multiple times.

DIRENT_MUST_BE can be removed, but that's an unrelated cleanup (it was
dead before).

Florian

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-glob-Avoid-copying-the-d_name-field-of-struct-dirent.patch --]
[-- Type: text/x-patch; name="0001-glob-Avoid-copying-the-d_name-field-of-struct-dirent.patch", Size: 11172 bytes --]

2016-03-11  Florian Weimer  <fweimer@redhat.com>

	[BZ #19779]
	Avoid copying names of directory entries.
	* posix/glob.c (NAMELEN, CONVERT_D_NAMLEN, CONVERT_D_INO)
	(CONVERT_D_TYPE CONVERT_DIRENT_DIRENT64, REAL_DIR_ENTRY)
	(NAME_MAX): Remove macros.
	(struct abstract_dirent): New type.
	(convert_dirent): New function.
	(glob_in_dir): Use struct abstract_dirent.  Call convert_dirent.
	Adjust references to the dirent object.  Use strdup instead of
	NAMELEN and mempcpy.
	* sysdeps/gnu/glob64.c (COMPILE_GLOB64): Remove macro.
	* sysdeps/unix/sysv/linux/i386/glob64.c (COMPILE_GLOB64): Likewise.
	(convert_dirent): Redefine for second file inclusion.
	* posix/bug-glob2.c (LONG_NAME): Define.
	(filesystem): Add LONG_NAME.
	(my_DIR): Increase the size of room_for_dirent.

diff --git a/posix/bug-glob2.c b/posix/bug-glob2.c
index ddf5ec9..581f28e 100644
--- a/posix/bug-glob2.c
+++ b/posix/bug-glob2.c
@@ -40,6 +40,17 @@
 # define PRINTF(fmt, args...)
 #endif
 
+#define LONG_NAME \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
 
 static struct
 {
@@ -58,6 +69,7 @@ static struct
       { ".", 3, DT_DIR, 0755 },
       { "..", 3, DT_DIR, 0755 },
       { "a", 3, DT_REG, 0644 },
+      { LONG_NAME, 3, DT_REG, 0644 },
     { "unreadable", 2, DT_DIR, 0111 },
       { ".", 3, DT_DIR, 0111 },
       { "..", 3, DT_DIR, 0755 },
@@ -75,7 +87,7 @@ typedef struct
   int level;
   int idx;
   struct dirent d;
-  char room_for_dirent[NAME_MAX];
+  char room_for_dirent[1000];
 } my_DIR;
 
 
diff --git a/posix/glob.c b/posix/glob.c
index 0c04c3c..aea1b71 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -24,6 +24,7 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <stdbool.h>
 #include <stddef.h>
 
 /* Outcomment the following line for production quality code.  */
@@ -57,10 +58,8 @@
 
 #if defined HAVE_DIRENT_H || defined __GNU_LIBRARY__
 # include <dirent.h>
-# define NAMLEN(dirent) strlen((dirent)->d_name)
 #else
 # define dirent direct
-# define NAMLEN(dirent) (dirent)->d_namlen
 # ifdef HAVE_SYS_NDIR_H
 #  include <sys/ndir.h>
 # endif
@@ -75,13 +74,6 @@
 # endif /* HAVE_VMSDIR_H */
 #endif
 
-
-/* In GNU systems, <dirent.h> defines this macro for us.  */
-#ifdef _D_NAMLEN
-# undef NAMLEN
-# define NAMLEN(d) _D_NAMLEN(d)
-#endif
-
 /* When used in the GNU libc the symbol _DIRENT_HAVE_D_TYPE is available
    if the `d_type' member for `struct dirent' is available.
    HAVE_STRUCT_DIRENT_D_TYPE plays the same role in GNULIB.  */
@@ -103,54 +95,8 @@
 # define DIRENT_MIGHT_BE_DIR(d)		true
 #endif /* HAVE_D_TYPE */
 
-/* If the system has the `struct dirent64' type we use it internally.  */
-#if defined _LIBC && !defined COMPILE_GLOB64
-# if defined HAVE_DIRENT_H || defined __GNU_LIBRARY__
-#  define CONVERT_D_NAMLEN(d64, d32)
-# else
-#  define CONVERT_D_NAMLEN(d64, d32) \
-  (d64)->d_namlen = (d32)->d_namlen;
-# endif
-
-# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
-#  define CONVERT_D_INO(d64, d32)
-# else
-#  define CONVERT_D_INO(d64, d32) \
-  (d64)->d_ino = (d32)->d_ino;
-# endif
-
-# ifdef _DIRENT_HAVE_D_TYPE
-#  define CONVERT_D_TYPE(d64, d32) \
-  (d64)->d_type = (d32)->d_type;
-# else
-#  define CONVERT_D_TYPE(d64, d32)
-# endif
-
-# define CONVERT_DIRENT_DIRENT64(d64, d32) \
-  memcpy ((d64)->d_name, (d32)->d_name, NAMLEN (d32) + 1);		      \
-  CONVERT_D_NAMLEN (d64, d32)						      \
-  CONVERT_D_INO (d64, d32)						      \
-  CONVERT_D_TYPE (d64, d32)
-#endif
-
-
-#if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
-/* Posix does not require that the d_ino field be present, and some
-   systems do not provide it. */
-# define REAL_DIR_ENTRY(dp) 1
-#else
-# define REAL_DIR_ENTRY(dp) (dp->d_ino != 0)
-#endif /* POSIX */
-
 #include <stdlib.h>
 #include <string.h>
-
-/* NAME_MAX is usually defined in <dirent.h> or <limits.h>.  */
-#include <limits.h>
-#ifndef NAME_MAX
-# define NAME_MAX (sizeof (((struct dirent *) 0)->d_name))
-#endif
-
 #include <alloca.h>
 
 #ifdef _LIBC
@@ -195,8 +141,37 @@
 \f
 static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
 
+/* A representation of a directory entry which does not depend on the
+   layout of struct dirent, or the size of ino_t.  */
+struct abstract_dirent
+{
+  const char *d_name;
+  int d_type;
+  bool skip_entry;
+};
+
 #endif /* !defined _LIBC || !defined GLOB_ONLY_P */
 
+/* Extract name and type from directory entry.  No copy of the name is
+   made.  */
+static void
+convert_dirent (const struct dirent *source, struct abstract_dirent *target)
+{
+  target->d_name = source->d_name;
+#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
+  target->d_type = source->d_type;
+#else
+  target->d_type = 0;
+#endif
+#if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
+  /* Posix does not require that the d_ino field be present, and some
+     systems do not provide it. */
+  target->skip_entry = false;
+#else
+  target->skip_entry = source->d_ino == 0;
+#endif
+}
+
 #ifndef attribute_hidden
 # define attribute_hidden
 #endif
@@ -1553,56 +1528,31 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 
 	  while (1)
 	    {
-	      const char *name;
-	      size_t len;
-#if defined _LIBC && !defined COMPILE_GLOB64
-	      struct dirent64 *d;
-	      union
-		{
-		  struct dirent64 d64;
-		  char room [offsetof (struct dirent64, d_name[0])
-			     + NAME_MAX + 1];
-		}
-	      d64buf;
-
-	      if (__glibc_unlikely (flags & GLOB_ALTDIRFUNC))
-		{
-		  struct dirent *d32 = (*pglob->gl_readdir) (stream);
-		  if (d32 != NULL)
-		    {
-		      CONVERT_DIRENT_DIRENT64 (&d64buf.d64, d32);
-		      d = &d64buf.d64;
-		    }
-		  else
-		    d = NULL;
-		}
-	      else
-		d = __readdir64 (stream);
-#else
-	      struct dirent *d = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
-				  ? ((struct dirent *)
-				     (*pglob->gl_readdir) (stream))
-				  : __readdir (stream));
-#endif
-	      if (d == NULL)
-		break;
-	      if (! REAL_DIR_ENTRY (d))
+	      struct abstract_dirent e;
+	      {
+		struct dirent *d = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
+				    ? ((struct dirent *)
+				       (*pglob->gl_readdir) (stream))
+				    : __readdir (stream));
+		if (d == NULL)
+		  break;
+		convert_dirent (d, &e);
+	      }
+	      if (e.skip_entry)
 		continue;
 
 	      /* If we shall match only directories use the information
 		 provided by the dirent call if possible.  */
-	      if ((flags & GLOB_ONLYDIR) && !DIRENT_MIGHT_BE_DIR (d))
+	      if ((flags & GLOB_ONLYDIR) && !DIRENT_MIGHT_BE_DIR (&e))
 		continue;
 
-	      name = d->d_name;
-
-	      if (fnmatch (pattern, name, fnm_flags) == 0)
+	      if (fnmatch (pattern, e.d_name, fnm_flags) == 0)
 		{
 		  /* If the file we found is a symlink we have to
 		     make sure the target file exists.  */
-		  if (!DIRENT_MIGHT_BE_SYMLINK (d)
-		      || link_exists_p (dfd, directory, dirlen, name, pglob,
-					flags))
+		  if (!DIRENT_MIGHT_BE_SYMLINK (&e)
+		      || link_exists_p (dfd, directory, dirlen, e.d_name,
+					pglob, flags))
 		    {
 		      if (cur == names->count)
 			{
@@ -1622,12 +1572,10 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 			  names = newnames;
 			  cur = 0;
 			}
-		      len = NAMLEN (d);
-		      names->name[cur] = (char *) malloc (len + 1);
+		      names->name[cur] = strdup (e.d_name);
 		      if (names->name[cur] == NULL)
 			goto memory_error;
-		      *((char *) mempcpy (names->name[cur++], name, len))
-			= '\0';
+		      ++cur;
 		      ++nfound;
 		    }
 		}
diff --git a/sysdeps/gnu/glob64.c b/sysdeps/gnu/glob64.c
index d1e4e6f..d53d16d 100644
--- a/sysdeps/gnu/glob64.c
+++ b/sysdeps/gnu/glob64.c
@@ -1,3 +1,20 @@
+/* Copyright (C) 1998-2016 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/>.  */
+
 #include <dirent.h>
 #include <glob.h>
 #include <sys/stat.h>
@@ -17,8 +34,6 @@
 
 #define NO_GLOB_PATTERN_P 1
 
-#define COMPILE_GLOB64	1
-
 #include <posix/glob.c>
 
 libc_hidden_def (glob64)
diff --git a/sysdeps/unix/sysv/linux/i386/glob64.c b/sysdeps/unix/sysv/linux/i386/glob64.c
index b4fcd1a..727a468 100644
--- a/sysdeps/unix/sysv/linux/i386/glob64.c
+++ b/sysdeps/unix/sysv/linux/i386/glob64.c
@@ -1,3 +1,20 @@
+/* Copyright (C) 1998-2016 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/>.  */
+
 #include <dirent.h>
 #include <glob.h>
 #include <sys/stat.h>
@@ -17,8 +34,6 @@
 
 #define NO_GLOB_PATTERN_P 1
 
-#define COMPILE_GLOB64	1
-
 #include <posix/glob.c>
 
 #include "shlib-compat.h"
@@ -43,6 +58,7 @@ int __old_glob64 (const char *__pattern, int __flags,
 #undef glob
 #define glob(pattern, flags, errfunc, pglob) \
   __old_glob64 (pattern, flags, errfunc, pglob)
+#define convert_dirent __old_convert_dirent
 #define glob_in_dir __old_glob_in_dir
 #define GLOB_ATTRIBUTE attribute_compat_text_section
 

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Paul Eggert]

I merely read the patch. Comments:

 > +static void
 > +convert_dirent (const struct dirent *source, struct abstract_dirent *target)

Since this is all inlined, how about if making it a pure function, with a 
signature like this instead?

static struct abstract_dirent
convert_dirent (struct dirent const *source)

That should be a bit cleaner.

Otherwise, it looks good.

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

On 03/11/2016 11:00 PM, Paul Eggert wrote:
> I merely read the patch. Comments:
> 
>> +static void
>> +convert_dirent (const struct dirent *source, struct abstract_dirent
> *target)
> 
> Since this is all inlined, how about if making it a pure function, with
> a signature like this instead?
> 
> static struct abstract_dirent
> convert_dirent (struct dirent const *source)
> 
> That should be a bit cleaner.

The file is in gnulib, I feared that gnulib has to support compilers
which cannot return structs.

> Otherwise, it looks good.

Thanks.

Florian

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Roland McGrath]

> --- a/posix/bug-glob2.c
> +++ b/posix/bug-glob2.c
[...]
> @@ -75,7 +87,7 @@ typedef struct
>    int level;
>    int idx;
>    struct dirent d;
> -  char room_for_dirent[NAME_MAX];
> +  char room_for_dirent[1000];
>  } my_DIR;

There should be some comment about where this arbitrary size comes from.

> +/* A representation of a directory entry which does not depend on the
> +   layout of struct dirent, or the size of ino_t.  */
> +struct abstract_dirent
> +{
> +  const char *d_name;
> +  int d_type;
> +  bool skip_entry;
> +};

I'm not convinced that this is a good name for the struct.  It's not an
abstract form of 'struct dirent', because the name member points into some
other buffer whose lifetime is not intrinsically related to this struct.

I would tend away from using d_ prefixes on any member names here.  For
d_name it specifically gives the wrong impression that it is the same as
dirent.d_name, which is an array rather than a pointer.  The DIRENT_MIGHT_*
macros are now used only with the internal struct, so they can just use the
unprefixed member name 'type', and their names can lose the DIRENT_ prefix.

uint8_t is big enough for d_type, and will make the struct smaller on
ILP32.  All else being equal, I would use __typeof (((struct dirent)
{}).d_type) but just using uint8_t is adequately simpler for the gnulib
case concerned with sticking to standard C (and doesn't need any
#ifdef _DIRENT_HAVE_D_TYPE check).

> +/* Extract name and type from directory entry.  No copy of the name is
> +   made.  */
> +static void
> +convert_dirent (const struct dirent *source, struct abstract_dirent *target)

I like Paul's suggestion of using a struct-returning function here.  The
comment should be explicit that the result will point to SOURCE->d_name and
so SOURCE must be kept live.

> +#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
> +  target->d_type = source->d_type;
> +#else
> +  target->d_type = 0;
> +#endif
> +#if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
> +  /* Posix does not require that the d_ino field be present, and some
> +     systems do not provide it. */
> +  target->skip_entry = false;
> +#else
> +  target->skip_entry = source->d_ino == 0;
> +#endif

For both of these I'd prefer to see an inline or macro that encapsulates
the #if stuff without other repetition, e.g.:

	target->type = D_TYPE (source);
	target->skip_entry = D_SKIP_ME (source);

> +	      struct abstract_dirent e;
> +	      {
> +		struct dirent *d = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
> +				    ? ((struct dirent *)
> +				       (*pglob->gl_readdir) (stream))
> +				    : __readdir (stream));

Convert to __glibc_unlikely while you're here (unless the gnulib sharing
rules say not to, not clear on that off hand).

> +		if (d == NULL)
> +		  break;
> +		convert_dirent (d, &e);
> +	      }

This could all be nicely broken out into a function that takes STREAM,
FLAGS, PGLOB, and returns E.

> -		      len = NAMLEN (d);
> -		      names->name[cur] = (char *) malloc (len + 1);
> +		      names->name[cur] = strdup (e.d_name);
>  		      if (names->name[cur] == NULL)
>  			goto memory_error;
> -		      *((char *) mempcpy (names->name[cur++], name, len))
> -			= '\0';
> +		      ++cur;

In the _DIRENT_HAVE_D_NAMLEN case, this assumes that strdup is as efficient
as malloc+mempcpy (with no strlen required).  Perhaps it's close enough,
but that is a subtle change you didn't mention as intended.

> --- a/sysdeps/gnu/glob64.c
> +++ b/sysdeps/gnu/glob64.c
> @@ -1,3 +1,20 @@
> +/* Copyright (C) 1998-2016 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.

The top line must be a descriptive comment.  
Copyright goes on the second line.

> --- a/sysdeps/unix/sysv/linux/i386/glob64.c
> +++ b/sysdeps/unix/sysv/linux/i386/glob64.c
> @@ -1,3 +1,20 @@
> +/* Copyright (C) 1998-2016 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.

Ditto.


Thanks,
Roland

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

This patch is incorrect.  We need to call readdir64 even for the 32-bit
glob.  I removed too much code.

Florian

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Paul Eggert]

Florian Weimer wrote:
> I feared that gnulib has to support compilers
> which cannot return structs.

No, gnulib assumes C89 or better so it's OK to return a struct.

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 3694 bytes --]

On 03/11/2016 11:27 PM, Roland McGrath wrote:
>> --- a/posix/bug-glob2.c
>> +++ b/posix/bug-glob2.c
> [...]
>> @@ -75,7 +87,7 @@ typedef struct
>>    int level;
>>    int idx;
>>    struct dirent d;
>> -  char room_for_dirent[NAME_MAX];
>> +  char room_for_dirent[1000];
>>  } my_DIR;
> 
> There should be some comment about where this arbitrary size comes from.

I changed it to a sizeof.

>> +/* A representation of a directory entry which does not depend on the
>> +   layout of struct dirent, or the size of ino_t.  */
>> +struct abstract_dirent
>> +{
>> +  const char *d_name;
>> +  int d_type;
>> +  bool skip_entry;
>> +};
> 
> I'm not convinced that this is a good name for the struct.  It's not an
> abstract form of 'struct dirent', because the name member points into some
> other buffer whose lifetime is not intrinsically related to this struct.

It's now struct readdir_result.

> uint8_t is big enough for d_type, and will make the struct smaller on
> ILP32.

I assume the struct is turned into scalars anyway, but I added the
__typeof__.

>> +/* Extract name and type from directory entry.  No copy of the name is
>> +   made.  */
>> +static void
>> +convert_dirent (const struct dirent *source, struct abstract_dirent *target)
> 
> I like Paul's suggestion of using a struct-returning function here.  The
> comment should be explicit that the result will point to SOURCE->d_name and
> so SOURCE must be kept live.

I made these changes.

>> +#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
>> +  target->d_type = source->d_type;
>> +#else
>> +  target->d_type = 0;
>> +#endif
>> +#if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
>> +  /* Posix does not require that the d_ino field be present, and some
>> +     systems do not provide it. */
>> +  target->skip_entry = false;
>> +#else
>> +  target->skip_entry = source->d_ino == 0;
>> +#endif
> 
> For both of these I'd prefer to see an inline or macro that encapsulates
> the #if stuff without other repetition, e.g.:

There are now separate macros for that.

> 	target->type = D_TYPE (source);
> 	target->skip_entry = D_SKIP_ME (source);
> 
>> +	      struct abstract_dirent e;
>> +	      {
>> +		struct dirent *d = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
>> +				    ? ((struct dirent *)
>> +				       (*pglob->gl_readdir) (stream))
>> +				    : __readdir (stream));
> 
> Convert to __glibc_unlikely while you're here (unless the gnulib sharing
> rules say not to, not clear on that off hand).

>> +		if (d == NULL)
>> +		  break;
>> +		convert_dirent (d, &e);
>> +	      }
> 
> This could all be nicely broken out into a function that takes STREAM,
> FLAGS, PGLOB, and returns E.

It's slightly more complicated than that because I accidentally removed
the interesting parts.

Further cleanups should wait until we have a decision whether we will
ever try to synchronize with gnulib again.

>> -		      len = NAMLEN (d);
>> -		      names->name[cur] = (char *) malloc (len + 1);
>> +		      names->name[cur] = strdup (e.d_name);
>>  		      if (names->name[cur] == NULL)
>>  			goto memory_error;
>> -		      *((char *) mempcpy (names->name[cur++], name, len))
>> -			= '\0';
>> +		      ++cur;
> 
> In the _DIRENT_HAVE_D_NAMLEN case, this assumes that strdup is as efficient
> as malloc+mempcpy (with no strlen required).  Perhaps it's close enough,
> but that is a subtle change you didn't mention as intended.

I want to avoid conditionalized code because these things tend to break
after a while (or never work in the first place).

I have tested the attached patch with fake-disabled d_type support, and
on i386-redhat-linux-gnu and x86_64-redhat-linux-gnu.

Florian

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-glob-Avoid-copying-the-d_name-field-of-struct-dirent.patch --]
[-- Type: text/x-patch; name="0001-glob-Avoid-copying-the-d_name-field-of-struct-dirent.patch", Size: 12975 bytes --]

2016-03-30  Florian Weimer  <fweimer@redhat.com>

	[BZ #19779]
	CVE-2016-1234
	Avoid copying names of directory entries.
	* posix/glob.c (NAMELEN, DIRENT_MUST_BE, DIRENT_MIGHT_BE_SYMLINK)
	(DIRENT_MIGHT_BE_DIR, CONVERT_D_NAMLEN, CONVERT_D_INO)
	(CONVERT_D_TYPE, CONVERT_DIRENT_DIRENT64, REAL_DIR_ENTRY)
	(NAME_MAX): Remove macros.
	(struct readdir_result): New type.
	(D_TYPE_TO_RESULT, D_TYPE_TO_RESULT, GL_READDIR): New macros.
	(readdir_result_might_be_symlink, readdir_result_might_be_dir):
	New functions.
	(convert_dirent, convert_dirent64): New function.
	(glob_in_dir): Use struct dirent_result.  Call convert_dirent or
	convert_dirent64.  Adjust references to the readdir result.  Use
	strdup instead of NAMELEN and mempcpy.
	* sysdeps/unix/sysv/linux/i386/glob64.c:
	(convert_dirent, GL_READDIR): Redefine for second file inclusion.
	* posix/bug-glob2.c (LONG_NAME): Define.
	(filesystem): Add LONG_NAME.
	(my_DIR): Increase the size of room_for_dirent.

diff --git a/posix/bug-glob2.c b/posix/bug-glob2.c
index ddf5ec9..fca048b 100644
--- a/posix/bug-glob2.c
+++ b/posix/bug-glob2.c
@@ -40,6 +40,17 @@
 # define PRINTF(fmt, args...)
 #endif
 
+#define LONG_NAME \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
 
 static struct
 {
@@ -58,6 +69,7 @@ static struct
       { ".", 3, DT_DIR, 0755 },
       { "..", 3, DT_DIR, 0755 },
       { "a", 3, DT_REG, 0644 },
+      { LONG_NAME, 3, DT_REG, 0644 },
     { "unreadable", 2, DT_DIR, 0111 },
       { ".", 3, DT_DIR, 0111 },
       { "..", 3, DT_DIR, 0755 },
@@ -75,7 +87,7 @@ typedef struct
   int level;
   int idx;
   struct dirent d;
-  char room_for_dirent[NAME_MAX];
+  char room_for_dirent[sizeof (LONG_NAME)];
 } my_DIR;
 
 
diff --git a/posix/glob.c b/posix/glob.c
index 0c04c3c..e043edc 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -24,6 +24,7 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <stdbool.h>
 #include <stddef.h>
 
 /* Outcomment the following line for production quality code.  */
@@ -57,10 +58,8 @@
 
 #if defined HAVE_DIRENT_H || defined __GNU_LIBRARY__
 # include <dirent.h>
-# define NAMLEN(dirent) strlen((dirent)->d_name)
 #else
 # define dirent direct
-# define NAMLEN(dirent) (dirent)->d_namlen
 # ifdef HAVE_SYS_NDIR_H
 #  include <sys/ndir.h>
 # endif
@@ -75,82 +74,8 @@
 # endif /* HAVE_VMSDIR_H */
 #endif
 
-
-/* In GNU systems, <dirent.h> defines this macro for us.  */
-#ifdef _D_NAMLEN
-# undef NAMLEN
-# define NAMLEN(d) _D_NAMLEN(d)
-#endif
-
-/* When used in the GNU libc the symbol _DIRENT_HAVE_D_TYPE is available
-   if the `d_type' member for `struct dirent' is available.
-   HAVE_STRUCT_DIRENT_D_TYPE plays the same role in GNULIB.  */
-#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
-/* True if the directory entry D must be of type T.  */
-# define DIRENT_MUST_BE(d, t)	((d)->d_type == (t))
-
-/* True if the directory entry D might be a symbolic link.  */
-# define DIRENT_MIGHT_BE_SYMLINK(d) \
-    ((d)->d_type == DT_UNKNOWN || (d)->d_type == DT_LNK)
-
-/* True if the directory entry D might be a directory.  */
-# define DIRENT_MIGHT_BE_DIR(d)	 \
-    ((d)->d_type == DT_DIR || DIRENT_MIGHT_BE_SYMLINK (d))
-
-#else /* !HAVE_D_TYPE */
-# define DIRENT_MUST_BE(d, t)		false
-# define DIRENT_MIGHT_BE_SYMLINK(d)	true
-# define DIRENT_MIGHT_BE_DIR(d)		true
-#endif /* HAVE_D_TYPE */
-
-/* If the system has the `struct dirent64' type we use it internally.  */
-#if defined _LIBC && !defined COMPILE_GLOB64
-# if defined HAVE_DIRENT_H || defined __GNU_LIBRARY__
-#  define CONVERT_D_NAMLEN(d64, d32)
-# else
-#  define CONVERT_D_NAMLEN(d64, d32) \
-  (d64)->d_namlen = (d32)->d_namlen;
-# endif
-
-# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
-#  define CONVERT_D_INO(d64, d32)
-# else
-#  define CONVERT_D_INO(d64, d32) \
-  (d64)->d_ino = (d32)->d_ino;
-# endif
-
-# ifdef _DIRENT_HAVE_D_TYPE
-#  define CONVERT_D_TYPE(d64, d32) \
-  (d64)->d_type = (d32)->d_type;
-# else
-#  define CONVERT_D_TYPE(d64, d32)
-# endif
-
-# define CONVERT_DIRENT_DIRENT64(d64, d32) \
-  memcpy ((d64)->d_name, (d32)->d_name, NAMLEN (d32) + 1);		      \
-  CONVERT_D_NAMLEN (d64, d32)						      \
-  CONVERT_D_INO (d64, d32)						      \
-  CONVERT_D_TYPE (d64, d32)
-#endif
-
-
-#if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
-/* Posix does not require that the d_ino field be present, and some
-   systems do not provide it. */
-# define REAL_DIR_ENTRY(dp) 1
-#else
-# define REAL_DIR_ENTRY(dp) (dp->d_ino != 0)
-#endif /* POSIX */
-
 #include <stdlib.h>
 #include <string.h>
-
-/* NAME_MAX is usually defined in <dirent.h> or <limits.h>.  */
-#include <limits.h>
-#ifndef NAME_MAX
-# define NAME_MAX (sizeof (((struct dirent *) 0)->d_name))
-#endif
-
 #include <alloca.h>
 
 #ifdef _LIBC
@@ -195,8 +120,104 @@
 \f
 static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
 
+/* A representation of a directory entry which does not depend on the
+   layout of struct dirent, or the size of ino_t.  */
+struct readdir_result
+{
+  const char *name;
+  bool skip_entry;
+# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
+  __typeof__ (((struct dirent) {}).d_type) type;
+# endif
+};
+
+# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
+/* Designated initializer based on the d_type member of struct
+   dirent.  */
+#  define D_TYPE_TO_RESULT(source) .type = (source)->d_type
+
+/* True if the directory entry D might be a symbolic link.  */
+static inline bool
+readdir_result_might_be_symlink (struct readdir_result d)
+{
+  return d.type == DT_UNKNOWN || d.type == DT_LNK;
+}
+
+/* True if the directory entry D might be a directory.  */
+static inline bool
+readdir_result_might_be_dir (struct readdir_result d)
+{
+  return d.type == DT_DIR || readdir_result_might_be_symlink (d);
+}
+# else
+#  define D_TYPE_TO_RESULT(source)
+
+/* If we do not have type information, symbolic links and directories
+   are always a possibility.  */
+
+static inline bool
+readdir_result_might_be_symlink (struct readdir_result d)
+{
+  return true;
+}
+
+static inline bool
+readdir_result_might_be_dir (struct readdir_result d)
+{
+  return true;
+}
+
+# endif
+
+# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
+/* Designated initializer for skip_entry.  POSIX does not require that
+   the d_ino field be present, and some systems do not provide it. */
+#  define D_INO_TO_RESULT(source) .skip_entry = false
+# else
+#  define D_INO_TO_RESULT(source) .skip_entry = (source)->d_ino == 0
+# endif
+
 #endif /* !defined _LIBC || !defined GLOB_ONLY_P */
 
+/* Call gl_readdir on STREAM.  This macro can be overridden to reduce
+   type safety if an old interface version needs to be supported.  */
+#ifndef GL_READDIR
+# define GL_READDIR(pglob, stream) (pglob)->gl_readdir ((stream))
+#endif
+
+/* Extract name and type from directory entry.  No copy of the name is
+   made.  If SOURCE is NULL, result name is NULL.  */
+static struct readdir_result
+convert_dirent (const struct dirent *source)
+{
+  if (source == NULL)
+    return (struct readdir_result) {};
+  else
+    return (struct readdir_result)
+      {
+	.name = source->d_name,
+	D_INO_TO_RESULT (source),
+	D_TYPE_TO_RESULT (source)
+      };
+}
+
+#ifndef COMPILE_GLOB64
+/* Like convert_dirent, but works on struct dirent64 instead.  */
+static struct readdir_result
+convert_dirent64 (const struct dirent64 *source)
+{
+  if (source == NULL)
+    return (struct readdir_result) {};
+  else
+    return (struct readdir_result)
+      {
+	.name = source->d_name,
+	D_INO_TO_RESULT (source),
+	D_TYPE_TO_RESULT (source)
+      };
+}
+#endif
+
 #ifndef attribute_hidden
 # define attribute_hidden
 #endif
@@ -1553,56 +1574,36 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 
 	  while (1)
 	    {
-	      const char *name;
-	      size_t len;
-#if defined _LIBC && !defined COMPILE_GLOB64
-	      struct dirent64 *d;
-	      union
-		{
-		  struct dirent64 d64;
-		  char room [offsetof (struct dirent64, d_name[0])
-			     + NAME_MAX + 1];
-		}
-	      d64buf;
-
-	      if (__glibc_unlikely (flags & GLOB_ALTDIRFUNC))
-		{
-		  struct dirent *d32 = (*pglob->gl_readdir) (stream);
-		  if (d32 != NULL)
-		    {
-		      CONVERT_DIRENT_DIRENT64 (&d64buf.d64, d32);
-		      d = &d64buf.d64;
-		    }
-		  else
-		    d = NULL;
-		}
-	      else
-		d = __readdir64 (stream);
+	      struct readdir_result d;
+	      {
+		if (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0))
+		  d = convert_dirent (GL_READDIR (pglob, stream));
+		else
+		  {
+#ifdef COMPILE_GLOB64
+		    d = convert_dirent (__readdir (stream));
 #else
-	      struct dirent *d = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
-				  ? ((struct dirent *)
-				     (*pglob->gl_readdir) (stream))
-				  : __readdir (stream));
+		    d = convert_dirent64 (__readdir64 (stream));
 #endif
-	      if (d == NULL)
+		  }
+	      }
+	      if (d.name == NULL)
 		break;
-	      if (! REAL_DIR_ENTRY (d))
+	      if (d.skip_entry)
 		continue;
 
 	      /* If we shall match only directories use the information
 		 provided by the dirent call if possible.  */
-	      if ((flags & GLOB_ONLYDIR) && !DIRENT_MIGHT_BE_DIR (d))
+	      if ((flags & GLOB_ONLYDIR) && !readdir_result_might_be_dir (d))
 		continue;
 
-	      name = d->d_name;
-
-	      if (fnmatch (pattern, name, fnm_flags) == 0)
+	      if (fnmatch (pattern, d.name, fnm_flags) == 0)
 		{
 		  /* If the file we found is a symlink we have to
 		     make sure the target file exists.  */
-		  if (!DIRENT_MIGHT_BE_SYMLINK (d)
-		      || link_exists_p (dfd, directory, dirlen, name, pglob,
-					flags))
+		  if (!readdir_result_might_be_symlink (d)
+		      || link_exists_p (dfd, directory, dirlen, d.name,
+					pglob, flags))
 		    {
 		      if (cur == names->count)
 			{
@@ -1622,12 +1623,10 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 			  names = newnames;
 			  cur = 0;
 			}
-		      len = NAMLEN (d);
-		      names->name[cur] = (char *) malloc (len + 1);
+		      names->name[cur] = strdup (d.name);
 		      if (names->name[cur] == NULL)
 			goto memory_error;
-		      *((char *) mempcpy (names->name[cur++], name, len))
-			= '\0';
+		      ++cur;
 		      ++nfound;
 		    }
 		}
diff --git a/sysdeps/unix/sysv/linux/i386/glob64.c b/sysdeps/unix/sysv/linux/i386/glob64.c
index b4fcd1a..bc0fabc 100644
--- a/sysdeps/unix/sysv/linux/i386/glob64.c
+++ b/sysdeps/unix/sysv/linux/i386/glob64.c
@@ -1,3 +1,21 @@
+/* Two glob variants with 64-bit support, for dirent64 and __olddirent64.
+   Copyright (C) 1998-2016 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/>.  */
+
 #include <dirent.h>
 #include <glob.h>
 #include <sys/stat.h>
@@ -38,11 +56,15 @@ int __old_glob64 (const char *__pattern, int __flags,
 
 #undef dirent
 #define dirent __old_dirent64
+#undef GL_READDIR
+# define GL_READDIR(pglob, stream) \
+  ((struct __old_dirent64 *) (pglob)->gl_readdir ((stream)))
 #undef __readdir
 #define __readdir(dirp) __old_readdir64 (dirp)
 #undef glob
 #define glob(pattern, flags, errfunc, pglob) \
   __old_glob64 (pattern, flags, errfunc, pglob)
+#define convert_dirent __old_convert_dirent
 #define glob_in_dir __old_glob_in_dir
 #define GLOB_ATTRIBUTE attribute_compat_text_section
 

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Roland McGrath]

> > uint8_t is big enough for d_type, and will make the struct smaller on
> > ILP32.
> 
> I assume the struct is turned into scalars anyway, but I added the
> __typeof__.

I like that fine, but we need to check if it's OK in gnulib.

> Further cleanups should wait until we have a decision whether we will
> ever try to synchronize with gnulib again.

The starting place should always be to expect that we want to share
everything we can with gnulib.  If there is something we in the abstract
can share with gnulib but you want to duplicate and maintain separately
rather than share, you need to make a clear case justifying the ways in
which it's better not to share.  Just that we have an annoying amount of
divergence already is not such a justification.  Rather, we should always
start by seeing if we can reharmonize before doing any other changes.  If
some cleanups on one side or the other make it easier to reharmonize, then
that's a reason to do them first.  But otherwise, we should be achieving
harmony first and then fixing things afterwards so the fixes are uniform
in both places.

> >> -		      len = NAMLEN (d);
> >> -		      names->name[cur] = (char *) malloc (len + 1);
> >> +		      names->name[cur] = strdup (e.d_name);
> >>  		      if (names->name[cur] == NULL)
> >>  			goto memory_error;
> >> -		      *((char *) mempcpy (names->name[cur++], name, len))
> >> -			= '\0';
> >> +		      ++cur;
> > 
> > In the _DIRENT_HAVE_D_NAMLEN case, this assumes that strdup is as efficient
> > as malloc+mempcpy (with no strlen required).  Perhaps it's close enough,
> > but that is a subtle change you didn't mention as intended.
> 
> I want to avoid conditionalized code because these things tend to break
> after a while (or never work in the first place).

That might be a sufficient reason to avoid writing such code in the first
place.  But when you are changing something from how it already is, you
need to be clear about what you are changing and why.  My point was not
that I had no idea what your motivation for the specific change might have
been.  My point was that you billed this whole larger change as doing one
thing, and then slipped this in without comment.

In this case, we already have supported configurations of both cases.
Hurd is _DIRENT_HAVE_D_NAMLEN (it uses the generic bits/dirent.h), while
others are not.  So just the configurations that work today not being
broken in the future (and being adequately tested) prevents "tending to
break after a while".

I am making a big deal out of this little case, and it's not because I
think this particular little performance-relevant change is so likely to
be important or even that I personally necessarily object to it.  What I
want to make a big deal about is the project's standards for attention
to detail and for clarity and transparency about intent and effect of
changes.  Sometimes "and I slipped this other little change in while I
was at it" is fine, though the starting place is always that every
separately-motivated change should be a separate change.  What's never
fine is slipping such a change in without comment or discussion, which
is what happened here.  Now we're discussing it rather than it going
unnoticed, which is what matters most.  But I am also concerned by the
way you responded to my review, which to me read as an off-hand remark
about a personal preference as if that were an adequate response.  Such
preferences are normal in the reasoning behind writing new code in a
particular way, and in such cases will likely go unremarked.  But here
you are changing existing code in a way that changes the compiled code
and its performance characteristics, and the only motivation you cited
was your preferences for how code should be all else being equal.  That
completely ignores the history behind the code being how it is today,
and hence ignores the project's key principle of conservatism.

> +/* Extract name and type from directory entry.  No copy of the name is
> +   made.  If SOURCE is NULL, result name is NULL.  */
> +static struct readdir_result
> +convert_dirent (const struct dirent *source)
> +{
> +  if (source == NULL)
> +    return (struct readdir_result) {};
> +  else
> +    return (struct readdir_result)
> +      {
> +	.name = source->d_name,
> +	D_INO_TO_RESULT (source),
> +	D_TYPE_TO_RESULT (source)
> +      };
> +}
> +
> +#ifndef COMPILE_GLOB64
> +/* Like convert_dirent, but works on struct dirent64 instead.  */
> +static struct readdir_result
> +convert_dirent64 (const struct dirent64 *source)
> +{
> +  if (source == NULL)
> +    return (struct readdir_result) {};
> +  else
> +    return (struct readdir_result)
> +      {
> +	.name = source->d_name,
> +	D_INO_TO_RESULT (source),
> +	D_TYPE_TO_RESULT (source)
> +      };
> +}
> +#endif

While inlines are better than macros when all else is equal, this
duplication of something that lends itself to a single-expression form
(?:) is a good reason to make it a macro instead.

Also, compound literals are a GNU extension and I'm not sure if they
meet gnulib's portability requirements or not.


Thanks,
Roland

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Paul Eggert]

 > +# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
 > +  __typeof__ (((struct dirent) {}).d_type) type;
 > +# endif

__typeof__ isn't portable to non-GCC compilers. How about if we just 
declare d_type to be 'unsigned char'? That should be portable enough in 
practice.


 > +# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
 > +/* Designated initializer based on the d_type member of struct
 > +   dirent.  */
 > +#  define D_TYPE_TO_RESULT(source) .type = (source)->d_type

Gnulib still ports to C89, which makes compound literals dicey. We've 
been thinking of upping that to C99 so this is not an insurmountable 
obstacle (so long as the code sticks to C99-compatible compound 
literals, which I think it does). Still, this macro and its relatives 
are a bit tricky, and if we're going to use tricky macros anyway perhaps 
we'd be better off having them expand to C89 as that shouldn't make 
things much more obscure.


 > +/* True if the directory entry D might be a symbolic link.  */
 > +static inline bool

In Gnulib we typically prefer to say just 'static' without the clutter 
of 'inline', for the same reason we typically don't bother with the 
clutter of 'register'.


 > +# define GL_READDIR(pglob, stream) (pglob)->gl_readdir ((stream))

No need for double parenthesization.

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

On 03/31/2016 01:27 AM, Roland McGrath wrote:
>>> uint8_t is big enough for d_type, and will make the struct smaller on
>>> ILP32.
>>
>> I assume the struct is turned into scalars anyway, but I added the
>> __typeof__.
> 
> I like that fine, but we need to check if it's OK in gnulib.

I'm going to use unsigned char instead.

>>>> -		      len = NAMLEN (d);
>>>> -		      names->name[cur] = (char *) malloc (len + 1);
>>>> +		      names->name[cur] = strdup (e.d_name);
>>>>  		      if (names->name[cur] == NULL)
>>>>  			goto memory_error;
>>>> -		      *((char *) mempcpy (names->name[cur++], name, len))
>>>> -			= '\0';
>>>> +		      ++cur;
>>>
>>> In the _DIRENT_HAVE_D_NAMLEN case, this assumes that strdup is as efficient
>>> as malloc+mempcpy (with no strlen required).  Perhaps it's close enough,
>>> but that is a subtle change you didn't mention as intended.
>>
>> I want to avoid conditionalized code because these things tend to break
>> after a while (or never work in the first place).
> 
> That might be a sufficient reason to avoid writing such code in the first
> place.  But when you are changing something from how it already is, you
> need to be clear about what you are changing and why.  My point was not
> that I had no idea what your motivation for the specific change might have
> been.  My point was that you billed this whole larger change as doing one
> thing, and then slipped this in without comment.

Two more things:

For the non-glob64 case, I removed a copy of the name, which amounts to
one traversal less.  The internal strlen operation in strdup adds back a
string traversal.  So there is no net difference of the work done
(except that there are fewer writes).  CONVERT_DIRENT_DIRENT64 is used
in the glob64 code, so there is an unnecessary strlen operation there.

But the glibc tests for GLOB_ALTDIRFUNC do not set the d_namlen field.
Neither does the code in OpenSSH.  (GNU make sets d_namlen, though.)
Not looking at d_namlen avoids introducing application bugs.

d_ino/d_fileno has a similar problem, and even make doesn't set it. We
should probably ignore it altogether in GLOB_ALTDIRFUNC mode.

> In this case, we already have supported configurations of both cases.
> Hurd is _DIRENT_HAVE_D_NAMLEN (it uses the generic bits/dirent.h), while
> others are not.  So just the configurations that work today not being
> broken in the future (and being adequately tested) prevents "tending to
> break after a while".

I'm surprised there aren't any test failures on Hurd.  d_namlen is an
unsigned char, probably there is a sufficiently large value in it by
accident so that we end copying entire names, including the null
terminator, plus some extra bytes.

> I am making a big deal out of this little case, and it's not because I
> think this particular little performance-relevant change is so likely to
> be important or even that I personally necessarily object to it.  What I
> want to make a big deal about is the project's standards for attention
> to detail and for clarity and transparency about intent and effect of
> changes.  Sometimes "and I slipped this other little change in while I
> was at it" is fine, though the starting place is always that every
> separately-motivated change should be a separate change.

I can remove the use of d_namlen as a separate patch, before the copy
avoidance.  I did not realize it has implications on application
behavior at the time, for which I apologize.  But now, I still think
it's a desirable improvement.

> What's never
> fine is slipping such a change in without comment or discussion, which
> is what happened here.  Now we're discussing it rather than it going
> unnoticed, which is what matters most.  But I am also concerned by the
> way you responded to my review, which to me read as an off-hand remark
> about a personal preference as if that were an adequate response.  Such
> preferences are normal in the reasoning behind writing new code in a
> particular way, and in such cases will likely go unremarked.  But here
> you are changing existing code in a way that changes the compiled code
> and its performance characteristics, and the only motivation you cited
> was your preferences for how code should be all else being equal.  That
> completely ignores the history behind the code being how it is today,
> and hence ignores the project's key principle of conservatism.

Sorry, I don't follow.  I expressed a preference for a coding strategy
that reduces risk by aligning behavior across architectures, so that we
can reuse the results of testing on one set of architectures for others.

I think this approach is better than to check in code that wasn't tested
at all, or demand that every developer tests changes in generic code on
Hurd, x32, alpha, and other architectures which have interesting
differences not found anywhere else.

>> +/* Extract name and type from directory entry.  No copy of the name is
>> +   made.  If SOURCE is NULL, result name is NULL.  */
>> +static struct readdir_result
>> +convert_dirent (const struct dirent *source)
>> +{
>> +  if (source == NULL)
>> +    return (struct readdir_result) {};
>> +  else
>> +    return (struct readdir_result)
>> +      {
>> +	.name = source->d_name,
>> +	D_INO_TO_RESULT (source),
>> +	D_TYPE_TO_RESULT (source)
>> +      };
>> +}
>> +
>> +#ifndef COMPILE_GLOB64
>> +/* Like convert_dirent, but works on struct dirent64 instead.  */
>> +static struct readdir_result
>> +convert_dirent64 (const struct dirent64 *source)
>> +{
>> +  if (source == NULL)
>> +    return (struct readdir_result) {};
>> +  else
>> +    return (struct readdir_result)
>> +      {
>> +	.name = source->d_name,
>> +	D_INO_TO_RESULT (source),
>> +	D_TYPE_TO_RESULT (source)
>> +      };
>> +}
>> +#endif
> 
> While inlines are better than macros when all else is equal, this
> duplication of something that lends itself to a single-expression form
> (?:) is a good reason to make it a macro instead.

The call site relies on the function nature because the source argument
is evaluated multiple times.  I could change that by using a temporary
at the call site, but ...

> Also, compound literals are a GNU extension and I'm not sure if they
> meet gnulib's portability requirements or not.

... avoiding the compound literal requires writing a function.  I could
put the shared body into a macro, though.

Florian

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

On 03/31/2016 07:16 PM, Florian Weimer wrote:
> On 03/31/2016 01:27 AM, Roland McGrath wrote:
>>>> uint8_t is big enough for d_type, and will make the struct smaller on
>>>> ILP32.
>>>
>>> I assume the struct is turned into scalars anyway, but I added the
>>> __typeof__.
>>
>> I like that fine, but we need to check if it's OK in gnulib.
> 
> I'm going to use unsigned char instead.
> 
>>>>> -		      len = NAMLEN (d);
>>>>> -		      names->name[cur] = (char *) malloc (len + 1);
>>>>> +		      names->name[cur] = strdup (e.d_name);
>>>>>  		      if (names->name[cur] == NULL)
>>>>>  			goto memory_error;
>>>>> -		      *((char *) mempcpy (names->name[cur++], name, len))
>>>>> -			= '\0';
>>>>> +		      ++cur;
>>>>
>>>> In the _DIRENT_HAVE_D_NAMLEN case, this assumes that strdup is as efficient
>>>> as malloc+mempcpy (with no strlen required).  Perhaps it's close enough,
>>>> but that is a subtle change you didn't mention as intended.
>>>
>>> I want to avoid conditionalized code because these things tend to break
>>> after a while (or never work in the first place).
>>
>> That might be a sufficient reason to avoid writing such code in the first
>> place.  But when you are changing something from how it already is, you
>> need to be clear about what you are changing and why.  My point was not
>> that I had no idea what your motivation for the specific change might have
>> been.  My point was that you billed this whole larger change as doing one
>> thing, and then slipped this in without comment.
> 
> Two more things:
> 
> For the non-glob64 case, I removed a copy of the name, which amounts to
> one traversal less.  The internal strlen operation in strdup adds back a
> string traversal.  So there is no net difference of the work done
> (except that there are fewer writes).  CONVERT_DIRENT_DIRENT64 is used
> in the glob64 code, so there is an unnecessary strlen operation there.
> 
> But the glibc tests for GLOB_ALTDIRFUNC do not set the d_namlen field.
> Neither does the code in OpenSSH.  (GNU make sets d_namlen, though.)
> Not looking at d_namlen avoids introducing application bugs.
> 
> d_ino/d_fileno has a similar problem, and even make doesn't set it. We
> should probably ignore it altogether in GLOB_ALTDIRFUNC mode.

Correction: make initializes d_ino to 1.

Florian

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Roland McGrath]

>  > +# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
>  > +  __typeof__ (((struct dirent) {}).d_type) type;
>  > +# endif
> 
> __typeof__ isn't portable to non-GCC compilers. How about if we just 
> declare d_type to be 'unsigned char'? That should be portable enough in 
> practice.

Fine by me.

> Gnulib still ports to C89, which makes compound literals dicey. We've 
> been thinking of upping that to C99 so this is not an insurmountable 
> obstacle (so long as the code sticks to C99-compatible compound 
> literals, which I think it does). Still, this macro and its relatives 
> are a bit tricky, and if we're going to use tricky macros anyway perhaps 
> we'd be better off having them expand to C89 as that shouldn't make 
> things much more obscure.

In this case I think it's adequate to use a non-initializing
definition followed by assignments.  That avoids the vagaries of
relying on field order.  Florian can verify that it compiles to the
same thing, which I expect it will.

>  > +/* True if the directory entry D might be a symbolic link.  */
>  > +static inline bool
> 
> In Gnulib we typically prefer to say just 'static' without the clutter 
> of 'inline', for the same reason we typically don't bother with the 
> clutter of 'register'.

Actually, in libc it's our standard style to use just "static" in C
source files.  The principle is that the compiler will decide whether
to inline.  I failed to catch this style violation in my review.  (We
do still use "static inline" in header files, but basically just
because it's shorter to write than "static __attribute__ ((unused))".)

>  > +# define GL_READDIR(pglob, stream) (pglob)->gl_readdir ((stream))
> 
> No need for double parenthesization.

Canonical parenthesization would be:

# define GL_READDIR(pglob, stream) ((pglob)->gl_readdir (stream))


Thanks,
Roland

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Roland McGrath]

> I can remove the use of d_namlen as a separate patch, before the copy
> avoidance.  I did not realize it has implications on application
> behavior at the time, for which I apologize.  But now, I still think
> it's a desirable improvement.

That sounds great.

> Sorry, I don't follow.  I expressed a preference for a coding strategy
> that reduces risk by aligning behavior across architectures, so that we
> can reuse the results of testing on one set of architectures for others.

My point was that you included a change that did not seem relevant to the
stated goal, and your response to my review still didn't clearly say why
the status quo had to be changed.  Separating small changes out into their
own review threads is the best way to be sure that the rationale for the
change was stated clearly and gets discussed fully.

> > While inlines are better than macros when all else is equal, this
> > duplication of something that lends itself to a single-expression form
> > (?:) is a good reason to make it a macro instead.
> 
> The call site relies on the function nature because the source argument
> is evaluated multiple times.  I could change that by using a temporary
> at the call site, but ...
> 
> > Also, compound literals are a GNU extension and I'm not sure if they
> > meet gnulib's portability requirements or not.
> 
> ... avoiding the compound literal requires writing a function.  I could
> put the shared body into a macro, though.

As you can tell, I didn't drill down to all the details of what's possible.
I just noticed the glaring duplication and wanted to push back on that.
Sometimes duplication is the best of the available options, which are all
imperfect.  When that's so, it merits a comment about why duplication is
what makes the most sense.  The way to read my comment is, "This looks like
a fishy level of duplication; see if you can find something better, or
explain why this is the best we can do."


Thanks,
Roland

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

On 04/01/2016 11:55 PM, Roland McGrath wrote:
>> I can remove the use of d_namlen as a separate patch, before the copy
>> avoidance.  I did not realize it has implications on application
>> behavior at the time, for which I apologize.  But now, I still think
>> it's a desirable improvement.
>
> That sounds great.

Now that the d_namlen change is in, here is a rebased version of the 
CVE-2016-1234 fix.

This version keeps the d_ino-guided skipping.

Tested on x86_64 and i386.

Thanks,
Florian

[-- Attachment #2: 0001-CVE-2016-1234-glob-Do-not-copy-d_name-field-of-struc.patch --]
[-- Type: text/x-patch, Size: 11964 bytes --]

2016-04-01  Florian Weimer  <fweimer@redhat.com>

	[BZ #19779]
	CVE-2016-1234
	Avoid copying names of directory entries.
	* posix/glob.c (DIRENT_MUST_BE, DIRENT_MIGHT_BE_SYMLINK)
	(DIRENT_MIGHT_BE_DIR, CONVERT_D_INO, CONVERT_D_TYPE)
	(CONVERT_DIRENT_DIRENT64, REAL_DIR_ENTRY): Remove macros.
	(struct readdir_result): New type.
	(D_TYPE_TO_RESULT, D_INO_TO_RESULT, READDIR_RESULT_INITIALIZER)
	(GL_READDIR): New macros.
	(readdir_result_might_be_symlink, readdir_result_might_be_dir)
	(convert_dirent, convert_dirent64): New functions.
	(glob_in_dir): Use struct readdir_result.  Call convert_dirent or
	convert_dirent64.  Adjust references to the readdir result.
	* sysdeps/unix/sysv/linux/i386/glob64.c:
	(convert_dirent, GL_READDIR): Redefine for second file inclusion.
	* posix/bug-glob2.c (LONG_NAME): Define.
	(filesystem): Add LONG_NAME.
	(my_DIR): Increase the size of room_for_dirent.

diff --git a/posix/bug-glob2.c b/posix/bug-glob2.c
index 0fdc5d0..5873e08 100644
--- a/posix/bug-glob2.c
+++ b/posix/bug-glob2.c
@@ -40,6 +40,17 @@
 # define PRINTF(fmt, args...)
 #endif
 
+#define LONG_NAME \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
 
 static struct
 {
@@ -58,6 +69,7 @@ static struct
       { ".", 3, DT_DIR, 0755 },
       { "..", 3, DT_DIR, 0755 },
       { "a", 3, DT_REG, 0644 },
+      { LONG_NAME, 3, DT_REG, 0644 },
     { "unreadable", 2, DT_DIR, 0111 },
       { ".", 3, DT_DIR, 0111 },
       { "..", 3, DT_DIR, 0755 },
@@ -75,7 +87,7 @@ typedef struct
   int level;
   int idx;
   struct dirent d;
-  char room_for_dirent[NAME_MAX];
+  char room_for_dirent[sizeof (LONG_NAME)];
 } my_DIR;
 
 
diff --git a/posix/glob.c b/posix/glob.c
index 9ae76ac..f687023 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -24,6 +24,7 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <stdbool.h>
 #include <stddef.h>
 
 /* Outcomment the following line for production quality code.  */
@@ -73,69 +74,8 @@
 # endif /* HAVE_VMSDIR_H */
 #endif
 
-
-/* When used in the GNU libc the symbol _DIRENT_HAVE_D_TYPE is available
-   if the `d_type' member for `struct dirent' is available.
-   HAVE_STRUCT_DIRENT_D_TYPE plays the same role in GNULIB.  */
-#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
-/* True if the directory entry D must be of type T.  */
-# define DIRENT_MUST_BE(d, t)	((d)->d_type == (t))
-
-/* True if the directory entry D might be a symbolic link.  */
-# define DIRENT_MIGHT_BE_SYMLINK(d) \
-    ((d)->d_type == DT_UNKNOWN || (d)->d_type == DT_LNK)
-
-/* True if the directory entry D might be a directory.  */
-# define DIRENT_MIGHT_BE_DIR(d)	 \
-    ((d)->d_type == DT_DIR || DIRENT_MIGHT_BE_SYMLINK (d))
-
-#else /* !HAVE_D_TYPE */
-# define DIRENT_MUST_BE(d, t)		false
-# define DIRENT_MIGHT_BE_SYMLINK(d)	true
-# define DIRENT_MIGHT_BE_DIR(d)		true
-#endif /* HAVE_D_TYPE */
-
-/* If the system has the `struct dirent64' type we use it internally.  */
-#if defined _LIBC && !defined COMPILE_GLOB64
-
-# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
-#  define CONVERT_D_INO(d64, d32)
-# else
-#  define CONVERT_D_INO(d64, d32) \
-  (d64)->d_ino = (d32)->d_ino;
-# endif
-
-# ifdef _DIRENT_HAVE_D_TYPE
-#  define CONVERT_D_TYPE(d64, d32) \
-  (d64)->d_type = (d32)->d_type;
-# else
-#  define CONVERT_D_TYPE(d64, d32)
-# endif
-
-# define CONVERT_DIRENT_DIRENT64(d64, d32) \
-  strcpy ((d64)->d_name, (d32)->d_name);				      \
-  CONVERT_D_INO (d64, d32)						      \
-  CONVERT_D_TYPE (d64, d32)
-#endif
-
-
-#if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
-/* Posix does not require that the d_ino field be present, and some
-   systems do not provide it. */
-# define REAL_DIR_ENTRY(dp) 1
-#else
-# define REAL_DIR_ENTRY(dp) (dp->d_ino != 0)
-#endif /* POSIX */
-
 #include <stdlib.h>
 #include <string.h>
-
-/* NAME_MAX is usually defined in <dirent.h> or <limits.h>.  */
-#include <limits.h>
-#ifndef NAME_MAX
-# define NAME_MAX (sizeof (((struct dirent *) 0)->d_name))
-#endif
-
 #include <alloca.h>
 
 #ifdef _LIBC
@@ -180,8 +120,109 @@
 \f
 static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
 
+/* A representation of a directory entry which does not depend on the
+   layout of struct dirent, or the size of ino_t.  */
+struct readdir_result
+{
+  const char *name;
+# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
+  unsigned char type;
+# endif
+  bool skip_entry;
+};
+
+# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
+/* Initializer based on the d_type member of struct dirent.  */
+#  define D_TYPE_TO_RESULT(source) (source)->d_type,
+
+/* True if the directory entry D might be a symbolic link.  */
+static bool
+readdir_result_might_be_symlink (struct readdir_result d)
+{
+  return d.type == DT_UNKNOWN || d.type == DT_LNK;
+}
+
+/* True if the directory entry D might be a directory.  */
+static bool
+readdir_result_might_be_dir (struct readdir_result d)
+{
+  return d.type == DT_DIR || readdir_result_might_be_symlink (d);
+}
+# else /* defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE */
+#  define D_TYPE_TO_RESULT(source)
+
+/* If we do not have type information, symbolic links and directories
+   are always a possibility.  */
+
+static bool
+readdir_result_might_be_symlink (struct readdir_result d)
+{
+  return true;
+}
+
+static bool
+readdir_result_might_be_dir (struct readdir_result d)
+{
+  return true;
+}
+
+# endif /* defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE */
+
+# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
+/* Initializer for skip_entry.  POSIX does not require that the d_ino
+   field be present, and some systems do not provide it. */
+#  define D_INO_TO_RESULT(source) false,
+# else
+#  define D_INO_TO_RESULT(source) (source)->d_ino == 0,
+# endif
+
+/* Construct an initializer for a struct readdir_result object from a
+   struct dirent *.  No copy of the name is made.  */
+#define READDIR_RESULT_INITIALIZER(source) \
+  {					   \
+    source->d_name,			   \
+    D_TYPE_TO_RESULT (source)		   \
+    D_INO_TO_RESULT (source)		   \
+  }
+
 #endif /* !defined _LIBC || !defined GLOB_ONLY_P */
 
+/* Call gl_readdir on STREAM.  This macro can be overridden to reduce
+   type safety if an old interface version needs to be supported.  */
+#ifndef GL_READDIR
+# define GL_READDIR(pglob, stream) ((pglob)->gl_readdir (stream))
+#endif
+
+/* Extract name and type from directory entry.  No copy of the name is
+   made.  If SOURCE is NULL, result name is NULL.  */
+static struct readdir_result
+convert_dirent (const struct dirent *source)
+{
+  if (source == NULL)
+    {
+      struct readdir_result result = {NULL};
+      return result;
+    }
+  struct readdir_result result = READDIR_RESULT_INITIALIZER (source);
+  return result;
+}
+
+#ifndef COMPILE_GLOB64
+/* Like convert_dirent, but works on struct dirent64 instead.  */
+static struct readdir_result
+convert_dirent64 (const struct dirent64 *source)
+{
+  if (source == NULL)
+    {
+      struct readdir_result result = {NULL};
+      return result;
+    }
+  struct readdir_result result = READDIR_RESULT_INITIALIZER (source);
+  return result;
+}
+#endif
+
+
 #ifndef attribute_hidden
 # define attribute_hidden
 #endif
@@ -1538,55 +1579,36 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 
 	  while (1)
 	    {
-	      const char *name;
-#if defined _LIBC && !defined COMPILE_GLOB64
-	      struct dirent64 *d;
-	      union
-		{
-		  struct dirent64 d64;
-		  char room [offsetof (struct dirent64, d_name[0])
-			     + NAME_MAX + 1];
-		}
-	      d64buf;
-
-	      if (__glibc_unlikely (flags & GLOB_ALTDIRFUNC))
-		{
-		  struct dirent *d32 = (*pglob->gl_readdir) (stream);
-		  if (d32 != NULL)
-		    {
-		      CONVERT_DIRENT_DIRENT64 (&d64buf.d64, d32);
-		      d = &d64buf.d64;
-		    }
-		  else
-		    d = NULL;
-		}
-	      else
-		d = __readdir64 (stream);
+	      struct readdir_result d;
+	      {
+		if (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0))
+		  d = convert_dirent (GL_READDIR (pglob, stream));
+		else
+		  {
+#ifdef COMPILE_GLOB64
+		    d = convert_dirent (__readdir (stream));
 #else
-	      struct dirent *d = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
-				  ? ((struct dirent *)
-				     (*pglob->gl_readdir) (stream))
-				  : __readdir (stream));
+		    d = convert_dirent64 (__readdir64 (stream));
 #endif
-	      if (d == NULL)
+		  }
+	      }
+	      if (d.name == NULL)
 		break;
-	      if (! REAL_DIR_ENTRY (d))
+	      if (d.skip_entry)
 		continue;
 
 	      /* If we shall match only directories use the information
 		 provided by the dirent call if possible.  */
-	      if ((flags & GLOB_ONLYDIR) && !DIRENT_MIGHT_BE_DIR (d))
+	      if ((flags & GLOB_ONLYDIR) && !readdir_result_might_be_dir (d))
 		continue;
 
-	      name = d->d_name;
-
-	      if (fnmatch (pattern, name, fnm_flags) == 0)
+	      if (fnmatch (pattern, d.name, fnm_flags) == 0)
 		{
 		  /* If the file we found is a symlink we have to
 		     make sure the target file exists.  */
-		  if (!DIRENT_MIGHT_BE_SYMLINK (d)
-		      || link_exists_p (dfd, directory, dirlen, name, pglob,
-					flags))
+		  if (!readdir_result_might_be_symlink (d)
+		      || link_exists_p (dfd, directory, dirlen, d.name,
+					pglob, flags))
 		    {
 		      if (cur == names->count)
 			{
@@ -1606,7 +1628,7 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 			  names = newnames;
 			  cur = 0;
 			}
-		      names->name[cur] = strdup (d->d_name);
+		      names->name[cur] = strdup (d.name);
 		      if (names->name[cur] == NULL)
 			goto memory_error;
 		      ++cur;
diff --git a/sysdeps/unix/sysv/linux/i386/glob64.c b/sysdeps/unix/sysv/linux/i386/glob64.c
index b4fcd1a..802c957 100644
--- a/sysdeps/unix/sysv/linux/i386/glob64.c
+++ b/sysdeps/unix/sysv/linux/i386/glob64.c
@@ -1,3 +1,21 @@
+/* Two glob variants with 64-bit support, for dirent64 and __olddirent64.
+   Copyright (C) 1998-2016 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/>.  */
+
 #include <dirent.h>
 #include <glob.h>
 #include <sys/stat.h>
@@ -38,11 +56,15 @@ int __old_glob64 (const char *__pattern, int __flags,
 
 #undef dirent
 #define dirent __old_dirent64
+#undef GL_READDIR
+# define GL_READDIR(pglob, stream) \
+  ((struct __old_dirent64 *) (pglob)->gl_readdir (stream))
 #undef __readdir
 #define __readdir(dirp) __old_readdir64 (dirp)
 #undef glob
 #define glob(pattern, flags, errfunc, pglob) \
   __old_glob64 (pattern, flags, errfunc, pglob)
+#define convert_dirent __old_convert_dirent
 #define glob_in_dir __old_glob_in_dir
 #define GLOB_ATTRIBUTE attribute_compat_text_section
 

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Roland McGrath]

Pretty much OK.  
Please follow up quickly on making sure we are harmonized with gnulib.

> +# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
> +  unsigned char type;
> +# endif

uint8_t seems better for something that is not a character.

> +      struct readdir_result result = {NULL};

Put spaces around NULL, or just use {} (which has the same semantics).

> +#ifndef COMPILE_GLOB64

Ditto in the copy.  Ick for having a copy.  If it seems best to have a
copy, then at least put a comment on the copy saying it needs to match.

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

On 05/03/2016 12:48 AM, Roland McGrath wrote:
> Pretty much OK.
> Please follow up quickly on making sure we are harmonized with gnulib.
>
>> +# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
>> +  unsigned char type;
>> +# endif
>
> uint8_t seems better for something that is not a character.

I made the change and included <stdint.h> (for which gnulib has an 
emulation header).

>> +      struct readdir_result result = {NULL};
>
> Put spaces around NULL, or just use {} (which has the same semantics).

I have to use NULL because empty initializers are not part of C89.  So I 
added the spaces.

>> +#ifndef COMPILE_GLOB64
>
> Ditto in the copy.  Ick for having a copy.  If it seems best to have a
> copy, then at least put a comment on the copy saying it needs to match.

I prefer having the copy, it is quite short and avoids another level of 
macro nesting.

I'm attaching the new version.

Florian

[-- Attachment #2: 0001-CVE-2016-1234-glob-Do-not-copy-d_name-field-of-struc.patch --]
[-- Type: text/x-patch, Size: 12099 bytes --]

2016-04-29  Florian Weimer  <fweimer@redhat.com>

	[BZ #19779]
	CVE-2016-1234
	Avoid copying names of directory entries.
	* posix/glob.c (DIRENT_MUST_BE, DIRENT_MIGHT_BE_SYMLINK)
	(DIRENT_MIGHT_BE_DIR, CONVERT_D_INO, CONVERT_D_TYPE)
	(CONVERT_DIRENT_DIRENT64, REAL_DIR_ENTRY): Remove macros.
	(struct readdir_result): New type.
	(D_TYPE_TO_RESULT, D_INO_TO_RESULT, READDIR_RESULT_INITIALIZER)
	(GL_READDIR): New macros.
	(readdir_result_might_be_symlink, readdir_result_might_be_dir)
	(convert_dirent, convert_dirent64): New functions.
	(glob_in_dir): Use struct readdir_result.  Call convert_dirent or
	convert_dirent64.  Adjust references to the readdir result.
	* sysdeps/unix/sysv/linux/i386/glob64.c:
	(convert_dirent, GL_READDIR): Redefine for second file inclusion.
	* posix/bug-glob2.c (LONG_NAME): Define.
	(filesystem): Add LONG_NAME.
	(my_DIR): Increase the size of room_for_dirent.

diff --git a/posix/bug-glob2.c b/posix/bug-glob2.c
index 0fdc5d0..5873e08 100644
--- a/posix/bug-glob2.c
+++ b/posix/bug-glob2.c
@@ -40,6 +40,17 @@
 # define PRINTF(fmt, args...)
 #endif
 
+#define LONG_NAME \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \
+  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
 
 static struct
 {
@@ -58,6 +69,7 @@ static struct
       { ".", 3, DT_DIR, 0755 },
       { "..", 3, DT_DIR, 0755 },
       { "a", 3, DT_REG, 0644 },
+      { LONG_NAME, 3, DT_REG, 0644 },
     { "unreadable", 2, DT_DIR, 0111 },
       { ".", 3, DT_DIR, 0111 },
       { "..", 3, DT_DIR, 0755 },
@@ -75,7 +87,7 @@ typedef struct
   int level;
   int idx;
   struct dirent d;
-  char room_for_dirent[NAME_MAX];
+  char room_for_dirent[sizeof (LONG_NAME)];
 } my_DIR;
 
 
diff --git a/posix/glob.c b/posix/glob.c
index 9ae76ac..d2ecca7 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -24,7 +24,9 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <stdbool.h>
 #include <stddef.h>
+#include <stdint.h>
 
 /* Outcomment the following line for production quality code.  */
 /* #define NDEBUG 1 */
@@ -73,69 +75,8 @@
 # endif /* HAVE_VMSDIR_H */
 #endif
 
-
-/* When used in the GNU libc the symbol _DIRENT_HAVE_D_TYPE is available
-   if the `d_type' member for `struct dirent' is available.
-   HAVE_STRUCT_DIRENT_D_TYPE plays the same role in GNULIB.  */
-#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
-/* True if the directory entry D must be of type T.  */
-# define DIRENT_MUST_BE(d, t)	((d)->d_type == (t))
-
-/* True if the directory entry D might be a symbolic link.  */
-# define DIRENT_MIGHT_BE_SYMLINK(d) \
-    ((d)->d_type == DT_UNKNOWN || (d)->d_type == DT_LNK)
-
-/* True if the directory entry D might be a directory.  */
-# define DIRENT_MIGHT_BE_DIR(d)	 \
-    ((d)->d_type == DT_DIR || DIRENT_MIGHT_BE_SYMLINK (d))
-
-#else /* !HAVE_D_TYPE */
-# define DIRENT_MUST_BE(d, t)		false
-# define DIRENT_MIGHT_BE_SYMLINK(d)	true
-# define DIRENT_MIGHT_BE_DIR(d)		true
-#endif /* HAVE_D_TYPE */
-
-/* If the system has the `struct dirent64' type we use it internally.  */
-#if defined _LIBC && !defined COMPILE_GLOB64
-
-# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
-#  define CONVERT_D_INO(d64, d32)
-# else
-#  define CONVERT_D_INO(d64, d32) \
-  (d64)->d_ino = (d32)->d_ino;
-# endif
-
-# ifdef _DIRENT_HAVE_D_TYPE
-#  define CONVERT_D_TYPE(d64, d32) \
-  (d64)->d_type = (d32)->d_type;
-# else
-#  define CONVERT_D_TYPE(d64, d32)
-# endif
-
-# define CONVERT_DIRENT_DIRENT64(d64, d32) \
-  strcpy ((d64)->d_name, (d32)->d_name);				      \
-  CONVERT_D_INO (d64, d32)						      \
-  CONVERT_D_TYPE (d64, d32)
-#endif
-
-
-#if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
-/* Posix does not require that the d_ino field be present, and some
-   systems do not provide it. */
-# define REAL_DIR_ENTRY(dp) 1
-#else
-# define REAL_DIR_ENTRY(dp) (dp->d_ino != 0)
-#endif /* POSIX */
-
 #include <stdlib.h>
 #include <string.h>
-
-/* NAME_MAX is usually defined in <dirent.h> or <limits.h>.  */
-#include <limits.h>
-#ifndef NAME_MAX
-# define NAME_MAX (sizeof (((struct dirent *) 0)->d_name))
-#endif
-
 #include <alloca.h>
 
 #ifdef _LIBC
@@ -180,8 +121,111 @@
 \f
 static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
 
+/* A representation of a directory entry which does not depend on the
+   layout of struct dirent, or the size of ino_t.  */
+struct readdir_result
+{
+  const char *name;
+# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
+  uint8_t type;
+# endif
+  bool skip_entry;
+};
+
+# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
+/* Initializer based on the d_type member of struct dirent.  */
+#  define D_TYPE_TO_RESULT(source) (source)->d_type,
+
+/* True if the directory entry D might be a symbolic link.  */
+static bool
+readdir_result_might_be_symlink (struct readdir_result d)
+{
+  return d.type == DT_UNKNOWN || d.type == DT_LNK;
+}
+
+/* True if the directory entry D might be a directory.  */
+static bool
+readdir_result_might_be_dir (struct readdir_result d)
+{
+  return d.type == DT_DIR || readdir_result_might_be_symlink (d);
+}
+# else /* defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE */
+#  define D_TYPE_TO_RESULT(source)
+
+/* If we do not have type information, symbolic links and directories
+   are always a possibility.  */
+
+static bool
+readdir_result_might_be_symlink (struct readdir_result d)
+{
+  return true;
+}
+
+static bool
+readdir_result_might_be_dir (struct readdir_result d)
+{
+  return true;
+}
+
+# endif /* defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE */
+
+# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__
+/* Initializer for skip_entry.  POSIX does not require that the d_ino
+   field be present, and some systems do not provide it. */
+#  define D_INO_TO_RESULT(source) false,
+# else
+#  define D_INO_TO_RESULT(source) (source)->d_ino == 0,
+# endif
+
+/* Construct an initializer for a struct readdir_result object from a
+   struct dirent *.  No copy of the name is made.  */
+#define READDIR_RESULT_INITIALIZER(source) \
+  {					   \
+    source->d_name,			   \
+    D_TYPE_TO_RESULT (source)		   \
+    D_INO_TO_RESULT (source)		   \
+  }
+
 #endif /* !defined _LIBC || !defined GLOB_ONLY_P */
 
+/* Call gl_readdir on STREAM.  This macro can be overridden to reduce
+   type safety if an old interface version needs to be supported.  */
+#ifndef GL_READDIR
+# define GL_READDIR(pglob, stream) ((pglob)->gl_readdir (stream))
+#endif
+
+/* Extract name and type from directory entry.  No copy of the name is
+   made.  If SOURCE is NULL, result name is NULL.  Keep in sync with
+   convert_dirent64 below.  */
+static struct readdir_result
+convert_dirent (const struct dirent *source)
+{
+  if (source == NULL)
+    {
+      struct readdir_result result = { NULL };
+      return result;
+    }
+  struct readdir_result result = READDIR_RESULT_INITIALIZER (source);
+  return result;
+}
+
+#ifndef COMPILE_GLOB64
+/* Like convert_dirent, but works on struct dirent64 instead.  Keep in
+   sync with convert_dirent above.  */
+static struct readdir_result
+convert_dirent64 (const struct dirent64 *source)
+{
+  if (source == NULL)
+    {
+      struct readdir_result result = { NULL };
+      return result;
+    }
+  struct readdir_result result = READDIR_RESULT_INITIALIZER (source);
+  return result;
+}
+#endif
+
+
 #ifndef attribute_hidden
 # define attribute_hidden
 #endif
@@ -1538,55 +1582,36 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 
 	  while (1)
 	    {
-	      const char *name;
-#if defined _LIBC && !defined COMPILE_GLOB64
-	      struct dirent64 *d;
-	      union
-		{
-		  struct dirent64 d64;
-		  char room [offsetof (struct dirent64, d_name[0])
-			     + NAME_MAX + 1];
-		}
-	      d64buf;
-
-	      if (__glibc_unlikely (flags & GLOB_ALTDIRFUNC))
-		{
-		  struct dirent *d32 = (*pglob->gl_readdir) (stream);
-		  if (d32 != NULL)
-		    {
-		      CONVERT_DIRENT_DIRENT64 (&d64buf.d64, d32);
-		      d = &d64buf.d64;
-		    }
-		  else
-		    d = NULL;
-		}
-	      else
-		d = __readdir64 (stream);
+	      struct readdir_result d;
+	      {
+		if (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0))
+		  d = convert_dirent (GL_READDIR (pglob, stream));
+		else
+		  {
+#ifdef COMPILE_GLOB64
+		    d = convert_dirent (__readdir (stream));
 #else
-	      struct dirent *d = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
-				  ? ((struct dirent *)
-				     (*pglob->gl_readdir) (stream))
-				  : __readdir (stream));
+		    d = convert_dirent64 (__readdir64 (stream));
 #endif
-	      if (d == NULL)
+		  }
+	      }
+	      if (d.name == NULL)
 		break;
-	      if (! REAL_DIR_ENTRY (d))
+	      if (d.skip_entry)
 		continue;
 
 	      /* If we shall match only directories use the information
 		 provided by the dirent call if possible.  */
-	      if ((flags & GLOB_ONLYDIR) && !DIRENT_MIGHT_BE_DIR (d))
+	      if ((flags & GLOB_ONLYDIR) && !readdir_result_might_be_dir (d))
 		continue;
 
-	      name = d->d_name;
-
-	      if (fnmatch (pattern, name, fnm_flags) == 0)
+	      if (fnmatch (pattern, d.name, fnm_flags) == 0)
 		{
 		  /* If the file we found is a symlink we have to
 		     make sure the target file exists.  */
-		  if (!DIRENT_MIGHT_BE_SYMLINK (d)
-		      || link_exists_p (dfd, directory, dirlen, name, pglob,
-					flags))
+		  if (!readdir_result_might_be_symlink (d)
+		      || link_exists_p (dfd, directory, dirlen, d.name,
+					pglob, flags))
 		    {
 		      if (cur == names->count)
 			{
@@ -1606,7 +1631,7 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
 			  names = newnames;
 			  cur = 0;
 			}
-		      names->name[cur] = strdup (d->d_name);
+		      names->name[cur] = strdup (d.name);
 		      if (names->name[cur] == NULL)
 			goto memory_error;
 		      ++cur;
diff --git a/sysdeps/unix/sysv/linux/i386/glob64.c b/sysdeps/unix/sysv/linux/i386/glob64.c
index b4fcd1a..802c957 100644
--- a/sysdeps/unix/sysv/linux/i386/glob64.c
+++ b/sysdeps/unix/sysv/linux/i386/glob64.c
@@ -1,3 +1,21 @@
+/* Two glob variants with 64-bit support, for dirent64 and __olddirent64.
+   Copyright (C) 1998-2016 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/>.  */
+
 #include <dirent.h>
 #include <glob.h>
 #include <sys/stat.h>
@@ -38,11 +56,15 @@ int __old_glob64 (const char *__pattern, int __flags,
 
 #undef dirent
 #define dirent __old_dirent64
+#undef GL_READDIR
+# define GL_READDIR(pglob, stream) \
+  ((struct __old_dirent64 *) (pglob)->gl_readdir (stream))
 #undef __readdir
 #define __readdir(dirp) __old_readdir64 (dirp)
 #undef glob
 #define glob(pattern, flags, errfunc, pglob) \
   __old_glob64 (pattern, flags, errfunc, pglob)
+#define convert_dirent __old_convert_dirent
 #define glob_in_dir __old_glob_in_dir
 #define GLOB_ATTRIBUTE attribute_compat_text_section
 

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Roland McGrath]

> >> +      struct readdir_result result = {NULL};
> >
> > Put spaces around NULL, or just use {} (which has the same semantics).
> 
> I have to use NULL because empty initializers are not part of C89.  So I 
> added the spaces.

OK.  FWIW, I usually use a trailing comma i.e. "{ NULL, }" as implicit
documentation that it's zero-initializing members I didn't list explicitly.

> I prefer having the copy, it is quite short and avoids another level of 
> macro nesting.

The libc sources are riddled with the evidence of my pathological aversion
to duplicated code, and there are certainly cases to be made that it has
not always been the easiest thing to maintain.  Knee-jerk reactions aside,
my true motivation is to ease maintenance and minimize chances of omitting
a change in one place when making it in the other.  So if we manifestly
achieve that not happening, then it's fine.

> I'm attaching the new version.

OK by me.

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Florian Weimer]

On 05/03/2016 11:46 PM, Roland McGrath wrote:
>>>> +      struct readdir_result result = {NULL};
>>>
>>> Put spaces around NULL, or just use {} (which has the same semantics).
>>
>> I have to use NULL because empty initializers are not part of C89.  So I
>> added the spaces.
>
> OK.  FWIW, I usually use a trailing comma i.e. "{ NULL, }" as implicit
> documentation that it's zero-initializing members I didn't list explicitly.

I pushed the patch with this change.  I added this NEWS entry:

* The glob function suffered from a stack-based buffer overflow when it was
   called with the GLOB_ALTDIRFUNC flag and encountered a long file name.
   Reported by Alexander Cherepanov.  (CVE-2016-1234)

Thanks for your patience.

Florian

Re: [PATCH] glob: Avoid copying the d_name field of struct dirent [BZ #19779]

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

I merged those changes into gnulib's glob.c here:

http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=36cc6c33ade715a844662cefd256f8f8fdd8a05d

Gnulib still differs from glibc glob.c in that (a) Gnulib uses spaces 
and not tabs, (b) Gnulib's comments quote 'like this' and not `like 
this', and (c) Gnulib contains several other portability changes briefly 
described below and detailed in the attached patch. The first bullet is 
probably worth a more-detailed look in glibc; the other stuff doesn't 
appear to be urgent.

* glibc glob.c has an assignment 'name = alloca_account (buflen, 
alloca_used)' that is not guarded by __libc_use_alloca.

* glibc glob.c is missing some Gnulib-only portability hacks, protected 
inside "#ifndef _LIBC", "#ifndef  __attribute_noinline__", etc.

* glibc glob.c has some now-unnecessary "#if _LIBC"s that can be omitted 
to simplify the code.

* glibc glob.c has some unnecessary casts, e.g., the cast in 'char *new 
= (char *) malloc (dirlen + 1 + eltlen);'.

* A glibc glob.c comment says that something is "illegal" where it is 
merely invalid. (This is a pet peeve of RMS's.)

* glibc glob.c assumes C99 statements-before-decls in a couple of places.

* glibc glob.c has a complicated cast '*(const char *const * const) a' 
that confuses some compilers.

* There is some complicated stuff in the link_exists2_p area that 
doesn't port well to platforms with fstatat.



[-- Attachment #2: glob.diff --]
[-- Type: text/x-patch, Size: 18287 bytes --]

--- -	2016-05-04 10:25:26.318937701 -0700
+++ glob.c	2016-05-04 10:13:21.859892515 -0700
@@ -15,7 +15,10 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#ifdef  HAVE_CONFIG_H
+#ifndef _LIBC
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   optimizes away the pattern == NULL || pglob == NULL tests below.  */
+# define _GL_ARG_NONNULL(params)
 # include <config.h>
 #endif
 
@@ -34,22 +37,19 @@
 
 #include <stdio.h>              /* Needed on stupid SunOS for assert.  */
 
-#if !defined _LIBC || !defined GLOB_ONLY_P
-#if defined HAVE_UNISTD_H || defined _LIBC
-# include <unistd.h>
-# ifndef POSIX
-#  ifdef _POSIX_VERSION
-#   define POSIX
-#  endif
-# endif
+#ifndef GLOB_ONLY_P
+
+#include <unistd.h>
+#if !defined POSIX && defined _POSIX_VERSION
+# define POSIX
 #endif
 
-#include <pwd.h>
+#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+# define WINDOWS32
+#endif
 
-#if defined HAVE_STDINT_H || defined _LIBC
-# include <stdint.h>
-#elif !defined UINTPTR_MAX
-# define UINTPTR_MAX (~((size_t) 0))
+#ifndef WINDOWS32
+# include <pwd.h>
 #endif
 
 #include <errno.h>
@@ -57,24 +57,7 @@
 # define __set_errno(val) errno = (val)
 #endif
 
-#if defined HAVE_DIRENT_H || defined __GNU_LIBRARY__
-# include <dirent.h>
-#else
-# define dirent direct
-# ifdef HAVE_SYS_NDIR_H
-#  include <sys/ndir.h>
-# endif
-# ifdef HAVE_SYS_DIR_H
-#  include <sys/dir.h>
-# endif
-# ifdef HAVE_NDIR_H
-#  include <ndir.h>
-# endif
-# ifdef HAVE_VMSDIR_H
-#  include "vmsdir.h"
-# endif /* HAVE_VMSDIR_H */
-#endif
-
+#include <dirent.h>
 #include <stdlib.h>
 #include <string.h>
 #include <alloca.h>
@@ -93,17 +76,14 @@
 # endif
 # define struct_stat64          struct stat64
 #else /* !_LIBC */
-# include "getlogin_r.h"
-# include "mempcpy.h"
-# include "stat-macros.h"
-# include "strdup.h"
+# define __getlogin_r(buf, len) getlogin_r (buf, len)
 # define __stat64(fname, buf)   stat (fname, buf)
+# define __fxstatat64(_, d, f, st, flag) fstatat (d, f, st, flag)
 # define struct_stat64          struct stat
-# define __stat(fname, buf)     stat (fname, buf)
 # define __alloca               alloca
 # define __readdir              readdir
-# define __readdir64            readdir64
 # define __glob_pattern_p       glob_pattern_p
+# define COMPILE_GLOB64
 #endif /* _LIBC */
 
 #include <fnmatch.h>
@@ -186,7 +166,7 @@
     D_INO_TO_RESULT (source)               \
   }
 
-#endif /* !defined _LIBC || !defined GLOB_ONLY_P */
+#endif /* !defined GLOB_ONLY_P */
 
 /* Call gl_readdir on STREAM.  This macro can be overridden to reduce
    type safety if an old interface version needs to be supported.  */
@@ -230,13 +210,49 @@
 # define attribute_hidden
 #endif
 
+#ifndef __attribute_noinline__
+# if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 1)
+#  define __attribute_noinline__ /* Ignore */
+#else
+#  define __attribute_noinline__ __attribute__ ((__noinline__))
+# endif
+#endif
+
+#if ! defined __builtin_expect && __GNUC__ < 3
+# define __builtin_expect(expr, expected) (expr)
+#endif
+
+#ifndef __glibc_unlikely
+# define __glibc_unlikely(expr) __builtin_expect (expr, 0)
+#endif
+
+#ifndef _LIBC
+/* The results of opendir() in this file are not used with dirfd and fchdir,
+   and we do not leak fds to any single-threaded code that could use stdio,
+   therefore save some unnecessary recursion in fchdir.c and opendir_safer.c.
+   FIXME - if the kernel ever adds support for multi-thread safety for
+   avoiding standard fds, then we should use opendir_safer.  */
+# ifdef GNULIB_defined_opendir
+#  undef opendir
+# endif
+# ifdef GNULIB_defined_closedir
+#  undef closedir
+# endif
+
+/* Just use malloc.  */
+# define __libc_use_alloca(n) false
+# define alloca_account(len, avar) ((void) (len), (void) (avar), (void *) 0)
+# define extend_alloca_account(buf, len, newlen, avar) \
+    ((void) (buf), (void) (len), (void) (newlen), (void) (avar), (void *) 0)
+#endif
+
 static int glob_in_dir (const char *pattern, const char *directory,
                         int flags, int (*errfunc) (const char *, int),
                         glob_t *pglob, size_t alloca_used);
 extern int __glob_pattern_type (const char *pattern, int quote)
     attribute_hidden;
 
-#if !defined _LIBC || !defined GLOB_ONLY_P
+#ifndef GLOB_ONLY_P
 static int prefix_array (const char *prefix, char **array, size_t n) __THROWNL;
 static int collated_compare (const void *, const void *) __THROWNL;
 
@@ -265,7 +281,7 @@
   return *cp != '\0' ? cp : NULL;
 }
 
-#endif /* !defined _LIBC || !defined GLOB_ONLY_P */
+#endif /* !defined GLOB_ONLY_P */
 
 /* Do glob searching for PATTERN, placing results in PGLOB.
    The bits defined above may be set in FLAGS.
@@ -292,9 +308,7 @@
   int malloc_dirname = 0;
   glob_t dirs;
   int retval = 0;
-#ifdef _LIBC
   size_t alloca_used = 0;
-#endif
 
   if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0)
     {
@@ -350,14 +364,12 @@
           size_t rest_len;
           char *onealt;
           size_t pattern_len = strlen (pattern) - 1;
-#ifdef _LIBC
           int alloca_onealt = __libc_use_alloca (alloca_used + pattern_len);
           if (alloca_onealt)
             onealt = alloca_account (pattern_len, alloca_used);
           else
-#endif
             {
-              onealt = (char *) malloc (pattern_len);
+              onealt = malloc (pattern_len);
               if (onealt == NULL)
                 {
                   if (!(flags & GLOB_APPEND))
@@ -377,11 +389,9 @@
           next = next_brace_sub (begin + 1, flags);
           if (next == NULL)
             {
-              /* It is an illegal expression.  */
+              /* It is an invalid expression.  */
             illegal_brace:
-#ifdef _LIBC
               if (__glibc_unlikely (!alloca_onealt))
-#endif
                 free (onealt);
               return glob (pattern, flags & ~GLOB_BRACE, errfunc, pglob);
             }
@@ -429,9 +439,7 @@
               /* If we got an error, return it.  */
               if (result && result != GLOB_NOMATCH)
                 {
-#ifdef _LIBC
                   if (__glibc_unlikely (!alloca_onealt))
-#endif
                     free (onealt);
                   if (!(flags & GLOB_APPEND))
                     {
@@ -450,9 +458,7 @@
               assert (next != NULL);
             }
 
-#ifdef _LIBC
           if (__glibc_unlikely (!alloca_onealt))
-#endif
             free (onealt);
 
           if (pglob->gl_pathc != firstc)
@@ -475,8 +481,7 @@
           if (pglob->gl_offs >= ~((size_t) 0) / sizeof (char *))
             return GLOB_NOSPACE;
 
-          pglob->gl_pathv = (char **) malloc ((pglob->gl_offs + 1)
-                                              * sizeof (char *));
+          pglob->gl_pathv = malloc ((pglob->gl_offs + 1) * sizeof (char *));
           if (pglob->gl_pathv == NULL)
             return GLOB_NOSPACE;
 
@@ -549,7 +554,7 @@
           char *drive_spec;
 
           ++dirlen;
-          drive_spec = (char *) __alloca (dirlen + 1);
+          drive_spec = __alloca (dirlen + 1);
           *((char *) mempcpy (drive_spec, pattern, dirlen)) = '\0';
           /* For now, disallow wildcards in the drive spec, to
              prevent infinite recursion in glob.  */
@@ -560,11 +565,9 @@
              from "d:/", since "d:" and "d:/" are not the same.*/
         }
 #endif
-#ifdef _LIBC
       if (__libc_use_alloca (alloca_used + dirlen + 1))
         newp = alloca_account (dirlen + 1, alloca_used);
       else
-#endif
         {
           newp = malloc (dirlen + 1);
           if (newp == NULL)
@@ -585,6 +588,7 @@
         /* "pattern/".  Expand "pattern", appending slashes.  */
         {
           int orig_flags = flags;
+          int val;
           if (!(flags & GLOB_NOESCAPE) && dirname[dirlen - 1] == '\\')
             {
               /* "pattern\\/".  Remove the final backslash if it hasn't
@@ -598,7 +602,7 @@
                   flags &= ~(GLOB_NOCHECK | GLOB_NOMAGIC);
                 }
             }
-          int val = glob (dirname, flags | GLOB_MARK, errfunc, pglob);
+          val = glob (dirname, flags | GLOB_MARK, errfunc, pglob);
           if (val == 0)
             pglob->gl_flags = ((pglob->gl_flags & ~GLOB_MARK)
                                | (flags & GLOB_MARK));
@@ -615,7 +619,6 @@
         }
     }
 
-#ifndef VMS
   if ((flags & (GLOB_TILDE|GLOB_TILDE_CHECK)) && dirname[0] == '~')
     {
       if (dirname[1] == '\0' || dirname[1] == '/'
@@ -630,20 +633,50 @@
             home_dir = "SYS:";
 # else
 #  ifdef WINDOWS32
+          /* Windows NT defines HOMEDRIVE and HOMEPATH.  But give preference
+             to HOME, because the user can change HOME.  */
           if (home_dir == NULL || home_dir[0] == '\0')
-            home_dir = "c:/users/default"; /* poor default */
+            {
+              const char *home_drive = getenv ("HOMEDRIVE");
+              const char *home_path = getenv ("HOMEPATH");
+
+              if (home_drive != NULL && home_path != NULL)
+                {
+                  size_t home_drive_len = strlen (home_drive);
+                  size_t home_path_len = strlen (home_path);
+                  char *mem = alloca (home_drive_len + home_path_len + 1);
+
+                  memcpy (mem, home_drive, home_drive_len);
+                  memcpy (mem + home_drive_len, home_path, home_path_len + 1);
+                  home_dir = mem;
+                }
+              else
+                home_dir = "c:/users/default"; /* poor default */
+            }
 #  else
           if (home_dir == NULL || home_dir[0] == '\0')
             {
               int success;
               char *name;
+              int malloc_name = 0;
               size_t buflen = GET_LOGIN_NAME_MAX () + 1;
 
               if (buflen == 0)
                 /* 'sysconf' does not support _SC_LOGIN_NAME_MAX.  Try
                    a moderate value.  */
                 buflen = 20;
-              name = alloca_account (buflen, alloca_used);
+              if (__libc_use_alloca (alloca_used + buflen))
+                name = alloca_account (buflen, alloca_used);
+              else
+                {
+                  name = malloc (buflen);
+                  if (name == NULL)
+                    {
+                      retval = GLOB_NOSPACE;
+                      goto out;
+                    }
+                  malloc_name = 1;
+                }
 
               success = __getlogin_r (name, buflen) == 0;
               if (success)
@@ -711,6 +744,8 @@
 #   else
                   p = getpwnam (name);
 #   endif
+                  if (__glibc_unlikely (malloc_name))
+                    free (name);
                   if (p != NULL)
                     {
                       if (!malloc_pwtmpbuf)
@@ -974,7 +1009,6 @@
         }
 # endif /* Not Amiga && not WINDOWS32.  */
     }
-#endif  /* Not VMS.  */
 
   /* Now test whether we looked for "~" or "~NAME".  In this case we
      can give the answer now.  */
@@ -1003,9 +1037,8 @@
               return GLOB_NOSPACE;
             }
 
-          new_gl_pathv
-            = (char **) realloc (pglob->gl_pathv,
-                                 (newcount + 1 + 1) * sizeof (char *));
+          new_gl_pathv = realloc (pglob->gl_pathv,
+                                  (newcount + 1 + 1) * sizeof (char *));
           if (new_gl_pathv == NULL)
             goto nospace;
           pglob->gl_pathv = new_gl_pathv;
@@ -1091,7 +1124,7 @@
         {
           size_t old_pathc;
 
-#ifdef  SHELL
+#ifdef SHELL
           {
             /* Make globbing interruptible in the bash shell. */
             extern int interrupt_state;
@@ -1155,14 +1188,13 @@
                   return GLOB_NOSPACE;
                 }
 
-              new_gl_pathv = (char **) realloc (pglob->gl_pathv,
-                                                (newcount + 2)
-                                                * sizeof (char *));
+              new_gl_pathv = realloc (pglob->gl_pathv,
+                                      (newcount + 2) * sizeof (char *));
               if (new_gl_pathv == NULL)
                 goto nospace2;
               pglob->gl_pathv = new_gl_pathv;
 
-              pglob->gl_pathv[newcount] = __strdup (pattern);
+              pglob->gl_pathv[newcount] = strdup (pattern);
               if (pglob->gl_pathv[newcount] == NULL)
                 {
                   globfree (&dirs);
@@ -1289,7 +1321,7 @@
 #endif
 
 
-#if !defined _LIBC || !defined GLOB_ONLY_P
+#ifndef GLOB_ONLY_P
 
 /* Free storage allocated in PGLOB by a previous 'glob' call.  */
 void
@@ -1313,8 +1345,8 @@
 static int
 collated_compare (const void *a, const void *b)
 {
-  const char *const s1 = *(const char *const * const) a;
-  const char *const s2 = *(const char *const * const) b;
+  char *const *ps1 = a; char *s1 = *ps1;
+  char *const *ps2 = b; char *s2 = *ps2;
 
   if (s1 == s2)
     return 0;
@@ -1364,7 +1396,7 @@
   for (i = 0; i < n; ++i)
     {
       size_t eltlen = strlen (array[i]) + 1;
-      char *new = (char *) malloc (dirlen + 1 + eltlen);
+      char *new = malloc (dirlen + 1 + eltlen);
       if (new == NULL)
         {
           while (i > 0)
@@ -1386,7 +1418,7 @@
 
 
 /* We must not compile this function twice.  */
-#if !defined _LIBC || !defined NO_GLOB_PATTERN_P
+#ifndef NO_GLOB_PATTERN_P
 int
 __glob_pattern_type (const char *pattern, int quote)
 {
@@ -1434,50 +1466,55 @@
 # endif
 #endif
 
-#endif /* !GLOB_ONLY_P */
-
 
 /* We put this in a separate function mainly to allow the memory
    allocated with alloca to be recycled.  */
-#if !defined _LIBC || !defined GLOB_ONLY_P
 static int
 __attribute_noinline__
 link_exists2_p (const char *dir, size_t dirlen, const char *fname,
                glob_t *pglob
-# ifndef _LIBC
+# if !defined _LIBC && !HAVE_FSTATAT
                 , int flags
 # endif
                 )
 {
   size_t fnamelen = strlen (fname);
-  char *fullname = (char *) __alloca (dirlen + 1 + fnamelen + 1);
+  char *fullname = __alloca (dirlen + 1 + fnamelen + 1);
   struct stat st;
-# ifndef _LIBC
-  struct_stat64 st64;
-# endif
 
   mempcpy (mempcpy (mempcpy (fullname, dir, dirlen), "/", 1),
            fname, fnamelen + 1);
 
-# ifdef _LIBC
-  return (*pglob->gl_stat) (fullname, &st) == 0;
-# else
-  return ((__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
-           ? (*pglob->gl_stat) (fullname, &st)
-           : __stat64 (fullname, &st64)) == 0);
+# if !defined _LIBC && !HAVE_FSTATAT
+  if (__builtin_expect ((flags & GLOB_ALTDIRFUNC) == 0, 1))
+    {
+      struct_stat64 st64;
+      return __stat64 (fullname, &st64) == 0;
+    }
 # endif
+  return (*pglob->gl_stat) (fullname, &st) == 0;
 }
-# ifdef _LIBC
-#  define link_exists_p(dfd, dirname, dirnamelen, fname, pglob, flags) \
-  (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)                              \
-   ? link_exists2_p (dirname, dirnamelen, fname, pglob)                       \
-   : ({ struct stat64 st64;                                                   \
-       __fxstatat64 (_STAT_VER, dfd, fname, &st64, 0) == 0; }))
+
+/* Return true if DIR/FNAME exists.  */
+static int
+link_exists_p (int dfd, const char *dir, size_t dirlen, const char *fname,
+               glob_t *pglob, int flags)
+{
+# if defined _LIBC || HAVE_FSTATAT
+  if (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0))
+    return link_exists2_p (dir, dirlen, fname, pglob);
+  else
+    {
+      /* dfd cannot be -1 here, because dirfd never returns -1 on
+         glibc, or on hosts that have fstatat.  */
+      struct_stat64 st64;
+      return __fxstatat64 (_STAT_VER, dfd, fname, &st64, 0) == 0;
+    }
 # else
-#  define link_exists_p(dfd, dirname, dirnamelen, fname, pglob, flags) \
-  link_exists2_p (dirname, dirnamelen, fname, pglob, flags)
+  return link_exists2_p (dir, dirlen, fname, pglob, flags);
 # endif
-#endif
+}
+#endif /* !defined GLOB_ONLY_P */
 
 
 /* Like 'glob', but PATTERN is a final pathname component,
@@ -1505,6 +1542,7 @@
   size_t cur = 0;
   int meta;
   int save;
+  int result;
 
   alloca_used += sizeof (init_names);
 
@@ -1568,10 +1606,8 @@
         }
       else
         {
-#ifdef _LIBC
           int dfd = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
                      ? -1 : dirfd ((DIR *) stream));
-#endif
           int fnm_flags = ((!(flags & GLOB_PERIOD) ? FNM_PERIOD : 0)
                            | ((flags & GLOB_NOESCAPE) ? FNM_NOESCAPE : 0)
 #if defined _AMIGA || defined VMS
@@ -1646,15 +1682,16 @@
     {
       size_t len = strlen (pattern);
       nfound = 1;
-      names->name[cur] = (char *) malloc (len + 1);
+      names->name[cur] = malloc (len + 1);
       if (names->name[cur] == NULL)
         goto memory_error;
       *((char *) mempcpy (names->name[cur++], pattern, len)) = '\0';
     }
 
-  int result = GLOB_NOMATCH;
+  result = GLOB_NOMATCH;
   if (nfound != 0)
     {
+      char **new_gl_pathv;
       result = 0;
 
       if (pglob->gl_pathc > UINTPTR_MAX - pglob->gl_offs
@@ -1664,18 +1701,19 @@
               > UINTPTR_MAX / sizeof (char *)))
         goto memory_error;
 
-      char **new_gl_pathv;
       new_gl_pathv
-        = (char **) realloc (pglob->gl_pathv,
-                             (pglob->gl_pathc + pglob->gl_offs + nfound + 1)
-                             * sizeof (char *));
+        = realloc (pglob->gl_pathv,
+                   (pglob->gl_pathc + pglob->gl_offs + nfound + 1)
+                   * sizeof (char *));
+
       if (new_gl_pathv == NULL)
         {
         memory_error:
           while (1)
             {
               struct globnames *old = names;
-              for (size_t i = 0; i < cur; ++i)
+              size_t i;
+              for (i = 0; i < cur; ++i)
                 free (names->name[i]);
               names = names->next;
               /* NB: we will not leak memory here if we exit without
@@ -1700,7 +1738,8 @@
           while (1)
             {
               struct globnames *old = names;
-              for (size_t i = 0; i < cur; ++i)
+              size_t i;
+              for (i = 0; i < cur; ++i)
                 new_gl_pathv[pglob->gl_offs + pglob->gl_pathc++]
                   = names->name[i];
               names = names->next;