This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]