Fix FileSystemWatcher state cleanup on dispose/finalization on Windows (#116830)

* Fix FileSystemWatcher state cleanup on dispose/finalization on Windows

When the FSW is disposed or finalized while it's in use, on Windows the implementation disposes of the directory handle as a mechanism to stop the monitoring. If by that point the state object's weak reference to the FSW has been cleared out (which it always will have been in the case of finalization), the state object's relevant members are not disposed, leaking several objects. This fixes that to ensure the state is cleaned up, by cleaning it up when the callback is unable to get the FSW instance.

This also addresses concerns over long-pinned managed buffers by switching to using a native buffer. Today the buffer is allocated and then immediately pinned, due to being passed immediately to the OS via a P/Invoke, and all writes to that buffer happen in unmanaged code. All reading also is already happening via a bounds-checked span. So instead of allocating and pinning a managed array, just allocate a native buffer. 

* Address PR feedback

* Address PR feedback

Author: stephentoub

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadDirectoryChangesW.cs

@@ -14,7 +14,7 @@ internal static partial class Kernel32
         [return: MarshalAs(UnmanagedType.Bool)]
         internal static unsafe partial bool ReadDirectoryChangesW(
             SafeFileHandle hDirectory,
-            byte[] lpBuffer,
+            void* lpBuffer,
             uint nBufferLength,
             [MarshalAs(UnmanagedType.Bool)] bool bWatchSubtree,
             uint dwNotifyFilter,

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs

@@ -84,6 +84,20 @@ private void StartRaisingEvents()
             }
         }
 
+        /// <summary>Allocates a buffer of the requested internal buffer size.</summary>
+        /// <returns>The allocated buffer.</returns>
+        private byte[] AllocateBuffer()
+        {
+            try
+            {
+                return new byte[_internalBufferSize];
+            }
+            catch (OutOfMemoryException)
+            {
+                throw new OutOfMemoryException(SR.Format(SR.BufferSizeTooLarge, _internalBufferSize));
+            }
+        }
+
         /// <summary>Cancels the currently running watch operation if there is one.</summary>
         private void StopRaisingEvents()
         {

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs

@@ -25,58 +25,67 @@ private void StartRaisingEvents()
             }
 
             // If we're already running, don't do anything.
-            if (!IsHandleInvalid(_directoryHandle))
+            if (!IsHandleClosed(_directoryHandle))
                 return;
 
             // Create handle to directory being monitored
-            _directoryHandle = Interop.Kernel32.CreateFile(
+            SafeFileHandle directoryHandle = Interop.Kernel32.CreateFile(
                 lpFileName: _directory,
                 dwDesiredAccess: Interop.Kernel32.FileOperations.FILE_LIST_DIRECTORY,
                 dwShareMode: FileShare.Read | FileShare.Delete | FileShare.Write,
                 dwCreationDisposition: FileMode.Open,
                 dwFlagsAndAttributes: Interop.Kernel32.FileOperations.FILE_FLAG_BACKUP_SEMANTICS | Interop.Kernel32.FileOperations.FILE_FLAG_OVERLAPPED);
 
-            if (IsHandleInvalid(_directoryHandle))
+            if (directoryHandle.IsInvalid)
             {
-                _directoryHandle = null;
                 throw new FileNotFoundException(SR.Format(SR.FSW_IOError, _directory));
             }
 
             // Create the state associated with the operation of monitoring the direction
-            AsyncReadState state;
+            AsyncReadState state = new(
+                Interlocked.Increment(ref _currentSession), // ignore all events that were initiated before this
+                this,
+                directoryHandle);
             try
             {
-                // Start ignoring all events that were initiated before this, and
-                // allocate the buffer to be pinned and used for the duration of the operation
-                int session = Interlocked.Increment(ref _currentSession);
-                byte[] buffer = AllocateBuffer();
+                unsafe
+                {
+                    uint bufferSize = _internalBufferSize;
+                    state.Buffer = NativeMemory.Alloc(bufferSize);
+                    state.BufferByteLength = bufferSize;
+                }
+
+                state.ThreadPoolBinding = ThreadPoolBoundHandle.BindHandle(directoryHandle);
 
                 // Store all state, including a preallocated overlapped, into the state object that'll be
                 // passed from iteration to iteration during the lifetime of the operation.  The buffer will be pinned
                 // from now until the end of the operation.
-                state = new AsyncReadState(session, buffer, _directoryHandle, ThreadPoolBoundHandle.BindHandle(_directoryHandle), this);
                 unsafe
                 {
                     state.PreAllocatedOverlapped = new PreAllocatedOverlapped((errorCode, numBytes, overlappedPointer) =>
                     {
                         AsyncReadState state = (AsyncReadState)ThreadPoolBoundHandle.GetNativeOverlappedState(overlappedPointer)!;
-                        state.ThreadPoolBinding.FreeNativeOverlapped(overlappedPointer);
+                        state.ThreadPoolBinding!.FreeNativeOverlapped(overlappedPointer);
+
                         if (state.WeakWatcher.TryGetTarget(out FileSystemWatcher? watcher))
                         {
                             watcher.ReadDirectoryChangesCallback(errorCode, numBytes, state);
                         }
-                    }, state, buffer);
+                        else
+                        {
+                            state.Dispose();
+                        }
+                    }, state, null);
                 }
             }
             catch
             {
-                // Make sure we don't leave a valid directory handle set if we're not running
-                _directoryHandle.Dispose();
-                _directoryHandle = null;
+                state.Dispose();
                 throw;
             }
 
             // Start monitoring
