Re: [PATCH] Fix readdir_r with long file names

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

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)