From: Stephen Toub <stoub@microsoft.com>
Date: Fri, 10 Jun 2022 14:17:24 +0000 (-0400)
Subject: Fix async handle leak when sync RandomAccess read/write on async handle fails (#69956)
X-Git-Tag: accepted/tizen/unified/riscv/20231226.055536~8646
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=57c88e9e51379bb3d1df8f13257e93c87febe4c1;p=platform%2Fupstream%2Fdotnet%2Fruntime.git

Fix async handle leak when sync RandomAccess read/write on async handle fails (#69956)

* Fix async handle leak when sync RandomAccess read/write on async handle fails

* Use UnsafeAllocateNativeOverlapped in GetNativeOverlappedForAsyncHandle

This isn't invoking user code and it's entirely for synchronous operations; there's no reason it needs to capture ExecutionContext.
---

diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
index 234782b..6a33837 100644
--- a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
@@ -104,6 +104,14 @@ namespace System.IO
 
                         errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle);
                     }
+                    else
+                    {
+                        // The initial errorCode was neither ERROR_IO_PENDING nor ERROR_SUCCESS, so the operation
+                        // failed with an error and the callback won't be invoked.  We thus need to decrement the
+                        // ref count on the resetEvent that was initialized to a value under the expectation that
+                        // the callback would be invoked and decrement it.
+                        resetEvent.ReleaseRefCount(overlapped);
+                    }
 
                     switch (errorCode)
                     {
@@ -125,7 +133,7 @@ namespace System.IO
             {
                 if (overlapped != null)
                 {
-                    resetEvent.FreeNativeOverlapped(overlapped);
+                    resetEvent.ReleaseRefCount(overlapped);
                 }
 
                 resetEvent.Dispose();
@@ -203,6 +211,14 @@ namespace System.IO
 
                         errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle);
                     }
+                    else
+                    {
+                        // The initial errorCode was neither ERROR_IO_PENDING nor ERROR_SUCCESS, so the operation
+                        // failed with an error and the callback won't be invoked.  We thus need to decrement the
+                        // ref count on the resetEvent that was initialized to a value under the expectation that
+                        // the callback would be invoked and decrement it.
+                        resetEvent.ReleaseRefCount(overlapped);
+                    }
 
                     switch (errorCode)
                     {
@@ -225,7 +241,7 @@ namespace System.IO
             {
                 if (overlapped != null)
                 {
-                    resetEvent.FreeNativeOverlapped(overlapped);
+                    resetEvent.ReleaseRefCount(overlapped);
                 }
 
                 resetEvent.Dispose();
@@ -711,7 +727,7 @@ namespace System.IO
         {
             // After SafeFileHandle is bound to ThreadPool, we need to use ThreadPoolBinding
             // to allocate a native overlapped and provide a valid callback.
-            NativeOverlapped* result = handle.ThreadPoolBinding!.AllocateNativeOverlapped(s_callback, resetEvent, null);
+            NativeOverlapped* result = handle.ThreadPoolBinding!.UnsafeAllocateNativeOverlapped(s_callback, resetEvent, null);
 
             if (handle.CanSeek)
             {
@@ -751,7 +767,7 @@ namespace System.IO
             static unsafe void Callback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped)
             {
                 CallbackResetEvent state = (CallbackResetEvent)ThreadPoolBoundHandle.GetNativeOverlappedState(pOverlapped)!;
-                state.FreeNativeOverlapped(pOverlapped);
+                state.ReleaseRefCount(pOverlapped);
             }
         }
 
@@ -767,7 +783,7 @@ namespace System.IO
         private static bool IsEndOfFileForNoBuffering(SafeFileHandle fileHandle, long fileOffset)
             => fileHandle.IsNoBuffering && fileHandle.CanSeek && fileOffset >= fileHandle.GetFileLength();
 
-        // We need to store the reference count (see the comment in FreeNativeOverlappedIfItIsSafe) and an EventHandle to signal the completion.
+        // We need to store the reference count (see the comment in ReleaseRefCount) and an EventHandle to signal the completion.
         // We could keep these two things separate, but since ManualResetEvent is sealed and we want to avoid any extra allocations, this type has been created.
         // It's basically ManualResetEvent with reference count.
         private sealed class CallbackResetEvent : EventWaitHandle
@@ -780,7 +796,7 @@ namespace System.IO
                 _threadPoolBoundHandle = threadPoolBoundHandle;
             }
 
-            internal unsafe void FreeNativeOverlapped(NativeOverlapped* pOverlapped)
+            internal unsafe void ReleaseRefCount(NativeOverlapped* pOverlapped)
             {
                 // Each SafeFileHandle opened for async IO is bound to ThreadPool.
                 // It requires us to provide a callback even if we want to use EventHandle and use GetOverlappedResult to obtain the result.