+            _directoryHandle = directoryHandle;
             _enabled = true;
             Monitor(state);
         }
@@ -90,7 +99,7 @@ private void StopRaisingEvents()
                 return;
 
             // If we're not running, do nothing.
-            if (IsHandleInvalid(_directoryHandle))
+            if (IsHandleClosed(_directoryHandle))
                 return;
 
             // Start ignoring all events occurring after this.
@@ -118,7 +127,7 @@ private void FinalizeDispose()
             // We must explicitly dispose the handle to ensure it gets closed before this object is finalized.
             // Otherwise, it is possible that the GC will decide to finalize the handle after this,
             // leaving a window of time where our callback could be invoked on a non-existent object.
-            if (!IsHandleInvalid(_directoryHandle))
+            if (!IsHandleClosed(_directoryHandle))
                 _directoryHandle.Dispose();
         }
 
@@ -128,11 +137,23 @@ private void FinalizeDispose()
         // Unmanaged handle to monitored directory
         private SafeFileHandle? _directoryHandle;
 
-        private static bool IsHandleInvalid([NotNullWhen(false)] SafeFileHandle? handle)
+        /// <summary>Allocates a buffer of the requested internal buffer size.</summary>
+        /// <returns>The allocated buffer.</returns>
+        private unsafe byte* AllocateBuffer()
         {
-            return handle == null || handle.IsInvalid || handle.IsClosed;
+            try
+            {
+                return (byte*)NativeMemory.Alloc(_internalBufferSize);
+            }
+            catch (OutOfMemoryException)
+            {
+                throw new OutOfMemoryException(SR.Format(SR.BufferSizeTooLarge, _internalBufferSize));
+            }
         }
 
+        private static bool IsHandleClosed([NotNullWhen(false)] SafeFileHandle? handle) =>
+            handle is null || handle.IsClosed;
+
         /// <summary>
         /// Initiates the next asynchronous read operation if monitoring is still desired.
         /// If the directory handle has been closed due to an error or due to event monitoring
@@ -141,6 +162,7 @@ private static bool IsHandleInvalid([NotNullWhen(false)] SafeFileHandle? handle)
         private unsafe void Monitor(AsyncReadState state)
         {
             Debug.Assert(state.PreAllocatedOverlapped != null);
+            Debug.Assert(state.ThreadPoolBinding is not null && state.Buffer is not null, "Members should have been set at construction");
 
             // This method should only ever access the directory handle via the state object passed in, and not access it
             // via _directoryHandle.  While this function is executing asynchronously, another thread could set
@@ -150,35 +172,35 @@ private unsafe void Monitor(AsyncReadState state)
 
             NativeOverlapped* overlappedPointer = null;
             bool continueExecuting = false;
+            int win32Error = 0;
             try
             {
                 // If shutdown has been requested, exit.  The finally block will handle
                 // cleaning up the entire operation, as continueExecuting will remain false.
-                if (!_enabled || IsHandleInvalid(state.DirectoryHandle))
+                if (!_enabled || IsHandleClosed(state.DirectoryHandle))
                     return;
 
                 // Get the overlapped pointer to use for this iteration.
                 overlappedPointer = state.ThreadPoolBinding.AllocateNativeOverlapped(state.PreAllocatedOverlapped);
                 continueExecuting = Interop.Kernel32.ReadDirectoryChangesW(
                     state.DirectoryHandle,
-                    state.Buffer, // the buffer is kept pinned for the duration of the sync and async operation by the PreAllocatedOverlapped
-                    (uint)state.Buffer.Length,
+                    state.Buffer,
+                    (uint)state.BufferByteLength,
                     _includeSubdirectories,
                     (uint)_notifyFilters,
                     null,
                     overlappedPointer,
                     null);
+                if (!continueExecuting)
+                {
+                    win32Error = Marshal.GetLastWin32Error();
+                }
             }
             catch (ObjectDisposedException)
             {
                 // Ignore.  Disposing of the handle is the mechanism by which the FSW communicates
                 // to the asynchronous operation to stop processing.
             }
-            catch (ArgumentNullException)
-            {
-                //Ignore.  The disposed handle could also manifest as an ArgumentNullException.
-                Debug.Assert(IsHandleInvalid(state.DirectoryHandle), "ArgumentNullException from something other than SafeHandle?");
-            }
             finally
             {
                 // At this point the operation has either been initiated and we'll let the callback
@@ -192,15 +214,18 @@ private unsafe void Monitor(AsyncReadState state)
                         state.ThreadPoolBinding.FreeNativeOverlapped(overlappedPointer);
                     }
 
-                    // Clean up the thread pool binding created for the entire operation
-                    state.PreAllocatedOverlapped.Dispose();
-                    state.ThreadPoolBinding.Dispose();
+                    // Check whether the directory handle is still valid _before_ we dispose of the state,
+                    // which will invalidate the handle.
+                    bool handleClosed = IsHandleClosed(state.DirectoryHandle);
+
+                    // Clean up the state created for the whole operation.
+                    state.Dispose();
 
                     // Finally, if the handle was for some reason changed or closed during this call,
                     // then don't throw an exception.  Otherwise, it's a valid error.
-                    if (!IsHandleInvalid(state.DirectoryHandle))
+                    if (!handleClosed)
                     {
-                        OnError(new ErrorEventArgs(new Win32Exception()));
+                        OnError(new ErrorEventArgs(new Win32Exception(win32Error)));
                     }
                 }
             }
