Austin Group Defect Tracker

Aardvark Mark III


Viewing Issue Simple Details Jump to Notes ] Issue History ] Print ]
ID Category Severity Type Date Submitted Last Update
0000696 [1003.1(2013)/Issue7+TC1] Base Definitions and Headers Objection Clarification Requested 2013-05-14 17:32 2015-10-04 01:01
Reporter philip-guenther View Status public  
Assigned To
Priority normal Resolution Open  
Status New  
Name Philip Guenther
Organization OpenBSD
User Reference
Section <limits.h>
Page Number 273
Line Number 8986-8990
Interp Status ---
Final Accepted Text
Summary 0000696: either NAME_MAX shouldn't be optional, or readdir_r() needs clarification
Description The <limits.h> description says this about the Pathname Variable Values, which include NAME_MAX:
    A definition of one of the symbolic constants in the following
    list shall be omitted from the <limits.h> header on specific
    implementations where the corresponding value is equal to or
    greater than the stated minimum, but where the value can vary
    depending on the file to which it is applied. The actual value
    supported for a specific pathname shall be provided by the
    pathconf() function.

So implementations can leave NAME_MAX undefined if the limit varies. Meanwhile, the readdir_r() description says:
    The storage pointed to by entry shall be large enough for a
    dirent with an array of char d_name members containing at least
    {NAME_MAX}+1 elements.

If NAME_MAX really is optional, then the above text should be replaced with a discussion of NAME_MAX vs pathconf(dir, _PC_NAME_MAX). And what should you do if pathconf() returns -1, which it does for 32bit Linux binaries accessing filesystems where statvfs() fails with EOVERFLOW?
Desired Action Require NAME_MAX to be defined by <limits.h>

Amend the readdir_r() and <dirent.h> description to specify that applications can use pathconf(dir, _PC_NAME_MAX) to get a _lower_ limit to reduce the allocation size for the readdir_r() buffer, but that if it fails NAME_MAX will always be sufficient.
Tags No tags attached.
Attached Files

- Relationships
related to 0000697New Adding of a getdirentries() function 
has duplicate 0000831Closed How to allocate memory for readdir_r is unclear. 

-  Notes
(0001604)
wlerch (reporter)
2013-05-14 18:03

A side note: "a dirent with an array of char d_name members containing at least {NAME_MAX}+1 elements" is really bad wording -- it sounds as if developers were expected to reverse-engineer the implementation's headers to compute the size that a struct dirent would have if the implementation defined it differently.

As far as I can tell, both these are allowed definitions of dirent:

struct dirent {
  char d_name[ NAME_MAX + 1 ];
  other stuff;
};

struct dirent {
  other stuff;
  char d_name[ 1 ];
};

How is an application supposed to compute the amount of storage to give to readdir_r? Either or the following will work reasonably well with the second definition of dirent:

  sizeof(struct dirent) + NAME_MAX + 1
  offsetof(struct dirent, d_name) + NAME_MAX + 1

but with the first version one will allocate double the necessary amount of memory, and the other will allocate too little.

Or is "char d_name[]" supposed to be taken literally as a requirement that implementation must define d_name as a flexible array element at the end of the structure?
(0001605)
philip-guenther (reporter)
2013-05-14 18:57

One might be tempted to use sizeof() on the d_name element to determine whether it's at least NAME_MAX+1 characters long, but if it's allowed to declared as an actual flexible array member then it's a constraint violation to use sizeof() on it....

It seems that right now, implementations that want to prevent buffer overflow by applications will use something equivalent to "char d_name[NAME_MAX+1];", while applications that want to maximize portability will allocate a buffer of sizeof(struct dirent) + NAME_MAX + 1 bytes, leading to almost twice as large an allocation as is necessary when the two meet.

Or is the POSIX answer that because implementations can't come to an agreement on this, we should all be writing this:
        if (offsetof(struct dirent, d_name) + NAME_MAX + 1
            <= sizeof(struct dirent))
                size = sizeof(struct dirent);
        else
                size = sizeof(struct dirent) + NAME_MAX + 1;

...because if so we've failed as standards engineers by releasing a standard which makes it unreasonably difficult to develop correct applications.
(0001606)
dalias (reporter)
2013-05-15 16:17

I would like to propose an alternate solution. For readdir, replace the text:

"The readdir() function need not be thread-safe."

with:

"If multiple threads call the readdir() function with the same directory stream argument and without synchronization to preclude simultaneous access, then the behavior is undefined."

With this change, the clunky readdir_r function is no longer needed or useful, and should probably be deprecated. As the only reasonable way to meet the implementation requirements for readdir is to have the dirent buffer in the DIR structure, this change should not require any change to existing implementations.
(0001632)
david-bartley (reporter)
2013-05-31 09:08

Using pathconf(dir, _PC_NAME_MAX) isn't acceptable because the target could change filesystems (where NAME_MAX might differ) between the call to pathconf and readdir_r, potentially leading to a buffer overflow.

dalias, a conformant implementation could still use a static buffer internally, perhaps as a temporary storage location, making it thread-unsafe. The change you suggest sounds good, but I'd suggest different wording: "Concurrent readdir() invocations with differing dirp arguments must be thread-safe; concurrent readdir() invocations with identical dirp arguments need not be thread-safe."
(0001633)
geoffclare (manager)
2013-05-31 09:46
edited on: 2013-05-31 09:53

(Response to Note: 0001632)
The location could change between pathconf() and opendir(), but that
issue can be avoided by using dirfd() and fpathconf(). However, this
still has the problem that fpathconf() could return -1 indicating that
{NAME_MAX} is indeterminate, in which case readdir_r() can't be used.

In yesterday's teleconference we agreed that we would make
readdir() thread safe except for concurrent calls with the same
directory stream, and mark readdir_r() obsolescent. Don Cragun
is working on the wording changes.

(0001634)
dalias (reporter)
2013-05-31 11:56

In regards to note 0001632, I tried to write the proposed wording so as to avoid using terms like "concurrent" that don't seem to be defined in the standard. I think without further clarification, "concurrent" could be misread as implying that calls from multiple threads only invoke undefined behavior if, due to scheduling, they actually happen to execute at the same time, as opposed to just failing to be synchronized (via a mutex or other means) with respect to one another. However if others are satisfied that the simpler wording conveys the intended meaning, I have no objection to it.
(0001697)
shware_systems (reporter)
2013-08-05 02:37
edited on: 2013-08-05 02:46

{NAME_MAX}
Maximum number of bytes in a filename (not including the terminating null).
Minimum Acceptable Value: {_POSIX_NAME_MAX}
XSI Minimum Acceptable Value: {_XOPEN_NAME_MAX}

(Response to Note: 0001632)
The location could change between pathconf() and opendir(), but that
issue can be avoided by using dirfd() and fpathconf(). However, this
still has the problem that fpathconf() could return -1 indicating that
{NAME_MAX} is indeterminate, in which case readdir_r() can't be used.
---
When -1 is returned from (f)pathconf the Minimum Acceptable Values as shown are
what a conforming application can rely upon, nothing larger, and for these
systems they are required to define MAX_PATH in limits.h to be at least those
values. The -1 is more an indication that the device didn't report that it
couldn't handle that value when first accessed, or confirm that it could.
Requiring use of larger values without reporting such via those interfaces I
would construe as non-conformant behavior. When an item such as NAME_MAX is
allowed to be undefined its implementation of (f)pathconf is required to return
larger than the minimum acceptable. Returning -1 is considered non-conforming
in this instance. If a device requires a value smaller than these minimums it
is not POSIX compatible to begin with and any mounting operation or network
connection attempt should have failed. As LINUX is not POSIX, caveat emptor.

This places the onus on applications to do explicit #ifdef checks on those
items for code to be fully portable; e.g.

#include <unistd.h> // for pathconf() and _PC_NAME_MAX
#include <limits.h> // for NAME_MAX, if defined
long GetNameMax(char *dir) {
  long MyNameMax = 0;
  if ((dir == NULL) || (*dir == "/0")) dir = "//"; // default to filesystem root if bogus entry passed in
#ifdef NAME_MAX
  if ( (MyNameMax = pathconf(dir, _PC_NAME_MAX)) < NAME_MAX ) /*-1, in other words*/
    MyNameMax = NAME_MAX; /* else is >= Minimum Acceptable */
#else
  MyNameMax = pathconf(dir, _PC_NAME_MAX); /* Always greater than minimum NAME_MAX, either POSIX or XSI */
#endif
  return MyNameMax;
}

