Skip to content

Commit

Permalink
Fix async handle leak when sync RandomAccess read/write on async hand…
Browse files Browse the repository at this point in the history
…le fails (#69956) (#70591)

* 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.
  • Loading branch information
stephentoub authored Jun 14, 2022
1 parent 3ab87d9 commit b3ac8e8
Showing 1 changed file with 22 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,14 @@ private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span<b

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)
{
Expand All @@ -118,7 +126,7 @@ private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span<b
{
if (overlapped != null)
{
resetEvent.FreeNativeOverlapped(overlapped);
resetEvent.ReleaseRefCount(overlapped);
}

resetEvent.Dispose();
Expand Down Expand Up @@ -196,6 +204,14 @@ private static unsafe void WriteSyncUsingAsyncHandle(SafeFileHandle handle, Read

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)
{
Expand All @@ -218,7 +234,7 @@ private static unsafe void WriteSyncUsingAsyncHandle(SafeFileHandle handle, Read
{
if (overlapped != null)
{
resetEvent.FreeNativeOverlapped(overlapped);
resetEvent.ReleaseRefCount(overlapped);
}

resetEvent.Dispose();
Expand Down Expand Up @@ -702,7 +718,7 @@ private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFile
{
// 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)
{
Expand Down Expand Up @@ -742,11 +758,11 @@ private static unsafe IOCompletionCallback AllocateCallback()
static unsafe void Callback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped)
{
CallbackResetEvent state = (CallbackResetEvent)ThreadPoolBoundHandle.GetNativeOverlappedState(pOverlapped)!;
state.FreeNativeOverlapped(pOverlapped);
state.ReleaseRefCount(pOverlapped);
}
}

// 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
Expand All @@ -759,7 +775,7 @@ internal CallbackResetEvent(ThreadPoolBoundHandle threadPoolBoundHandle) : base(
_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.
Expand Down

0 comments on commit b3ac8e8

Please sign in to comment.