[PATCH] Fix readdir_r with long file names

[PATCH] Fix readdir_r with long file names

[Florian Weimer]

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

This patch changes readdir_r to return ENAMETOOLONG if the kernel 
returns a file name longer than NAME_MAX characters, after the end of 
the directory has been reached (so that the directory contents is not 
truncated).  It also makes the padding compensation code 
architecture-agnostic and enables it everywhere.

I have tested this on ppc-redhat-linux (which didn't have the padding 
compensation code) and ppc64-redhat-linux (which did).  Adding a test 
case is difficult because typical file systems enforce the NAME_MAX limit.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: glibc-readdir_r.patch --]
[-- Type: text/x-patch, Size: 4272 bytes --]

2013-05-14  Florian Weimer  <fweimer@redhat.com>

	[BZ #14699]
	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
	member.
	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
	member.
	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
	conditional.
	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
	GETDENTS_64BIT_ALIGNED.
	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.

diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
index a7a074d..8e8570d 100644
--- a/sysdeps/posix/dirstream.h
+++ b/sysdeps/posix/dirstream.h
@@ -39,6 +39,8 @@ struct __dirstream
 
     off_t filepos;		/* Position of next entry to read.  */
 
+    int errcode;		/* Delayed error code.  */
+
     /* Directory block.  */
     char data[0] __attribute__ ((aligned (__alignof__ (void*))));
   };
diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index ddfc3a7..fc05b0f 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
   dirp->size = 0;
   dirp->offset = 0;
   dirp->filepos = 0;
+  dirp->errcode = 0;
 
   return dirp;
 }
diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
index b5a8e2e..25db350 100644
--- a/sysdeps/posix/readdir_r.c
+++ b/sysdeps/posix/readdir_r.c
@@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
   DIRENT_TYPE *dp;
   size_t reclen;
   const int saved_errno = errno;
+  int ret;
 
   __libc_lock_lock (dirp->lock);
 
@@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
 		  bytes = 0;
 		  __set_errno (saved_errno);
 		}
+	      if (bytes < 0)
+		dirp->errcode = errno;
 
 	      dp = NULL;
-	      /* Reclen != 0 signals that an error occurred.  */
-	      reclen = bytes != 0;
 	      break;
 	    }
 	  dirp->size = (size_t) bytes;
@@ -106,29 +107,47 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
       dirp->filepos += reclen;
 #endif
 
-      /* Skip deleted files.  */
+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+	{
+	  /* The record is very long.  It could still fit into the
+	     caller-supplied buffer if we can skip padding at the
+	     end.  */
+	  size_t namelen = strlen(dp->d_name);
+	  if (namelen <= NAME_MAX)
+	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+	  else
+	    {
+	      /* The name is too long.  Ignore this file.  */
+	      dirp->errcode = ENAMETOOLONG;
+	      dp->d_ino = 0;
+	      continue;
+	    }
+	}
+#endif
+
+      /* Skip deleted and ignored files.  */
     }
   while (dp->d_ino == 0);
 
   if (dp != NULL)
     {
-#ifdef GETDENTS_64BIT_ALIGNED
-      /* The d_reclen value might include padding which is not part of
-	 the DIRENT_TYPE data structure.  */
-      reclen = MIN (reclen,
-		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
-#endif
       *result = memcpy (entry, dp, reclen);
-#ifdef GETDENTS_64BIT_ALIGNED
+#ifdef _DIRENT_HAVE_D_RECLEN
       entry->d_reclen = reclen;
 #endif
     }
   else
     *result = NULL;
 
+  if (dp != 0)
+    ret = 0;
+  else
+    ret = dirp->errcode;
+
   __libc_lock_unlock (dirp->lock);
 
-  return dp != NULL ? 0 : reclen ? errno : 0;
+  return ret;
 }
 
 #ifdef __READDIR_R_ALIAS
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
index 8ebbcfd..a7d114e 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
@@ -18,7 +18,6 @@
 #define __READDIR_R __readdir64_r
 #define __GETDENTS __getdents64
 #define DIRENT_TYPE struct dirent64
-#define GETDENTS_64BIT_ALIGNED 1
 
 #include <sysdeps/posix/readdir_r.c>
 
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
index 5ed8e95..290f2c8 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
+++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
@@ -1,5 +1,4 @@
 #define readdir64_r __no_readdir64_r_decl
-#define GETDENTS_64BIT_ALIGNED 1
 #include <sysdeps/posix/readdir_r.c>
 #undef readdir64_r
 weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Siddhesh Poyarekar]

On Tue, May 14, 2013 at 01:32:23PM +0200, Florian Weimer wrote:
> This patch changes readdir_r to return ENAMETOOLONG if the kernel
> returns a file name longer than NAME_MAX characters, after the end of
> the directory has been reached (so that the directory contents is not
> truncated).  It also makes the padding compensation code
> architecture-agnostic and enables it everywhere.

The specification for readdir/readdir_r does not mention ENAMETOOLONG
as a possible error return[1], so this should at least be mentioned in
the glibc manual, if not also in the man page.  So a manual patch is
needed on top of this.

Also, I don't think returning an error at the end of the traversal is
very useful.  If we're adding an extension, we might as well go the
whole way and define something more useful: if the errno returned is
ENAMETOOLONG, then the directory can still be traversed and subsequent
readdir calls are still valid.  At least one knows the position at
which the error occurred.  This obviously makes the add-on
documentation even more important to highlight the difference.

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/readdir_r.html

> diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
> index a7a074d..8e8570d 100644
> --- a/sysdeps/posix/dirstream.h
> +++ b/sysdeps/posix/dirstream.h
> @@ -39,6 +39,8 @@ struct __dirstream
>  
>      off_t filepos;		/* Position of next entry to read.  */
>  
> +    int errcode;		/* Delayed error code.  */
> +
>      /* Directory block.  */
>      char data[0] __attribute__ ((aligned (__alignof__ (void*))));
>    };
> diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
> index ddfc3a7..fc05b0f 100644
> --- a/sysdeps/posix/opendir.c
> +++ b/sysdeps/posix/opendir.c
> @@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
>    dirp->size = 0;
>    dirp->offset = 0;
>    dirp->filepos = 0;
> +  dirp->errcode = 0;
>  
>    return dirp;
>  }
> diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
> index b5a8e2e..25db350 100644
> --- a/sysdeps/posix/readdir_r.c
> +++ b/sysdeps/posix/readdir_r.c
> @@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>    DIRENT_TYPE *dp;
>    size_t reclen;
>    const int saved_errno = errno;
> +  int ret;
>  
>    __libc_lock_lock (dirp->lock);
>  
> @@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>  		  bytes = 0;
>  		  __set_errno (saved_errno);
>  		}
> +	      if (bytes < 0)
> +		dirp->errcode = errno;
>  
>  	      dp = NULL;
> -	      /* Reclen != 0 signals that an error occurred.  */
> -	      reclen = bytes != 0;
>  	      break;
>  	    }
>  	  dirp->size = (size_t) bytes;
> @@ -106,29 +107,47 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>        dirp->filepos += reclen;
>  #endif
>  
> -      /* Skip deleted files.  */
> +#ifdef NAME_MAX
> +      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
> +	{
> +	  /* The record is very long.  It could still fit into the
> +	     caller-supplied buffer if we can skip padding at the
> +	     end.  */
> +	  size_t namelen = strlen(dp->d_name);
> +	  if (namelen <= NAME_MAX)
> +	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
> +	  else
> +	    {
> +	      /* The name is too long.  Ignore this file.  */
> +	      dirp->errcode = ENAMETOOLONG;
> +	      dp->d_ino = 0;

Why do you need to set d_ino?  Doesn't seem like it is needed.

> +	      continue;
> +	    }
> +	}
> +#endif
> +
> +      /* Skip deleted and ignored files.  */
>      }
>    while (dp->d_ino == 0);
>  
>    if (dp != NULL)
>      {
> -#ifdef GETDENTS_64BIT_ALIGNED
> -      /* The d_reclen value might include padding which is not part of
> -	 the DIRENT_TYPE data structure.  */
> -      reclen = MIN (reclen,
> -		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
> -#endif
>        *result = memcpy (entry, dp, reclen);
> -#ifdef GETDENTS_64BIT_ALIGNED
> +#ifdef _DIRENT_HAVE_D_RECLEN
>        entry->d_reclen = reclen;
>  #endif
>      }
>    else
>      *result = NULL;
>  
> +  if (dp != 0)
> +    ret = 0;
> +  else
> +    ret = dirp->errcode;
> +
>    __libc_lock_unlock (dirp->lock);

This could be consolidated with the if/else above it, so that it looks
like:

  if (dp != NULL)
    {
      *result = memcpy (entry, dp, reclen);
#ifdef _DIRENT_HAVE_D_RECLEN
      entry->d_reclen = reclen;
#endif
      ret = 0;
    }
  else
    {
      *result = NULL;
      ret = dirp->errorcode;
    }

Finally, you need to change rewinddir to reset errorcode.

Siddhesh

> -  return dp != NULL ? 0 : reclen ? errno : 0;
> +  return ret;
>  }
>  
>  #ifdef __READDIR_R_ALIAS
> diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> index 8ebbcfd..a7d114e 100644
> --- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> +++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> @@ -18,7 +18,6 @@
>  #define __READDIR_R __readdir64_r
>  #define __GETDENTS __getdents64
>  #define DIRENT_TYPE struct dirent64
> -#define GETDENTS_64BIT_ALIGNED 1
>  
>  #include <sysdeps/posix/readdir_r.c>
>  
> diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> index 5ed8e95..290f2c8 100644
> --- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> +++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> @@ -1,5 +1,4 @@
>  #define readdir64_r __no_readdir64_r_decl
> -#define GETDENTS_64BIT_ALIGNED 1
>  #include <sysdeps/posix/readdir_r.c>
>  #undef readdir64_r
>  weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Siddhesh Poyarekar]

On Thu, May 16, 2013 at 04:31:36PM +0530, Siddhesh Poyarekar wrote:
> > +	      dirp->errcode = ENAMETOOLONG;
> > +	      dp->d_ino = 0;
> 
> Why do you need to set d_ino?  Doesn't seem like it is needed.
> 

Ugh, please ignore this - I don't know what I was thinking when I
wrote that.

Siddhesh

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

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

On 05/16/2013 01:01 PM, Siddhesh Poyarekar wrote:
> On Tue, May 14, 2013 at 01:32:23PM +0200, Florian Weimer wrote:
>> This patch changes readdir_r to return ENAMETOOLONG if the kernel
>> returns a file name longer than NAME_MAX characters, after the end of
>> the directory has been reached (so that the directory contents is not
>> truncated).  It also makes the padding compensation code
>> architecture-agnostic and enables it everywhere.
>
> The specification for readdir/readdir_r does not mention ENAMETOOLONG
> as a possible error return[1], so this should at least be mentioned in
> the glibc manual, if not also in the man page.  So a manual patch is
> needed on top of this.

Thanks for your comments.

We could use EOVERFLOW instead.  But ENAMETOOLONG is more informative.

I updated the documentation in the new version of the patch, and made it 
more clear that readdir is the recommended interface.

> Also, I don't think returning an error at the end of the traversal is
> very useful.  If we're adding an extension, we might as well go the
> whole way and define something more useful: if the errno returned is
> ENAMETOOLONG, then the directory can still be traversed and subsequent
> readdir calls are still valid.  At least one knows the position at
> which the error occurred.  This obviously makes the add-on
> documentation even more important to highlight the difference.

I think you really have to stop on the first error, otherwise you can 
end up with an endless loop (e.g., if the kernel reports a very 
low-level error such as EIO/ENXIO).

> This could be consolidated with the if/else above it, so that it looks
> like:
>
>    if (dp != NULL)
>      {
>        *result = memcpy (entry, dp, reclen);
> #ifdef _DIRENT_HAVE_D_RECLEN
>        entry->d_reclen = reclen;
> #endif
>        ret = 0;
>      }
>    else
>      {
>        *result = NULL;
>        ret = dirp->errorcode;
>      }

Done.

> Finally, you need to change rewinddir to reset errorcode.

Good point.

Please have a look at the updated version.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: glibc-readdir_r.patch --]
[-- Type: text/x-patch, Size: 8871 bytes --]

2013-05-16  Florian Weimer  <fweimer@redhat.com>

	[BZ #14699]
	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
	member.
	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
	member.
	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
	conditional.
	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
	GETDENTS_64BIT_ALIGNED.
	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
	* manual/filesys.texi (Reading/Closing Directory): Document
	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
	strongly.

diff --git a/manual/filesys.texi b/manual/filesys.texi
index 1df9cf2..c4c7896 100644
--- a/manual/filesys.texi
+++ b/manual/filesys.texi
@@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
 @comment POSIX.1
 @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
 This function reads the next entry from the directory.  It normally
-returns a pointer to a structure containing information about the file.
-This structure is statically allocated and can be rewritten by a
-subsequent call.
+returns a pointer to a structure containing information about the
+file.  This structure is associated with the @var{dirstream} handle
+and can be rewritten by a subsequent call.
 
 @strong{Portability Note:} On some systems @code{readdir} may not
 return entries for @file{.} and @file{..}, even though these are always
@@ -461,19 +461,26 @@ conditions are defined for this function:
 The @var{dirstream} argument is not valid.
 @end table
 
-@code{readdir} is not thread safe.  Multiple threads using
-@code{readdir} on the same @var{dirstream} may overwrite the return
-value.  Use @code{readdir_r} when this is critical.
+To tell the regular end-of-directory condition and errors apart, you
+need to set @code{errno} to zero directly before calling
+@code{readdir}.  To avoid entering an infinite loop, you should stop
+reading from the directory on the first error.
+
+@code{readdir} is thread safe as long as only a single thread accesses
+the @var{dirstream} handle without synchronization.  The alternative
+@code{readdir_r} function has significant portability issues.
+Therefore, you should always use @code{readdir} and external locking.
 @end deftypefun
 
 @comment dirent.h
 @comment GNU
 @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
-This function is the reentrant version of @code{readdir}.  Like
-@code{readdir} it returns the next entry from the directory.  But to
-prevent conflicts between simultaneously running threads the result is
-not stored in statically allocated memory.  Instead the argument
-@var{entry} points to a place to store the result.
+This function is version of @code{readdir} which performs internal
+locking.  Like @code{readdir} it returns the next entry from the
+directory.  But to prevent conflicts between simultaneously running
+threads, the result is not stored inside the @var{dirstream} handle.
+Instead the argument @var{entry} points to a place to store the
+result.
 
 Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
 to @var{entry}.  If there are no more entries in the directory or an
@@ -481,14 +488,14 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
 null pointer and returns a nonzero error code, also stored in
 @code{errno}, as described for @code{readdir}.
 
-@strong{Portability Note:} On some systems @code{readdir_r} may not
-return a NUL terminated string for the file name, even when there is no
-@code{d_reclen} field in @code{struct dirent} and the file
-name is the maximum allowed size.  Modern systems all have the
-@code{d_reclen} field, and on old systems multi-threading is not
-critical.  In any case there is no such problem with the @code{readdir}
-function, so that even on systems without the @code{d_reclen} member one
-could use multiple threads by using external locking.
+@strong{Portability Note:} On some systems, @code{readdir_r} cannot
+read directory entries with very long names.  If such a name is
+encountered, @code{readdir_r} returns with an error code of
+@code{ENAMETOOLONG} after the final directory entry has been read.  On
+other systems, @code{readdir_r} can return successfully, but the
+@code{d_name} member is not NUL-terminated or is otherwise truncated.
+Therefore, you should always prefer @code{readdir} (with external
+locking if necessary) over @code{readdir_r}.
 
 It is also important to look at the definition of the @code{struct
 dirent} type.  Simply passing a pointer to an object of this type for
diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
index a7a074d..8e8570d 100644
--- a/sysdeps/posix/dirstream.h
+++ b/sysdeps/posix/dirstream.h
@@ -39,6 +39,8 @@ struct __dirstream
 
     off_t filepos;		/* Position of next entry to read.  */
 
+    int errcode;		/* Delayed error code.  */
+
     /* Directory block.  */
     char data[0] __attribute__ ((aligned (__alignof__ (void*))));
   };
diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index ddfc3a7..fc05b0f 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
   dirp->size = 0;
   dirp->offset = 0;
   dirp->filepos = 0;
+  dirp->errcode = 0;
 
   return dirp;
 }
diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
index b5a8e2e..a780c3f 100644
--- a/sysdeps/posix/readdir_r.c
+++ b/sysdeps/posix/readdir_r.c
@@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
   DIRENT_TYPE *dp;
   size_t reclen;
   const int saved_errno = errno;
+  int ret;
 
   __libc_lock_lock (dirp->lock);
 
@@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
 		  bytes = 0;
 		  __set_errno (saved_errno);
 		}
+	      if (bytes < 0)
+		dirp->errcode = errno;
 
 	      dp = NULL;
-	      /* Reclen != 0 signals that an error occurred.  */
-	      reclen = bytes != 0;
 	      break;
 	    }
 	  dirp->size = (size_t) bytes;
@@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
       dirp->filepos += reclen;
 #endif
 
-      /* Skip deleted files.  */
+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+	{
+	  /* The record is very long.  It could still fit into the
+	     caller-supplied buffer if we can skip padding at the
+	     end.  */
+	  size_t namelen = strlen(dp->d_name);
+	  if (namelen <= NAME_MAX)
+	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+	  else
+	    {
+	      /* The name is too long.  Ignore this file.  */
+	      dirp->errcode = ENAMETOOLONG;
+	      dp->d_ino = 0;
+	      continue;
+	    }
+	}
+#endif
+
+      /* Skip deleted and ignored files.  */
     }
   while (dp->d_ino == 0);
 
   if (dp != NULL)
     {
-#ifdef GETDENTS_64BIT_ALIGNED
-      /* The d_reclen value might include padding which is not part of
-	 the DIRENT_TYPE data structure.  */
-      reclen = MIN (reclen,
-		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
-#endif
       *result = memcpy (entry, dp, reclen);
-#ifdef GETDENTS_64BIT_ALIGNED
+#ifdef _DIRENT_HAVE_D_RECLEN
       entry->d_reclen = reclen;
 #endif
+      ret = 0;
     }
   else
-    *result = NULL;
+    {
+      *result = NULL;
+      ret = dirp->errcode;
+    }
 
   __libc_lock_unlock (dirp->lock);
 
-  return dp != NULL ? 0 : reclen ? errno : 0;
+  return ret;
 }
 
 #ifdef __READDIR_R_ALIAS
diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
index 2935a8e..d4991ad 100644
--- a/sysdeps/posix/rewinddir.c
+++ b/sysdeps/posix/rewinddir.c
@@ -33,6 +33,7 @@ rewinddir (dirp)
   dirp->filepos = 0;
   dirp->offset = 0;
   dirp->size = 0;
+  dirp->errcode = 0;
 #ifndef NOT_IN_libc
   __libc_lock_unlock (dirp->lock);
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
index 8ebbcfd..a7d114e 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
@@ -18,7 +18,6 @@
 #define __READDIR_R __readdir64_r
 #define __GETDENTS __getdents64
 #define DIRENT_TYPE struct dirent64
-#define GETDENTS_64BIT_ALIGNED 1
 
 #include <sysdeps/posix/readdir_r.c>
 
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
index 5ed8e95..290f2c8 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
+++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
@@ -1,5 +1,4 @@
 #define readdir64_r __no_readdir64_r_decl
-#define GETDENTS_64BIT_ALIGNED 1
 #include <sysdeps/posix/readdir_r.c>
 #undef readdir64_r
 weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Andreas Jaeger]

On 05/16/2013 02:15 PM, Florian Weimer wrote:
> On 05/16/2013 01:01 PM, Siddhesh Poyarekar wrote:
>> On Tue, May 14, 2013 at 01:32:23PM +0200, Florian Weimer wrote:
>>> This patch changes readdir_r to return ENAMETOOLONG if the kernel
>>> returns a file name longer than NAME_MAX characters, after the end of
>>> the directory has been reached (so that the directory contents is not
>>> truncated).  It also makes the padding compensation code
>>> architecture-agnostic and enables it everywhere.
>>
>> The specification for readdir/readdir_r does not mention ENAMETOOLONG
>> as a possible error return[1], so this should at least be mentioned in
>> the glibc manual, if not also in the man page.  So a manual patch is
>> needed on top of this.
>
> Thanks for your comments.
>
> We could use EOVERFLOW instead.  But ENAMETOOLONG is more informative.

I think the description for EOVERFLOW:
"One of the values in the structure to be returned cannot be represented 
correctly." fits this case - so, let's use that one to follow POSIX,

Andreas
-- 
  Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
   SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
    GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
     GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

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

On 05/16/2013 02:18 PM, Andreas Jaeger wrote:
> On 05/16/2013 02:15 PM, Florian Weimer wrote:
>> On 05/16/2013 01:01 PM, Siddhesh Poyarekar wrote:
>>> On Tue, May 14, 2013 at 01:32:23PM +0200, Florian Weimer wrote:
>>>> This patch changes readdir_r to return ENAMETOOLONG if the kernel
>>>> returns a file name longer than NAME_MAX characters, after the end of
>>>> the directory has been reached (so that the directory contents is not
>>>> truncated).  It also makes the padding compensation code
>>>> architecture-agnostic and enables it everywhere.
>>>
>>> The specification for readdir/readdir_r does not mention ENAMETOOLONG
>>> as a possible error return[1], so this should at least be mentioned in
>>> the glibc manual, if not also in the man page.  So a manual patch is
>>> needed on top of this.
>>
>> Thanks for your comments.
>>
>> We could use EOVERFLOW instead.  But ENAMETOOLONG is more informative.
>
> I think the description for EOVERFLOW:
> "One of the values in the structure to be returned cannot be represented
> correctly." fits this case - so, let's use that one to follow POSIX,

Okay, here's the most recent version.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: glibc-readdir_r.patch --]
[-- Type: text/x-patch, Size: 8866 bytes --]