Kludgy, and probably an example like this should be part of the text, but the
language does provide for deterministic reliable values whether NAME_MAX is
#define'd or not, as at least _POSIX_NAME_MAX is required to be defined in
<unistd.h>. Whether target changes or not, each target must support at least
that minimum value, or a larger one reported by pathconf(), or mounting such a
file system is an extension beyond the scope of the standard.

The only grey area is when an implementation sets NAME_MAX to some higher value
and a device only supports the minimum value or some value in between. The
standard is not clear on whether an implementation is required to use an
intermediate value like this, when reported by (f)pathconf, as superseding
NAME_MAX or is it required to consider it non-compliant hardware and refuse to
access it. My example presumes the latter.

Using this code as preamble:
long dirent_size = (IsXSI() ? sizeof(ino_t) : 0) + (GetNameMax(dir)+1) * sizeof(char) + sizeof(internal-implementation-specific-fields-if-any);
dirent *MyDirEnt = (dirent *) malloc(dirent_size);

for use with any interface requiring a dirent structure suitable for a
particular mounted filesystem. The explicit NAME_MAX reference by itself in
readdir just needs changing to "effective NAME_MAX value" + 1, with a reference
to how that's calculated. What is missing from <dirent.h> is either the word
'only' between 'include' and 'these members' or a standard #define, like
_DIRENT_EXTRA_SIZE, an implementation shall set to greater than 0 if more fields
are defined for internal use in the dirent structure. Either would resolve the
ambiguity. In the absence of such a #define an application would assume it's 0.

I include sizeof(char) as a placeholder that Version 8 supporting Unicode might
imply support for filesystems that use wide chars directly in directory
entries. I would actually expect a new pathconf code to indicate such support
but other mechanisms are plausible also.

As to obsoleting readdir_r and "thread-safing" readdir(), I believe the only
way this can be reliably done is if opendir() is required to explicitly acquire
a mutex on the DIR stream and whatever buffer it reads from the filesystem,
that is released by closedir() or as a side effect of thread termination. For
unbuffered doing a malloc() per readdir() call is plausible, but this would
require the application to remember to free it after use. Aren't changes like
this beyond the scope of a TC?

I thought the purpose of readdir_r() was so that such mutexes could be set and
released around the copy to the passed buffer during the call to it, not lock
down the stream indefinitely, and then only when another thread, or process,
has the stream open for writing when the implementation fully buffers the
stream...

I could see such an indefinite lock being used to serialize write attempts,
with the writing interface being responsible for adjusting or invalidating
concurrently open read handle's positions in the stream, but the locks should
be superfluous when the stream, buffered or not, is just being copied piecemeal
like readdir_r does. Such serializing has to go on anyways at the inter-process
level, so the same sector of a disk doesn't get written to multiple times
without other processes being given an opportunity to update their per-process
buffer caches of the stream, but since they are per-process it doesn't impinge
on a single threaded process being able to accomplish that transparently to the
application.

(0001698)
dalias (reporter)
2013-08-05 04:00

There are a lot of misconceptions in note 0001697 which I will try to address.

First, sizeof(char) is 1. The C definition is in units of char. sizeof(char) means "how many chars are there in a char?" Further, POSIX requires CHAR_BIT to be 8. I'm not sure what "Version 8 supporting Unicode" means, since POSIX has always supported Unicode. If this text was intended to mean "supporting wide strings as arguments/results of functions which deal with the filesystem", that would require a completely new, parallel set of filesystem interfaces, in which case it's irrelevant to the discussion of bugs in a current interface which does not deal with wide characters. I'll refrain from further comment on the underlying merits of such hypothetical new interfaces.

The text about opendir being "required to explicitly acquire
a mutex on the DIR stream" also makes no sense to me. If there is only one thread using the DIR stream, the only (otherwise-)reasonable implementation of readdir which would fail to be thread-safe is one that uses a single static buffer independent of which DIR stream it's reading from. However, such an implementation is already non-conforming, since:

"The returned pointer, and pointers within the structure, might be invalidated or the structure or the storage areas might be overwritten by a subsequent call to readdir() on the same directory stream. They shall not be affected by a call to readdir() on a different directory stream."

