Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Jun 10, 2022
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a94addc
Fix async handle leak when sync RandomAccess read/write on async hand…
stephentoub May 29, 2022
97b6ebb
Use UnsafeAllocateNativeOverlapped in GetNativeOverlappedForAsyncHandle
stephentoub May 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
View file
Open in desktop
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