2013-05-16  Florian Weimer  <fweimer@redhat.com>

	[BZ #14699]
	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
	member.
	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
	member.
	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
	conditional.
	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
	GETDENTS_64BIT_ALIGNED.
	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
	* manual/filesys.texi (Reading/Closing Directory): Document the
	EOVERFLOW return value of readdir_r.  Recommend readdir more
	strongly.

diff --git a/manual/filesys.texi b/manual/filesys.texi
index 1df9cf2..b227030 100644
--- a/manual/filesys.texi
+++ b/manual/filesys.texi
@@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
 @comment POSIX.1
 @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
 This function reads the next entry from the directory.  It normally
-returns a pointer to a structure containing information about the file.
-This structure is statically allocated and can be rewritten by a
-subsequent call.
+returns a pointer to a structure containing information about the
+file.  This structure is associated with the @var{dirstream} handle
+and can be rewritten by a subsequent call.
 
 @strong{Portability Note:} On some systems @code{readdir} may not
 return entries for @file{.} and @file{..}, even though these are always
@@ -461,19 +461,26 @@ conditions are defined for this function:
 The @var{dirstream} argument is not valid.
 @end table
 
-@code{readdir} is not thread safe.  Multiple threads using
-@code{readdir} on the same @var{dirstream} may overwrite the return
-value.  Use @code{readdir_r} when this is critical.
+To tell the regular end-of-directory condition and errors apart, you
+need to set @code{errno} to zero directly before calling
+@code{readdir}.  To avoid entering an infinite loop, you should stop
+reading from the directory on the first error.
+
+@code{readdir} is thread safe as long as only a single thread accesses
+the @var{dirstream} handle without synchronization.  The alternative
+@code{readdir_r} function has significant portability issues.
+Therefore, you should always use @code{readdir} and external locking.
 @end deftypefun
 
 @comment dirent.h
 @comment GNU
 @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
-This function is the reentrant version of @code{readdir}.  Like
-@code{readdir} it returns the next entry from the directory.  But to
-prevent conflicts between simultaneously running threads the result is
-not stored in statically allocated memory.  Instead the argument
-@var{entry} points to a place to store the result.
+This function is version of @code{readdir} which performs internal
+locking.  Like @code{readdir} it returns the next entry from the
+directory.  But to prevent conflicts between simultaneously running
+threads, the result is not stored inside the @var{dirstream} handle.
+Instead the argument @var{entry} points to a place to store the
+result.
 
 Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
 to @var{entry}.  If there are no more entries in the directory or an
@@ -481,14 +488,14 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
 null pointer and returns a nonzero error code, also stored in
 @code{errno}, as described for @code{readdir}.
 
-@strong{Portability Note:} On some systems @code{readdir_r} may not
-return a NUL terminated string for the file name, even when there is no
-@code{d_reclen} field in @code{struct dirent} and the file
-name is the maximum allowed size.  Modern systems all have the
-@code{d_reclen} field, and on old systems multi-threading is not
-critical.  In any case there is no such problem with the @code{readdir}
-function, so that even on systems without the @code{d_reclen} member one
-could use multiple threads by using external locking.
+@strong{Portability Note:} On some systems, @code{readdir_r} cannot
+read directory entries with very long names.  If such a name is
+encountered, @code{readdir_r} returns with an error code of
+@code{EOVERFLOW} after the final directory entry has been read.  On
+other systems, @code{readdir_r} can return successfully, but the
+@code{d_name} member is not NUL-terminated or is otherwise truncated.
+Therefore, you should always prefer @code{readdir} (with external
+locking if necessary) over @code{readdir_r}.
 
 It is also important to look at the definition of the @code{struct
 dirent} type.  Simply passing a pointer to an object of this type for
diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
index a7a074d..8e8570d 100644
--- a/sysdeps/posix/dirstream.h
+++ b/sysdeps/posix/dirstream.h
@@ -39,6 +39,8 @@ struct __dirstream
 
     off_t filepos;		/* Position of next entry to read.  */
 
+    int errcode;		/* Delayed error code.  */
+
     /* Directory block.  */
     char data[0] __attribute__ ((aligned (__alignof__ (void*))));
   };
diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index ddfc3a7..fc05b0f 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
   dirp->size = 0;
   dirp->offset = 0;
   dirp->filepos = 0;
+  dirp->errcode = 0;
 
   return dirp;
 }
diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
index b5a8e2e..2fb73cb 100644
--- a/sysdeps/posix/readdir_r.c
+++ b/sysdeps/posix/readdir_r.c
@@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
   DIRENT_TYPE *dp;
   size_t reclen;
   const int saved_errno = errno;
+  int ret;
 
   __libc_lock_lock (dirp->lock);
 
@@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
 		  bytes = 0;
 		  __set_errno (saved_errno);
 		}
+	      if (bytes < 0)
+		dirp->errcode = errno;
 
 	      dp = NULL;
-	      /* Reclen != 0 signals that an error occurred.  */
-	      reclen = bytes != 0;
 	      break;
 	    }
 	  dirp->size = (size_t) bytes;
@@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
       dirp->filepos += reclen;
 #endif
 
-      /* Skip deleted files.  */
+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+	{
+	  /* The record is very long.  It could still fit into the
+	     caller-supplied buffer if we can skip padding at the
+	     end.  */
+	  size_t namelen = strlen(dp->d_name);
+	  if (namelen <= NAME_MAX)
+	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+	  else
+	    {
+	      /* The name is too long.  Ignore this file.  */
+	      dirp->errcode = EOVERFLOW;
+	      dp->d_ino = 0;
+	      continue;
+	    }
+	}
+#endif
+
+      /* Skip deleted and ignored files.  */
     }
   while (dp->d_ino == 0);
 
   if (dp != NULL)
     {
-#ifdef GETDENTS_64BIT_ALIGNED
-      /* The d_reclen value might include padding which is not part of
-	 the DIRENT_TYPE data structure.  */
-      reclen = MIN (reclen,
-		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
-#endif
       *result = memcpy (entry, dp, reclen);
-#ifdef GETDENTS_64BIT_ALIGNED
+#ifdef _DIRENT_HAVE_D_RECLEN
       entry->d_reclen = reclen;
 #endif
+      ret = 0;
     }
   else
-    *result = NULL;
+    {
+      *result = NULL;
+      ret = dirp->errcode;
+    }
 
   __libc_lock_unlock (dirp->lock);
 
-  return dp != NULL ? 0 : reclen ? errno : 0;
+  return ret;
 }
 
 #ifdef __READDIR_R_ALIAS
diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
index 2935a8e..d4991ad 100644
--- a/sysdeps/posix/rewinddir.c
+++ b/sysdeps/posix/rewinddir.c
@@ -33,6 +33,7 @@ rewinddir (dirp)
   dirp->filepos = 0;
   dirp->offset = 0;
   dirp->size = 0;
+  dirp->errcode = 0;
 #ifndef NOT_IN_libc
   __libc_lock_unlock (dirp->lock);
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
index 8ebbcfd..a7d114e 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
@@ -18,7 +18,6 @@
 #define __READDIR_R __readdir64_r
 #define __GETDENTS __getdents64
 #define DIRENT_TYPE struct dirent64
-#define GETDENTS_64BIT_ALIGNED 1
 
 #include <sysdeps/posix/readdir_r.c>
 
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
index 5ed8e95..290f2c8 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
+++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
@@ -1,5 +1,4 @@
 #define readdir64_r __no_readdir64_r_decl
-#define GETDENTS_64BIT_ALIGNED 1
 #include <sysdeps/posix/readdir_r.c>
 #undef readdir64_r
 weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Andreas Schwab]

Andreas Jaeger <aj@suse.com> writes:

> On 05/16/2013 02:15 PM, Florian Weimer wrote:
>> On 05/16/2013 01:01 PM, Siddhesh Poyarekar wrote:
>>> On Tue, May 14, 2013 at 01:32:23PM +0200, Florian Weimer wrote:
>>>> This patch changes readdir_r to return ENAMETOOLONG if the kernel
>>>> returns a file name longer than NAME_MAX characters, after the end of
>>>> the directory has been reached (so that the directory contents is not
>>>> truncated).  It also makes the padding compensation code
>>>> architecture-agnostic and enables it everywhere.
>>>
>>> The specification for readdir/readdir_r does not mention ENAMETOOLONG
>>> as a possible error return[1], so this should at least be mentioned in
>>> the glibc manual, if not also in the man page.  So a manual patch is
>>> needed on top of this.
>>
>> Thanks for your comments.
>>
>> We could use EOVERFLOW instead.  But ENAMETOOLONG is more informative.
>
> I think the description for EOVERFLOW:
> "One of the values in the structure to be returned cannot be represented
> correctly." fits this case - so, let's use that one to follow POSIX,

No, that doesn't fit.  It's a buffer overflow, so ERANGE would be
better.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix readdir_r with long file names

[Siddhesh Poyarekar]

On Thu, May 16, 2013 at 02:15:42PM +0200, Florian Weimer wrote:
> >Also, I don't think returning an error at the end of the traversal is
> >very useful.  If we're adding an extension, we might as well go the
> >whole way and define something more useful: if the errno returned is
> >ENAMETOOLONG, then the directory can still be traversed and subsequent
> >readdir calls are still valid.  At least one knows the position at
> >which the error occurred.  This obviously makes the add-on
> >documentation even more important to highlight the difference.
> 
> I think you really have to stop on the first error, otherwise you can
> end up with an endless loop (e.g., if the kernel reports a very
> low-level error such as EIO/ENXIO).

I did not suggest modifying the general error handling mechanism.
It's only in the case of the name buffer overflow that we ought to set
offsets to skip the entry and move ahead.  That won't result in an
infinite loop.  The recommendation in the manual for this would be
that if you encounter $THE_ERRNO_WE_FINALLY_AGREE_ON, it is safe to
proceed to the next entry since that errno only means that the current
entry name was too long; there was no system error that should
necessitate aborting the directory traversal.

Siddhesh

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 05/16/2013 02:50 PM, Siddhesh Poyarekar wrote:
> I did not suggest modifying the general error handling mechanism.
> It's only in the case of the name buffer overflow that we ought to set
> offsets to skip the entry and move ahead.  That won't result in an
> infinite loop.  The recommendation in the manual for this would be
> that if you encounter $THE_ERRNO_WE_FINALLY_AGREE_ON, it is safe to
> proceed to the next entry since that errno only means that the current
> entry name was too long; there was no system error that should
> necessitate aborting the directory traversal.

This will break existing applications which currently treat the garbled 
d_name as a concurrently deleted directory entry and move on.  If we 
change the behavior as you suggest, the directory list will be truncated 
at the first long name until the application is updated.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[Siddhesh Poyarekar]

On Thu, May 16, 2013 at 02:52:39PM +0200, Florian Weimer wrote:
> On 05/16/2013 02:50 PM, Siddhesh Poyarekar wrote:
> >I did not suggest modifying the general error handling mechanism.
> >It's only in the case of the name buffer overflow that we ought to set
> >offsets to skip the entry and move ahead.  That won't result in an
> >infinite loop.  The recommendation in the manual for this would be
> >that if you encounter $THE_ERRNO_WE_FINALLY_AGREE_ON, it is safe to
> >proceed to the next entry since that errno only means that the current
> >entry name was too long; there was no system error that should
> >necessitate aborting the directory traversal.
> 
> This will break existing applications which currently treat the
> garbled d_name as a concurrently deleted directory entry and move on.
> If we change the behavior as you suggest, the directory list will be
> truncated at the first long name until the application is updated.

Hrm, right.  I still don't like the idea of putting the errno at the
end, but I guess that is the best available option.

The reposted patch looks fine to me, but I'd like a native English
speaker to review the manual bits.  I've added Michael Kerrisk to cc
in case he wants to update the man page for this.  Of course, the
errno still needs to be agreed upon - I like the original NAMETOOLONG
since it conveys perfectly what the failure really is, but I don't
mind if it's something else that makes enough sense.

Siddhesh

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 05/16/2013 03:09 PM, Siddhesh Poyarekar wrote:

> The reposted patch looks fine to me, but I'd like a native English
> speaker to review the manual bits.

I agree.  Any takers?

> I like the original NAMETOOLONG
> since it conveys perfectly what the failure really is, but I don't
> mind if it's something else that makes enough sense.

I would prefer ENAMETOOLONG as well.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

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

On 05/21/2013 01:22 PM, Florian Weimer wrote:
> On 05/16/2013 03:09 PM, Siddhesh Poyarekar wrote:
>
>> The reposted patch looks fine to me, but I'd like a native English
>> speaker to review the manual bits.
>
> I agree.  Any takers?
>
>> I like the original NAMETOOLONG
>> since it conveys perfectly what the failure really is, but I don't
>> mind if it's something else that makes enough sense.
>
> I would prefer ENAMETOOLONG as well.

This is what I would like to commit.

I switched back to ENAMETOOLONG and added additional notes to 
manual/conf.texi.  Both realpath and get_current_dir_name can exceed 
PATH_MAX, so I listed it as well.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: readdir_r.patch --]
[-- Type: text/x-patch, Size: 10298 bytes --]

2013-06-06  Florian Weimer  <fweimer@redhat.com>

	[BZ #14699]
	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
	member.
	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
	member.
	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
	conditional.
	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
	GETDENTS_64BIT_ALIGNED.
	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
	* manual/filesys.texi (Reading/Closing Directory): Document
	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
	strongly.
	* manual/conf.texi (Limits for Files): Add portability note to
	NAME_MAX, PATH_MAX.
	(Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.

diff --git a/manual/conf.texi b/manual/conf.texi
index 7eb8b36..32d6409 100644
--- a/manual/conf.texi
+++ b/manual/conf.texi
@@ -1149,6 +1149,9 @@ typed ahead as input.  @xref{I/O Queues}.
 @deftypevr Macro int NAME_MAX
 The uniform system limit (if any) for the length of a file name component, not
 including the terminating null character.
+
+@strong{Portability Note:} On some systems, @theglibc{} defines
+@code{NAME_MAX}, but does not actually enforce this limit.
 @end deftypevr
 
 @comment limits.h
@@ -1157,6 +1160,9 @@ including the terminating null character.
 The uniform system limit (if any) for the length of an entire file name (that
 is, the argument given to system calls such as @code{open}), including the
 terminating null character.
+
+@strong{Portability Note:} @Theglibc{} does not enforce this limit
+even if @code{PATH_MAX} is defined.
 @end deftypevr
 
 @cindex limits, pipe buffer size
@@ -1476,6 +1482,10 @@ Inquire about the value of @code{POSIX_REC_MIN_XFER_SIZE}.
 Inquire about the value of @code{POSIX_REC_XFER_ALIGN}.
 @end table
 
+@strong{Portability Note:} On some systems, @theglibc{} does not
+actually enforce the @code{_PC_NAME_MAX} and @code{_PC_PATH_MAX}
+limits.
+
 @node Utility Limits
 @section Utility Program Capacity Limits
 
diff --git a/manual/filesys.texi b/manual/filesys.texi
index 1df9cf2..c4c7896 100644
--- a/manual/filesys.texi
+++ b/manual/filesys.texi
@@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
 @comment POSIX.1
 @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
 This function reads the next entry from the directory.  It normally
-returns a pointer to a structure containing information about the file.
-This structure is statically allocated and can be rewritten by a
-subsequent call.
+returns a pointer to a structure containing information about the
+file.  This structure is associated with the @var{dirstream} handle
+and can be rewritten by a subsequent call.
 
 @strong{Portability Note:} On some systems @code{readdir} may not
 return entries for @file{.} and @file{..}, even though these are always
@@ -461,19 +461,26 @@ conditions are defined for this function:
 The @var{dirstream} argument is not valid.
 @end table
 
-@code{readdir} is not thread safe.  Multiple threads using
-@code{readdir} on the same @var{dirstream} may overwrite the return
-value.  Use @code{readdir_r} when this is critical.
+To tell the regular end-of-directory condition and errors apart, you
+need to set @code{errno} to zero directly before calling
+@code{readdir}.  To avoid entering an infinite loop, you should stop
+reading from the directory on the first error.
+
+@code{readdir} is thread safe as long as only a single thread accesses
+the @var{dirstream} handle without synchronization.  The alternative
+@code{readdir_r} function has significant portability issues.
+Therefore, you should always use @code{readdir} and external locking.
 @end deftypefun
 
 @comment dirent.h
 @comment GNU
 @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
-This function is the reentrant version of @code{readdir}.  Like
-@code{readdir} it returns the next entry from the directory.  But to
-prevent conflicts between simultaneously running threads the result is
-not stored in statically allocated memory.  Instead the argument
-@var{entry} points to a place to store the result.
+This function is version of @code{readdir} which performs internal
+locking.  Like @code{readdir} it returns the next entry from the
+directory.  But to prevent conflicts between simultaneously running
+threads, the result is not stored inside the @var{dirstream} handle.
+Instead the argument @var{entry} points to a place to store the
+result.
 
 Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
 to @var{entry}.  If there are no more entries in the directory or an
@@ -481,14 +488,14 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
 null pointer and returns a nonzero error code, also stored in
 @code{errno}, as described for @code{readdir}.
 
-@strong{Portability Note:} On some systems @code{readdir_r} may not
-return a NUL terminated string for the file name, even when there is no
-@code{d_reclen} field in @code{struct dirent} and the file
-name is the maximum allowed size.  Modern systems all have the
-@code{d_reclen} field, and on old systems multi-threading is not
-critical.  In any case there is no such problem with the @code{readdir}
-function, so that even on systems without the @code{d_reclen} member one
-could use multiple threads by using external locking.
+@strong{Portability Note:} On some systems, @code{readdir_r} cannot
+read directory entries with very long names.  If such a name is
+encountered, @code{readdir_r} returns with an error code of
+@code{ENAMETOOLONG} after the final directory entry has been read.  On
+other systems, @code{readdir_r} can return successfully, but the
+@code{d_name} member is not NUL-terminated or is otherwise truncated.
+Therefore, you should always prefer @code{readdir} (with external
+locking if necessary) over @code{readdir_r}.
 
 It is also important to look at the definition of the @code{struct
 dirent} type.  Simply passing a pointer to an object of this type for
diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
index a7a074d..8e8570d 100644
--- a/sysdeps/posix/dirstream.h
+++ b/sysdeps/posix/dirstream.h
@@ -39,6 +39,8 @@ struct __dirstream
 
     off_t filepos;		/* Position of next entry to read.  */
 
+    int errcode;		/* Delayed error code.  */
+
     /* Directory block.  */
     char data[0] __attribute__ ((aligned (__alignof__ (void*))));
   };
diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index ddfc3a7..fc05b0f 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
   dirp->size = 0;
   dirp->offset = 0;
   dirp->filepos = 0;
+  dirp->errcode = 0;
 
   return dirp;
 }
diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
index b5a8e2e..a780c3f 100644
--- a/sysdeps/posix/readdir_r.c
+++ b/sysdeps/posix/readdir_r.c
@@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
   DIRENT_TYPE *dp;
   size_t reclen;
   const int saved_errno = errno;
+  int ret;
 
   __libc_lock_lock (dirp->lock);
 
@@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
 		  bytes = 0;
 		  __set_errno (saved_errno);
 		}
+	      if (bytes < 0)
+		dirp->errcode = errno;
 
 	      dp = NULL;
-	      /* Reclen != 0 signals that an error occurred.  */
-	      reclen = bytes != 0;
 	      break;
 	    }
 	  dirp->size = (size_t) bytes;
@@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
       dirp->filepos += reclen;
 #endif
 
-      /* Skip deleted files.  */
+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+	{
+	  /* The record is very long.  It could still fit into the
+	     caller-supplied buffer if we can skip padding at the
+	     end.  */
+	  size_t namelen = strlen(dp->d_name);
+	  if (namelen <= NAME_MAX)
+	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+	  else
+	    {
+	      /* The name is too long.  Ignore this file.  */
+	      dirp->errcode = ENAMETOOLONG;
+	      dp->d_ino = 0;
+	      continue;
+	    }
+	}
+#endif
+
+      /* Skip deleted and ignored files.  */
     }
   while (dp->d_ino == 0);
 
   if (dp != NULL)
     {
-#ifdef GETDENTS_64BIT_ALIGNED
-      /* The d_reclen value might include padding which is not part of
-	 the DIRENT_TYPE data structure.  */
-      reclen = MIN (reclen,
-		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
-#endif
       *result = memcpy (entry, dp, reclen);
-#ifdef GETDENTS_64BIT_ALIGNED
+#ifdef _DIRENT_HAVE_D_RECLEN
       entry->d_reclen = reclen;
 #endif
+      ret = 0;
     }
   else
-    *result = NULL;
+    {
+      *result = NULL;
+      ret = dirp->errcode;
+    }
 
   __libc_lock_unlock (dirp->lock);
 
-  return dp != NULL ? 0 : reclen ? errno : 0;
+  return ret;
 }
 
 #ifdef __READDIR_R_ALIAS
diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
index 2935a8e..d4991ad 100644
--- a/sysdeps/posix/rewinddir.c
+++ b/sysdeps/posix/rewinddir.c
@@ -33,6 +33,7 @@ rewinddir (dirp)
   dirp->filepos = 0;
   dirp->offset = 0;
   dirp->size = 0;
+  dirp->errcode = 0;
 #ifndef NOT_IN_libc
   __libc_lock_unlock (dirp->lock);
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
index 8ebbcfd..a7d114e 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
@@ -18,7 +18,6 @@
 #define __READDIR_R __readdir64_r
 #define __GETDENTS __getdents64
 #define DIRENT_TYPE struct dirent64
-#define GETDENTS_64BIT_ALIGNED 1
 
 #include <sysdeps/posix/readdir_r.c>
 
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
index 5ed8e95..290f2c8 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
+++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
@@ -1,5 +1,4 @@
 #define readdir64_r __no_readdir64_r_decl
-#define GETDENTS_64BIT_ALIGNED 1
 #include <sysdeps/posix/readdir_r.c>
 #undef readdir64_r
 weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Carlos O'Donell]