So as far as I can tell, despite there being no explicit text to this effect, a conforming implementation of readdir must already manage a per-DIR-stream buffer.

Perhaps the misconception is that the thread which opens the stream would obtain a permanent lock on the stream, and the implementation would thereby enforce that other threads could not operate on it. That is NOT the intended change. The intended change is from the current situation:

Undefined behavior is invoked if readdir is called under almost any circumstance in a multithreaded process.

to:

Undefined behavior is invoked if multiple threads call readdir on the same DIR stream without application-level synchronization.

There is no proposal to require any new synchronization by the implementation, or even any changes by implementations at all unless they're doing something very weird, since all natural implementations of readdir that conform to the other existing requirements are automatically thread-safe.
(0001700)
shware_systems (reporter)
2013-08-06 11:23
edited on: 2013-08-06 11:37

To summarize reflector response:
1. POSIX only requires support of a subset of UTF-8 in the pax file format.
All other uses of Unicode in the standard are informative suggestions, not
normative requirements. Any support of Unicode by an implementation is a
vendor-specific extension of the standard, not conforming.

2. The changes proposed would require modification to existing applications that
currently use readdir_r(). To advertise readdir() as being thread safe places
some onus on implementations to ensure that it is since it's the implementations
that are forcing the application changes. To me, this means what contention
locking that was implicitly handled in readdir_r() needs to be done elsewhere,
as transparently as practical to the application. Given the semantics of
readdir() I don't believe it can be done there. It can be done in opendir()
and closedir() so that these, and readdir(), may be safely called in either a
main or separate thread.

Doing it this way does not require any changes to be made to single threaded
applications that are using readdir(). Those using readdir_r() could substitute
readdir() for it safely. Multi-threaded apps may have to check that opendir()
indicates they must wait for access to a particular DIR buffer, but this will
not require overall changes to the structure of the application such that the
qualification of "application-level synchronization" needs to be added, as this
sort of change performs thread-level synchronization so that readdir() can be
substituted for readdir_r(). The effective qualification would be that each
thread procedure call opendir() itself, not rely on it happening in a
thread-safe manner in any other part of the application. The recommendation
that closedir() be called as soon as practical by the thread after what
readdir() calls are necessary for it to perform its function properly, so other
threads are waiting as little as necessary for them to safely access it also,
would be appropriate.

At the least, this is considerably more robust than a nebulous "You must define
your own mutex or spin-lock semaphores and tracking in the main thread so that
it ensures collisions are not possible" as implied by "application-level". In
some instances application performance, speed wise, might even improve. The
majority will still be speedier than a simplistic single threaded approach
because they can still take some advantage of hardware latencies. It also
requires no changes to current implementations of readdir(), I believe, beyond
checking that the calling thread is the same as that which called opendir( ).

====================================================================
Suggested wording changes: Whether these should be put in Future Directions for
readdir( ), each interface, or directly as normative I leave as an editorial
decision.

After line 27469, SUSV7 fdopendir() (c082.pdf):
Both opendir( ) and fdopendir( ) shall notify the thread termination handler,
in an implementation-defined manner, that the handle has been opened. The
handler shall be responsible for calling closedir( ) on the returned DIR * in
the absence of an explicit closedir( ) call made by the thread procedure,
whether due to cancellation or normal thread termination. The handler shall
close these handles in reverse order of being notified of a handle being opened.

After line 27481:
The opendir( ) or fdopendir( ) function may fail if:
[EBUSY] Some other thread in the process already has a handle open to the DIR
structure and to prevent data loss this handle must be closed first by that
thread.

