Remove readdir_r entirely and handle filenames > 255 bytes (#116639)

* Increase d_name size

* Handle filenames that wouldn't fit in a 256 decoding buffer

* Remove readdir_r

* Address feedback

* Apply suggestions from code review

---------

Co-authored-by: Jan Kotas <[email protected]>

Author: jozkee

src/libraries/Common/src/Interop/Unix/System.Native/Interop.ReadDir.cs

@@ -29,39 +29,32 @@ internal unsafe struct DirectoryEntry
             internal byte* Name;
             internal int NameLength;
             internal NodeType InodeType;
-            internal const int NameBufferSize = 256; // sizeof(dirent->d_name) == NAME_MAX + 1
 
             internal ReadOnlySpan<char> GetName(Span<char> buffer)
             {
-                // -1 for null terminator (buffer will not include one),
-                //  and -1 because GetMaxCharCount pessimistically assumes the buffer may start with a partial surrogate
-                Debug.Assert(buffer.Length >= Encoding.UTF8.GetMaxCharCount(NameBufferSize - 1 - 1));
-
                 Debug.Assert(Name != null, "should not have a null name");
 
                 ReadOnlySpan<byte> nameBytes = (NameLength == -1)
-                    // In this case the struct was allocated via struct dirent *readdir(DIR *dirp);
-                    ? new ReadOnlySpan<byte>(Name, new ReadOnlySpan<byte>(Name, NameBufferSize).IndexOf<byte>(0))
+                    ? MemoryMarshal.CreateReadOnlySpanFromNullTerminated(Name)
                     : new ReadOnlySpan<byte>(Name, NameLength);
 
                 Debug.Assert(nameBytes.Length > 0, "we shouldn't have gotten a garbage value from the OS");
 
-                int charCount = Encoding.UTF8.GetChars(nameBytes, buffer);
-                ReadOnlySpan<char> value = buffer.Slice(0, charCount);
-                Debug.Assert(NameLength != -1 || !value.Contains('\0'), "should not have embedded nulls if we parsed the end of string");
-                return value;
+                ReadOnlySpan<char> result = !Encoding.UTF8.TryGetChars(nameBytes, buffer, out int charsWritten)
+                    ? Encoding.UTF8.GetString(nameBytes) // Fallback to allocation since this is a rare case
+                    : buffer.Slice(0, charsWritten);
+
+                Debug.Assert(!result.Contains('\0'), "should not have embedded nulls");
+
+                return result;
             }
         }
 
         [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_OpenDir", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
         internal static partial IntPtr OpenDir(string path);
 
-        [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetReadDirRBufferSize", SetLastError = false)]
-        [SuppressGCTransition]
-        internal static partial int GetReadDirRBufferSize();
-
-        [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadDirR")]
-        internal static unsafe partial int ReadDirR(IntPtr dir, byte* buffer, int bufferSize, DirectoryEntry* outputEntry);
+        [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadDir")]
+        internal static unsafe partial int ReadDir(IntPtr dir, DirectoryEntry* outputEntry);
 
         [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_CloseDir", SetLastError = true)]
         internal static partial int CloseDir(IntPtr dir);

src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEntry.Unix.cs

@@ -3,6 +3,7 @@
 
 using System.Diagnostics;
 using System.Runtime.InteropServices;
+using System.Runtime.CompilerServices;
 
 namespace System.IO.Enumeration
 {
@@ -11,6 +12,9 @@ namespace System.IO.Enumeration
     /// </summary>
     public unsafe ref partial struct FileSystemEntry
     {
+        // A reasonable size for a decoding buffer. If it's not enough, we fall back to allocating a string.
+        // Some filesystem can have filenames larger than that.
+        private const int DecodedNameBufferLength = 256;
         private Interop.Sys.DirectoryEntry _directoryEntry;
         private bool _isDirectory;
         private FileStatus _status;
@@ -19,10 +23,10 @@ public unsafe ref partial struct FileSystemEntry
         private ReadOnlySpan<char> _fileName;
         private FileNameBuffer _fileNameBuffer;
 
-        // Wrap the fixed buffer to workaround visibility issues in api compat verification
+        [InlineArray(DecodedNameBufferLength)]
         private struct FileNameBuffer
         {
-            internal fixed char _buffer[Interop.Sys.DirectoryEntry.NameBufferSize];
+            internal char _char0;
         }
 
         internal static FileAttributes Initialize(
@@ -95,7 +99,9 @@ public ReadOnlySpan<char> FileName
             {
                 if (_directoryEntry.NameLength != 0 && _fileName.Length == 0)
                 {
-                    Span<char> buffer = MemoryMarshal.CreateSpan(ref _fileNameBuffer._buffer[0], Interop.Sys.DirectoryEntry.NameBufferSize);
+                    // Use unsafe API to create the Span to allow it to escape. It is safe as long as
+                    // the whole FileSystemEntry is never copied.
+                    Span<char> buffer = MemoryMarshal.CreateSpan(ref _fileNameBuffer._char0, DecodedNameBufferLength);
                     _fileName = _directoryEntry.GetName(buffer);
                 }
 

src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs

@@ -29,8 +29,6 @@ public abstract unsafe partial class FileSystemEnumerator<TResult> : CriticalFin
 
         // Used for creating full paths
         private char[]? _pathBuffer;
-        // Used to get the raw entry data
-        private byte[]? _entryBuffer;
 
         private void Init()
         {
@@ -45,8 +43,6 @@ private void Init()
             try
             {
                 _pathBuffer = ArrayPool<char>.Shared.Rent(StandardBufferSize);
-                int size = Interop.Sys.GetReadDirRBufferSize();
-                _entryBuffer = size > 0 ? ArrayPool<byte>.Shared.Rent(size) : null;
             }
             catch
             {
@@ -103,13 +99,11 @@ public bool MoveNext()
                 // If HAVE_READDIR_R is defined for the platform FindNextEntry depends on _entryBuffer being fixed since
                 // _entry will point to a string in the middle of the array. If the array is not fixed GC can move it after
                 // the native call and _entry will point to a bogus file name.
-                fixed (byte* entryBufferPtr = _entryBuffer)
+                do
                 {
-                    do
-                    {
-                        FindNextEntry(entryBufferPtr, _entryBuffer == null ? 0 : _entryBuffer.Length);
-                        if (_lastEntryFound)
-                            return false;
+                    FindNextEntry();
+                    if (_lastEntryFound)
+                        return false;
 
                         FileAttributes attributes = FileSystemEntry.Initialize(
                             ref entry, _entry, _currentPath, _rootDirectory, _originalRootDirectory, new Span<char>(_pathBuffer));
@@ -152,32 +146,23 @@ public bool MoveNext()
                             }
                         }
 
-                        if (ShouldIncludeEntry(ref entry))
-                        {
-                            _current = TransformEntry(ref entry);
-                            return true;
-                        }
-                    } while (true);
-                }
+                    if (ShouldIncludeEntry(ref entry))
+                    {
+                        _current = TransformEntry(ref entry);
+                        return true;
+                    }
+                } while (true);
             }
 
             bool ShouldSkip(FileAttributes attributeToSkip) => (_options.AttributesToSkip & attributeToSkip) != 0;
         }
 
         private unsafe void FindNextEntry()
-        {
-            fixed (byte* entryBufferPtr = _entryBuffer)
-            {
-                FindNextEntry(entryBufferPtr, _entryBuffer == null ? 0 : _entryBuffer.Length);
-            }
-        }
-
-        private unsafe void FindNextEntry(byte* entryBufferPtr, int bufferLength)
         {
             int result;
             fixed (Interop.Sys.DirectoryEntry* e = &_entry)
             {
-                result = Interop.Sys.ReadDirR(_directoryHandle, entryBufferPtr, bufferLength, e);
+                result = Interop.Sys.ReadDir(_directoryHandle, e);
             }
 
             switch (result)
@@ -245,12 +230,6 @@ private void InternalDispose(bool disposing)
                         _pathBuffer = null;
                         ArrayPool<char>.Shared.Return(pathBuffer);
                     }
-
-                    if (_entryBuffer is byte[] entryBuffer)
-                    {
-                        _entryBuffer = null;
-                        ArrayPool<byte>.Shared.Return(entryBuffer);
-                    }
                 }
             }
 

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.NonAndroid.cs

@@ -260,17 +260,6 @@ private static List<string> ParseTimeZoneIds(StreamReader reader)
             return id;
         }
 
-        private static string? GetDirectoryEntryFullPath(ref Interop.Sys.DirectoryEntry dirent, string currentPath)
-        {
-            ReadOnlySpan<char> direntName = dirent.GetName(stackalloc char[Interop.Sys.DirectoryEntry.NameBufferSize]);
-
-            if ((direntName.Length == 1 && direntName[0] == '.') ||
-                (direntName.Length == 2 && direntName[0] == '.' && direntName[1] == '.'))
-                return null;
-
-            return Path.Join(currentPath.AsSpan(), direntName);
-        }
-
         private static bool CompareTimeZoneFile(string filePath, byte[] buffer, byte[] rawData)
         {
             try

src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/Directory/EnumerableTests.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
@@ -34,6 +35,26 @@ public void FileEnumeratorIsThreadSafe()
             }
         }
 
+        [Fact]
+        public void FileEnumeratorIsThreadSafe_ParallelForEach()
+        {
+            List<string> expected = [];
+            string directory = Directory.CreateDirectory(GetTestFilePath()).FullName;
+            for (int i = 0; i < 100; i++)
+            {
+                string file = Path.Join(directory, GetTestFileName());
+                File.Create(file).Dispose();
+                expected.Add(file);
+            }
+
+            for (int i = 0; i < 100; i++) // test multiple times to ensure thread safety.
+            {
+                ConcurrentBag<string> result = [];
+                ParallelLoopResult parallelResult = Parallel.ForEach(Directory.EnumerateFiles(directory), f => result.Add(f));
+                AssertExtensions.CollectionEqual(expected, result, StringComparer.Ordinal);
+            }
+        }
+
         [Fact]
         public void EnumerateDirectories_NonBreakingSpace()
         {

src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/ManualTests/NtfsOnLinuxSetup.cs

@@ -0,0 +1,56 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Diagnostics;
+using Xunit.Sdk;
+
+namespace System.IO.ManualTests
+{
+    public class NtfsOnLinuxSetup : IDisposable
+    {
+        public NtfsOnLinuxSetup()
+        {
+            if (!NtfsOnLinuxTests.IsManualTestsEnabledAndElevated)
+                throw new XunitException("Set MANUAL_TESTS envvar and run as elevated to execute this test setup.");
+            
+            ExecuteShell("""
+                dd if=/dev/zero of=my_loop_device.img bs=1M count=100
+                losetup /dev/loop99 my_loop_device.img
+                mkfs -t ntfs /dev/loop99
+                mkdir -p /mnt/ntfs
+                mount /dev/loop99 /mnt/ntfs
+                """);
+        }
+
+        public void Dispose()
+        {
+            ExecuteShell("""
+                umount /mnt/ntfs
+                losetup -d /dev/loop99
+                rm my_loop_device.img
+                """);
+        }
+        
+        private static void ExecuteShell(string command)
+        {
+            using Process process = new Process
+            {
+                StartInfo = new ProcessStartInfo
+                {
+                    FileName = "/bin/sh",
+                    ArgumentList = { "-c", command },
+                    RedirectStandardOutput = true,
+                    RedirectStandardError = true,
+                    UseShellExecute = false
+                }
+            };
+            process.OutputDataReceived += (sender, e) => Console.WriteLine($"[OUTPUT] {e.Data}");
+            process.ErrorDataReceived += (sender, e) => Console.WriteLine($"[ERROR] {e.Data}");
+
+            process.Start();
+            process.BeginOutputReadLine();
+            process.BeginErrorReadLine();
+            process.WaitForExit();
+        }
+    }
+}

src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/ManualTests/NtfsOnLinuxTests.cs

@@ -0,0 +1,29 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Linq;
+using System.Text;
+using Xunit;
+
+namespace System.IO.ManualTests
+{
+    public class NtfsOnLinuxTests : IClassFixture<NtfsOnLinuxSetup>
+    {
+        internal static bool IsManualTestsEnabledAndElevated => FileSystemManualTests.ManualTestsEnabled && AdminHelpers.IsProcessElevated();
+
+        [ConditionalTheory(nameof(IsManualTestsEnabledAndElevated))]
+        [PlatformSpecific(TestPlatforms.Linux)]
+        [InlineData("Ω", 255)]
+        [InlineData("あ", 255)]
+        [InlineData("😀", 127)]
+        public void NtfsOnLinux_FilenamesLongerThan255Bytes_FileEnumerationSucceeds(string codePoint, int maxAllowedLength)
+        {
+            string filename = string.Concat(Enumerable.Repeat(codePoint, maxAllowedLength));
+            Assert.True(Encoding.UTF8.GetByteCount(filename) > 255);
+
+            string filePath = $"/mnt/ntfs/{filename}";
+            File.Create(filePath).Dispose();
+            Assert.Contains(filePath, Directory.GetFiles("/mnt/ntfs"));
+        }
+    }
+}

src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/ManualTests/System.IO.FileSystem.Manual.Tests.csproj

@@ -4,5 +4,7 @@
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="ManualTests.cs" />
+    <Compile Include="NtfsOnLinuxSetup.cs" />
+    <Compile Include="NtfsOnLinuxTests.cs" />
   </ItemGroup>
 </Project>

src/native/libs/Common/pal_config.h.in

@@ -27,7 +27,6 @@
 #cmakedefine01 HAVE_STAT_FLAGS
 #cmakedefine01 HAVE_LCHFLAGS
 #cmakedefine01 HAVE_GNU_STRERROR_R
-#cmakedefine01 HAVE_READDIR_R
 #cmakedefine01 HAVE_DIRENT_NAME_LEN
 #cmakedefine01 HAVE_MNTINFO
 #cmakedefine01 HAVE_STATFS_FSTYPENAME

src/native/libs/System.Native/entrypoints.c

@@ -66,8 +66,7 @@ static const Entry s_sysNative[] =
     DllImportEntry(SystemNative_MemfdCreate)
     DllImportEntry(SystemNative_ShmOpen)
     DllImportEntry(SystemNative_ShmUnlink)
-    DllImportEntry(SystemNative_GetReadDirRBufferSize)
-    DllImportEntry(SystemNative_ReadDirR)
+    DllImportEntry(SystemNative_ReadDir)
     DllImportEntry(SystemNative_OpenDir)
     DllImportEntry(SystemNative_CloseDir)
     DllImportEntry(SystemNative_Pipe)