On 06/06/2013 10:55 AM, Florian Weimer wrote:
> On 05/21/2013 01:22 PM, Florian Weimer wrote:
>> On 05/16/2013 03:09 PM, Siddhesh Poyarekar wrote:
>>
>>> The reposted patch looks fine to me, but I'd like a native English
>>> speaker to review the manual bits.
>>
>> I agree.  Any takers?
>>
>>> I like the original NAMETOOLONG
>>> since it conveys perfectly what the failure really is, but I don't
>>> mind if it's something else that makes enough sense.
>>
>> I would prefer ENAMETOOLONG as well.
> 
> This is what I would like to commit.
> 
> I switched back to ENAMETOOLONG and added additional notes to
> manual/conf.texi. Both realpath and get_current_dir_name can exceed
> PATH_MAX, so I listed it as well.
> 
> -- 
> Florian Weimer / Red Hat Product Security Team
> 
> readdir_r.patch

Florian,

Thanks for fixing this.

> 2013-06-06  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #14699]
> 	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
> 	member.
> 	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
> 	member.
> 	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
> 	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
> 	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
> 	conditional.
> 	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
> 	GETDENTS_64BIT_ALIGNED.
> 	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
> 	* manual/filesys.texi (Reading/Closing Directory): Document
> 	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
> 	strongly.
> 	* manual/conf.texi (Limits for Files): Add portability note to
> 	NAME_MAX, PATH_MAX.
> 	(Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.

Is there already a test case that tests for this?

This seems like the kind of thing we should really be testing.
 
> diff --git a/manual/conf.texi b/manual/conf.texi
> index 7eb8b36..32d6409 100644
> --- a/manual/conf.texi
> +++ b/manual/conf.texi
> @@ -1149,6 +1149,9 @@ typed ahead as input.  @xref{I/O Queues}.
>  @deftypevr Macro int NAME_MAX
>  The uniform system limit (if any) for the length of a file name component, not
>  including the terminating null character.
> +
> +@strong{Portability Note:} On some systems, @theglibc{} defines
> +@code{NAME_MAX}, but does not actually enforce this limit.
>  @end deftypevr

OK.

>  
>  @comment limits.h
> @@ -1157,6 +1160,9 @@ including the terminating null character.
>  The uniform system limit (if any) for the length of an entire file name (that
>  is, the argument given to system calls such as @code{open}), including the
>  terminating null character.
> +
> +@strong{Portability Note:} @Theglibc{} does not enforce this limit
> +even if @code{PATH_MAX} is defined.
>  @end deftypevr
>

OK.

>  @cindex limits, pipe buffer size
> @@ -1476,6 +1482,10 @@ Inquire about the value of @code{POSIX_REC_MIN_XFER_SIZE}.
>  Inquire about the value of @code{POSIX_REC_XFER_ALIGN}.
>  @end table
>  
> +@strong{Portability Note:} On some systems, @theglibc{} does not
> +actually enforce the @code{_PC_NAME_MAX} and @code{_PC_PATH_MAX}
> +limits.
> +

Suggest:
On some systems, @theglibc{} does not enforce 
@code{_PC_NAME_MAX} or @code{_PC_PATH_MAX} limits.

>  @node Utility Limits
>  @section Utility Program Capacity Limits
>  
> diff --git a/manual/filesys.texi b/manual/filesys.texi
> index 1df9cf2..c4c7896 100644
> --- a/manual/filesys.texi
> +++ b/manual/filesys.texi
> @@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
>  @comment POSIX.1
>  @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
>  This function reads the next entry from the directory.  It normally
> -returns a pointer to a structure containing information about the file.
> -This structure is statically allocated and can be rewritten by a
> -subsequent call.
> +returns a pointer to a structure containing information about the
> +file.  This structure is associated with the @var{dirstream} handle
> +and can be rewritten by a subsequent call.

... "subsequent call with the same handle." ???

>  @strong{Portability Note:} On some systems @code{readdir} may not
>  return entries for @file{.} and @file{..}, even though these are always
> @@ -461,19 +461,26 @@ conditions are defined for this function:
>  The @var{dirstream} argument is not valid.
>  @end table
>  
> -@code{readdir} is not thread safe.  Multiple threads using
> -@code{readdir} on the same @var{dirstream} may overwrite the return
> -value.  Use @code{readdir_r} when this is critical.
> +To tell the regular end-of-directory condition and errors apart, you
> +need to set @code{errno} to zero directly before calling
> +@code{readdir}.

Suggest:
To distinguish between an end-of-directory condition or an error, you
must set @code{errno} to zero before calling @code{readdir}.

>                   To avoid entering an infinite loop, you should stop
> +reading from the directory on the first error.

s/on/after/g

> +
> +@code{readdir} is thread safe as long as only a single thread accesses
> +the @var{dirstream} handle without synchronization.  The alternative
> +@code{readdir_r} function has significant portability issues.
> +Therefore, you should always use @code{readdir} and external locking.

It's not thread safe, please do not say it is.

If readdir_r is not-portable then it needs to be fixed or the specifics
of the non-portability documented.

Suggest:
@code{readdir} is not thread safe.  Multiple threads using
@code{readdir} on the same @var{dirstream} may overwrite the return
value.  The use of @code{readdir_r} is a thread-safe alternative,
but may suffer from poor portability. If portability is required
it is recommended that you use @code{readdir} with external locking.

>  @end deftypefun
>  
>  @comment dirent.h
>  @comment GNU
>  @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
> -This function is the reentrant version of @code{readdir}.  Like
> -@code{readdir} it returns the next entry from the directory.  But to
> -prevent conflicts between simultaneously running threads the result is
> -not stored in statically allocated memory.  Instead the argument
> -@var{entry} points to a place to store the result.
> +This function is version of @code{readdir} which performs internal

s/is version/is a version/g

> +locking.  Like @code{readdir} it returns the next entry from the
> +directory.  

>              But to prevent conflicts between simultaneously running
> +threads, the result is not stored inside the @var{dirstream} handle.
> +Instead the argument @var{entry} points to a place to store the
> +result.

Suggest:

To prevent conflicts between simultaneously running
threads the result is stored inside the @var{entry} object.

>  
>  Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
>  to @var{entry}.  If there are no more entries in the directory or an
> @@ -481,14 +488,14 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
>  null pointer and returns a nonzero error code, also stored in
>  @code{errno}, as described for @code{readdir}.
>  
> -@strong{Portability Note:} On some systems @code{readdir_r} may not
> -return a NUL terminated string for the file name, even when there is no
> -@code{d_reclen} field in @code{struct dirent} and the file
> -name is the maximum allowed size.  Modern systems all have the
> -@code{d_reclen} field, and on old systems multi-threading is not
> -critical.  In any case there is no such problem with the @code{readdir}
> -function, so that even on systems without the @code{d_reclen} member one
> -could use multiple threads by using external locking.
> +@strong{Portability Note:} On some systems, @code{readdir_r} cannot
> +read directory entries with very long names.  If such a name is
> +encountered, @code{readdir_r} returns with an error code of
> +@code{ENAMETOOLONG} after the final directory entry has been read.  



> On
> +other systems, @code{readdir_r} can return successfully, but the
> +@code{d_name} member is not NUL-terminated or is otherwise truncated.

Suggest:

On other systems, @code{readdir_r} may return successfully, but
@code{d_name} may not be NULL-terminated or may be truncated.

> +Therefore, you should always prefer @code{readdir} (with external
> +locking if necessary) over @code{readdir_r}.
>  
>  It is also important to look at the definition of the @code{struct
>  dirent} type.  Simply passing a pointer to an object of this type for
> diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
> index a7a074d..8e8570d 100644
> --- a/sysdeps/posix/dirstream.h
> +++ b/sysdeps/posix/dirstream.h
> @@ -39,6 +39,8 @@ struct __dirstream
>  
>      off_t filepos;		/* Position of next entry to read.  */
>  
> +    int errcode;		/* Delayed error code.  */
> +

OK.

>      /* Directory block.  */
>      char data[0] __attribute__ ((aligned (__alignof__ (void*))));
>    };
> diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
> index ddfc3a7..fc05b0f 100644
> --- a/sysdeps/posix/opendir.c
> +++ b/sysdeps/posix/opendir.c
> @@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
>    dirp->size = 0;
>    dirp->offset = 0;
>    dirp->filepos = 0;
> +  dirp->errcode = 0;

OK.

>  
>    return dirp;
>  }
> diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
> index b5a8e2e..a780c3f 100644
> --- a/sysdeps/posix/readdir_r.c
> +++ b/sysdeps/posix/readdir_r.c
> @@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>    DIRENT_TYPE *dp;
>    size_t reclen;
>    const int saved_errno = errno;
> +  int ret;
>  
>    __libc_lock_lock (dirp->lock);
>  
> @@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>  		  bytes = 0;
>  		  __set_errno (saved_errno);
>  		}
> +	      if (bytes < 0)
> +		dirp->errcode = errno;
>  
>  	      dp = NULL;
> -	      /* Reclen != 0 signals that an error occurred.  */
> -	      reclen = bytes != 0;
>  	      break;
>  	    }
>  	  dirp->size = (size_t) bytes;
> @@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>        dirp->filepos += reclen;
>  #endif
>  
> -      /* Skip deleted files.  */
> +#ifdef NAME_MAX
> +      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
> +	{
> +	  /* The record is very long.  It could still fit into the
> +	     caller-supplied buffer if we can skip padding at the
> +	     end.  */
> +	  size_t namelen = strlen(dp->d_name);
> +	  if (namelen <= NAME_MAX)
> +	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
> +	  else
> +	    {
> +	      /* The name is too long.  Ignore this file.  */
> +	      dirp->errcode = ENAMETOOLONG;

OK.

> +	      dp->d_ino = 0;
> +	      continue;
> +	    }
> +	}
> +#endif
> +
> +      /* Skip deleted and ignored files.  */
>      }
>    while (dp->d_ino == 0);
>  
>    if (dp != NULL)
>      {
> -#ifdef GETDENTS_64BIT_ALIGNED
> -      /* The d_reclen value might include padding which is not part of
> -	 the DIRENT_TYPE data structure.  */
> -      reclen = MIN (reclen,
> -		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
> -#endif
>        *result = memcpy (entry, dp, reclen);
> -#ifdef GETDENTS_64BIT_ALIGNED
> +#ifdef _DIRENT_HAVE_D_RECLEN
>        entry->d_reclen = reclen;
>  #endif
> +      ret = 0;
>      }
>    else
> -    *result = NULL;
> +    {
> +      *result = NULL;
> +      ret = dirp->errcode;
> +    }
>  
>    __libc_lock_unlock (dirp->lock);
>  
> -  return dp != NULL ? 0 : reclen ? errno : 0;
> +  return ret;
>  }
>  
>  #ifdef __READDIR_R_ALIAS
> diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
> index 2935a8e..d4991ad 100644
> --- a/sysdeps/posix/rewinddir.c
> +++ b/sysdeps/posix/rewinddir.c
> @@ -33,6 +33,7 @@ rewinddir (dirp)
>    dirp->filepos = 0;
>    dirp->offset = 0;
>    dirp->size = 0;
> +  dirp->errcode = 0;
>  #ifndef NOT_IN_libc
>    __libc_lock_unlock (dirp->lock);
>  #endif
> diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> index 8ebbcfd..a7d114e 100644
> --- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> +++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> @@ -18,7 +18,6 @@
>  #define __READDIR_R __readdir64_r
>  #define __GETDENTS __getdents64
>  #define DIRENT_TYPE struct dirent64
> -#define GETDENTS_64BIT_ALIGNED 1
>  
>  #include <sysdeps/posix/readdir_r.c>
>  
> diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> index 5ed8e95..290f2c8 100644
> --- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> +++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> @@ -1,5 +1,4 @@
>  #define readdir64_r __no_readdir64_r_decl
> -#define GETDENTS_64BIT_ALIGNED 1
>  #include <sysdeps/posix/readdir_r.c>
>  #undef readdir64_r
>  weak_alias (__readdir_r, readdir64_r)

Please post a new version.

Cheers,
Carlos.

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

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

On 06/06/2013 06:06 PM, Carlos O'Donell wrote:
> Florian,
>
> Thanks for fixing this.

Thanks for the review, Carlos.

> Is there already a test case that tests for this?
>
> This seems like the kind of thing we should really be testing.

Most Linux file systems enforce the NAME_MAX limit, so it's difficult to 
trigger.  It requires a FUSE file system or a networked file system on a 
system which does not enforce the NAME_MAX limit.

>>   @comment POSIX.1
>>   @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
>>   This function reads the next entry from the directory.  It normally
>> -returns a pointer to a structure containing information about the file.
>> -This structure is statically allocated and can be rewritten by a
>> -subsequent call.
>> +returns a pointer to a structure containing information about the
>> +file.  This structure is associated with the @var{dirstream} handle
>> +and can be rewritten by a subsequent call.
>
> ... "subsequent call with the same handle." ???

On glibc, yes.  There are rumors that there used to be systems where DIR 
* was actually an integer file descriptor and readdir used a statically 
allocated buffer.  On such systems, readdir_r might actually be better 
than readdir, but on GNU/Linux, there are directories where readdir_r 
fails and readdir succeeds.

> Suggest:
> @code{readdir} is not thread safe.  Multiple threads using
> @code{readdir} on the same @var{dirstream} may overwrite the return
> value.  The use of @code{readdir_r} is a thread-safe alternative,
> but may suffer from poor portability. If portability is required
> it is recommended that you use @code{readdir} with external locking.

Here's what I'm proposing instead:

@code{readdir} is not thread safe.  In @theglibc{} implementation,
multiple threads using @code{readdir} on the same @var{dirstream} may
overwrite the return value.  The use of @code{readdir_r} is a
thread-safe alternative, but may suffer from poor portability.  If
portability is required it is recommended that you use @code{readdir},
with external locking if multiple threads access the same
@var{dirstream}.

I followed your other suggestions.

> Please post a new version.

Attached.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: readdir_r.patch --]
[-- Type: text/x-patch, Size: 10330 bytes --]

2013-06-06  Florian Weimer  <fweimer@redhat.com>

	[BZ #14699]
	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
	member.
	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
	member.
	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
	conditional.
	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
	GETDENTS_64BIT_ALIGNED.
	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
	* manual/filesys.texi (Reading/Closing Directory): Document
	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
	strongly.
	* manual/conf.texi (Limits for Files): Add portability note to
	NAME_MAX, PATH_MAX.
	(Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.

diff --git a/manual/conf.texi b/manual/conf.texi
index 7eb8b36..c720063 100644
--- a/manual/conf.texi
+++ b/manual/conf.texi
@@ -1149,6 +1149,9 @@ typed ahead as input.  @xref{I/O Queues}.
 @deftypevr Macro int NAME_MAX
 The uniform system limit (if any) for the length of a file name component, not
 including the terminating null character.
+
+@strong{Portability Note:} On some systems, @theglibc{} defines
+@code{NAME_MAX}, but does not actually enforce this limit.
 @end deftypevr
 
 @comment limits.h
@@ -1157,6 +1160,9 @@ including the terminating null character.
 The uniform system limit (if any) for the length of an entire file name (that
 is, the argument given to system calls such as @code{open}), including the
 terminating null character.
+
+@strong{Portability Note:} @Theglibc{} does not enforce this limit
+even if @code{PATH_MAX} is defined.
 @end deftypevr
 
 @cindex limits, pipe buffer size
@@ -1476,6 +1482,9 @@ Inquire about the value of @code{POSIX_REC_MIN_XFER_SIZE}.
 Inquire about the value of @code{POSIX_REC_XFER_ALIGN}.
 @end table
 
+@strong{Portability Note:} On some systems, @theglibc{} does not
+enforce @code{_PC_NAME_MAX} or @code{_PC_PATH_MAX} limits.
+
 @node Utility Limits
 @section Utility Program Capacity Limits
 
diff --git a/manual/filesys.texi b/manual/filesys.texi
index 1df9cf2..de45eac 100644
--- a/manual/filesys.texi
+++ b/manual/filesys.texi
@@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
 @comment POSIX.1
 @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
 This function reads the next entry from the directory.  It normally
-returns a pointer to a structure containing information about the file.
-This structure is statically allocated and can be rewritten by a
-subsequent call.
+returns a pointer to a structure containing information about the
+file.  This structure is associated with the @var{dirstream} handle
+and can be rewritten by a subsequent call.
 
 @strong{Portability Note:} On some systems @code{readdir} may not
 return entries for @file{.} and @file{..}, even though these are always
@@ -461,19 +461,27 @@ conditions are defined for this function:
 The @var{dirstream} argument is not valid.
 @end table
 
-@code{readdir} is not thread safe.  Multiple threads using
-@code{readdir} on the same @var{dirstream} may overwrite the return
-value.  Use @code{readdir_r} when this is critical.
+To distinguish between an end-of-directory condition or an error, you
+must set @code{errno} to zero before calling @code{readdir}.  To avoid
+entering an infinite loop, you should stop reading from the directory
+after the first error.
+
+@code{readdir} is not thread safe.  In @theglibc{} implementation,
+multiple threads using @code{readdir} on the same @var{dirstream} may
+overwrite the return value.  The use of @code{readdir_r} is a
+thread-safe alternative, but may suffer from poor portability.  If
+portability is required it is recommended that you use @code{readdir},
+with external locking if multiple threads access the same
+@var{dirstream}.
 @end deftypefun
 
 @comment dirent.h
 @comment GNU
 @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
-This function is the reentrant version of @code{readdir}.  Like
-@code{readdir} it returns the next entry from the directory.  But to
-prevent conflicts between simultaneously running threads the result is
-not stored in statically allocated memory.  Instead the argument
-@var{entry} points to a place to store the result.
+This function is a version of @code{readdir} which performs internal
+locking.  Like @code{readdir} it returns the next entry from the
+directory.  To prevent conflicts between simultaneously running
+threads the result is stored inside the @var{entry} object.
 
 Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
 to @var{entry}.  If there are no more entries in the directory or an
@@ -481,14 +489,14 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
 null pointer and returns a nonzero error code, also stored in
 @code{errno}, as described for @code{readdir}.
 
-@strong{Portability Note:} On some systems @code{readdir_r} may not
-return a NUL terminated string for the file name, even when there is no
-@code{d_reclen} field in @code{struct dirent} and the file
-name is the maximum allowed size.  Modern systems all have the
-@code{d_reclen} field, and on old systems multi-threading is not
-critical.  In any case there is no such problem with the @code{readdir}
-function, so that even on systems without the @code{d_reclen} member one
-could use multiple threads by using external locking.
+@strong{Portability Note:} On some systems, @code{readdir_r} cannot
+read directory entries with very long names.  If such a name is
+encountered, @code{readdir_r} returns with an error code of
+@code{ENAMETOOLONG} after the final directory entry has been read.  On
+other systems, @code{readdir_r} may return successfully, but the
+@code{d_name} member may not be NUL-terminated or may be truncated.
+Therefore, you should always prefer @code{readdir} (with external
+locking if necessary) over @code{readdir_r}.
 
 It is also important to look at the definition of the @code{struct
 dirent} type.  Simply passing a pointer to an object of this type for
diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
index a7a074d..8e8570d 100644
--- a/sysdeps/posix/dirstream.h
+++ b/sysdeps/posix/dirstream.h
@@ -39,6 +39,8 @@ struct __dirstream
 
     off_t filepos;		/* Position of next entry to read.  */
 
+    int errcode;		/* Delayed error code.  */
+
     /* Directory block.  */
     char data[0] __attribute__ ((aligned (__alignof__ (void*))));
   };
diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index ddfc3a7..fc05b0f 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
   dirp->size = 0;
   dirp->offset = 0;
   dirp->filepos = 0;
+  dirp->errcode = 0;
 
   return dirp;
 }
diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
index b5a8e2e..a780c3f 100644
--- a/sysdeps/posix/readdir_r.c
+++ b/sysdeps/posix/readdir_r.c
@@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
   DIRENT_TYPE *dp;
   size_t reclen;
   const int saved_errno = errno;
+  int ret;
 
   __libc_lock_lock (dirp->lock);
 
@@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
 		  bytes = 0;
 		  __set_errno (saved_errno);
 		}
+	      if (bytes < 0)
+		dirp->errcode = errno;
 
 	      dp = NULL;
-	      /* Reclen != 0 signals that an error occurred.  */
-	      reclen = bytes != 0;
 	      break;
 	    }
 	  dirp->size = (size_t) bytes;
@@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
       dirp->filepos += reclen;
 #endif
 
-      /* Skip deleted files.  */
+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+	{
+	  /* The record is very long.  It could still fit into the
+	     caller-supplied buffer if we can skip padding at the
+	     end.  */
+	  size_t namelen = strlen(dp->d_name);
+	  if (namelen <= NAME_MAX)
+	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+	  else
+	    {
+	      /* The name is too long.  Ignore this file.  */
+	      dirp->errcode = ENAMETOOLONG;
+	      dp->d_ino = 0;
+	      continue;
+	    }
+	}
+#endif
+
+      /* Skip deleted and ignored files.  */
     }
   while (dp->d_ino == 0);
 
   if (dp != NULL)
     {
-#ifdef GETDENTS_64BIT_ALIGNED
-      /* The d_reclen value might include padding which is not part of
-	 the DIRENT_TYPE data structure.  */
-      reclen = MIN (reclen,
-		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
-#endif
       *result = memcpy (entry, dp, reclen);
-#ifdef GETDENTS_64BIT_ALIGNED
+#ifdef _DIRENT_HAVE_D_RECLEN
       entry->d_reclen = reclen;
 #endif
+      ret = 0;
     }
   else
-    *result = NULL;
+    {
+      *result = NULL;
+      ret = dirp->errcode;
+    }
 
   __libc_lock_unlock (dirp->lock);
 
-  return dp != NULL ? 0 : reclen ? errno : 0;
+  return ret;
 }
 
 #ifdef __READDIR_R_ALIAS
diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
index 2935a8e..d4991ad 100644
--- a/sysdeps/posix/rewinddir.c
+++ b/sysdeps/posix/rewinddir.c
@@ -33,6 +33,7 @@ rewinddir (dirp)
   dirp->filepos = 0;
   dirp->offset = 0;
   dirp->size = 0;
+  dirp->errcode = 0;
 #ifndef NOT_IN_libc
   __libc_lock_unlock (dirp->lock);
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
index 8ebbcfd..a7d114e 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
@@ -18,7 +18,6 @@
 #define __READDIR_R __readdir64_r
 #define __GETDENTS __getdents64
 #define DIRENT_TYPE struct dirent64