[ENOSPC] The implementation-defined space reserved for the registration of
open handles with the thread termination handler is insufficient to store the
handle referenced. (Ed. Not sure this is needed, but it doesn't hurt)

After line 23009, closedir( ):
The closedir( ) function shall remove the effect of the thread termination
registration done by (fd)opendir( ) for dirp. Such removal shall not affect the
termination handling of any other DIR * handles a thread has currently opened
via (fd)opendir( ).

(Ed. This wording permits closedir( )s to be called in any order, and does not
affect any pthread_handler_push( ) or pop( ) sequences used by the application.
The net effect on opendir( ) implied here is that each dirent buffer it is
allocating now anyways needs an extra field to hold the thread id that opened
it as the effective spin-lock that makes readdir( ) thread safe. Whether in
SUSV8 a new interface is defined, allowing query of which thread has ownership
of the buffer by a blocked thread so that the thread can send an inter-thread
signal saying "Hey, I need it NOW", or not I leave open. Makng each thread
responsible for handle cleanup prevents deadlock conditions occurring when
cancelled. This will propagate to a call to exit( ) cancelling threads also.)

At line 55683, readdir( ):
Replace

The pointer returned by readdir( ) points to data which may be overwritten by another call to
readdir( ) on the same directory stream. This data is not overwritten by another call to readdir( )
on a different directory stream.
If a file is removed from or added to the directory after the most recent call to opendir( ) or
rewinddir( ), whether a subsequent call to readdir( ) returns an entry for that file is unspecified.

with

If a file is removed from or added to the directory after the most recent call
to readdir( ) before the effective position maintained by the DIR structure
for readdir( ), the next call to readdir( ) shall return NULL and set errno
to EAGAIN to indicate a rewinddir( ) is required to reliably process the DIR
stream. It is unspecified whether a valid entry is returned if readdir( ) is
then called without such a rewinddir( ) call.

Remove line 55694

After line 55719:
[EAGAIN] A write to the device has invalidated the previous position used by
readdir( ) such that a rewinddir( ) is required.

(Ed. Whether changes happen or not to entries after the current position won't
affect that readdir( ) can process them properly or not. It's just those before
where it is now that can cause misalignment's. I'm not sure if a seekdir( )
past the current record where a readdir( ) was also successfully performed also
needs to be tracked to invalidate the buffer, with appropriate wording changes.
For simple sequential processing of entries I believe this wording suffices.)

After line 55800, Future Directions:
Since the new implementation requirements for opendir( ) and closedir( ) have
the net effect of making readdir( ) thread safe, readdir_r( ) is being marked
OBSOLESCENT as superfluous.
===================================================================

A call to opendir( ) with the same dir argument while the DIR handle is already
allocated to a thread I believe should return that same handle. Whether this
has the same effect as if rewinddir( ) is called, or the readdir( ) position is
left unchanged I'm ambivalent about. To return a different handle would require
an implicit closedir( ) of the prior handle to prevent contention. It should
also be mentioned explicitly somewhere that handles cannot be passed to
different threads, as each interface will be validating which thread is
calling, and the current_thread id won't match the opening_thread id stored in
the dirent. I'm also ambivalent about whether a separate errno code needs to be
specified for each ___dir( ) interface to indicate this if an application tries
to do it anyways. They can certainly pass dir argument strings, or fdopen
handles, but not DIR * handles.

Lastly, the example loop becomes:
{errno=0; while ((dirp=opendir(dir))==NULL&&(errno==EBUSY)) {errno=0};}
if (dirp==NULL) { HandleOtherErrno( ) };
. . .
readdir( ), blah, blah;
. . .
closedir(dirp);

If preemptive thread scheduling is not employed, a time slice release interface
must be called in the while loop after errno reset to prevent deadlock. Calling
such an interface anyways should enhance perceived application liveliness.

(0001701)
shware_systems (reporter)
2013-08-06 18:55

Just a P.S., For SUSV8 getting rid of the example while could be done by defining
an (fd)opendir_b( ) version, where the b would be for 'block until available'.
(0001702)
dalias (reporter)
2013-08-06 21:26

There is no proposed change which requires any modification of any application which is working now(*) nor of any implementation which defines NAME_MAX. No requirements on locking are changed. There is no requirement of locking for any of these interfaces, except in the case where the DIR stream is buffered, in which case readdir_r and the position-related functions must synchronize access to the buffer and position offset relative to the buffer. Existing implementations of readdir are already thread-safe in the proposed sense. All objections raised in note 1700 are failures to understand the present situation and proposed changes, not any flaws in the proposal.

(*) Note that applications using readdir_r on implementations without NAME_MAX are already unable to work correctly.
(0002854)
Florian Weimer (reporter)
2015-10-02 12:51

glibc would like to remove readdir_r completely. There is no safe way to allocate the dirent buffer because its size is not passed to readdir_r. This is why glibc has started to return ENAMETOOLONG errors if a name longer than NAME_MAX is encountered (at the end of the directory stream, to avoid unnecessary truncation). readdir has no such issues and will return longer names, although that semi-flexible array member at the end surely is a wart.

A NAME_MAX value of 255 is generally incorrect because most systems support file systems which have a limit of 255 UCS-2 characters, which, when encoded in a multibyte character set, are longer than 255.
(0002857)
shware_systems (reporter)
2015-10-04 01:01
edited on: 2015-10-04 01:02

An Action Item is still open to propose text of a resolution to this. IIRC, this will include marking readdir_r() OBSOLETE, immediately or as a Future Direction. The consensus ended up at that while for some existing implementations the concerns of Note 1700 were not an issue, the current language did permit types of implementations as conforming for which it was valid, so changes more extensive than that proposed in Note 1606 were warranted before requirements for readdir() to be used in a thread safe way could be specified.

From the 2013-08-08 Etherpad meeting notes, reflecting some of the discussion on this:
--------------------------
If a file is removed from or added to the directory after the most recent call to opendir( ) or
rewinddir( ), whether a subsequent call to readdir_r( ) returns an entry for that file is unspecified.

(lines 56626-7)

line 56604:
They shall not be affected by a call to readdir() on a different directory stream.

line 56587
The type DIR, which is defined in the <dirent.h> header, represents a directory stream, which is
an ordered sequence of all the directory entries in a particular directory.

and in XBD
3.131 Directory Stream
A sequence of all the directory entries in a particular directory. An open directory stream may be
implemented using a file descriptor.

That last sentence is possibly confusing...

page 829 fdopendir()
clarify 2nd paragraph to say that opening two directory streams on the same open file description (whether or not the same fd) is bad, unless you use handle hand-off rules with rewinddir() as part of the hand-off
clarify that two directory streams cannot share buffers, but might share file description

We have 2.5.1 Interaction of File Descriptors and Standard I/O Streams (page 496)
should we have a similar section for interaction of Directory streams and file descriptors?
I'm not sure there's enough meat to warrant another section; a separate paragraph in an existing section might suffice.
------------------------
The changes to page 829ff above is the primary additional change it was felt was needed. I forget whether this was to target TC2 with an interpretation or Issue 8.


- Issue History
Date Modified Username Field Change
2013-05-14 17:32 philip-guenther New Issue
2013-05-14 17:32 philip-guenther Name => Philip Guenther
2013-05-14 17:32 philip-guenther Organization => OpenBSD
2013-05-14 17:32 philip-guenther Section => <limits.h>
2013-05-14 17:32 philip-guenther Page Number => 273
2013-05-14 17:32 philip-guenther Line Number => 8986-8990
2013-05-14 18:03 wlerch Note Added: 0001604
2013-05-14 18:57 philip-guenther Note Added: 0001605
2013-05-15 16:17 dalias Note Added: 0001606
2013-05-30 15:37 eblake Relationship added related to 0000697
2013-05-31 09:08 david-bartley Note Added: 0001632
2013-05-31 09:46 geoffclare Note Added: 0001633
2013-05-31 09:53 geoffclare Note Edited: 0001633
2013-05-31 11:56 dalias Note Added: 0001634
2013-08-05 02:37 shware_systems Note Added: 0001697
2013-08-05 02:46 shware_systems Note Edited: 0001697
2013-08-05 04:00 dalias Note Added: 0001698
2013-08-06 11:23 shware_systems Note Added: 0001700
2013-08-06 11:30 shware_systems Note Edited: 0001700
2013-08-06 11:37 shware_systems Note Edited: 0001700
2013-08-06 18:55 shware_systems Note Added: 0001701
2013-08-06 21:26 dalias Note Added: 0001702
2013-08-08 07:45 Don Cragun Tag Attached: issue8
2013-08-08 07:45 Don Cragun Tag Detached: issue8
2014-03-29 23:32 eblake Relationship added has duplicate 0000831
2014-03-30 00:33 sstewartgallus Issue Monitored: sstewartgallus
2015-10-02 12:51 Florian Weimer Note Added: 0002854
2015-10-02 12:51 Florian Weimer Issue Monitored: Florian Weimer
2015-10-04 01:01 shware_systems Note Added: 0002857
2015-10-04 01:02 shware_systems Note Edited: 0002857


Mantis 1.1.6[^]
Copyright © 2000 - 2008 Mantis Group
Powered by Mantis Bugtracker