Re: [PATCH v5 1/5] linux: Use getdents64 on non-LFS readdir

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

> diff --git a/sysdeps/unix/sysv/linux/dirstream.h b/sysdeps/unix/sysv/linux/dirstream.h
> index 3cb313b410..adcf8234f1 100644
> --- a/sysdeps/unix/sysv/linux/dirstream.h
> +++ b/sysdeps/unix/sysv/linux/dirstream.h
> @@ -18,6 +18,7 @@
>  #ifndef	_DIRSTREAM_H
>  #define	_DIRSTREAM_H	1
>  
> +#include <dirent.h>
>  #include <sys/types.h>
>  
>  #include <libc-lock.h>
> @@ -41,6 +42,10 @@ struct __dirstream
>  
>      int errcode;		/* Delayed error code.  */
>  
> +#if !defined __OFF_T_MATCHES_OFF64_T || !defined __INO_T_MATCHES_INO64_T
> +    struct dirent tdp;
> +#endif

I don't quite see how this can work given that d_name in the struct may
not be large enough.  The new member needs a comment to explain its
purpose.

> diff --git a/sysdeps/unix/sysv/linux/readdir.c b/sysdeps/unix/sysv/linux/readdir.c
> index 4a4c00ea07..cd0ccaf33a 100644
> --- a/sysdeps/unix/sysv/linux/readdir.c
> +++ b/sysdeps/unix/sysv/linux/readdir.c
> @@ -21,42 +21,71 @@
>  #if !_DIRENT_MATCHES_DIRENT64
>  #include <dirstream.h>
>  
> +/* Translate the DP64 entry to the non-LFS one in the translation entry
> +   at dirstream DS.  Return true is the translation was possible or
> +   false if either an internal field can not be represented in the non-LFS
> +   entry or if the name is too long.  */
> +static bool
> +dirstream_entry (struct __dirstream *ds, const struct dirent64 *dp64)
> +{
> +  /* Check for overflow.  */
> +  if (!in_off_t_range (dp64->d_off) || !in_ino_t_range (dp64->d_ino))
> +    return false;
> +
> +  /* And if name is too large.  */
> +  if (dp64->d_reclen - offsetof (struct dirent64, d_name) > NAME_MAX)
> +    return false;

Sorry, I don't think this is the direction we should be going.  In
readdir_r, we at least delay the NAME_MAX error to the end of the
directory.  This just adds another rare case where 32-bit code fails and
64-bit code works.

struct dirent is always shorter than struct dirent64, right?  It should
be possible to do the translation in-place.  Or turn tdp into a pointer
and reallocate as needed.

However, I think we should fix only readdir64, not readdir.  It's simply
not possible to fix readdir fully because of d_ino, so applications
which use readdir instead of readdir64 will remain buggy even with this
change.

What do you think?

Thanks,
Florian