-#define GETDENTS_64BIT_ALIGNED 1
 
 #include <sysdeps/posix/readdir_r.c>
 
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
index 5ed8e95..290f2c8 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
+++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
@@ -1,5 +1,4 @@
 #define readdir64_r __no_readdir64_r_decl
-#define GETDENTS_64BIT_ALIGNED 1
 #include <sysdeps/posix/readdir_r.c>
 #undef readdir64_r
 weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[KOSAKI Motohiro]

+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+ {
+  /* The record is very long.  It could still fit into the
+     caller-supplied buffer if we can skip padding at the
+     end.  */
+  size_t namelen = strlen(dp->d_name);
+  if (namelen <= NAME_MAX)
+    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+  else
+    {
+      /* The name is too long.  Ignore this file.  */
+      dirp->errcode = ENAMETOOLONG;
+      dp->d_ino = 0;
+      continue;
+    }
+ }
+#endif

Hmm...
Linux man pages say:

>Since POSIX.1 does not specify the size of the d_name field, and other
>nonstandard fields may precede that field within the dirent structure, portable
>applications that use readdir_r() should allocate the buffer whose address is
>passed in entry as follows:
>
> name_max = pathconf(dirpath, _PC_NAME_MAX);
> if (name_max == -1)         /* Limit not defined, or error */
>    name_max = 255;         /* Take a guess */
> len = offsetof(struct dirent, d_name) + name_max + 1;
> entryp = malloc(len);
> (POSIX.1 requires that d_name is the last field in a struct dirent.)

So, only broken applications may hit this? If so, do you really think such
broken application checks return code correctly?

If a fuse filesystem which allow >256 file names is legal, this patch breaks
right applications. In the other hands, if it is illegal, I'd suggest
to fix such
broken filesystems instead.

I mean, portable applications should use readdir_r correctly and Linux specific
one should use readdir instead.

Side note: the above man page is not a theoretical issue. At least, Solaris
requires it.

Am I missing something?

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 06/06/2013 09:53 PM, KOSAKI Motohiro wrote:
> +#ifdef NAME_MAX
> +      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
> + {
> +  /* The record is very long.  It could still fit into the
> +     caller-supplied buffer if we can skip padding at the
> +     end.  */
> +  size_t namelen = strlen(dp->d_name);
> +  if (namelen <= NAME_MAX)
> +    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
> +  else
> +    {
> +      /* The name is too long.  Ignore this file.  */
> +      dirp->errcode = ENAMETOOLONG;
> +      dp->d_ino = 0;
> +      continue;
> +    }
> + }
> +#endif
>
> Hmm...
> Linux man pages say:
>
>> Since POSIX.1 does not specify the size of the d_name field, and other
>> nonstandard fields may precede that field within the dirent structure, portable
>> applications that use readdir_r() should allocate the buffer whose address is
>> passed in entry as follows:
>>
>> name_max = pathconf(dirpath, _PC_NAME_MAX);
>> if (name_max == -1)         /* Limit not defined, or error */
>>     name_max = 255;         /* Take a guess */
>> len = offsetof(struct dirent, d_name) + name_max + 1;
>> entryp = malloc(len);
>> (POSIX.1 requires that d_name is the last field in a struct dirent.)
>
> So, only broken applications may hit this? If so, do you really think such
> broken application checks return code correctly?

There's precedent for my approach in realpath, where we don't use 
pathconf(path, _PC_PATH_MAX), either, but cap the path length at PATH_MAX.

> If a fuse filesystem which allow >256 file names is legal, this patch breaks
> right applications. In the other hands, if it is illegal, I'd suggest
> to fix such broken filesystems instead.

Hmm.  We could fix the file systems and patch all affected applications 
to use the approach from the manual page, with s/pathconf/fpathconf/ to 
avoid the race, and a hard failure on failure ("take a guess" is a very 
bad idea here).

For some reason, I discarded that approach.  Probably I didn't know 
about the f_namemax member of fstatvfs at the time (and I didn't look at 
the fpathconf implementation).  f_namemax is wrong for the file systems 
in question, but that could be fixed.

We'd still have to patch readdir_r to pass through long file names.

The best approach would be a readdir4 function which takes an explicit 
size argument.  A bit unsual would be a newdirent function which returns 
a suitably sized dirent object, based on a DIR *.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[KOSAKI Motohiro]

>> So, only broken applications may hit this? If so, do you really think such
>> broken application checks return code correctly?
>
> There's precedent for my approach in realpath, where we don't use
> pathconf(path, _PC_PATH_MAX), either, but cap the path length at PATH_MAX.

PATH_MAX is used VFS as hard coded limitations. So, I don't think any Linux filesystem
have a long path exceed PATH_MAX.

And, to be honest, I didn't think any Linux filesystem can have >256 len path
components until I saw your email. Could you please tell us which filesystem
allow such long file name if you can disclose?


>> If a fuse filesystem which allow >256 file names is legal, this patch
>> breaks
>> right applications. In the other hands, if it is illegal, I'd suggest
>> to fix such broken filesystems instead.
>
>
> Hmm.  We could fix the file systems and patch all affected applications to
> use the approach from the manual page, with s/pathconf/fpathconf/ to avoid
> the race, and a hard failure on failure ("take a guess" is a very bad idea
> here).

I don't want to talk pathconf vs fpathconf thing. But I completely agree you
are right. All applications should use fpathconf instead of pathconf.


> For some reason, I discarded that approach.  Probably I didn't know about
> the f_namemax member of fstatvfs at the time (and I didn't look at the
> fpathconf implementation).  f_namemax is wrong for the file systems in
> question, but that could be fixed.
>
> We'd still have to patch readdir_r to pass through long file names.

I guess the right way is to fix a filesystem instead glibc because 1) your
patch assume >256 is illegal and if so, there is no reason not to fix a source
of trouble. 2) adding new error code may bring us new application breakage.
Is it impossible? Or Are these file system unfixable lots?


> The best approach would be a readdir4 function which takes an explicit size
> argument.  A bit unsual would be a newdirent function which returns a
> suitably sized dirent object, based on a DIR *.

Right.

readdir_r considered harmful <http://womble.decadent.org.uk/readdir_r-advisory.html> says
the same conclusion.

Or, drepper suggests to use readdir() instead. http://udrepper.livejournal.com/18555.html
Because "readdir_r considered harmful" afraid readdir() is not thread safe, but it is, at
least, all of i observed modern platforms. I recently almost always use readdir().

Re: [PATCH] Fix readdir_r with long file names

[Rich Felker]

On Thu, Jun 06, 2013 at 03:53:16PM -0400, KOSAKI Motohiro wrote:
> I mean, portable applications should use readdir_r correctly and Linux specific
> one should use readdir instead.
> 
> Side note: the above man page is not a theoretical issue. At least, Solaris
> requires it.
> 
> Am I missing something?

Yes, the fact that the Austin Group is planning to require readdir to
be thread-safe and to mark readdir_r obsolescent. So effort put into
making readdir_r more usable, or worse yet, adding a readdir4, is a
waste of effort. Just make sure readdir_r is _safe_ against buffer
overflows from buggy FUSE modules, and advise application developers
to use readdir, not readdir_r.

Rich

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 06/07/2013 03:30 AM, Rich Felker wrote:
> On Thu, Jun 06, 2013 at 03:53:16PM -0400, KOSAKI Motohiro wrote:
>> I mean, portable applications should use readdir_r correctly and Linux specific
>> one should use readdir instead.
>>
>> Side note: the above man page is not a theoretical issue. At least, Solaris
>> requires it.
>>
>> Am I missing something?
>
> Yes, the fact that the Austin Group is planning to require readdir to
> be thread-safe and to mark readdir_r obsolescent.

This is good news.

 > So effort put into
> making readdir_r more usable, or worse yet, adding a readdir4, is a
> waste of effort. Just make sure readdir_r is _safe_ against buffer
> overflows from buggy FUSE modules, and advise application developers
> to use readdir, not readdir_r.

Does this mean that you agree with the basic approach of the patch?

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 06/06/2013 11:06 PM, KOSAKI Motohiro wrote:
>>> So, only broken applications may hit this? If so, do you really think
>>> such
>>> broken application checks return code correctly?
>>
>> There's precedent for my approach in realpath, where we don't use
>> pathconf(path, _PC_PATH_MAX), either, but cap the path length at
>> PATH_MAX.
>
> PATH_MAX is used VFS as hard coded limitations. So, I don't think any
> Linux filesystem have a long path exceed PATH_MAX.

The canonical name of a file can be longer than PATH_MAX characters. 
This is probably true for all file systems supported by Linux (with the 
possible exception of FAT).  I don't think Linux enforces a directory 
nesting limit, but if it does, it is significantly larger than 4095 
divided by 255.

You cannot directly open such files, but it's possible to reach them 
with (potentially multiple) calls to openat.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[Rich Felker]

On Fri, Jun 07, 2013 at 09:55:47AM +0200, Florian Weimer wrote:
> On 06/07/2013 03:30 AM, Rich Felker wrote:
> >On Thu, Jun 06, 2013 at 03:53:16PM -0400, KOSAKI Motohiro wrote:
> >>I mean, portable applications should use readdir_r correctly and Linux specific
> >>one should use readdir instead.
> >>
> >>Side note: the above man page is not a theoretical issue. At least, Solaris
> >>requires it.
> >>
> >>Am I missing something?
> >
> >Yes, the fact that the Austin Group is planning to require readdir to
> >be thread-safe and to mark readdir_r obsolescent.
> 
> This is good news.

Very good news. I've wanted this change ever since I first learned
about readdir_r, and I'm very glad this NAME_MAX issue has provided
the push to get it done.

> > So effort put into
> >making readdir_r more usable, or worse yet, adding a readdir4, is a
> >waste of effort. Just make sure readdir_r is _safe_ against buffer
> >overflows from buggy FUSE modules, and advise application developers
> >to use readdir, not readdir_r.
> 
> Does this mean that you agree with the basic approach of the patch?

Yes. I just disagree with recommending that portable applications use
readdir_r (as discussed on the Austin Group tracker/list, it has major
problems related to NAME_MAX not being mandatory) and with the idea
(by someone else, not you) to add a readdir4 rather than just
deprecating caller-provided buffers for reading directories. Those
were the only things I was commenting on.

Rich

Re: [PATCH] Fix readdir_r with long file names

[KOSAKI Motohiro]

(6/7/13 10:41 AM), Rich Felker wrote:
> On Fri, Jun 07, 2013 at 09:55:47AM +0200, Florian Weimer wrote:
>> On 06/07/2013 03:30 AM, Rich Felker wrote:
>>> On Thu, Jun 06, 2013 at 03:53:16PM -0400, KOSAKI Motohiro wrote:
>>>> I mean, portable applications should use readdir_r correctly and Linux specific
>>>> one should use readdir instead.
>>>>
>>>> Side note: the above man page is not a theoretical issue. At least, Solaris
>>>> requires it.
>>>>
>>>> Am I missing something?
>>>
>>> Yes, the fact that the Austin Group is planning to require readdir to
>>> be thread-safe and to mark readdir_r obsolescent.
>>
>> This is good news.
>
> Very good news. I've wanted this change ever since I first learned
> about readdir_r, and I'm very glad this NAME_MAX issue has provided
> the push to get it done.

That's pretty good news. So then, I'm ok to make a limitation into readdir_r()
and discourage to use it.

So....

> +@code{readdir} is not thread safe.  In @theglibc{} implementation,
> +multiple threads using @code{readdir} on the same @var{dirstream} may
> +overwrite the return value.

This is true. but not good wording choice. Many developers may parse this
mean you encourage to use readdir_r().

How's this?

+@code{readdir} is thread safe while @var{dirstream} is not shared from
+multiple threads. In @theglibc{} implementation, multiple threads using
+@code{readdir} on the same @var{dirstream} mayoverwrite the return value.


> +The use of @code{readdir_r} is a
> +thread-safe alternative, but may suffer from poor portability.  If
> +portability is required it is recommended that you use @code{readdir},
> +with external locking if multiple threads access the same
> +@var{dirstream}.

I suggest to remove this because I don't think we need to suggest semi
obsolete functions.



>
>>> So effort put into
>>> making readdir_r more usable, or worse yet, adding a readdir4, is a
>>> waste of effort. Just make sure readdir_r is _safe_ against buffer
>>> overflows from buggy FUSE modules, and advise application developers
>>> to use readdir, not readdir_r.
>>
>> Does this mean that you agree with the basic approach of the patch?
>
> Yes. I just disagree with recommending that portable applications use
> readdir_r (as discussed on the Austin Group tracker/list, it has major
> problems related to NAME_MAX not being mandatory) and with the idea
> (by someone else, not you) to add a readdir4 rather than just
> deprecating caller-provided buffers for reading directories. Those
> were the only things I was commenting on.

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 06/07/2013 04:41 PM, Rich Felker wrote:
> Yes. I just disagree with recommending that portable applications use
> readdir_r (as discussed on the Austin Group tracker/list, it has major
> problems related to NAME_MAX not being mandatory) and with the idea
> (by someone else, not you) to add a readdir4 rather than just
> deprecating caller-provided buffers for reading directories. Those
> were the only things I was commenting on.

