Fix FSW freeing of NativeOverlapped on wrong ThreadPoolBoundHandle

If a FileSystemWatcher is stopped and restarted while processing events, it can end up trying to free an allocated NativeOverlapped on the ThreadPoolBoundHandle associated with the new session even if it was created with the old one.  This commit changes the code to store the appropriate binding and directory handle in locals and in the async state object so that the same binding can be used to free as was used to allocate.

Author: stephentoub

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

@@ -108,12 +108,11 @@ private void FinalizeDispose()
         // Thread gate holder and constats
         private bool _stopListening = false;
 
-        private bool IsHandleInvalid
+        private bool IsHandleInvalid { get { return IsHandleInstanceInvalid(_directoryHandle); } }
+
+        private bool IsHandleInstanceInvalid(SafeFileHandle handle)
         {
-            get
-            {
-                return (_directoryHandle == null || _directoryHandle.IsInvalid || _directoryHandle.IsClosed);
-            }
+            return handle == null || handle.IsInvalid || handle.IsClosed;
         }
 
         /// <devdoc>
@@ -132,12 +131,21 @@ private unsafe void Monitor(byte[] buffer)
                 buffer = AllocateBuffer();
             }
 
+            // Use a local copy of _threadPoolBinding for the remainder of the method.
+            // Otherwise, the value of the field could get replaced by a different
+            // instance if events are stopped and then restarted, and we could end up
+            // trying to use the new binding with an overlapped created on the old binding.
+            // Similarly for the directory handle.
+            ThreadPoolBoundHandle threadPoolBinding = _threadPoolBinding;
+            SafeFileHandle directoryHandle = _directoryHandle;
+
             // Pass "session" counter to callback:
             FSWAsyncResult asyncResult = new FSWAsyncResult();
             asyncResult.session = _currentSession;
             asyncResult.buffer = buffer;
+            asyncResult.threadPoolBinding = threadPoolBinding;
 
-            NativeOverlapped* overlappedPointer = _threadPoolBinding.AllocateNativeOverlapped(new IOCompletionCallback(this.CompletionStatusChanged), asyncResult, buffer);
+            NativeOverlapped* overlappedPointer = threadPoolBinding.AllocateNativeOverlapped(new IOCompletionCallback(this.CompletionStatusChanged), asyncResult, buffer);
 
             // Can now call OS:
             int size;
@@ -149,12 +157,12 @@ private unsafe void Monitor(byte[] buffer)
                 // and when we get here from CompletionStatusChanged.
                 // We might need to take a lock to prevent race absolutely, instead just catch
                 // ObjectDisposedException from SafeHandle in case it is disposed
-                if (!IsHandleInvalid)
+                if (!IsHandleInstanceInvalid(directoryHandle))
                 {
                     // An interrupt is possible here
                     fixed (byte* buffPtr = buffer)
                     {
-                        ok = UnsafeNativeMethods.ReadDirectoryChangesW(_directoryHandle,
+                        ok = UnsafeNativeMethods.ReadDirectoryChangesW(directoryHandle,
                                                            buffPtr,
                                                            _internalBufferSize,
                                                            _includeSubdirectories ? 1 : 0,
@@ -170,17 +178,17 @@ private unsafe void Monitor(byte[] buffer)
             }
             catch (ArgumentNullException)
             { //Ignore
-                Debug.Assert(IsHandleInvalid, "ArgumentNullException from something other than SafeHandle?");
+                Debug.Assert(IsHandleInstanceInvalid(directoryHandle), "ArgumentNullException from something other than SafeHandle?");
             }
             finally
             {
                 if (!ok)
                 {
-                    _threadPoolBinding.FreeNativeOverlapped(overlappedPointer);
+                    threadPoolBinding.FreeNativeOverlapped(overlappedPointer);
 
                     // If the handle was for some reason changed or closed during this call, then don't throw an
                     // exception.  Else, it's a valid error.
-                    if (!IsHandleInvalid)
+                    if (!IsHandleInstanceInvalid(directoryHandle))
                     {
                         OnError(new ErrorEventArgs(new Win32Exception()));
                     }
@@ -354,7 +362,7 @@ rename event with the Name field null and then fire the next action.
             }
             finally
             {
-                _threadPoolBinding.FreeNativeOverlapped(overlappedPointer);
+                asyncResult.threadPoolBinding.FreeNativeOverlapped(overlappedPointer);
                 if (!_stopListening)
                 {
                     Monitor(asyncResult.buffer);
@@ -368,6 +376,7 @@ private sealed class FSWAsyncResult
         {
             internal int session;
             internal byte[] buffer;
+            internal ThreadPoolBoundHandle threadPoolBinding;
         }
 
         /// <devdoc>