-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Remove readdir_r entirely and handle filenames > 255 bytes #116639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the legacy readdir_r
support across the native libraries and switches to using readdir
exclusively, simplifying the buffer management and unifying the directory enumeration path. It also adds a manual test for NTFS filename length handling and extends the existing parallel file enumeration tests.
- Remove
HAVE_READDIR_R
checks and associated APIs in both CMake config and headers. - Replace
SystemNative_ReadDirR
/buffer-size API withSystemNative_ReadDir
in the PAL and interop layers. - Update
FileSystemEnumerator.Unix
and related code to drop manual buffer management. - Add manual NTFS-on-Linux mount setup and a new threaded enumeration test.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/native/libs/configure.cmake | Dropped check for readdir_r availability |
src/native/libs/System.Native/pal_io.h | Removed R-buffer APIs; added SystemNative_ReadDir |
src/native/libs/System.Native/pal_io.c | Deleted readdir_r path; streamlined SystemNative_ReadDir |
src/native/libs/System.Native/entrypoints.c | Updated entrypoints to use SystemNative_ReadDir |
src/native/libs/Common/pal_config.h.in | Removed HAVE_READDIR_R define |
src/libraries/System.Runtime/tests/.../ManualTests.csproj | Included new NTFS manual test source files |
src/libraries/System.Runtime/tests/.../NtfsOnLinuxTests.cs | Added manual NTFS filename-length test |
src/libraries/System.Runtime/tests/.../NtfsOnLinuxSetup.cs | Added loopback NTFS mount/unmount fixture for manual tests |
src/libraries/System.Runtime/tests/.../EnumerableTests.cs | Added a Parallel.ForEach enumeration test |
src/libraries/System.Private.CoreLib/.../NonAndroid.cs | Removed unused GetDirectoryEntryFullPath helper |
src/libraries/System.Private.CoreLib/.../FileSystemEnumerator.Unix.cs | Deleted entry-buffer logic; call into new ReadDir |
src/libraries/System.Private.CoreLib/.../FileSystemEntry.Unix.cs | Updated fixed buffer size constant and span usage |
src/libraries/Common/.../Interop.ReadDir.cs | Updated interop to only export ReadDir ; improved GetName |
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEntry.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEntry.Unix.cs
Show resolved
Hide resolved
Span<char> buffer = MemoryMarshal.CreateSpan(ref _fileNameBuffer._buffer[0], DecodedNameBufferLength); | ||
_fileName = _directoryEntry.GetName(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Span<char> buffer = MemoryMarshal.CreateSpan(ref _fileNameBuffer._buffer[0], DecodedNameBufferLength); | |
_fileName = _directoryEntry.GetName(buffer); | |
_fileName = _directoryEntry.GetName(_buffer); |
If you accept the suggestion above, this should be able to use default conversion from InlineArray to Span
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This failed for me locally:
error CS9084: Struct member returns 'this' or other instance members by reference
I pushed anyways to share the CI error, it does compile fine in sharplab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, I have not noticed that the Span escapes here and so it is not possible to create it in safe way (without large refactoring that would likely hurt overall readability).
LGTM otherwise. |
In my survey of open source libc's, |
@AustinWise the question is more about readdir using a global static buffer, see #116619 (comment). |
I think POSIX.2024 also alludes to the buffer being global, and not just stored in each
|
I don't see the I tried to find some example of a libc returning an "internal static buffer". I don't see it in the earliest available commits for illumos from 2005 or GLibC from 1995. BSD stopped using a static buffer in 1982. |
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEntry.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEntry.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEntry.Unix.cs
Show resolved
Hide resolved
Span<char> buffer = MemoryMarshal.CreateSpan(ref _fileNameBuffer._buffer[0], DecodedNameBufferLength); | ||
_fileName = _directoryEntry.GetName(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, I have not noticed that the Span escapes here and so it is not possible to create it in safe way (without large refactoring that would likely hurt overall readability).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/ba-g #116697. |
Won't this regress #47584? macOS used to be using |
Yes, it looks like a problem - even on Linux. @hamarb123 Would you like to submit a PR with the fix? |
@jkotas should I just fix the one in |
It would be great if you can fix all instances that you can find. |
@jkotas I've had a look at some other functions which have Or would you rather I just do the change for |
I think there should not be that many places that call the Linux syscalls directly in our shipping code. I am not sure whether we need a shared wrapper around the syscalls. Do you have a list of places that have the potential problems? |
I will have a proper look tomorrow, can you remind me which path/s I can limit my search to for just the actual shipping code? |
If the path contains "test", it is non-shipping code. Also, the code under |
A bit late, but my testing on illumos with this included seems fine. |
Alternative approach described in #116619 suitable for main but requires validation for multiple platforms, especially the ones where historical fixes are being removed.
@NattyNarwhal, as per the git history, you've been submitting multiple fixes for AIX, could you please help validating this one?
Tagging IllumOS folks and kindly asking the same: @AustinWise @gwr @am11.