Carlos, what do you think about this?  I tend to agree with Rich here 
and would like to back out this part of your suggestions again.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[Carlos O'Donell]

On 06/06/2013 12:47 PM, Florian Weimer wrote:
> On 06/06/2013 06:06 PM, Carlos O'Donell wrote:
>> Florian,
>>
>> Thanks for fixing this.
> 
> Thanks for the review, Carlos.
> 
>> Is there already a test case that tests for this?
>>
>> This seems like the kind of thing we should really be testing.
> 
> Most Linux file systems enforce the NAME_MAX limit, so it's difficult
> to trigger.  It requires a FUSE file system or a networked file
> system on a system which does not enforce the NAME_MAX limit.

OK, so it falls under the "too complex for the current framework 
to support" clause. Thanks for clarifying that. At some point I'll
try to propose some more complex QEMU/VM-based testing strategy
with fault injection to get coverage of all of these failure paths.

>>>   @comment POSIX.1
>>>   @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
>>>   This function reads the next entry from the directory.  It normally
>>> -returns a pointer to a structure containing information about the file.
>>> -This structure is statically allocated and can be rewritten by a
>>> -subsequent call.
>>> +returns a pointer to a structure containing information about the
>>> +file.  This structure is associated with the @var{dirstream} handle
>>> +and can be rewritten by a subsequent call.
>>
>> ... "subsequent call with the same handle." ???
> 
> On glibc, yes. There are rumors that there used to be systems where
> DIR * was actually an integer file descriptor and readdir used a
> statically allocated buffer. On such systems, readdir_r might
> actually be better than readdir, but on GNU/Linux, there are
> directories where readdir_r fails and readdir succeeds.

OK.

>> Suggest:
>> @code{readdir} is not thread safe.  Multiple threads using
>> @code{readdir} on the same @var{dirstream} may overwrite the return
>> value.  The use of @code{readdir_r} is a thread-safe alternative,
>> but may suffer from poor portability. If portability is required
>> it is recommended that you use @code{readdir} with external locking.
> 
> Here's what I'm proposing instead:
> 
> @code{readdir} is not thread safe.  In @theglibc{} implementation,
> multiple threads using @code{readdir} on the same @var{dirstream} may
> overwrite the return value.  The use of @code{readdir_r} is a
> thread-safe alternative, but may suffer from poor portability.  If
> portability is required it is recommended that you use @code{readdir},
> with external locking if multiple threads access the same
> @var{dirstream}.

Looks good to me.
 
> I followed your other suggestions.
> 
>> Please post a new version.
> 
> Attached.
> 
> -- 
> Florian Weimer / Red Hat Product Security Team
> 
> readdir_r.patch
> 
> 
> 2013-06-06  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #14699]
> 	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
> 	member.
> 	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
> 	member.
> 	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
> 	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
> 	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
> 	conditional.
> 	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
> 	GETDENTS_64BIT_ALIGNED.
> 	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
> 	* manual/filesys.texi (Reading/Closing Directory): Document
> 	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
> 	strongly.
> 	* manual/conf.texi (Limits for Files): Add portability note to
> 	NAME_MAX, PATH_MAX.
> 	(Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.

This new version looks good to me.

I haven't done thorough technical review, assuming that
the previous long conversation resolved all the technical
issues with the patch.

Cheers,
Carlos.

Re: [PATCH] Fix readdir_r with long file names

[Roland McGrath]

> There's precedent for my approach in realpath, where we don't use 
> pathconf(path, _PC_PATH_MAX), either, but cap the path length at PATH_MAX.

That's just following vanilla POSIX.1 rules: if PATH_MAX is defined at
compile time, it's a fixed system-wide limit.  If PATH_MAX is not defined
at compile time, there might or might not be a limit and if there is a
limit it might be filesystem-specific; pathconf reports that.

Re: [PATCH] Fix readdir_r with long file names

[Roland McGrath]

Respect _DIRENT_HAVE_D_NAMLEN and use strlen only if no d_namlen.
Make sure you have a space before paren in your function calls.

Re: [PATCH] Fix readdir_r with long file names

[Carlos O'Donell]

On 06/10/2013 03:20 AM, Florian Weimer wrote:
> On 06/07/2013 04:41 PM, Rich Felker wrote:
>> Yes. I just disagree with recommending that portable applications
>> use readdir_r (as discussed on the Austin Group tracker/list, it
>> has major problems related to NAME_MAX not being mandatory) and
>> with the idea (by someone else, not you) to add a readdir4 rather
>> than just deprecating caller-provided buffers for reading
>> directories. Those were the only things I was commenting on.
> 
> Carlos, what do you think about this?  I tend to agree with Rich here
> and would like to back out this part of your suggestions again.
 
I'm OK with backing out the recommendation of readdir_r as a portable
alternative, but the text should instead say *why* readdir_r is not 
a good portable alternative. That is to say we should specifically
dissuade the use if that's actually the truth.

Cheers,
Carlos.

Re: [PATCH] Fix readdir_r with long file names

[Rich Felker]

On Mon, Jun 10, 2013 at 07:17:59PM -0400, Carlos O'Donell wrote:
> On 06/10/2013 03:20 AM, Florian Weimer wrote:
> > On 06/07/2013 04:41 PM, Rich Felker wrote:
> >> Yes. I just disagree with recommending that portable applications
> >> use readdir_r (as discussed on the Austin Group tracker/list, it
> >> has major problems related to NAME_MAX not being mandatory) and
> >> with the idea (by someone else, not you) to add a readdir4 rather
> >> than just deprecating caller-provided buffers for reading
> >> directories. Those were the only things I was commenting on.
> > 
> > Carlos, what do you think about this?  I tend to agree with Rich here
> > and would like to back out this part of your suggestions again.
>  
> I'm OK with backing out the recommendation of readdir_r as a portable
> alternative, but the text should instead say *why* readdir_r is not 
> a good portable alternative. That is to say we should specifically
> dissuade the use if that's actually the truth.

I think the text should be informative and objective rather than
dogmatic. It should include the following information:

- On systems where NAME_MAX is not defined, readdir_r cannot be used
  safely, as the interface contract for readdir_r is specified in
  terms of NAME_MAX.

- On systems where NAME_MAX is defined but not enforced for all
  filesystems, there may be directory entries whose names are readable
  by readdir but not readdir_r, and attempts to read such names on
  older versions of glibc may result in exploitable buffer overflows.

- Historically, POSIX does not require readdir to be thread-safe, but
  on most (all?) known recent systems including glibc-based ones, it
  is thread-safe as long as the same directory stream (DIR*) is not
  accessed concurrently from multiple threads.

- Future versions of POSIX will mandate this level of thread-safety
  for the readdir function and mark readdir_r obsolescent.

If this is deemed "too technical", perhaps someone could write a short
summary that expresses the tradeoffs between the two interfaces. It
would be especially useful to know if my "all?" above really is "all
recent systems" or even "all historical implementations", since it
would make the choice for application developers much more clear-cut.

Rich

Re: [PATCH] Fix readdir_r with long file names

[Carlos O'Donell]

On 06/10/2013 09:13 PM, Rich Felker wrote:
> On Mon, Jun 10, 2013 at 07:17:59PM -0400, Carlos O'Donell wrote:
>> On 06/10/2013 03:20 AM, Florian Weimer wrote:
>>> On 06/07/2013 04:41 PM, Rich Felker wrote:
>>>> Yes. I just disagree with recommending that portable applications
>>>> use readdir_r (as discussed on the Austin Group tracker/list, it
>>>> has major problems related to NAME_MAX not being mandatory) and
>>>> with the idea (by someone else, not you) to add a readdir4 rather
>>>> than just deprecating caller-provided buffers for reading
>>>> directories. Those were the only things I was commenting on.
>>>
>>> Carlos, what do you think about this?  I tend to agree with Rich here
>>> and would like to back out this part of your suggestions again.
>>  
>> I'm OK with backing out the recommendation of readdir_r as a portable
>> alternative, but the text should instead say *why* readdir_r is not 
>> a good portable alternative. That is to say we should specifically
>> dissuade the use if that's actually the truth.
> 
> I think the text should be informative and objective rather than
> dogmatic. It should include the following information:
> 
> - On systems where NAME_MAX is not defined, readdir_r cannot be used
>   safely, as the interface contract for readdir_r is specified in
>   terms of NAME_MAX.
> 
> - On systems where NAME_MAX is defined but not enforced for all
>   filesystems, there may be directory entries whose names are readable
>   by readdir but not readdir_r, and attempts to read such names on
>   older versions of glibc may result in exploitable buffer overflows.
> 
> - Historically, POSIX does not require readdir to be thread-safe, but
>   on most (all?) known recent systems including glibc-based ones, it
>   is thread-safe as long as the same directory stream (DIR*) is not
>   accessed concurrently from multiple threads.
> 
> - Future versions of POSIX will mandate this level of thread-safety
>   for the readdir function and mark readdir_r obsolescent.
> 
> If this is deemed "too technical", perhaps someone could write a short
> summary that expresses the tradeoffs between the two interfaces. It
> would be especially useful to know if my "all?" above really is "all
> recent systems" or even "all historical implementations", since it
> would make the choice for application developers much more clear-cut.

I don't think it's too technical for a manual.

I can't comment on the "all?" bit, but I can say that you would be
safe simply saying that @theglibc does provide a thread-safe
implementation.

Cheers,
Carlos.

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 06/11/2013 12:39 AM, Roland McGrath wrote:
> Respect _DIRENT_HAVE_D_NAMLEN and use strlen only if no d_namlen.

You mean, replace the call to strlen with the _D_EXACT_NAMLEN macro?

 > Make sure you have a space before paren in your function calls.

*grr* Fixed.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 06/11/2013 12:36 AM, Roland McGrath wrote:
>> There's precedent for my approach in realpath, where we don't use
>> pathconf(path, _PC_PATH_MAX), either, but cap the path length at PATH_MAX.
>
> That's just following vanilla POSIX.1 rules: if PATH_MAX is defined at
> compile time, it's a fixed system-wide limit.  If PATH_MAX is not defined
> at compile time, there might or might not be a limit and if there is a
> limit it might be filesystem-specific; pathconf reports that.

We must continue to define PATH_MAX and NAME_MAX for backwards 
compatibility reasons.  This leads to ugly kludges when we're confronted 
with data from the kernel that exceeds the constants.  (getwd is another 
such place.)

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[Roland McGrath]

> On 06/11/2013 12:39 AM, Roland McGrath wrote:
> > Respect _DIRENT_HAVE_D_NAMLEN and use strlen only if no d_namlen.
> 
> You mean, replace the call to strlen with the _D_EXACT_NAMLEN macro?

Yes.

Re: [PATCH] Fix readdir_r with long file names

[Roland McGrath]

> On 06/11/2013 12:36 AM, Roland McGrath wrote:
> >> There's precedent for my approach in realpath, where we don't use
> >> pathconf(path, _PC_PATH_MAX), either, but cap the path length at PATH_MAX.
> >
> > That's just following vanilla POSIX.1 rules: if PATH_MAX is defined at
> > compile time, it's a fixed system-wide limit.  If PATH_MAX is not defined
> > at compile time, there might or might not be a limit and if there is a
> > limit it might be filesystem-specific; pathconf reports that.
> 
> We must continue to define PATH_MAX and NAME_MAX for backwards 
> compatibility reasons.  This leads to ugly kludges when we're confronted 
> with data from the kernel that exceeds the constants.  (getwd is another 
> such place.)

You cited the implementation details of canonicalize.c as an indication
that using PATH_MAX made some sort of intrinsic sense even if it might not
be the real limit.  I was pointing out that there was no such rationale
involved in the use there.  In fact, it was written that way to be right
for the GNU systems where PATH_MAX is not defined and there is no limit.

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

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

On 06/11/2013 03:13 AM, Rich Felker wrote:

> I think the text should be informative and objective rather than
> dogmatic. It should include the following information:
>
> - On systems where NAME_MAX is not defined, readdir_r cannot be used
>    safely, as the interface contract for readdir_r is specified in
>    terms of NAME_MAX.
>
> - On systems where NAME_MAX is defined but not enforced for all
>    filesystems, there may be directory entries whose names are readable
>    by readdir but not readdir_r, and attempts to read such names on
>    older versions of glibc may result in exploitable buffer overflows.
>
> - Historically, POSIX does not require readdir to be thread-safe, but
>    on most (all?) known recent systems including glibc-based ones, it
>    is thread-safe as long as the same directory stream (DIR*) is not
>    accessed concurrently from multiple threads.
>
> - Future versions of POSIX will mandate this level of thread-safety
>    for the readdir function and mark readdir_r obsolescent.

I tried to incorporate these points into the most recent version.  I 
will add the bug number to NEWS before the commit.  I also changed 
readdir_r to use _D_EXACT_NAMLEN, as requested by Roland.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: readdir_r.patch --]
[-- Type: text/x-patch, Size: 11293 bytes --]

2013-06-12  Florian Weimer  <fweimer@redhat.com>

	[BZ #14699]
	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
	member.
	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
	member.
	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
	conditional.
	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
	GETDENTS_64BIT_ALIGNED.
	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
	* manual/filesys.texi (Reading/Closing Directory): Document
	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
	strongly.
	* manual/conf.texi (Limits for Files): Add portability note to
	NAME_MAX, PATH_MAX.
	(Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.

diff --git a/manual/conf.texi b/manual/conf.texi
index 7eb8b36..c720063 100644
--- a/manual/conf.texi
+++ b/manual/conf.texi
@@ -1149,6 +1149,9 @@ typed ahead as input.  @xref{I/O Queues}.
 @deftypevr Macro int NAME_MAX
 The uniform system limit (if any) for the length of a file name component, not
 including the terminating null character.
+
+@strong{Portability Note:} On some systems, @theglibc{} defines
+@code{NAME_MAX}, but does not actually enforce this limit.
 @end deftypevr
 
 @comment limits.h
@@ -1157,6 +1160,9 @@ including the terminating null character.
 The uniform system limit (if any) for the length of an entire file name (that
 is, the argument given to system calls such as @code{open}), including the
 terminating null character.
+
+@strong{Portability Note:} @Theglibc{} does not enforce this limit
+even if @code{PATH_MAX} is defined.
 @end deftypevr
 
 @cindex limits, pipe buffer size
@@ -1476,6 +1482,9 @@ Inquire about the value of @code{POSIX_REC_MIN_XFER_SIZE}.
 Inquire about the value of @code{POSIX_REC_XFER_ALIGN}.
 @end table
 
+@strong{Portability Note:} On some systems, @theglibc{} does not
+enforce @code{_PC_NAME_MAX} or @code{_PC_PATH_MAX} limits.
+
 @node Utility Limits
 @section Utility Program Capacity Limits
 
diff --git a/manual/filesys.texi b/manual/filesys.texi
index 1df9cf2..79a510e 100644
--- a/manual/filesys.texi
+++ b/manual/filesys.texi
@@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
 @comment POSIX.1
 @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
 This function reads the next entry from the directory.  It normally
-returns a pointer to a structure containing information about the file.
-This structure is statically allocated and can be rewritten by a
-subsequent call.
+returns a pointer to a structure containing information about the
+file.  This structure is associated with the @var{dirstream} handle
+and can be rewritten by a subsequent call.
 
 @strong{Portability Note:} On some systems @code{readdir} may not
 return entries for @file{.} and @file{..}, even though these are always
@@ -461,19 +461,59 @@ conditions are defined for this function:
 The @var{dirstream} argument is not valid.
 @end table
 
-@code{readdir} is not thread safe.  Multiple threads using
-@code{readdir} on the same @var{dirstream} may overwrite the return
-value.  Use @code{readdir_r} when this is critical.
+To distinguish between an end-of-directory condition or an error, you
+must set @code{errno} to zero before calling @code{readdir}.  To avoid
+entering an infinite loop, you should stop reading from the directory
+after the first error.
+
+In POSIX.1-2008, @code{readdir} is not thread-safe.  In @theglibc{}
+implementation, it is safe to call @code{readdir} concurrently on
+different @var{dirstream}s (but multiple threads accessing the same
+@var{dirstream} result in undefined behavior).  @code{readdir_r} is a
+fully thread-safe alternative, but suffers from poor portability (see
+below).  It is recommended that you use @code{readdir}, with external
+locking if multiple threads access the same @var{dirstream}.
 @end deftypefun
 
 @comment dirent.h
 @comment GNU
 @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
-This function is the reentrant version of @code{readdir}.  Like
-@code{readdir} it returns the next entry from the directory.  But to
-prevent conflicts between simultaneously running threads the result is
-not stored in statically allocated memory.  Instead the argument
-@var{entry} points to a place to store the result.
+This function is a version of @code{readdir} which performs internal
+locking.  Like @code{readdir} it returns the next entry from the
+directory.  To prevent conflicts between simultaneously running
+threads the result is stored inside the @var{entry} object.
+
+@strong{Portability Note:} It is recommended to use @code{readdir}
+instead of @code{readdir_r} for the following reasons:
+
+@itemize @bullet
+@item
+On systems which do not define @code{NAME_MAX}, it may not be possible
+to use @code{readdir_r} safely because the caller does not specify the
+length of the buffer for the directory entry.
+
+@item
+On some systems, @code{readdir_r} cannot read directory entries with
+very long names.  If such a name is encountered, @theglibc{}
+implementation of @code{readdir_r} returns with an error code of
+@code{ENAMETOOLONG} after the final directory entry has been read.  On
+other systems, @code{readdir_r} may return successfully, but the
+@code{d_name} member may not be NUL-terminated or may be truncated.
+
+@item
+POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe,
+even when access to the same @var{dirstream} is serialized.  But in
+current implementations (including @theglibc{}), it is safe to call
+@code{readdir} concurrently on different @var{dirstream}s, so there is
+no requirement to use @code{readdir_r} even in multi-threaded
+programs.
+
+@item
+It is expected that future versions of POSIX will obsolete
+@code{readdir_r} and mandate the level of thread safety for
+@code{readdir} which is provided by @theglibc{} and other
+implementations today.
+@end itemize
 
 Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
 to @var{entry}.  If there are no more entries in the directory or an
@@ -481,15 +521,6 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
 null pointer and returns a nonzero error code, also stored in
 @code{errno}, as described for @code{readdir}.
 
-@strong{Portability Note:} On some systems @code{readdir_r} may not
-return a NUL terminated string for the file name, even when there is no
-@code{d_reclen} field in @code{struct dirent} and the file
-name is the maximum allowed size.  Modern systems all have the
-@code{d_reclen} field, and on old systems multi-threading is not
-critical.  In any case there is no such problem with the @code{readdir}
-function, so that even on systems without the @code{d_reclen} member one
-could use multiple threads by using external locking.
-
 It is also important to look at the definition of the @code{struct
 dirent} type.  Simply passing a pointer to an object of this type for
 the second parameter of @code{readdir_r} might not be enough.  Some
diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
index a7a074d..8e8570d 100644
--- a/sysdeps/posix/dirstream.h
+++ b/sysdeps/posix/dirstream.h
@@ -39,6 +39,8 @@ struct __dirstream
 
     off_t filepos;		/* Position of next entry to read.  */
 
+    int errcode;		/* Delayed error code.  */
+
     /* Directory block.  */
     char data[0] __attribute__ ((aligned (__alignof__ (void*))));
   };
diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index ddfc3a7..fc05b0f 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
   dirp->size = 0;
   dirp->offset = 0;
   dirp->filepos = 0;
+  dirp->errcode = 0;
 
   return dirp;
 }
diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
index b5a8e2e..8ed5c3f 100644
--- a/sysdeps/posix/readdir_r.c
+++ b/sysdeps/posix/readdir_r.c
@@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
   DIRENT_TYPE *dp;
   size_t reclen;
   const int saved_errno = errno;
+  int ret;
 
   __libc_lock_lock (dirp->lock);
 
@@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
 		  bytes = 0;
 		  __set_errno (saved_errno);
 		}
+	      if (bytes < 0)
+		dirp->errcode = errno;
 
 	      dp = NULL;
-	      /* Reclen != 0 signals that an error occurred.  */
-	      reclen = bytes != 0;
 	      break;
 	    }
 	  dirp->size = (size_t) bytes;
@@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
       dirp->filepos += reclen;
 #endif
 
-      /* Skip deleted files.  */
+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+	{
+	  /* The record is very long.  It could still fit into the
+	     caller-supplied buffer if we can skip padding at the
+	     end.  */
+	  size_t namelen = _D_EXACT_NAMLEN (dp);
+	  if (namelen <= NAME_MAX)
+	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+	  else
+	    {
+	      /* The name is too long.  Ignore this file.  */
+	      dirp->errcode = ENAMETOOLONG;
+	      dp->d_ino = 0;
+	      continue;
+	    }
+	}
+#endif
+
+      /* Skip deleted and ignored files.  */
     }
   while (dp->d_ino == 0);
 
   if (dp != NULL)
     {
-#ifdef GETDENTS_64BIT_ALIGNED
-      /* The d_reclen value might include padding which is not part of
-	 the DIRENT_TYPE data structure.  */
-      reclen = MIN (reclen,
-		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
-#endif
       *result = memcpy (entry, dp, reclen);
-#ifdef GETDENTS_64BIT_ALIGNED
+#ifdef _DIRENT_HAVE_D_RECLEN
       entry->d_reclen = reclen;
 #endif
+      ret = 0;
     }
   else
-    *result = NULL;
+    {
+      *result = NULL;
+      ret = dirp->errcode;
+    }
 
   __libc_lock_unlock (dirp->lock);
 
-  return dp != NULL ? 0 : reclen ? errno : 0;
+  return ret;
 }
 
 #ifdef __READDIR_R_ALIAS
diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
index 2935a8e..d4991ad 100644
--- a/sysdeps/posix/rewinddir.c
+++ b/sysdeps/posix/rewinddir.c
@@ -33,6 +33,7 @@ rewinddir (dirp)
   dirp->filepos = 0;
   dirp->offset = 0;
   dirp->size = 0;
+  dirp->errcode = 0;
 #ifndef NOT_IN_libc
   __libc_lock_unlock (dirp->lock);
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
index 8ebbcfd..a7d114e 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
@@ -18,7 +18,6 @@
 #define __READDIR_R __readdir64_r
 #define __GETDENTS __getdents64
 #define DIRENT_TYPE struct dirent64
-#define GETDENTS_64BIT_ALIGNED 1
 
 #include <sysdeps/posix/readdir_r.c>
 
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
index 5ed8e95..290f2c8 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
+++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
@@ -1,5 +1,4 @@
 #define readdir64_r __no_readdir64_r_decl
-#define GETDENTS_64BIT_ALIGNED 1
 #include <sysdeps/posix/readdir_r.c>
 #undef readdir64_r
 weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Siddhesh Poyarekar]

On Wed, Jun 12, 2013 at 02:57:17PM +0200, Florian Weimer wrote:
> I tried to incorporate these points into the most recent version.  I
> will add the bug number to NEWS before the commit.  I also changed
> readdir_r to use _D_EXACT_NAMLEN, as requested by Roland.

This is now CVE-2013-4237.

> 2013-06-12  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #14699]
> 	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
> 	member.
> 	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
> 	member.
> 	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
> 	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
> 	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
> 	conditional.
> 	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
> 	GETDENTS_64BIT_ALIGNED.
> 	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
> 	* manual/filesys.texi (Reading/Closing Directory): Document
> 	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
> 	strongly.
> 	* manual/conf.texi (Limits for Files): Add portability note to
> 	NAME_MAX, PATH_MAX.
> 	(Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.
> 
> diff --git a/manual/conf.texi b/manual/conf.texi
> index 7eb8b36..c720063 100644
> --- a/manual/conf.texi
> +++ b/manual/conf.texi
> @@ -1149,6 +1149,9 @@ typed ahead as input.  @xref{I/O Queues}.
>  @deftypevr Macro int NAME_MAX
>  The uniform system limit (if any) for the length of a file name component, not
>  including the terminating null character.
> +
> +@strong{Portability Note:} On some systems, @theglibc{} defines
> +@code{NAME_MAX}, but does not actually enforce this limit.
>  @end deftypevr
>  
>  @comment limits.h
> @@ -1157,6 +1160,9 @@ including the terminating null character.
>  The uniform system limit (if any) for the length of an entire file name (that
>  is, the argument given to system calls such as @code{open}), including the
>  terminating null character.
> +
> +@strong{Portability Note:} @Theglibc{} does not enforce this limit
> +even if @code{PATH_MAX} is defined.
>  @end deftypevr
>  
>  @cindex limits, pipe buffer size
> @@ -1476,6 +1482,9 @@ Inquire about the value of @code{POSIX_REC_MIN_XFER_SIZE}.
>  Inquire about the value of @code{POSIX_REC_XFER_ALIGN}.
>  @end table
>  
> +@strong{Portability Note:} On some systems, @theglibc{} does not
> +enforce @code{_PC_NAME_MAX} or @code{_PC_PATH_MAX} limits.
> +
>  @node Utility Limits
>  @section Utility Program Capacity Limits
>  
> diff --git a/manual/filesys.texi b/manual/filesys.texi
> index 1df9cf2..79a510e 100644
> --- a/manual/filesys.texi
> +++ b/manual/filesys.texi
> @@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
>  @comment POSIX.1
>  @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
>  This function reads the next entry from the directory.  It normally
> -returns a pointer to a structure containing information about the file.
> -This structure is statically allocated and can be rewritten by a
> -subsequent call.
> +returns a pointer to a structure containing information about the
> +file.  This structure is associated with the @var{dirstream} handle
> +and can be rewritten by a subsequent call.
>  
>  @strong{Portability Note:} On some systems @code{readdir} may not
>  return entries for @file{.} and @file{..}, even though these are always
> @@ -461,19 +461,59 @@ conditions are defined for this function:
>  The @var{dirstream} argument is not valid.
>  @end table
>  
> -@code{readdir} is not thread safe.  Multiple threads using
> -@code{readdir} on the same @var{dirstream} may overwrite the return
> -value.  Use @code{readdir_r} when this is critical.
> +To distinguish between an end-of-directory condition or an error, you
> +must set @code{errno} to zero before calling @code{readdir}.  To avoid
> +entering an infinite loop, you should stop reading from the directory
> +after the first error.
> +
> +In POSIX.1-2008, @code{readdir} is not thread-safe.  In @theglibc{}
> +implementation, it is safe to call @code{readdir} concurrently on
> +different @var{dirstream}s (but multiple threads accessing the same
> +@var{dirstream} result in undefined behavior).  @code{readdir_r} is a

Minor nit - you could get rid of the round brackets and simply write
the line as:

In @theglibc{} implementation, it is safe to call @code{readdir}
concurrently on different @var{dirstream}s, but multiple threads
accessing the same @var{dirstream} result in undefined behavior.

> +fully thread-safe alternative, but suffers from poor portability (see
> +below).  It is recommended that you use @code{readdir}, with external
> +locking if multiple threads access the same @var{dirstream}.
>  @end deftypefun
>  
>  @comment dirent.h
>  @comment GNU
>  @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
> -This function is the reentrant version of @code{readdir}.  Like
> -@code{readdir} it returns the next entry from the directory.  But to
> -prevent conflicts between simultaneously running threads the result is
> -not stored in statically allocated memory.  Instead the argument
> -@var{entry} points to a place to store the result.
> +This function is a version of @code{readdir} which performs internal
> +locking.  Like @code{readdir} it returns the next entry from the
> +directory.  To prevent conflicts between simultaneously running
> +threads the result is stored inside the @var{entry} object.
> +
> +@strong{Portability Note:} It is recommended to use @code{readdir}
> +instead of @code{readdir_r} for the following reasons:
> +
> +@itemize @bullet
> +@item
> +On systems which do not define @code{NAME_MAX}, it may not be possible
> +to use @code{readdir_r} safely because the caller does not specify the
> +length of the buffer for the directory entry.
> +
> +@item
> +On some systems, @code{readdir_r} cannot read directory entries with
> +very long names.  If such a name is encountered, @theglibc{}
> +implementation of @code{readdir_r} returns with an error code of
> +@code{ENAMETOOLONG} after the final directory entry has been read.  On
> +other systems, @code{readdir_r} may return successfully, but the
> +@code{d_name} member may not be NUL-terminated or may be truncated.
> +
> +@item
> +POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe,
> +even when access to the same @var{dirstream} is serialized.  But in
> +current implementations (including @theglibc{}), it is safe to call
> +@code{readdir} concurrently on different @var{dirstream}s, so there is
> +no requirement to use @code{readdir_r} even in multi-threaded
> +programs.
> +

This seems to gloss over the fact that one would need synchronization
(or readdir_r) if readdir is called concurrently on the same
dirstream.  It seems like a bad idea to do this at all, but we
probably should add a note about it anyway.

The rest seems fine to me, so could you post an updated patch (or
justification) for the above comments?

Thanks,
Siddhesh

> +@item
> +It is expected that future versions of POSIX will obsolete
> +@code{readdir_r} and mandate the level of thread safety for
> +@code{readdir} which is provided by @theglibc{} and other
> +implementations today.
> +@end itemize
>  
>  Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
>  to @var{entry}.  If there are no more entries in the directory or an
> @@ -481,15 +521,6 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
>  null pointer and returns a nonzero error code, also stored in
>  @code{errno}, as described for @code{readdir}.
>  
> -@strong{Portability Note:} On some systems @code{readdir_r} may not
> -return a NUL terminated string for the file name, even when there is no
> -@code{d_reclen} field in @code{struct dirent} and the file
> -name is the maximum allowed size.  Modern systems all have the
> -@code{d_reclen} field, and on old systems multi-threading is not
> -critical.  In any case there is no such problem with the @code{readdir}
> -function, so that even on systems without the @code{d_reclen} member one
> -could use multiple threads by using external locking.
> -
>  It is also important to look at the definition of the @code{struct
>  dirent} type.  Simply passing a pointer to an object of this type for
>  the second parameter of @code{readdir_r} might not be enough.  Some
> diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
> index a7a074d..8e8570d 100644
> --- a/sysdeps/posix/dirstream.h
> +++ b/sysdeps/posix/dirstream.h
> @@ -39,6 +39,8 @@ struct __dirstream
>  
>      off_t filepos;		/* Position of next entry to read.  */
>  
> +    int errcode;		/* Delayed error code.  */
> +
>      /* Directory block.  */
>      char data[0] __attribute__ ((aligned (__alignof__ (void*))));
>    };
> diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
> index ddfc3a7..fc05b0f 100644
> --- a/sysdeps/posix/opendir.c
> +++ b/sysdeps/posix/opendir.c
> @@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
>    dirp->size = 0;
>    dirp->offset = 0;
>    dirp->filepos = 0;
> +  dirp->errcode = 0;
>  
>    return dirp;
>  }
> diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
> index b5a8e2e..8ed5c3f 100644
> --- a/sysdeps/posix/readdir_r.c
> +++ b/sysdeps/posix/readdir_r.c
> @@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>    DIRENT_TYPE *dp;
>    size_t reclen;
>    const int saved_errno = errno;
> +  int ret;
>  
>    __libc_lock_lock (dirp->lock);
>  
> @@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>  		  bytes = 0;
>  		  __set_errno (saved_errno);
>  		}
> +	      if (bytes < 0)
> +		dirp->errcode = errno;
>  
>  	      dp = NULL;
> -	      /* Reclen != 0 signals that an error occurred.  */
> -	      reclen = bytes != 0;
>  	      break;
>  	    }
>  	  dirp->size = (size_t) bytes;
> @@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
>        dirp->filepos += reclen;
>  #endif
>  
> -      /* Skip deleted files.  */
> +#ifdef NAME_MAX
> +      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
> +	{
> +	  /* The record is very long.  It could still fit into the
> +	     caller-supplied buffer if we can skip padding at the
> +	     end.  */
> +	  size_t namelen = _D_EXACT_NAMLEN (dp);
> +	  if (namelen <= NAME_MAX)
> +	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
> +	  else
> +	    {
> +	      /* The name is too long.  Ignore this file.  */
> +	      dirp->errcode = ENAMETOOLONG;
> +	      dp->d_ino = 0;
> +	      continue;
> +	    }
> +	}
> +#endif
> +
> +      /* Skip deleted and ignored files.  */
>      }
>    while (dp->d_ino == 0);
>  
>    if (dp != NULL)
>      {
> -#ifdef GETDENTS_64BIT_ALIGNED
> -      /* The d_reclen value might include padding which is not part of
> -	 the DIRENT_TYPE data structure.  */
> -      reclen = MIN (reclen,
> -		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
> -#endif
>        *result = memcpy (entry, dp, reclen);
> -#ifdef GETDENTS_64BIT_ALIGNED
> +#ifdef _DIRENT_HAVE_D_RECLEN
>        entry->d_reclen = reclen;
>  #endif
> +      ret = 0;
>      }
>    else
> -    *result = NULL;
> +    {
> +      *result = NULL;
> +      ret = dirp->errcode;
> +    }
>  
>    __libc_lock_unlock (dirp->lock);
>  
> -  return dp != NULL ? 0 : reclen ? errno : 0;
> +  return ret;
>  }
>  
>  #ifdef __READDIR_R_ALIAS
> diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
> index 2935a8e..d4991ad 100644
> --- a/sysdeps/posix/rewinddir.c
> +++ b/sysdeps/posix/rewinddir.c
> @@ -33,6 +33,7 @@ rewinddir (dirp)
>    dirp->filepos = 0;
>    dirp->offset = 0;
>    dirp->size = 0;
> +  dirp->errcode = 0;
>  #ifndef NOT_IN_libc
>    __libc_lock_unlock (dirp->lock);
>  #endif
> diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> index 8ebbcfd..a7d114e 100644
> --- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> +++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
> @@ -18,7 +18,6 @@
>  #define __READDIR_R __readdir64_r
>  #define __GETDENTS __getdents64
>  #define DIRENT_TYPE struct dirent64
> -#define GETDENTS_64BIT_ALIGNED 1
>  
>  #include <sysdeps/posix/readdir_r.c>
>  
> diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> index 5ed8e95..290f2c8 100644
> --- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> +++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
> @@ -1,5 +1,4 @@
>  #define readdir64_r __no_readdir64_r_decl
> -#define GETDENTS_64BIT_ALIGNED 1
>  #include <sysdeps/posix/readdir_r.c>
>  #undef readdir64_r
>  weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

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

