Switch from using SafeHandle for Unix enumeration (#27052)

* Switch from using SafeHandle for Unix enumeration

* Address feedback

* Fix faceplant, dispose in test.

4 files changed, 59 insertions, 17 deletions

diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ReadDir.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ReadDir.cs
index 5712b20eb9..695d50ba2b 100644
--- a/src/Common/src/Interop/Unix/System.Native/Interop.ReadDir.cs
+++ b/src/Common/src/Interop/Unix/System.Native/Interop.ReadDir.cs
@@ -39,7 +39,7 @@ internal static partial class Interop
                 Debug.Assert(buffer.Length >= Encoding.UTF8.GetMaxCharCount(255), "should have enough space for the max file name");
                 Debug.Assert(Name != null, "should not have a null name");
 
-                ReadOnlySpan<byte> nameBytes = NameLength == -1
+                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, 255).IndexOf<byte>(0))
                     : new ReadOnlySpan<byte>(Name, NameLength);
@@ -50,13 +50,13 @@ internal static partial class Interop
 
                 int charCount = Encoding.UTF8.GetChars(nameBytes, buffer);
                 ReadOnlySpan<char> value = buffer.Slice(0, charCount);
-                Debug.Assert(value.IndexOf('\0') == -1, "should not have embedded nulls");
+                Debug.Assert(NameLength != -1 || value.IndexOf('\0') == -1, "should not have embedded nulls if we parsed the end of string");
                 return value;
             }
         }
 
         [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_OpenDir", SetLastError = true)]
-        internal static extern SafeDirectoryHandle OpenDir(string path);
+        internal static extern IntPtr OpenDir(string path);
 
         [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetReadDirRBufferSize", SetLastError = false)]
         internal static extern int GetReadDirRBufferSize();
@@ -74,13 +74,13 @@ internal static partial class Interop
         /// 
         /// Call <see cref="ReadBufferSize"/> to see what size buffer to allocate.
         /// </summary>
-        internal static int ReadDir(SafeDirectoryHandle dir, Span<byte> buffer, ref DirectoryEntry entry)
+        internal static int ReadDir(IntPtr dir, Span<byte> buffer, ref DirectoryEntry entry)
         {
             // The calling pattern for ReadDir is described in src/Native/Unix/System.Native/pal_io.cpp|.h
             Debug.Assert(buffer.Length >= ReadBufferSize, "should have a big enough buffer for the raw data");
 
             // ReadBufferSize is zero when the native implementation does not support reading into a buffer.
-            return ReadDirR(dir.DangerousGetHandle(), ref MemoryMarshal.GetReference(buffer), ReadBufferSize, ref entry);
+            return ReadDirR(dir, ref MemoryMarshal.GetReference(buffer), ReadBufferSize, ref entry);
         }
     }
 }
diff --git a/src/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs b/src/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs
index b37dd2a963..7e9bbd6e4c 100644
--- a/src/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs
+++ b/src/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs
@@ -4,9 +4,8 @@
 
 using System.Buffers;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.Runtime.ConstrainedExecution;
-using Microsoft.Win32.SafeHandles;
+using System.Threading;
 
 namespace System.IO.Enumeration
 {
@@ -22,7 +21,7 @@ namespace System.IO.Enumeration
         private readonly object _lock = new object();
 
         private string _currentPath;
-        private SafeDirectoryHandle _directoryHandle;
+        private IntPtr _directoryHandle;
         private bool _lastEntryFound;
         private Queue<string> _pending;
 
@@ -48,7 +47,7 @@ namespace System.IO.Enumeration
             // We need to initialize the directory handle up front to ensure
             // we immediately throw IO exceptions for missing directory/etc.
             _directoryHandle = CreateDirectoryHandle(_rootDirectory);
-            if (_directoryHandle == null)
+            if (_directoryHandle == IntPtr.Zero)
                 _lastEntryFound = true;
 
             _currentPath = _rootDirectory;
@@ -67,18 +66,16 @@ namespace System.IO.Enumeration
             }
         }
 
-        private SafeDirectoryHandle CreateDirectoryHandle(string path)
+        private IntPtr CreateDirectoryHandle(string path)
         {
-            // TODO: https://github.com/dotnet/corefx/issues/26715
-            // - Use IntPtr handle directly
-            SafeDirectoryHandle handle = Interop.Sys.OpenDir(path);
-            if (handle.IsInvalid)
+            IntPtr handle = Interop.Sys.OpenDir(path);
+            if (handle == IntPtr.Zero)
             {
                 Interop.ErrorInfo info = Interop.Sys.GetLastErrorInfo();
                 if ((_options.IgnoreInaccessible && IsAccessError(info.RawErrno))
                     || ContinueOnError(info.RawErrno))
                 {
-                    return null;
+                    return IntPtr.Zero;
                 }
                 throw Interop.GetExceptionForIoErrno(info, path, isDirectory: true);
             }
@@ -87,8 +84,9 @@ namespace System.IO.Enumeration
 
         private void CloseDirectoryHandle()
         {
-            _directoryHandle?.Dispose();
-            _directoryHandle = null;
+            IntPtr handle = Interlocked.Exchange(ref _directoryHandle, IntPtr.Zero);
+            if (handle != IntPtr.Zero)
+                Interop.Sys.CloseDir(handle);
         }
 
         public bool MoveNext()
diff --git a/src/System.IO.FileSystem/tests/Enumeration/ErrorHandlingTests.netcoreapp.cs b/src/System.IO.FileSystem/tests/Enumeration/ErrorHandlingTests.netcoreapp.cs
new file mode 100644
index 0000000000..24f07f8911
--- /dev/null
+++ b/src/System.IO.FileSystem/tests/Enumeration/ErrorHandlingTests.netcoreapp.cs
@@ -0,0 +1,43 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.IO.Enumeration;
+using Xunit;
+
+namespace System.IO.Tests
+{
+    public class ErrorHandlingTests : FileSystemTest
+    {
+        private class IgnoreErrors : FileSystemEnumerator<string>
+        {
+            public IgnoreErrors(string directory)
+                : base(directory)
+            { }
+
+            public int ErrorCount { get; private set; }
+
+            protected override string TransformEntry(ref FileSystemEntry entry)
+            {
+                throw new NotImplementedException();
+            }
+
+            protected override bool ContinueOnError(int error)
+            {
+                ErrorCount++;
+                return true;
+            }
+        }
+
+        [Fact]
+        public void OpenErrorDoesNotHappenAgainOnMoveNext()
+        {
+            using (IgnoreErrors ie = new IgnoreErrors(Path.GetRandomFileName()))
+            {
+                Assert.Equal(1, ie.ErrorCount);
+                Assert.False(ie.MoveNext());
+                Assert.Equal(1, ie.ErrorCount);
+            }
+        }
+    }
+}
diff --git a/src/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj b/src/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj
index 471bf4cd52..63a4b2c7cf 100644
--- a/src/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj
+++ b/src/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj
@@ -59,6 +59,7 @@
     <Compile Include="Enumeration\DosMatcherTests.netcoreapp.cs" />
     <Compile Include="Enumeration\MatchCasingTests.netcoreapp.cs" />
     <Compile Include="Enumeration\TrimmedPaths.netcoreapp.cs" />
+    <Compile Include="Enumeration\ErrorHandlingTests.netcoreapp.cs" />
   </ItemGroup>
   <ItemGroup>
     <!-- Rewritten -->