Eliminate dirent memcpy when using readdir. (#25407)

* Eliminate dirent memcpy when using readdir.

* Remove concurrent readdir check

* Update references

* Add comment about readdir concurrency assumption.

Author: tmds

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

@@ -4,13 +4,14 @@
 
 using System;
 using System.Runtime.InteropServices;
+using System.Threading;
 using Microsoft.Win32.SafeHandles;
 
 internal static partial class Interop
 {
     internal static partial class Sys
     {
-        private static readonly int s_direntSize = GetDirentSize();
+        private static readonly int s_readBufferSize = GetReadDirRBufferSize();
 
         internal enum NodeType : int
         {
@@ -42,29 +43,51 @@ internal struct DirectoryEntry
         [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_OpenDir", SetLastError = true)]
         internal static extern Microsoft.Win32.SafeHandles.SafeDirectoryHandle OpenDir(string path);
 
-        [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetDirentSize", SetLastError = false)]
-        internal static extern int GetDirentSize();
+        [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetReadDirRBufferSize", SetLastError = false)]
+        internal static extern int GetReadDirRBufferSize();
 
         [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadDirR", SetLastError = false)]
-        private static extern unsafe int ReadDirR(SafeDirectoryHandle dir, byte* buffer, int bufferSize, out InternalDirectoryEntry outputEntry);
+        private static extern unsafe int ReadDirR(IntPtr dir, byte* buffer, int bufferSize, out InternalDirectoryEntry outputEntry);
 
         [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_CloseDir", SetLastError = true)]
         internal static extern int CloseDir(IntPtr dir);
 
         // The calling pattern for ReadDir is described in src/Native/System.Native/pal_readdir.cpp
         internal static int ReadDir(SafeDirectoryHandle dir, out DirectoryEntry outputEntry)
         {
-            unsafe
+            bool addedRef = false;
+            try
             {
-                // To reduce strcpys, alloc a buffer here and get the result from OS, then copy it over for the caller.
-                byte* buffer = stackalloc byte[s_direntSize];
-                InternalDirectoryEntry temp;
-                int ret = ReadDirR(dir, buffer, s_direntSize, out temp);
-                outputEntry = ret == 0 ?
-                            new DirectoryEntry() { InodeName = GetDirectoryEntryName(temp), InodeType = temp.InodeType } : 
-                            default(DirectoryEntry);
+                // We avoid a native string copy into InternalDirectoryEntry.
+                // - If the platform suppors reading into a buffer, the data is read directly into the buffer. The
+                //   data can be read as long as the buffer is valid.
+                // - If the platform does not support reading into a buffer, the information returned in
+                //   InternalDirectoryEntry points to native memory owned by the SafeDirectoryHandle. The data is only
+                //   valid until the next call to CloseDir/ReadDir. We extend the reference until we have copied all data
+                //   to ensure it does not become invalid by a CloseDir; and we copy the data so our caller does not
+                //   use the native memory held by the SafeDirectoryHandle.
+                dir.DangerousAddRef(ref addedRef);
 
-                return ret;
+                unsafe
+                {
+                    // s_readBufferSize is zero when the native implementation does not support reading into a buffer.
+                    byte* buffer = stackalloc byte[s_readBufferSize];
+                    InternalDirectoryEntry temp;
+                    int ret = ReadDirR(dir.DangerousGetHandle(), buffer, s_readBufferSize, out temp);
+                    // We copy data into DirectoryEntry to ensure there are no dangling references.
+                    outputEntry = ret == 0 ?
+                                new DirectoryEntry() { InodeName = GetDirectoryEntryName(temp), InodeType = temp.InodeType } : 
+                                default(DirectoryEntry);
+
+                    return ret;
+                }
+            }
+            finally
+            {
+                if (addedRef)
+                {
+                    dir.DangerousRelease();
+                }
             }
         }
 

src/Native/Unix/System.Native/pal_io.c

@@ -324,41 +324,38 @@ static void ConvertDirent(const struct dirent* entry, struct DirectoryEntry* out
 #endif
 }
 
-int32_t SystemNative_GetDirentSize(void)
+int32_t SystemNative_GetReadDirRBufferSize(void)
 {
+#if HAVE_READDIR_R
     // dirent should be under 2k in size
     assert(sizeof(struct dirent) < 2048);
     return sizeof(struct dirent);
+#else
+    return 0;
+#endif
 }
 
-// To reduce the number of string copies, this function calling pattern works as follows:
-// 1) The managed code calls GetDirentSize() to get the platform-specific
-//    size of the dirent struct.
-// 2) The managed code creates a byte[] buffer of the size of the native dirent
-//    and passes a pointer to this buffer to this function.
-// 3) This function passes input byte[] buffer to the OS to fill with dirent
-//    data which makes the 1st strcpy.
-// 4) The ConvertDirent function will fill DirectoryEntry outputEntry with
-//    pointers from byte[] buffer.
-// 5) The managed code uses DirectoryEntry outputEntry to find start of d_name
-//    and the value of d_namelen, if avalable, to copy the name from
-//    byte[] buffer into a managed string that the caller can use; this makes
-//    the 2nd and final strcpy.
+// To reduce the number of string copies, the caller of this function is responsible to ensure the memory
+// referenced by outputEntry remains valid until it is read.
+// If the platform supports readdir_r, the caller provides a buffer into which the data is read.
+// If the platform uses readdir, the caller must ensure no calls are made to readdir/closedir since those will invalidate
+// the current dirent. We assume the platform supports concurrent readdir calls to different DIRs.
 int32_t SystemNative_ReadDirR(DIR* dir, void* buffer, int32_t bufferSize, struct DirectoryEntry* outputEntry)
 {
-    assert(buffer != NULL);
     assert(dir != NULL);
     assert(outputEntry != NULL);
 
+#if HAVE_READDIR_R
+    assert(buffer != NULL);
+
     if (bufferSize < (int32_t)sizeof(struct dirent))
     {
-        assert(false && "Buffer size too small; use GetDirentSize to get required buffer size");
+        assert(false && "Buffer size too small; use GetReadDirRBufferSize to get required buffer size");
         return ERANGE;
     }
 
     struct dirent* result = NULL;
     struct dirent* entry = (struct dirent*)buffer;
-#if HAVE_READDIR_R
     int error = readdir_r(dir, entry, &result);
 
     // positive error number returned -> failure
@@ -379,11 +376,13 @@ int32_t SystemNative_ReadDirR(DIR* dir, void* buffer, int32_t bufferSize, struct
     // 0 returned with non-null result (guaranteed to be set to entry arg) -> success
     assert(result == entry);
 #else
+    (void)buffer;     // unused
+    (void)bufferSize; // unused
     errno = 0;
-    result = readdir(dir);
+    struct dirent* entry = readdir(dir);
 
     // 0 returned with null result -> end-of-stream
-    if (result == NULL)
+    if (entry == NULL)
     {
         memset(outputEntry, 0, sizeof(*outputEntry)); // managed out param must be initialized
 
@@ -395,9 +394,6 @@ int32_t SystemNative_ReadDirR(DIR* dir, void* buffer, int32_t bufferSize, struct
         }
         return -1;
     }
-
-    assert(result->d_reclen <= bufferSize);
-    memcpy_s(entry, sizeof(struct dirent), result, (size_t)result->d_reclen);
 #endif
     ConvertDirent(entry, outputEntry);
     return 0;

src/Native/Unix/System.Native/pal_io.h

@@ -356,7 +356,7 @@ int32_t SystemNative_ShmUnlink(const char* name);
 /**
  * Returns the size of the dirent struct on the current architecture
  */
-int32_t SystemNative_GetDirentSize(void);
+int32_t SystemNative_GetReadDirRBufferSize(void);
 
 /**
  * Re-entrant readdir that will retrieve the next dirent from the directory stream pointed to by dir.