0000696: either NAME_MAX shouldn't be optional, or readdir_r() needs clarification

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.

(0005850) Don Cragun (manager) 2022-06-16 16:00	All page and line numbers refer to 202x D2.1. On page 501 line 17715 section 2.9.1, remove readdir() from the list of non-thread-safe functions On page 501 line 17737 section 2.9.1, append: The readdir() function need not be thread-safe if concurrent calls are made for the same directory stream. Shade P219, L7616-7617 (<dirent.h> readdir_r() declaration) with OB shading. Shade P1724, L56494-56495 (readdir_r() SYNOPSIS) with OB shading. Change P1724, L56528 (readdir() DESCRIPTION) from: The readdir() function need not be thread-safe. to: The readdir() function need not be thread-safe if concurrent calls are made for the same directory stream. Add after P1725, L56555 (readdir() ERRORS): [ENOMEM] Insufficient memory is available. Add new paragraph after P1725, L56944 (readdir() RATIONALE): Since the size of a filename has no limit on some filesystem types, there is no way to reliably allocate a buffer large enough to hold a filename being returned by readdir_r(). Therefore, readdir_r() has been marked obsolescent and readdir() is now required to be thread safe as long as there are no concurrent calls to it on a single stream.

(0005877) geoffclare (manager) 2022-06-30 14:11	When applying this bug I realised that all references to readdir_r() on the readdir() page need OB shading, not just the SYNOPSIS. I also added the usual FUTURE DIRECTIONS text for OB functions saying readdir_r() may be removed in a future version.

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
2022-06-16 16:00	Don Cragun	Note Added: 0005850
2022-06-16 16:01	Don Cragun	Interp Status	=> ---
2022-06-16 16:01	Don Cragun	Final Accepted Text	=> See Note: 0005850.
2022-06-16 16:01	Don Cragun	Status	New => Resolved
2022-06-16 16:01	Don Cragun	Resolution	Open => Accepted As Marked
2022-06-16 16:01	Don Cragun	Tag Attached: issue8
2022-06-30 14:11	geoffclare	Note Added: 0005877
2022-06-30 14:11	geoffclare	Status	Resolved => Applied
2023-04-11 15:38	geoffclare	Relationship added	related to 0001664
2024-06-11 09:02	agadmin	Status	Applied => Closed

Aardvark Mark IV