On 08/13/2013 06:00 AM, Siddhesh Poyarekar wrote:
>> +In POSIX.1-2008, @code{readdir} is not thread-safe.  In @theglibc{}
>> +implementation, it is safe to call @code{readdir} concurrently on
>> +different @var{dirstream}s (but multiple threads accessing the same
>> +@var{dirstream} result in undefined behavior).  @code{readdir_r} is a
>
> Minor nit - you could get rid of the round brackets and simply write
> the line as:
>
> In @theglibc{} implementation, it is safe to call @code{readdir}
> concurrently on different @var{dirstream}s, but multiple threads
> accessing the same @var{dirstream} result in undefined behavior.

Applied, thanks.

>> +@item
>> +POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe,
>> +even when access to the same @var{dirstream} is serialized.  But in
>> +current implementations (including @theglibc{}), it is safe to call
>> +@code{readdir} concurrently on different @var{dirstream}s, so there is
>> +no requirement to use @code{readdir_r} even in multi-threaded
>> +programs.
>> +
>
> This seems to gloss over the fact that one would need synchronization
> (or readdir_r) if readdir is called concurrently on the same
> dirstream.  It seems like a bad idea to do this at all, but we
> probably should add a note about it anyway.

I ended up with this:

+@item
+POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe,
+even when access to the same @var{dirstream} is serialized.  But in
+current implementations (including @theglibc{}), it is safe to call
+@code{readdir} concurrently on different @var{dirstream}s, so there is
+no need to use @code{readdir_r} in most multi-threaded programs.  In
+the rare case that multiple threads need to read from the same
+@var{dirstream}, it is still better to use @code{readdir} and external
+synchronization.

I'm attaching my current version.  It compiles, but tests are still running.

I'll update the NEWS file with the bug number on the final commit.

-- 
Florian Weimer / Red Hat Product Security Team

[-- Attachment #2: readdir_r-20130815.patch --]
[-- Type: text/x-patch, Size: 11457 bytes --]

2013-08-15  Florian Weimer  <fweimer@redhat.com>

	[BZ #14699]
	CVE-2013-4237
	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
	member.
	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
	member.
	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
	conditional.
	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
	GETDENTS_64BIT_ALIGNED.
	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
	* manual/filesys.texi (Reading/Closing Directory): Document
	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
	strongly.
	* manual/conf.texi (Limits for Files): Add portability note to
	NAME_MAX, PATH_MAX.
	(Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.

diff --git a/manual/conf.texi b/manual/conf.texi
index 7eb8b36..c720063 100644
--- a/manual/conf.texi
+++ b/manual/conf.texi
@@ -1149,6 +1149,9 @@ typed ahead as input.  @xref{I/O Queues}.
 @deftypevr Macro int NAME_MAX
 The uniform system limit (if any) for the length of a file name component, not
 including the terminating null character.
+
+@strong{Portability Note:} On some systems, @theglibc{} defines
+@code{NAME_MAX}, but does not actually enforce this limit.
 @end deftypevr
 
 @comment limits.h
@@ -1157,6 +1160,9 @@ including the terminating null character.
 The uniform system limit (if any) for the length of an entire file name (that
 is, the argument given to system calls such as @code{open}), including the
 terminating null character.
+
+@strong{Portability Note:} @Theglibc{} does not enforce this limit
+even if @code{PATH_MAX} is defined.
 @end deftypevr
 
 @cindex limits, pipe buffer size
@@ -1476,6 +1482,9 @@ Inquire about the value of @code{POSIX_REC_MIN_XFER_SIZE}.
 Inquire about the value of @code{POSIX_REC_XFER_ALIGN}.
 @end table
 
+@strong{Portability Note:} On some systems, @theglibc{} does not
+enforce @code{_PC_NAME_MAX} or @code{_PC_PATH_MAX} limits.
+
 @node Utility Limits
 @section Utility Program Capacity Limits
 
diff --git a/manual/filesys.texi b/manual/filesys.texi
index 1df9cf2..814c210 100644
--- a/manual/filesys.texi
+++ b/manual/filesys.texi
@@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
 @comment POSIX.1
 @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
 This function reads the next entry from the directory.  It normally
-returns a pointer to a structure containing information about the file.
-This structure is statically allocated and can be rewritten by a
-subsequent call.
+returns a pointer to a structure containing information about the
+file.  This structure is associated with the @var{dirstream} handle
+and can be rewritten by a subsequent call.
 
 @strong{Portability Note:} On some systems @code{readdir} may not
 return entries for @file{.} and @file{..}, even though these are always
@@ -461,19 +461,61 @@ conditions are defined for this function:
 The @var{dirstream} argument is not valid.
 @end table
 
-@code{readdir} is not thread safe.  Multiple threads using
-@code{readdir} on the same @var{dirstream} may overwrite the return
-value.  Use @code{readdir_r} when this is critical.
+To distinguish between an end-of-directory condition or an error, you
+must set @code{errno} to zero before calling @code{readdir}.  To avoid
+entering an infinite loop, you should stop reading from the directory
+after the first error.
+
+In POSIX.1-2008, @code{readdir} is not thread-safe.  In @theglibc{}
+implementation, it is safe to call @code{readdir} concurrently on
+different @var{dirstream}s, but multiple threads accessing the same
+@var{dirstream} result in undefined behavior.  @code{readdir_r} is a
+fully thread-safe alternative, but suffers from poor portability (see
+below).  It is recommended that you use @code{readdir}, with external
+locking if multiple threads access the same @var{dirstream}.
 @end deftypefun
 
 @comment dirent.h
 @comment GNU
 @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
-This function is the reentrant version of @code{readdir}.  Like
-@code{readdir} it returns the next entry from the directory.  But to
-prevent conflicts between simultaneously running threads the result is
-not stored in statically allocated memory.  Instead the argument
-@var{entry} points to a place to store the result.
+This function is a version of @code{readdir} which performs internal
+locking.  Like @code{readdir} it returns the next entry from the
+directory.  To prevent conflicts between simultaneously running
+threads the result is stored inside the @var{entry} object.
+
+@strong{Portability Note:} It is recommended to use @code{readdir}
+instead of @code{readdir_r} for the following reasons:
+
+@itemize @bullet
+@item
+On systems which do not define @code{NAME_MAX}, it may not be possible
+to use @code{readdir_r} safely because the caller does not specify the
+length of the buffer for the directory entry.
+
+@item
+On some systems, @code{readdir_r} cannot read directory entries with
+very long names.  If such a name is encountered, @theglibc{}
+implementation of @code{readdir_r} returns with an error code of
+@code{ENAMETOOLONG} after the final directory entry has been read.  On
+other systems, @code{readdir_r} may return successfully, but the
+@code{d_name} member may not be NUL-terminated or may be truncated.
+
+@item
+POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe,
+even when access to the same @var{dirstream} is serialized.  But in
+current implementations (including @theglibc{}), it is safe to call
+@code{readdir} concurrently on different @var{dirstream}s, so there is
+no need to use @code{readdir_r} in most multi-threaded programs.  In
+the rare case that multiple threads need to read from the same
+@var{dirstream}, it is still better to use @code{readdir} and external
+synchronization.
+
+@item
+It is expected that future versions of POSIX will obsolete
+@code{readdir_r} and mandate the level of thread safety for
+@code{readdir} which is provided by @theglibc{} and other
+implementations today.
+@end itemize
 
 Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
 to @var{entry}.  If there are no more entries in the directory or an
@@ -481,15 +523,6 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
 null pointer and returns a nonzero error code, also stored in
 @code{errno}, as described for @code{readdir}.
 
-@strong{Portability Note:} On some systems @code{readdir_r} may not
-return a NUL terminated string for the file name, even when there is no
-@code{d_reclen} field in @code{struct dirent} and the file
-name is the maximum allowed size.  Modern systems all have the
-@code{d_reclen} field, and on old systems multi-threading is not
-critical.  In any case there is no such problem with the @code{readdir}
-function, so that even on systems without the @code{d_reclen} member one
-could use multiple threads by using external locking.
-
 It is also important to look at the definition of the @code{struct
 dirent} type.  Simply passing a pointer to an object of this type for
 the second parameter of @code{readdir_r} might not be enough.  Some
diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
index a7a074d..8e8570d 100644
--- a/sysdeps/posix/dirstream.h
+++ b/sysdeps/posix/dirstream.h
@@ -39,6 +39,8 @@ struct __dirstream
 
     off_t filepos;		/* Position of next entry to read.  */
 
+    int errcode;		/* Delayed error code.  */
+
     /* Directory block.  */
     char data[0] __attribute__ ((aligned (__alignof__ (void*))));
   };
diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index ddfc3a7..fc05b0f 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
   dirp->size = 0;
   dirp->offset = 0;
   dirp->filepos = 0;
+  dirp->errcode = 0;
 
   return dirp;
 }
diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
index b5a8e2e..8ed5c3f 100644
--- a/sysdeps/posix/readdir_r.c
+++ b/sysdeps/posix/readdir_r.c
@@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
   DIRENT_TYPE *dp;
   size_t reclen;
   const int saved_errno = errno;
+  int ret;
 
   __libc_lock_lock (dirp->lock);
 
@@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
 		  bytes = 0;
 		  __set_errno (saved_errno);
 		}
+	      if (bytes < 0)
+		dirp->errcode = errno;
 
 	      dp = NULL;
-	      /* Reclen != 0 signals that an error occurred.  */
-	      reclen = bytes != 0;
 	      break;
 	    }
 	  dirp->size = (size_t) bytes;
@@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
       dirp->filepos += reclen;
 #endif
 
-      /* Skip deleted files.  */
+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+	{
+	  /* The record is very long.  It could still fit into the
+	     caller-supplied buffer if we can skip padding at the
+	     end.  */
+	  size_t namelen = _D_EXACT_NAMLEN (dp);
+	  if (namelen <= NAME_MAX)
+	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+	  else
+	    {
+	      /* The name is too long.  Ignore this file.  */
+	      dirp->errcode = ENAMETOOLONG;
+	      dp->d_ino = 0;
+	      continue;
+	    }
+	}
+#endif
+
+      /* Skip deleted and ignored files.  */
     }
   while (dp->d_ino == 0);
 
   if (dp != NULL)
     {
-#ifdef GETDENTS_64BIT_ALIGNED
-      /* The d_reclen value might include padding which is not part of
-	 the DIRENT_TYPE data structure.  */
-      reclen = MIN (reclen,
-		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
-#endif
       *result = memcpy (entry, dp, reclen);
-#ifdef GETDENTS_64BIT_ALIGNED
+#ifdef _DIRENT_HAVE_D_RECLEN
       entry->d_reclen = reclen;
 #endif
+      ret = 0;
     }
   else
-    *result = NULL;
+    {
+      *result = NULL;
+      ret = dirp->errcode;
+    }
 
   __libc_lock_unlock (dirp->lock);
 
-  return dp != NULL ? 0 : reclen ? errno : 0;
+  return ret;
 }
 
 #ifdef __READDIR_R_ALIAS
diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
index 2935a8e..d4991ad 100644
--- a/sysdeps/posix/rewinddir.c
+++ b/sysdeps/posix/rewinddir.c
@@ -33,6 +33,7 @@ rewinddir (dirp)
   dirp->filepos = 0;
   dirp->offset = 0;
   dirp->size = 0;
+  dirp->errcode = 0;
 #ifndef NOT_IN_libc
   __libc_lock_unlock (dirp->lock);
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
index 8ebbcfd..a7d114e 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
@@ -18,7 +18,6 @@
 #define __READDIR_R __readdir64_r
 #define __GETDENTS __getdents64
 #define DIRENT_TYPE struct dirent64
-#define GETDENTS_64BIT_ALIGNED 1
 
 #include <sysdeps/posix/readdir_r.c>
 
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
index 5ed8e95..290f2c8 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
+++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
@@ -1,5 +1,4 @@
 #define readdir64_r __no_readdir64_r_decl
-#define GETDENTS_64BIT_ALIGNED 1
 #include <sysdeps/posix/readdir_r.c>
 #undef readdir64_r
 weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Siddhesh Poyarekar]

On Thu, Aug 15, 2013 at 09:52:06AM +0200, Florian Weimer wrote:
> 
> I'm attaching my current version.  It compiles, but tests are still running.
> 
> I'll update the NEWS file with the bug number on the final commit.
> 

Looks OK.

Thanks,
Siddhesh

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 08/16/2013 05:44 AM, Siddhesh Poyarekar wrote:
> On Thu, Aug 15, 2013 at 09:52:06AM +0200, Florian Weimer wrote:
>>
>> I'm attaching my current version.  It compiles, but tests are still running.
>>
>> I'll update the NEWS file with the bug number on the final commit.

> Looks OK.

I have committed the patch.  Thanks for providing valuable feedback to 
all of you.

-- 
Florian Weimer / Red Hat Product Security Team

Re: [PATCH] Fix readdir_r with long file names

[Michael Kerrisk (man-pages)]

Hello Florian,

On 08/15/2013 09:52 AM, Florian Weimer wrote:
> On 08/13/2013 06:00 AM, Siddhesh Poyarekar wrote:
>>> +In POSIX.1-2008, @code{readdir} is not thread-safe.  In @theglibc{}
>>> +implementation, it is safe to call @code{readdir} concurrently on
>>> +different @var{dirstream}s (but multiple threads accessing the same
>>> +@var{dirstream} result in undefined behavior).  @code{readdir_r} is a
>>
>> Minor nit - you could get rid of the round brackets and simply write
>> the line as:
>>
>> In @theglibc{} implementation, it is safe to call @code{readdir}
>> concurrently on different @var{dirstream}s, but multiple threads
>> accessing the same @var{dirstream} result in undefined behavior.
> 
> Applied, thanks.
> 
>>> +@item
>>> +POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe,
>>> +even when access to the same @var{dirstream} is serialized.  But in
>>> +current implementations (including @theglibc{}), it is safe to call
>>> +@code{readdir} concurrently on different @var{dirstream}s, so there is
>>> +no requirement to use @code{readdir_r} even in multi-threaded
>>> +programs.
>>> +
>>
>> This seems to gloss over the fact that one would need synchronization
>> (or readdir_r) if readdir is called concurrently on the same
>> dirstream.  It seems like a bad idea to do this at all, but we
>> probably should add a note about it anyway.
> 
> I ended up with this:
> 
> +@item
> +POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe,
> +even when access to the same @var{dirstream} is serialized.  But in
> +current implementations (including @theglibc{}), it is safe to call
> +@code{readdir} concurrently on different @var{dirstream}s, so there is
> +no need to use @code{readdir_r} in most multi-threaded programs.  In
> +the rare case that multiple threads need to read from the same
> +@var{dirstream}, it is still better to use @code{readdir} and external
> +synchronization.
> 
> I'm attaching my current version.  It compiles, but tests are still running.
> 
> I'll update the NEWS file with the bug number on the final commit.

I see that glibc 2.23 deprecates readdir_r(), which prompted me to catch
up on this thread. I'd like to see the points you make documented in the
readdir_r(3) man page also. Would you be willing to allow that text to
be reused / reworked for the page, under that page's existing "verbatim"
license (https://www.kernel.org/doc/man-pages/licenses.html#verbatim)?

The text I'd propose to add to the man page would be (new material
starting at ===>):

  readdir_r()
       The readdir_r() function is a reentrant version  of  readdir().
       It  reads  the  next  directory entry from the directory stream
       dirp, and returns it in the caller-allocated buffer pointed  to
       by entry.  A pointer to the returned item is placed in *result;
       if the end of the directory stream was encountered,  then  NULL
       is instead returned in *result.

       Since  POSIX.1  does  not specify the size of the d_name field,
       and other nonstandard fields may precede that field within  the
       dirent  structure,  portable  applications that use readdir_r()
       could allocate the buffer whose address is passed in  entry  as
       follows:

           name_max = pathconf(dirpath, _PC_NAME_MAX);
           if (name_max == -1)         /* Limit not defined, or error */
               name_max = 255;         /* Take a guess */
           len = offsetof(struct dirent, d_name) + name_max + 1;
           entryp = malloc(len);

       (POSIX.1  requires  that  d_name  is the last field in a struct
       dirent.)

===>   However, the above approach has problems, and it is recommended
       that  applications  use readdir() instead of readdir_r().  Fur‐
       thermore, since version  2.23,  glibc  deprecates  readdir_r().
       The reasons are as follows:

       *  On  systems where NAME_MAX is undefined, calling readdir_r()
          may be unsafe because the interface does not allow the call‐
          er to specify the length of the buffer used for the returned
          directory entry.

       *  On some systems, readdir_r() can't  read  directory  entries
          with very long names.  When the glibc implementation encoun‐
          ters such a name, readdir_r() fails with the error ENAMETOO‐
          LONG after the final directory entry has been read.  On some
          other systems, readdir_r() may return a success status,  but
          the  returned d_name field may not be null terminated or may
          be truncated.

       *  In the current POSIX.1 specification  (POSIX.1-2008),  read‐
          dir_r() is not required to be thread-safe.  However, in mod‐
          ern implementations (including  the  glibc  implementation),
          concurrent  calls  to  readdir_r()  that  specify  different
          directory streams are thread-safe.  Therefore,  the  use  of
          readdir_r()  is  generally unnecessary in multithreaded pro‐
          grams.  In cases where multiple threads must read  from  the
          same  directory  stream,  using readdir() with external syn‐
          chronization is still preferable to the use of  readdir_r(),
          for the reasons given in the points above.

       *  It  is  expected  that a future version of POSIX.1 will make
          readdir_r() obsolete, and require that readdir() be  thread-
          safe  when  concurrently  employed  on  different  directory
          streams.

Cheers,

Michael

