Re: [patch] libstdc++/67747 Allocate space for dirent::d_name

[Jonathan Wakely <jwakely@redhat.com>]

On 29/09/15 12:54 -0600, Martin Sebor wrote:
>On 09/29/2015 05:37 AM, Jonathan Wakely wrote:
>>POSIX says that dirent::d_name has an unspecified length, so calls to
>>readdir_r must pass a buffer with enough trailing space for
>>{NAME_MAX}+1 characters. I wasn't doing that, which works OK on
>>GNU/Linux and BSD where d_name is a large array, but fails on Solaris
>>32-bit.
>>
>>This uses pathconf to get NAME_MAX and allocates a buffer.
>>
>>Tested powerpc64le-linux and x86_64-dragonfly4.1, I'm going to commit
>>this to trunk today (and backport all the filesystem fixes to
>>gcc-5-branch).
>
>Calling pathconf is only necessary when _POSIX_NO_TRUNC is zero
>which I think exists mainly for legacy file systems. Otherwise,
>it's safe to use NAME_MAX instead. Avoiding the call to pathconf

Oh, nice. I was using NAME_MAX originally but the glibc readdir_r(3)
man-page has an example using pathconf and says that should be used,
so I went down that route.

>also avoids the TOCTOU between it and the call to opendir, and

Also nice.

>hardcoding the value makes it possible to avoid dynamically
>allocating the dirent buffer.

Can we be sure NAME_MAX will never be too big for the stack?

As currently written _Dir::advance() can call itself recursively to
skip the . and .. directories, so if we put the dirent buffer on the
stack then maybe we should re-use the same one not create three large
frames.

>I didn't remember the MAX_PATH value on Windows anymore but from
>what I've just read online it sounds like it's defined to 260.

Yes, I found that value, but I think I found something saying the
individual components were limited to 255. I'll make it 260 anyway. I
think that might relate to UTF-16 characters, so we'd need a larger
buffer for narrow characters, but I'm not sure what mingw supports
here anyway. The Windows implementations are just stubs where someone
who cares can add working code.

>Defaulting to 255 on POSIX is appropriate. On XSI systems, the
>minimum required value is _XOPEN_NAME_MAX which is 255 (I would
>suggest using the macro instead when it's defined). Otherwise,
>the strictly conforming minimum value would be 14 -- the value
>of _POSIX_NAME_MAX, but since 255 is greater it's fine.
>
>Other than that, I tend to be leery of using plain char arrays
>as buffers for objects of bigger types. I don't know to what
>extent this is a problem for libstdc++ anymore as more and more
>hardware is tolerant of misaligned accesses and as the default
>new expression typically returns memory suitably aligned for
>the largest fundamental type. But since there is no requirement
>in the language that it do so and I would tend to err on the
>side of caution and use operator new (as opposed to
>new char[len]).

For some reason I thought new char[len] would be suitably aligned for
any type with sizeof <= len, but that's only true for operator new.
(I should check I haven't made the same assumption elsewhere in the
library!)

std::aligned_union<offsetof(dirent, d_name)+NAME_MAX, dirent>::type
will give us what we need.


>Martin
>
>PS I'm interpreting _POSIX_NO_TRUNC being zero as more
>restrictive than if it was non-zero and so calling pathconf(p,
>_PC_NO_TRUNC) should be required to also return non-zero for
>such an implementation, regardless of p. But let me check that
>I'm reading it right.
>