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)

* 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 10, 2022
1 parent e4ac6fa commit 57c88e9
Showing 1 changed file with 22 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,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 @@ -125,7 +133,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 @@ -203,6 +211,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 @@ -225,7 +241,7 @@ private static unsafe void WriteSyncUsingAsyncHandle(SafeFileHandle handle, Read
{
if (overlapped != null)
{
resetEvent.FreeNativeOverlapped(overlapped);
resetEvent.ReleaseRefCount(overlapped);
}

resetEvent.Dispose();
Expand Down Expand Up @@ -711,7 +727,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 @@ -751,7 +767,7 @@ 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);
}
}

Expand All @@ -767,7 +783,7 @@ static unsafe void Callback(uint errorCode, uint numBytes, NativeOverlapped* pOv
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
Expand All @@ -780,7 +796,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 57c88e9

Please sign in to comment.