===================================
2013-08-15  Florian Weimer  <fweimer@redhat.com>

	[BZ #14699]
	CVE-2013-4237
	* sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
	member.
	* sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
	member.
	* sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
	* sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
	Return delayed error code.  Remove GETDENTS_64BIT_ALIGNED
	conditional.
	* sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
	GETDENTS_64BIT_ALIGNED.
	* sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
	* manual/filesys.texi (Reading/Closing Directory): Document
	ENAMETOOLONG return value of readdir_r.  Recommend readdir more
	strongly.
	* manual/conf.texi (Limits for Files): Add portability note to
	NAME_MAX, PATH_MAX.
	(Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.

diff --git a/manual/conf.texi b/manual/conf.texi
index 7eb8b36..c720063 100644
--- a/manual/conf.texi
+++ b/manual/conf.texi
@@ -1149,6 +1149,9 @@ typed ahead as input.  @xref{I/O Queues}.
 @deftypevr Macro int NAME_MAX
 The uniform system limit (if any) for the length of a file name component, not
 including the terminating null character.
+
+@strong{Portability Note:} On some systems, @theglibc{} defines
+@code{NAME_MAX}, but does not actually enforce this limit.
 @end deftypevr
 
 @comment limits.h
@@ -1157,6 +1160,9 @@ including the terminating null character.
 The uniform system limit (if any) for the length of an entire file name (that
 is, the argument given to system calls such as @code{open}), including the
 terminating null character.
+
+@strong{Portability Note:} @Theglibc{} does not enforce this limit
+even if @code{PATH_MAX} is defined.
 @end deftypevr
 
 @cindex limits, pipe buffer size
@@ -1476,6 +1482,9 @@ Inquire about the value of @code{POSIX_REC_MIN_XFER_SIZE}.
 Inquire about the value of @code{POSIX_REC_XFER_ALIGN}.
 @end table
 
+@strong{Portability Note:} On some systems, @theglibc{} does not
+enforce @code{_PC_NAME_MAX} or @code{_PC_PATH_MAX} limits.
+
 @node Utility Limits
 @section Utility Program Capacity Limits
 
diff --git a/manual/filesys.texi b/manual/filesys.texi
index 1df9cf2..814c210 100644
--- a/manual/filesys.texi
+++ b/manual/filesys.texi
@@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}.
 @comment POSIX.1
 @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
 This function reads the next entry from the directory.  It normally
-returns a pointer to a structure containing information about the file.
-This structure is statically allocated and can be rewritten by a
-subsequent call.
+returns a pointer to a structure containing information about the
+file.  This structure is associated with the @var{dirstream} handle
+and can be rewritten by a subsequent call.
 
 @strong{Portability Note:} On some systems @code{readdir} may not
 return entries for @file{.} and @file{..}, even though these are always
@@ -461,19 +461,61 @@ conditions are defined for this function:
 The @var{dirstream} argument is not valid.
 @end table
 
-@code{readdir} is not thread safe.  Multiple threads using
-@code{readdir} on the same @var{dirstream} may overwrite the return
-value.  Use @code{readdir_r} when this is critical.
+To distinguish between an end-of-directory condition or an error, you
+must set @code{errno} to zero before calling @code{readdir}.  To avoid
+entering an infinite loop, you should stop reading from the directory
+after the first error.
+
+In POSIX.1-2008, @code{readdir} is not thread-safe.  In @theglibc{}
+implementation, it is safe to call @code{readdir} concurrently on
+different @var{dirstream}s, but multiple threads accessing the same
+@var{dirstream} result in undefined behavior.  @code{readdir_r} is a
+fully thread-safe alternative, but suffers from poor portability (see
+below).  It is recommended that you use @code{readdir}, with external
+locking if multiple threads access the same @var{dirstream}.
 @end deftypefun
 
 @comment dirent.h
 @comment GNU
 @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result})
-This function is the reentrant version of @code{readdir}.  Like
-@code{readdir} it returns the next entry from the directory.  But to
-prevent conflicts between simultaneously running threads the result is
-not stored in statically allocated memory.  Instead the argument
-@var{entry} points to a place to store the result.
+This function is a version of @code{readdir} which performs internal
+locking.  Like @code{readdir} it returns the next entry from the
+directory.  To prevent conflicts between simultaneously running
+threads the result is stored inside the @var{entry} object.
+
+@strong{Portability Note:} It is recommended to use @code{readdir}
+instead of @code{readdir_r} for the following reasons:
+
+@itemize @bullet
+@item
+On systems which do not define @code{NAME_MAX}, it may not be possible
+to use @code{readdir_r} safely because the caller does not specify the
+length of the buffer for the directory entry.
+
+@item
+On some systems, @code{readdir_r} cannot read directory entries with
+very long names.  If such a name is encountered, @theglibc{}
+implementation of @code{readdir_r} returns with an error code of
+@code{ENAMETOOLONG} after the final directory entry has been read.  On
+other systems, @code{readdir_r} may return successfully, but the
+@code{d_name} member may not be NUL-terminated or may be truncated.
+
+@item
+POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe,
+even when access to the same @var{dirstream} is serialized.  But in
+current implementations (including @theglibc{}), it is safe to call
+@code{readdir} concurrently on different @var{dirstream}s, so there is
+no need to use @code{readdir_r} in most multi-threaded programs.  In
+the rare case that multiple threads need to read from the same
+@var{dirstream}, it is still better to use @code{readdir} and external
+synchronization.
+
+@item
+It is expected that future versions of POSIX will obsolete
+@code{readdir_r} and mandate the level of thread safety for
+@code{readdir} which is provided by @theglibc{} and other
+implementations today.
+@end itemize
 
 Normally @code{readdir_r} returns zero and sets @code{*@var{result}}
 to @var{entry}.  If there are no more entries in the directory or an
@@ -481,15 +523,6 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a
 null pointer and returns a nonzero error code, also stored in
 @code{errno}, as described for @code{readdir}.
 
-@strong{Portability Note:} On some systems @code{readdir_r} may not
-return a NUL terminated string for the file name, even when there is no
-@code{d_reclen} field in @code{struct dirent} and the file
-name is the maximum allowed size.  Modern systems all have the
-@code{d_reclen} field, and on old systems multi-threading is not
-critical.  In any case there is no such problem with the @code{readdir}
-function, so that even on systems without the @code{d_reclen} member one
-could use multiple threads by using external locking.
-
 It is also important to look at the definition of the @code{struct
 dirent} type.  Simply passing a pointer to an object of this type for
 the second parameter of @code{readdir_r} might not be enough.  Some
diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h
index a7a074d..8e8570d 100644
--- a/sysdeps/posix/dirstream.h
+++ b/sysdeps/posix/dirstream.h
@@ -39,6 +39,8 @@ struct __dirstream
 
     off_t filepos;		/* Position of next entry to read.  */
 
+    int errcode;		/* Delayed error code.  */
+
     /* Directory block.  */
     char data[0] __attribute__ ((aligned (__alignof__ (void*))));
   };
diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
index ddfc3a7..fc05b0f 100644
--- a/sysdeps/posix/opendir.c
+++ b/sysdeps/posix/opendir.c
@@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
   dirp->size = 0;
   dirp->offset = 0;
   dirp->filepos = 0;
+  dirp->errcode = 0;
 
   return dirp;
 }
diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c
index b5a8e2e..8ed5c3f 100644
--- a/sysdeps/posix/readdir_r.c
+++ b/sysdeps/posix/readdir_r.c
@@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
   DIRENT_TYPE *dp;
   size_t reclen;
   const int saved_errno = errno;
+  int ret;
 
   __libc_lock_lock (dirp->lock);
 
@@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
 		  bytes = 0;
 		  __set_errno (saved_errno);
 		}
+	      if (bytes < 0)
+		dirp->errcode = errno;
 
 	      dp = NULL;
-	      /* Reclen != 0 signals that an error occurred.  */
-	      reclen = bytes != 0;
 	      break;
 	    }
 	  dirp->size = (size_t) bytes;
@@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result)
       dirp->filepos += reclen;
 #endif
 
-      /* Skip deleted files.  */
+#ifdef NAME_MAX
+      if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1)
+	{
+	  /* The record is very long.  It could still fit into the
+	     caller-supplied buffer if we can skip padding at the
+	     end.  */
+	  size_t namelen = _D_EXACT_NAMLEN (dp);
+	  if (namelen <= NAME_MAX)
+	    reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1;
+	  else
+	    {
+	      /* The name is too long.  Ignore this file.  */
+	      dirp->errcode = ENAMETOOLONG;
+	      dp->d_ino = 0;
+	      continue;
+	    }
+	}
+#endif
+
+      /* Skip deleted and ignored files.  */
     }
   while (dp->d_ino == 0);
 
   if (dp != NULL)
     {
-#ifdef GETDENTS_64BIT_ALIGNED
-      /* The d_reclen value might include padding which is not part of
-	 the DIRENT_TYPE data structure.  */
-      reclen = MIN (reclen,
-		    offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name));
-#endif
       *result = memcpy (entry, dp, reclen);
-#ifdef GETDENTS_64BIT_ALIGNED
+#ifdef _DIRENT_HAVE_D_RECLEN
       entry->d_reclen = reclen;
 #endif
+      ret = 0;
     }
   else
-    *result = NULL;
+    {
+      *result = NULL;
+      ret = dirp->errcode;
+    }
 
   __libc_lock_unlock (dirp->lock);
 
-  return dp != NULL ? 0 : reclen ? errno : 0;
+  return ret;
 }
 
 #ifdef __READDIR_R_ALIAS
diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c
index 2935a8e..d4991ad 100644
--- a/sysdeps/posix/rewinddir.c
+++ b/sysdeps/posix/rewinddir.c
@@ -33,6 +33,7 @@ rewinddir (dirp)
   dirp->filepos = 0;
   dirp->offset = 0;
   dirp->size = 0;
+  dirp->errcode = 0;
 #ifndef NOT_IN_libc
   __libc_lock_unlock (dirp->lock);
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
index 8ebbcfd..a7d114e 100644
--- a/sysdeps/unix/sysv/linux/i386/readdir64_r.c
+++ b/sysdeps/unix/sysv/linux/i386/readdir64_r.c
@@ -18,7 +18,6 @@
 #define __READDIR_R __readdir64_r
 #define __GETDENTS __getdents64
 #define DIRENT_TYPE struct dirent64
-#define GETDENTS_64BIT_ALIGNED 1
 
 #include <sysdeps/posix/readdir_r.c>
 
diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
index 5ed8e95..290f2c8 100644
--- a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
+++ b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c
@@ -1,5 +1,4 @@
 #define readdir64_r __no_readdir64_r_decl
-#define GETDENTS_64BIT_ALIGNED 1
 #include <sysdeps/posix/readdir_r.c>
 #undef readdir64_r
 weak_alias (__readdir_r, readdir64_r)

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 03/01/2016 09:07 AM, Michael Kerrisk (man-pages) wrote:

> I see that glibc 2.23 deprecates readdir_r(), which prompted me to catch
> up on this thread. I'd like to see the points you make documented in the
> readdir_r(3) man page also. Would you be willing to allow that text to
> be reused / reworked for the page, under that page's existing "verbatim"
> license (https://www.kernel.org/doc/man-pages/licenses.html#verbatim)?

Hi Michael,

thanks for keeping an eye on deprecations.  The deprecation happened for
glibc 2.24 (unrelased).

I'm happy to report that I may grant your request.

> The text I'd propose to add to the man page would be (new material
> starting at ===>):

It may make sense to move this documentation to a separate manual page,
specific to readdir_r.  This will keep the readdir documentation nice
and crisp.  Most programmers will never have to consult all these details.

You should remove the example using pathconf because it is not correct.
 The kernel does not return valid values for _PC_NAME_MAX and some file
systems (such as CIFS, and CD-ROMs with Joliet extensions once a kernel
bug is fixed).  The CIFS limit is somewhere around 765, and not 255 as
reported by the kernel.  If I recall correctly, Windows SMB servers can
actually exceed the 255 byte limit.  The reason is that Windows NTFS has
a limit based on 16-bit UCS-2 characters, and after UTF-8 conversion,
the maximum length is more than 255 bytes.

> ===>   However, the above approach has problems, and it is recommended
>        that  applications  use readdir() instead of readdir_r().  Fur‐
>        thermore, since version  2.23,  glibc  deprecates  readdir_r().
>        The reasons are as follows:
> 
>        *  On  systems where NAME_MAX is undefined, calling readdir_r()
>           may be unsafe because the interface does not allow the call‐
>           er to specify the length of the buffer used for the returned
>           directory entry.
> 
>        *  On some systems, readdir_r() can't  read  directory  entries
>           with very long names.  When the glibc implementation encoun‐
>           ters such a name, readdir_r() fails with the error ENAMETOO‐
>           LONG after the final directory entry has been read.  On some
>           other systems, readdir_r() may return a success status,  but
>           the  returned d_name field may not be null terminated or may
>           be truncated.
> 
>        *  In the current POSIX.1 specification  (POSIX.1-2008),  read‐
>           dir_r() is not required to be thread-safe.  However, in mod‐
>           ern implementations (including  the  glibc  implementation),
>           concurrent  calls  to  readdir_r()  that  specify  different
>           directory streams are thread-safe.  Therefore,  the  use  of

These two references to readdir_r should be to readdir instead.

I believe there was a historic implementation which implemented
fdopendir (fd) as (DIR *) fd, and used a global static buffer for
readdir.  This is about the only way readdir can be non-thread-safe.

>           readdir_r()  is  generally unnecessary in multithreaded pro‐
>           grams.  In cases where multiple threads must read  from  the
>           same  directory  stream,  using readdir() with external syn‐
>           chronization is still preferable to the use of  readdir_r(),
>           for the reasons given in the points above.
> 
>        *  It  is  expected  that a future version of POSIX.1 will make
>           readdir_r() obsolete, and require that readdir() be  thread-
>           safe  when  concurrently  employed  on  different  directory
>           streams.

Okay.

Thanks,
Florian

Re: [PATCH] Fix readdir_r with long file names

[Michael Kerrisk (man-pages)]

Hi Florian,

On 03/01/2016 05:59 PM, Florian Weimer wrote:
> On 03/01/2016 09:07 AM, Michael Kerrisk (man-pages) wrote:
> 
>> I see that glibc 2.23 deprecates readdir_r(), which prompted me to catch
>> up on this thread. I'd like to see the points you make documented in the
>> readdir_r(3) man page also. Would you be willing to allow that text to
>> be reused / reworked for the page, under that page's existing "verbatim"
>> license (https://www.kernel.org/doc/man-pages/licenses.html#verbatim)?
> 
> Hi Michael,
> 
> thanks for keeping an eye on deprecations.  The deprecation happened for
> glibc 2.24 (unrelased).

Ah yes, I was getting ahead of myself. Fixed that in the page text below.

> I'm happy to report that I may grant your request.

Thanks!

>> The text I'd propose to add to the man page would be (new material
>> starting at ===>):
> 
> It may make sense to move this documentation to a separate manual page,
> specific to readdir_r.  This will keep the readdir documentation nice
> and crisp.  Most programmers will never have to consult all these details.

Yes, seems reasonable. Done.

> You should remove the example using pathconf because it is not correct.

Done.

>  The kernel does not return valid values for _PC_NAME_MAX and some file
> systems (such as CIFS, and CD-ROMs with Joliet extensions once a kernel
> bug is fixed).  The CIFS limit is somewhere around 765, and not 255 as
> reported by the kernel.  If I recall correctly, Windows SMB servers can
> actually exceed the 255 byte limit.  The reason is that Windows NTFS has
> a limit based on 16-bit UCS-2 characters, and after UTF-8 conversion,
> the maximum length is more than 255 bytes.

What happens with readdir() when it gets a filename that is larger 
than 255 characters?
> 
>> ===>   However, the above approach has problems, and it is recommended
>>        that  applications  use readdir() instead of readdir_r().  Fur‐
>>        thermore, since version  2.23,  glibc  deprecates  readdir_r().

s/23/24/

>>        The reasons are as follows:
>>
>>        *  On  systems where NAME_MAX is undefined, calling readdir_r()
>>           may be unsafe because the interface does not allow the call‐
>>           er to specify the length of the buffer used for the returned
>>           directory entry.
>>
>>        *  On some systems, readdir_r() can't  read  directory  entries
>>           with very long names.  When the glibc implementation encoun‐
>>           ters such a name, readdir_r() fails with the error ENAMETOO‐
>>           LONG after the final directory entry has been read.  On some
>>           other systems, readdir_r() may return a success status,  but
>>           the  returned d_name field may not be null terminated or may
>>           be truncated.
>>
>>        *  In the current POSIX.1 specification  (POSIX.1-2008),  read‐
>>           dir_r() is not required to be thread-safe.  However, in mod‐
>>           ern implementations (including  the  glibc  implementation),
>>           concurrent  calls  to  readdir_r()  that  specify  different
>>           directory streams are thread-safe.  Therefore,  the  use  of
> 
> These two references to readdir_r should be to readdir instead.

Fixed.

> 
> I believe there was a historic implementation which implemented
> fdopendir (fd) as (DIR *) fd, and used a global static buffer for
> readdir.  This is about the only way readdir can be non-thread-safe.
> 
>>           readdir_r()  is  generally unnecessary in multithreaded pro‐
>>           grams.  In cases where multiple threads must read  from  the
>>           same  directory  stream,  using readdir() with external syn‐
>>           chronization is still preferable to the use of  readdir_r(),
>>           for the reasons given in the points above.
>>
>>        *  It  is  expected  that a future version of POSIX.1 will make
>>           readdir_r() obsolete, and require that readdir() be  thread-
>>           safe  when  concurrently  employed  on  different  directory
>>           streams.

Thanks for all of the feedback Florian! The current versions of the
readdir(3) and readdir_r(3) have been pushed to the repo.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 03/01/2016 09:14 PM, Michael Kerrisk (man-pages) wrote:

> What happens with readdir() when it gets a filename that is larger 
> than 255 characters?

Good question.  Ugh.

readdir will return a pointer to a struct dirent whose d_name member
will not be null-terminated, but the memory following the struct dirent
object will contain the rest of the name, and will eventually be
null-terminated.

This will work perfectly fine if you don't copy struct dirent objects
using memcpy, and the compiler does not optimize things too much.  We
should implement compiler support for this wart: inhibit optimizations
(I think there are already special cases for length-0 and length-1
arrays at the end, so it's not totally without precedent), and warn
about sizeof (struct dirent) and using it as a (non-pointer) declarator.
 The second part is likely generally useful for structs whose size is
not intended to be part of the ABI.

Florian

Re: [PATCH] Fix readdir_r with long file names

[Michael Kerrisk (man-pages)]

On 03/01/2016 09:27 PM, Florian Weimer wrote:
> On 03/01/2016 09:14 PM, Michael Kerrisk (man-pages) wrote:
> 
>> What happens with readdir() when it gets a filename that is larger 
>> than 255 characters?
> 
> Good question.  Ugh.
> 
> readdir will return a pointer to a struct dirent whose d_name member
> will not be null-terminated, but the memory following the struct dirent
> object will contain the rest of the name, and will eventually be
> null-terminated.

So, in other words, if the caller users a declaration of the form

    struct dirent d;

(rather than say allocating a large buffer dynamically), then we have 
a buffer overrun?

Cheers,

Michael

> This will work perfectly fine if you don't copy struct dirent objects
> using memcpy, and the compiler does not optimize things too much.  We
> should implement compiler support for this wart: inhibit optimizations
> (I think there are already special cases for length-0 and length-1
> arrays at the end, so it's not totally without precedent), and warn
> about sizeof (struct dirent) and using it as a (non-pointer) declarator.
>  The second part is likely generally useful for structs whose size is
> not intended to be part of the ABI.
> 
> Florian
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] Fix readdir_r with long file names

[Paul Eggert]

On 03/01/2016 12:27 PM, Florian Weimer wrote:
> We
> should implement compiler support for this wart: inhibit optimizations
> (I think there are already special cases for length-0 and length-1
> arrays at the end, so it's not totally without precedent), and warn
> about sizeof (struct dirent) and using it as a (non-pointer) declarator.

Why not use a flexible array member for this? Sure, that assumes C99, 
but flexible array members are pretty much universally supported now 
(and we can fall back on the current layout for pre-C99 compilers). This 
would work better with modern compilers that treat small arrays with 
more respect than traditional C compilers did. And as I understand 
things, it would conform to POSIX (and if I'm wrong, POSIX should get 
fixed....).

For what it's worth, portable code cannot copy struct dirent values 
anyway, as this loses file names in operating systems like Solaris where 
d_name has size 1.

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 03/01/2016 10:20 PM, Paul Eggert wrote:
> On 03/01/2016 12:27 PM, Florian Weimer wrote:
>> We
>> should implement compiler support for this wart: inhibit optimizations
>> (I think there are already special cases for length-0 and length-1
>> arrays at the end, so it's not totally without precedent), and warn
>> about sizeof (struct dirent) and using it as a (non-pointer) declarator.
> 
> Why not use a flexible array member for this?

For which part, and how exactly?

You can't put a flexible array member into a transparent union.

> Sure, that assumes C99,
> but flexible array members are pretty much universally supported now
> (and we can fall back on the current layout for pre-C99 compilers). This
> would work better with modern compilers that treat small arrays with
> more respect than traditional C compilers did. And as I understand
> things, it would conform to POSIX (and if I'm wrong, POSIX should get
> fixed....).

If you mean to add some zero-width padding member at the end of the
struct, after the d_name member, then I'm worried that makes overrunning
the d_name array member even more undefined than it already is. :)

> For what it's worth, portable code cannot copy struct dirent values
> anyway, as this loses file names in operating systems like Solaris where
> d_name has size 1.

Interesting.  I feel slightly better now about having this overrunning
d_name member.

Florian

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 03/01/2016 10:01 PM, Michael Kerrisk (man-pages) wrote:
> On 03/01/2016 09:27 PM, Florian Weimer wrote:
>> On 03/01/2016 09:14 PM, Michael Kerrisk (man-pages) wrote:
>>
>>> What happens with readdir() when it gets a filename that is larger 
>>> than 255 characters?
>>
>> Good question.  Ugh.
>>
>> readdir will return a pointer to a struct dirent whose d_name member
>> will not be null-terminated, but the memory following the struct dirent
>> object will contain the rest of the name, and will eventually be
>> null-terminated.
> 
> So, in other words, if the caller users a declaration of the form
> 
>     struct dirent d;
> 
> (rather than say allocating a large buffer dynamically), then we have 
> a buffer overrun?

readdir gives you only a struct dirent * to an internal buffer.  If you do

  struct dirent *e = readdir (dir);
  memcpy (&d, e, sizeof (d));

you can end up with a truncated name.  According to Paul's comment, this
kind of truncation is very visible on Solaris.

Florian

Re: [PATCH] Fix readdir_r with long file names

[Rich Felker]

On Tue, Mar 01, 2016 at 11:21:11PM +0100, Florian Weimer wrote:
> On 03/01/2016 10:01 PM, Michael Kerrisk (man-pages) wrote:
> > On 03/01/2016 09:27 PM, Florian Weimer wrote:
> >> On 03/01/2016 09:14 PM, Michael Kerrisk (man-pages) wrote:
> >>
> >>> What happens with readdir() when it gets a filename that is larger 
> >>> than 255 characters?
> >>
> >> Good question.  Ugh.
> >>
> >> readdir will return a pointer to a struct dirent whose d_name member
> >> will not be null-terminated, but the memory following the struct dirent
> >> object will contain the rest of the name, and will eventually be
> >> null-terminated.
> > 
> > So, in other words, if the caller users a declaration of the form
> > 
> >     struct dirent d;
> > 
> > (rather than say allocating a large buffer dynamically), then we have 
> > a buffer overrun?
> 
> readdir gives you only a struct dirent * to an internal buffer.  If you do
> 
>   struct dirent *e = readdir (dir);
>   memcpy (&d, e, sizeof (d));
> 
> you can end up with a truncated name.  According to Paul's comment, this
> kind of truncation is very visible on Solaris.

POSIX also cautions you that this is a permitted definition. See:

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/dirent.h.html

It's covered under the description and rationale.

Rich

Re: [PATCH] Fix readdir_r with long file names

[Paul Eggert]

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

On 03/01/2016 02:16 PM, Florian Weimer wrote:
>> Why not use a flexible array member for this?
> For which part, and how exactly?

Something like the attached patch, say.  (Totally untested.)

> You can't put a flexible array member into a transparent union.

That's OK. Any such usage of struct dirent would be unportable anyway.

> If you mean to add some zero-width padding member at the end of the
> struct, after the d_name member, then I'm worried that makes overrunning
> the d_name array member even more undefined than it already is.

No, no padding member, just use C99 the way it was designed.  This 
should improve overrun detection in programs like valgrind. With glibc's 
current definition these programs can be fooled into thinking that 
struct dirent accesses are invalid (outside of array bounds) when they 
are actually OK, so people shut off array-bounds checking. If we used 
flexible array members, valgrind etc. should know that the array's upper 
bound is unknown and should not issue so many false alarms, so people 
can leave bounds checking on.

Also, I expect this sort of thing will become more important as GCC 
-fbounds-check becomes more practical.

If flexible arrays are no-go for some reason, I suppose we could use 
'char 'd_name[SIZE_MAX - 1000];' instead. That should get peoples' 
attention. :-)

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

diff --git a/bits/dirent.h b/bits/dirent.h
index 7b79a53..8546c29 100644
--- a/bits/dirent.h
+++ b/bits/dirent.h
@@ -32,7 +32,7 @@ struct dirent
     unsigned char d_namlen;	/* Length of the file name.  */
 
     /* Only this member is in the POSIX standard.  */
-    char d_name[1];		/* File name (actually longer).  */
+    char d_name __flexarr;	/* File name.  */
   };
 
 #ifdef __USE_LARGEFILE64