@@ -211,7 +236,7 @@ private void ReadDirectoryChangesCallback(uint errorCode, uint numBytes, AsyncRe
         {
             try
             {
-                if (IsHandleInvalid(state.DirectoryHandle))
+                if (IsHandleClosed(state.DirectoryHandle))
                     return;
 
                 if (errorCode != 0)
@@ -236,13 +261,17 @@ private void ReadDirectoryChangesCallback(uint errorCode, uint numBytes, AsyncRe
                 if (state.Session != Volatile.Read(ref _currentSession))
                     return;
 
-                if (numBytes == 0)
+                if (numBytes == 0 || numBytes > state.BufferByteLength)
                 {
+                    Debug.Assert(numBytes == 0, "ReadDirectoryChangesW returned more bytes than the buffer can hold!");
                     NotifyInternalBufferOverflowEvent();
                 }
                 else
                 {
-                    ParseEventBufferAndNotifyForEach(new ReadOnlySpan<byte>(state.Buffer, 0, (int)numBytes));
+                    unsafe
+                    {
+                        ParseEventBufferAndNotifyForEach(new ReadOnlySpan<byte>(state.Buffer, (int)numBytes));
+                    }
                 }
             }
             finally
@@ -374,26 +403,36 @@ private unsafe void ParseEventBufferAndNotifyForEach(ReadOnlySpan<byte> buffer)
         /// </summary>
         private sealed class AsyncReadState
         {
-            internal AsyncReadState(int session, byte[] buffer, SafeFileHandle handle, ThreadPoolBoundHandle binding, FileSystemWatcher parent)
+            internal AsyncReadState(int session, FileSystemWatcher parent, SafeFileHandle directoryHandle)
             {
-                Debug.Assert(buffer != null);
-                Debug.Assert(buffer.Length > 0);
-                Debug.Assert(handle != null);
-                Debug.Assert(binding != null);
-
                 Session = session;
-                Buffer = buffer;
-                DirectoryHandle = handle;
-                ThreadPoolBinding = binding;
                 WeakWatcher = new WeakReference<FileSystemWatcher>(parent);
+                DirectoryHandle = directoryHandle;
             }
 
             internal int Session { get; }
-            internal byte[] Buffer { get; }
-            internal SafeFileHandle DirectoryHandle { get; }
-            internal ThreadPoolBoundHandle ThreadPoolBinding { get; }
-            internal PreAllocatedOverlapped? PreAllocatedOverlapped { get; set; }
             internal WeakReference<FileSystemWatcher> WeakWatcher { get; }
+            internal SafeFileHandle DirectoryHandle { get; set; }
+
+            internal unsafe void* Buffer { get; set; }
+            internal uint BufferByteLength { get; set; }
+
+            internal ThreadPoolBoundHandle? ThreadPoolBinding { get; set; }
+
+            internal PreAllocatedOverlapped? PreAllocatedOverlapped { get; set; }
+
+            public void Dispose()
+            {
+                unsafe
+                {
+                    NativeMemory.Free(Buffer);
+                    Buffer = null;
+                }
+
+                PreAllocatedOverlapped?.Dispose();
+                ThreadPoolBinding?.Dispose();
+                DirectoryHandle?.Dispose();
+            }
         }
     }
 }

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs

@@ -223,20 +223,6 @@ public int InternalBufferSize
             }
         }
 
-        /// <summary>Allocates a buffer of the requested internal buffer size.</summary>
-        /// <returns>The allocated buffer.</returns>
-        private byte[] AllocateBuffer()
-        {
-            try
-            {
-                return new byte[_internalBufferSize];
-            }
-            catch (OutOfMemoryException)
-            {
-                throw new OutOfMemoryException(SR.Format(SR.BufferSizeTooLarge, _internalBufferSize));
-            }
-        }
-
         /// <devdoc>
         ///    Gets or sets the path of the directory to watch.
         /// </devdoc>