Re: [PATCH] Fix readdir_r with long file names

[Florian Weimer <fweimer at redhat dot com>]

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