@@ -42,8 +42,7 @@ struct dirent64
     unsigned short int d_reclen;
     unsigned char d_type;
     unsigned char d_namlen;
-
-    char d_name[1];
+    char d_name __flexarr;	/* File name.  */
   };
 #endif
 

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 03/01/2016 11:41 PM, Paul Eggert wrote:
> On 03/01/2016 02:16 PM, Florian Weimer wrote:
>>> Why not use a flexible array member for this?
>> For which part, and how exactly?
> 
> Something like the attached patch, say.  (Totally untested.)
> 
>> You can't put a flexible array member into a transparent union.
> 
> That's OK. Any such usage of struct dirent would be unportable anyway.
> 
>> If you mean to add some zero-width padding member at the end of the
>> struct, after the d_name member, then I'm worried that makes overrunning
>> the d_name array member even more undefined than it already is.
> 
> No, no padding member, just use C99 the way it was designed.  This
> should improve overrun detection in programs like valgrind. With glibc's
> current definition these programs can be fooled into thinking that
> struct dirent accesses are invalid (outside of array bounds) when they
> are actually OK, so people shut off array-bounds checking. If we used
> flexible array members, valgrind etc. should know that the array's upper
> bound is unknown and should not issue so many false alarms, so people
> can leave bounds checking on.

I don't think valgrind can see the difference, but you are correct in
principle (this is essentially the “undefined” part I was worried about).

Unfortunately, GCC does not produce a warning for taking the size of a
struct with a flexible member, or for using it in a non-pointer
declarator, so it does only half of what we want.  And at the cost of
changing sizeof (struct dirent), which can't be a good thing.

> If flexible arrays are no-go for some reason, I suppose we could use
> 'char 'd_name[SIZE_MAX - 1000];' instead. That should get peoples'
> attention. :-)

GCC refuses to compile the type definition, not just declarations.
Refusing declarations with an error would break quite a lot of existing
configure tests.

  struct dirent d; int z; z - d.d_ino;

is a common idiom to check for struct members.

Florian

Re: [PATCH] Fix readdir_r with long file names

[Paul Eggert]

On 03/01/2016 03:07 PM, Florian Weimer wrote:
> GCC does not produce a warning for taking the size of a
> struct with a flexible member, or for using it in a non-pointer
> declarator, so it does only half of what we want.

Ouch. That's annoying, but I guess C11 requires it.  At least we get 
half a loaf....

> And at the cost of
> changing sizeof (struct dirent), which can't be a good thing.

Any program that depends on sizeof (struct dirent) is broken already, so 
this isn't that worrisome.

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 03/02/2016 12:25 AM, Paul Eggert wrote:

>> And at the cost of
>> changing sizeof (struct dirent), which can't be a good thing.
> 
> Any program that depends on sizeof (struct dirent) is broken already, so
> this isn't that worrisome.

Just to be clear, you looked at the wrong struct dirent definition for
GNU/Linux, there is a sysdeps override.

Right now, most programs relying on sizeof (struct dirent) work well in
almost all cases.  We really don't want to break that.  There appears to
be an overlap between these programs and users of readdir_r, so once we
remove that from the API, we should have better story for struct dirent
declarators as well.

Florian

Re: [PATCH] Fix readdir_r with long file names

[Michael Kerrisk (man-pages)]

On 03/01/2016 11:21 PM, Florian Weimer wrote:
> On 03/01/2016 10:01 PM, Michael Kerrisk (man-pages) wrote:
>> On 03/01/2016 09:27 PM, Florian Weimer wrote:
>>> On 03/01/2016 09:14 PM, Michael Kerrisk (man-pages) wrote:
>>>
>>>> What happens with readdir() when it gets a filename that is larger 
>>>> than 255 characters?
>>>
>>> Good question.  Ugh.
>>>
>>> readdir will return a pointer to a struct dirent whose d_name member
>>> will not be null-terminated, but the memory following the struct dirent
>>> object will contain the rest of the name, and will eventually be
>>> null-terminated.
>>
>> So, in other words, if the caller users a declaration of the form
>>
>>     struct dirent d;
>>
>> (rather than say allocating a large buffer dynamically), then we have 
>> a buffer overrun?
> 
> readdir gives you only a struct dirent * to an internal buffer.  

D'oh! Yes, of course. I wasn't thinking clearly as I wrote that last night.

> If you do
> 
>   struct dirent *e = readdir (dir);
>   memcpy (&d, e, sizeof (d));
> 
> you can end up with a truncated name.  

Got it.

> According to Paul's comment, this
> kind of truncation is very visible on Solaris.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] Fix readdir_r with long file names

[Michael Kerrisk (man-pages)]

On 03/02/2016 12:44 AM, Florian Weimer wrote:
> On 03/02/2016 12:25 AM, Paul Eggert wrote:
> 
>>> And at the cost of
>>> changing sizeof (struct dirent), which can't be a good thing.
>>
>> Any program that depends on sizeof (struct dirent) is broken already, so
>> this isn't that worrisome.
> 
> Just to be clear, you looked at the wrong struct dirent definition for
> GNU/Linux, there is a sysdeps override.
> 
> Right now, most programs relying on sizeof (struct dirent) work well in
> almost all cases.  We really don't want to break that.  There appears to
> be an overlap between these programs and users of readdir_r, so once we
> remove that from the API, we should have better story for struct dirent
> declarators as well.

So, it seems like much more could be said about this in documentation.
How about the following text for the man page?

   DESCRIPTION

       [...]

       In the glibc implementation, the dirent structure is defined as
       follows:

           struct dirent {
               ino_t          d_ino;       /* Inode number */
               off_t          d_off;       /* Not an offset; see below */
               unsigned short d_reclen;    /* Length of this record */
               unsigned char  d_type;      /* Type of file; not supported
                                              by all filesystem types */
               char           d_name[256]; /* Null-terminated filename */
           };

       [...]
   NOTES
     The d_name field
       The  dirent  structure definition shown above is taken from the
       glibc headers, and shows the d_name field with a fixed size.

       Warning: applications should avoid any dependence on  the  size
       of the dname field.  POSIX defines it as char d_name[], a char‐
       acter array of unspecified size, with at most NAME_MAX  charac‐
       ters preceding the terminating null byte ('\0').

       POSIX.1  explicitly notes that this field should not be used as
       an  lvalue.   The  standard  also  notes  that   the   use   of
       sizeof(d_name)  (and  by  implication sizeof(struct dirent)) is
       incorrect; use strlen(d_name) instead.  (On some systems,  this
       field is defined as char d_name[1]!)

       Note that while the call

           fpathconf(fd, _PC_NAME_MAX)

       returns the value 255 for most filesystems, on some filesystems
       (e.g., CIFS, Windows SMB servers), the null-terminated filename
       that is (correctly) returned in d_name can actually exceed this
       size.  (In such cases, the d_reclen field will contain a  value
       that  exceeds  the  size  of  the  glibc dirent structure shown
       above.)

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] Fix readdir_r with long file names

[Paul Eggert]

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

Florian Weimer wrote:

> Just to be clear, you looked at the wrong struct dirent definition for
> GNU/Linux, there is a sysdeps override.

Yes, we'd need a similar fix in sysdeps/unix/sysv/linux/bits/dirent.h.

> Right now, most programs relying on sizeof (struct dirent) work well in
> almost all cases.  We really don't want to break that.  There appears to
> be an overlap between these programs and users of readdir_r, so once we
> remove that from the API, we should have better story for struct dirent
> declarators as well.

I see your point in worrying about GNU/Linux programs that use 'sizeof 
(struct dirent)', even though these programs are not portable and won't 
work on other POSIX platforms.

How about something like the attached (untested) patch, then? It keeps 
the structure the same size, while still using flexible arrays to 
indicate to analyzers that the array in question may be larger. It also 
adds a compile-time option (default off) to simply use flexible arrays 
without the backwards-compatibility hack. We could use that option in 
Gnulib.

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

diff --git a/bits/dirent.h b/bits/dirent.h
index 7b79a53..69a7892 100644
--- a/bits/dirent.h
+++ b/bits/dirent.h
@@ -32,7 +32,7 @@ struct dirent
     unsigned char d_namlen;	/* Length of the file name.  */
 
     /* Only this member is in the POSIX standard.  */
-    char d_name[1];		/* File name (actually longer).  */
+    __flexarray (d_name, char, 1); /* File name.  */
   };
 
 #ifdef __USE_LARGEFILE64
@@ -42,8 +42,7 @@ struct dirent64
     unsigned short int d_reclen;
     unsigned char d_type;
     unsigned char d_namlen;
-
-    char d_name[1];
+    __flexarray (d_name, char, 1); /* File name.  */
   };
 #endif
 
diff --git a/include/features.h b/include/features.h
index 9514d35..ca162d3 100644
--- a/include/features.h
+++ b/include/features.h
@@ -45,6 +45,7 @@
    _THREAD_SAFE		Same as _REENTRANT, often used by other systems.
    _FORTIFY_SOURCE	If set to numeric value > 0 additional security
 			measures are defined, according to level.
+   _FLEXARRAY_SOURCE	Select flexible array data structures when applicable.
 
    The `-ansi' switch to the GNU C compiler, and standards conformance
    options such as `-std=c99', define __STRICT_ANSI__.  If none of
@@ -80,6 +81,7 @@
    __USE_GNU		Define GNU extensions.
    __USE_REENTRANT	Define reentrant/thread-safe *_r functions.
    __USE_FORTIFY_LEVEL	Additional security measures used, according to level.
+   __USE_FLEXARRAY	Use flexible arrays for data structures.
 
    The macros `__GNU_LIBRARY__', `__GLIBC__', and `__GLIBC_MINOR__' are
    defined by this file unconditionally.  `__GNU_LIBRARY__' is provided
@@ -117,6 +119,7 @@
 #undef	__USE_GNU
 #undef	__USE_REENTRANT
 #undef	__USE_FORTIFY_LEVEL
+#undef	__USE_FLEXARRAY
 #undef	__KERNEL_STRICT_NAMES
 
 /* Suppress kernel-name space pollution unless user expressedly asks
@@ -341,6 +344,10 @@
 # define __USE_FORTIFY_LEVEL 0
 #endif
 
+#ifdef _FLEXARRAY_SOURCE
+# define __USE_FLEXARRAY 1
+#endif
+
 /* Get definitions of __STDC_* predefined macros, if the compiler has
    not preincluded this header automatically.  */
 #include <stdc-predef.h>
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 7fd4154..facfdc3 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -157,10 +157,29 @@
 #  else
 /* Some other non-C99 compiler.  Approximate with [1].  */
 #   define __flexarr	[1]
+#   define __flexarray(id, type, size) type id[size]
 #  endif
 # endif
 #endif
 
+/* Declare a structure member named ID, with type "array of TYPE", and
+   with actual size unspecified but with nominal size SIZE for the
+   benefit of traditional applications.  If the application is
+   compiled with _FLEXARRAY_SOURCE and if the compiler is C99 or
+   better, model this as a flexible array.  Otherwise, model it as a
+   fixed-size array of size SIZE (followed by a flexible array
+   afterwards, if C99, for the benefit of source-code analyzers that
+   can use this as a cue that the array is really varying-size).  The
+   fixed-size array is for compatibility with programs that unwisely
+   take the size of structures that contain flexible arrays, and which
+   rely on traditional glibc which uses fixed-size arrays.  */
+#ifndef __flexarray
+# ifdef __USE_FLEXARRAY
+#  define __flexarray(id, type, size) type id[]
+# else
+#  define __flexarray(id, type, size) type id[size]; type __flexarray __flexarr
+# endif
+#endif
 
 /* __asm__ ("xyz") is used throughout the headers to rename functions
    at the assembly language level.  This is wrapped by the __REDIRECT
diff --git a/sysdeps/nacl/bits/dirent.h b/sysdeps/nacl/bits/dirent.h
index d4eb7fe..2e38a58 100644
--- a/sysdeps/nacl/bits/dirent.h
+++ b/sysdeps/nacl/bits/dirent.h
@@ -30,7 +30,7 @@ struct dirent
     unsigned short int d_reclen; /* Length of the whole `struct dirent'.  */
 
     /* Only this member is in the POSIX standard.  */
-    char d_name[256];		/* We must not include limits.h! */
+    __flexarray (d_name, char, 256); /* We must not include limits.h! */
   };
 
 #ifdef __USE_LARGEFILE64
@@ -42,7 +42,7 @@ struct dirent64
     unsigned short int d_reclen; /* Length of the whole `struct dirent'.  */
 
     /* Only this member is in the POSIX standard.  */
-    char d_name[256];		/* We must not include limits.h! */
+    __flexarray (d_name, char, 256); /* We must not include limits.h! */
   };
 #endif
 
diff --git a/sysdeps/unix/sysv/linux/alpha/bits/dirent.h b/sysdeps/unix/sysv/linux/alpha/bits/dirent.h
index 98d297e..2470d0f 100644
--- a/sysdeps/unix/sysv/linux/alpha/bits/dirent.h
+++ b/sysdeps/unix/sysv/linux/alpha/bits/dirent.h
@@ -29,7 +29,7 @@ struct dirent
     __off_t d_off;
     unsigned short int d_reclen;
     unsigned char d_type;
-    char d_name[256];		/* We must not include limits.h! */
+    __flexarray (d_name, char, 256); /* We must not include limits.h! */
   };
 
 #ifdef __USE_LARGEFILE64
@@ -40,7 +40,7 @@ struct dirent64
     __off64_t d_off;
     unsigned short int d_reclen;
     unsigned char d_type;
-    char d_name[256];		/* We must not include limits.h! */
+    __flexarray (d_name, char, 256); /* We must not include limits.h! */
   };
 #endif
 
diff --git a/sysdeps/unix/sysv/linux/bits/dirent.h b/sysdeps/unix/sysv/linux/bits/dirent.h
index 31b1961..b7137e0 100644
--- a/sysdeps/unix/sysv/linux/bits/dirent.h
+++ b/sysdeps/unix/sysv/linux/bits/dirent.h
@@ -30,7 +30,7 @@ struct dirent
 #endif
     unsigned short int d_reclen;
     unsigned char d_type;
-    char d_name[256];		/* We must not include limits.h! */
+    __flexarray (d_name, char, 256); /* We must not include limits.h! */
   };
 
 #ifdef __USE_LARGEFILE64
@@ -40,7 +40,7 @@ struct dirent64
     __off64_t d_off;
     unsigned short int d_reclen;
     unsigned char d_type;
-    char d_name[256];		/* We must not include limits.h! */
+    __flexarray (d_name, char, 256); /* We must not include limits.h! */
   };
 #endif
 

Re: [PATCH] Fix readdir_r with long file names

[Joseph Myers]

On Wed, 2 Mar 2016, Paul Eggert wrote:

> analyzers that the array in question may be larger. It also adds a
> compile-time option (default off) to simply use flexible arrays without the
> backwards-compatibility hack. We could use that option in Gnulib.

I don't think such a compile-time option is desirable (especially not 
undocumented, all feature test macros should be documented in 
creature.texi); we shouldn't profilerate feature-test macros for things 
that aren't well-defined API groups.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 03/02/2016 06:44 PM, Paul Eggert wrote:
> +/* Declare a structure member named ID, with type "array of TYPE", and
> +   with actual size unspecified but with nominal size SIZE for the
> +   benefit of traditional applications.  If the application is
> +   compiled with _FLEXARRAY_SOURCE and if the compiler is C99 or
> +   better, model this as a flexible array.  Otherwise, model it as a
> +   fixed-size array of size SIZE (followed by a flexible array
> +   afterwards, if C99, for the benefit of source-code analyzers that
> +   can use this as a cue that the array is really varying-size).  The
> +   fixed-size array is for compatibility with programs that unwisely
> +   take the size of structures that contain flexible arrays, and which
> +   rely on traditional glibc which uses fixed-size arrays.  */
> +#ifndef __flexarray
> +# ifdef __USE_FLEXARRAY
> +#  define __flexarray(id, type, size) type id[]
> +# else
> +#  define __flexarray(id, type, size) type id[size]; type __flexarray __flexarr
> +# endif
> +#endif

Sorry, I'm not convinced this is good compiler hint.  I suspect it moves
things in the opposite direction.  Some compilers likely have hacks to
support arrays of length 1 as flexible arrays if the array is at the end
of a struct, and after your change, the member is no longer at the end,
potentially disabling the flexible array kludge.

I think we should interact better with compilers in this area, but this
needs fairly explicit mark-up, showing what we need, and not something
that requires the compiler to guess.

Thanks,
Florian

Re: [PATCH] Fix readdir_r with long file names

[Michael Kerrisk (man-pages)]

Ping on this man page text. Anyone have an opinion for or against?

Thanks,

Michael


On 03/02/2016 11:39 AM, Michael Kerrisk (man-pages) wrote:
> On 03/02/2016 12:44 AM, Florian Weimer wrote:
>> On 03/02/2016 12:25 AM, Paul Eggert wrote:
>>
>>>> And at the cost of
>>>> changing sizeof (struct dirent), which can't be a good thing.
>>>
>>> Any program that depends on sizeof (struct dirent) is broken already, so
>>> this isn't that worrisome.
>>
>> Just to be clear, you looked at the wrong struct dirent definition for
>> GNU/Linux, there is a sysdeps override.
>>
>> Right now, most programs relying on sizeof (struct dirent) work well in
>> almost all cases.  We really don't want to break that.  There appears to
>> be an overlap between these programs and users of readdir_r, so once we
>> remove that from the API, we should have better story for struct dirent
>> declarators as well.
> 
> So, it seems like much more could be said about this in documentation.
> How about the following text for the man page?
> 
>    DESCRIPTION
> 
>        [...]
> 
>        In the glibc implementation, the dirent structure is defined as
>        follows:
> 
>            struct dirent {
>                ino_t          d_ino;       /* Inode number */
>                off_t          d_off;       /* Not an offset; see below */
>                unsigned short d_reclen;    /* Length of this record */
>                unsigned char  d_type;      /* Type of file; not supported
>                                               by all filesystem types */
>                char           d_name[256]; /* Null-terminated filename */
>            };
> 
>        [...]
>    NOTES
>      The d_name field
>        The  dirent  structure definition shown above is taken from the
>        glibc headers, and shows the d_name field with a fixed size.
> 
>        Warning: applications should avoid any dependence on  the  size
>        of the dname field.  POSIX defines it as char d_name[], a char‐
>        acter array of unspecified size, with at most NAME_MAX  charac‐
>        ters preceding the terminating null byte ('\0').
> 
>        POSIX.1  explicitly notes that this field should not be used as
>        an  lvalue.   The  standard  also  notes  that   the   use   of
>        sizeof(d_name)  (and  by  implication sizeof(struct dirent)) is
>        incorrect; use strlen(d_name) instead.  (On some systems,  this
>        field is defined as char d_name[1]!)
> 
>        Note that while the call
> 
>            fpathconf(fd, _PC_NAME_MAX)
> 
>        returns the value 255 for most filesystems, on some filesystems
>        (e.g., CIFS, Windows SMB servers), the null-terminated filename
>        that is (correctly) returned in d_name can actually exceed this
>        size.  (In such cases, the d_reclen field will contain a  value
>        that  exceeds  the  size  of  the  glibc dirent structure shown
>        above.)
> 
> Cheers,
> 
> Michael
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer]

On 03/02/2016 11:39 AM, Michael Kerrisk (man-pages) wrote:
>        of the dname field.  POSIX defines it as char d_name[], a char‐

Should be “d_name” instead of “dname”.

Otherwise looks fine.

Thanks,
Florian

Re: [PATCH] Fix readdir_r with long file names

[Michael Kerrisk (man-pages)]

On 03/10/2016 12:22 PM, Florian Weimer wrote:
> On 03/02/2016 11:39 AM, Michael Kerrisk (man-pages) wrote:
>>        of the dname field.  POSIX defines it as char d_name[], a char‐
> 
> Should be “d_name” instead of “dname”.

Fixed now.

> Otherwise looks fine.

Thanks, Florian!

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/