Fix readdir_r with long file names

Quoted 11 lines · Siddhesh Poyarekar

Review

Quoted 4 lines

(quoted diff) +4 -0

+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.

Quoted 5 lines

Quoted 5 lines

+@item

(quoted diff) +4 -0

+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

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+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

(quoted diff) +8 -0

+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 ===>):

===> 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.

Quoted 5 lines · Michael Kerrisk (man-pages)

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.

Quoted 2 lines

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.

Quoted 4 lines

===> 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().

Quoted 15 lines

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.

Quoted 5 lines

* 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.

Quoted 10 lines

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.

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

Quoted 2 lines · Michael Kerrisk (man-pages)

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

Quoted 11 lines · Florian Weimer

Quoted 2 lines · Michael Kerrisk (man-pages)

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?

Quoted 13 lines · Michael Kerrisk (man-pages)

Quoted 11 lines · Florian Weimer

Quoted 2 lines · Michael Kerrisk (man-pages)

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.

Quoted 6 lines

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.

Quoted 13 lines · Michael Kerrisk (man-pages)

Quoted 11 lines · Florian Weimer

Quoted 2 lines · Michael Kerrisk (man-pages)

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.

Quoted 6 lines

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.

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.

Quoted 5 lines · Florian Weimer

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.

Quoted 1 line

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.

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.

Quoted 2 lines · Florian Weimer

Quoted 1 line

Why not use a flexible array member for this?

For which part, and how exactly?

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

Quoted 1 line

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.

Quoted 3 lines

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.

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 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.

Quoted 2 lines

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.

Quoted 5 lines · Paul Eggert

Quoted 2 lines

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.

Quoted 8 lines · Florian Weimer

Quoted 5 lines · Paul Eggert

Quoted 2 lines

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.

Quoted 8 lines

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

       of the dname field.  POSIX defines it as char d_name[], a char‐

Quoted 1 line · Michael Kerrisk (man-pages)

       of the dname field.  POSIX defines it as char d_name[], a char‐

Should be “d_name” instead of “dname”.

Otherwise looks fine.

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.

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.

misc/sys/cdefs.h +8 -0

+/* 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