Skip to content

Don't use callbacks when performing sync IO with async handles #126845

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

Open
Copilot wants to merge 8 commits into
base: main
Choose a base branch
from
+67 −92
Open

Don't use callbacks when performing sync IO with async handles #126845

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4d6f586
Initial plan
Copilot Apr 13, 2026
b3325a4
Replace CallbackResetEvent with ManualResetEvent and low-order bit hE…
Copilot Apr 13, 2026
a34ad37
Address review nits: use target-typed new and rename AllocNativeOverl…
Copilot Apr 14, 2026
e387e23
Merge branch 'main' into copilot/remove-callbacks-sync-io
adamsitnik Apr 14, 2026
99c34d4
Remove unnecessary InternalLow zeroing since NativeMemory.Free has no…
Copilot Apr 14, 2026
d0fc732
Replace ManualResetEvent + WaitOne with native CreateEventExW + GetOv…
Copilot Apr 14, 2026
8ed0afc
Cache event handle in SafeFileHandle, reuse existing CreateEventEx P/…
Copilot Apr 25, 2026
ac128bb
Move event creation logic into SafeFileHandle.RentSyncWaitHandle
Copilot Apr 25, 2026
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
84 changes: 13 additions & 71 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 @@ -16,8 +16,6 @@ namespace System.IO
{
public static partial class RandomAccess
{
private static readonly IOCompletionCallback s_callback = AllocateCallback();

internal static unsafe void SetFileLength(SafeFileHandle handle, long length)
{
var eofInfo = new Interop.Kernel32.FILE_END_OF_FILE_INFO
Expand Down Expand Up @@ -69,14 +67,12 @@ _ when IsEndOfFile(errorCode, handle, fileOffset) => 0,

private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span<byte> buffer, long fileOffset)
{
handle.EnsureThreadPoolBindingInitialized();

CallbackResetEvent resetEvent = new CallbackResetEvent(handle.ThreadPoolBinding!);
ManualResetEvent resetEvent = new ManualResetEvent(false);
Comment thread
adamsitnik marked this conversation as resolved.
Outdated
NativeOverlapped* overlapped = null;

try
{
overlapped = GetNativeOverlappedForAsyncHandle(handle, fileOffset, resetEvent);
overlapped = AllocNativeOverlappedForSyncHandle(handle, fileOffset, resetEvent);

fixed (byte* pinned = &MemoryMarshal.GetReference(buffer))
{
Expand All @@ -100,18 +96,9 @@ 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);
}

if (IsEndOfFile(errorCode, handle, fileOffset))
{
// EOF on a pipe. Callback will not be called.
// We clear the overlapped status bit for this special case (failure
// to do so looks like we are freeing a pending overlapped later).
overlapped->InternalLow = IntPtr.Zero;
Expand All @@ -125,7 +112,7 @@ private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span<b
{
if (overlapped != null)
{
resetEvent.ReleaseRefCount(overlapped);
NativeMemory.Free(overlapped);
}
Comment on lines 103 to 107
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NativeMemory.Free(overlapped) is unconditional in finally. If the WaitOne() above throws (e.g., ThreadInterruptedException) or the method otherwise exits before the OS has finished the overlapped operation, freeing this memory can result in use-after-free when the kernel writes completion state / signals the event. Consider restructuring so the overlapped is only freed after completion is guaranteed (e.g., use GetOverlappedResult(..., bWait: true) instead of managed WaitOne, or ensure completion in a finally before freeing).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g., ThreadInterruptedException

ThreadInterruptedException is legacy, I will defer to other code reviewers.

Copy link
Copy Markdown
Member

@jkotas jkotas Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WaitOne can throw arbitrary exceptions. Note that it can call back into user code via SynchronizationContext.

We should not be corrupting unmanaged heap when that happens.

Copy link
Copy Markdown
Member

@adamsitnik adamsitnik Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot let's try to implement following approach:

  • don't allocate ManualResetEvent, instead perform the windows sys-call with minimal access right required to perform ReadFile
CreateEventExW(
    NULL,
    NULL,
    CREATE_EVENT_MANUAL_RESET,
    SYNCHRONIZE | EVENT_MODIFY_STATE
);
  • pass the event to AllocNativeOverlappedWithEventHandle instead of ManualResetEvent
  • instead of waiting on the event handle, pass bWait = true to GetOverlappedResult
  • close the event handle your own by calling CloseHandle


resetEvent.Dispose();
Expand Down Expand Up @@ -166,14 +153,12 @@ private static unsafe void WriteSyncUsingAsyncHandle(SafeFileHandle handle, Read
return;
}

handle.EnsureThreadPoolBindingInitialized();

CallbackResetEvent resetEvent = new CallbackResetEvent(handle.ThreadPoolBinding!);
ManualResetEvent resetEvent = new ManualResetEvent(false);
NativeOverlapped* overlapped = null;

try
{
overlapped = GetNativeOverlappedForAsyncHandle(handle, fileOffset, resetEvent);
overlapped = AllocNativeOverlappedForSyncHandle(handle, fileOffset, resetEvent);

fixed (byte* pinned = &MemoryMarshal.GetReference(buffer))
{
Expand All @@ -197,14 +182,6 @@ 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);
}

throw errorCode switch
{
Expand All @@ -221,7 +198,7 @@ private static unsafe void WriteSyncUsingAsyncHandle(SafeFileHandle handle, Read
{
if (overlapped != null)
{
resetEvent.ReleaseRefCount(overlapped);
NativeMemory.Free(overlapped);
}
Comment thread
adamsitnik marked this conversation as resolved.

resetEvent.Dispose();
Expand Down Expand Up @@ -724,11 +701,9 @@ private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFile
}
}

private static unsafe NativeOverlapped* GetNativeOverlappedForAsyncHandle(SafeFileHandle handle, long fileOffset, CallbackResetEvent resetEvent)
private static unsafe NativeOverlapped* AllocNativeOverlappedForSyncHandle(SafeFileHandle handle, long fileOffset, EventWaitHandle waitHandle)
Comment thread
adamsitnik marked this conversation as resolved.
Outdated
{
// 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!.UnsafeAllocateNativeOverlapped(s_callback, resetEvent, null);
NativeOverlapped* result = (NativeOverlapped*)NativeMemory.AllocZeroed((nuint)sizeof(NativeOverlapped));

if (handle.CanSeek)
{
Expand All @@ -743,7 +718,11 @@ private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFile
// are performed on the same file, named pipe, or communications device.
// In this situation, there is no way to know which operation caused the object's state to be signaled."
// Since we want RandomAccess APIs to be thread-safe, we provide a dedicated wait handle.
result->EventHandle = resetEvent.SafeWaitHandle.DangerousGetHandle();
// From https://learn.microsoft.com/windows/win32/api/ioapiset/nf-ioapiset-getqueuedcompletionstatus:
// "If the file handle associated with the completion packet was previously associated with an I/O completion port
// [...] setting the low-order bit of hEvent in the OVERLAPPED structure prevents the I/O completion
// from being queued to a completion port."
result->EventHandle = waitHandle.SafeWaitHandle.DangerousGetHandle() | 1;
Comment thread
jkotas marked this conversation as resolved.
Outdated

return result;
}
Expand All @@ -761,17 +740,6 @@ private static NativeOverlapped GetNativeOverlappedForSyncHandle(SafeFileHandle
return result;
}

private static unsafe IOCompletionCallback AllocateCallback()
{
return new IOCompletionCallback(Callback);

static void Callback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped)
{
CallbackResetEvent state = (CallbackResetEvent)ThreadPoolBoundHandle.GetNativeOverlappedState(pOverlapped)!;
state.ReleaseRefCount(pOverlapped);
}
}

internal static bool IsEndOfFile(int errorCode, SafeFileHandle handle, long fileOffset)
{
switch (errorCode)
Expand All @@ -798,32 +766,6 @@ internal static bool IsEndOfFile(int errorCode, SafeFileHandle handle, long file
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 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
{
private readonly ThreadPoolBoundHandle _threadPoolBoundHandle;
private int _freeWhenZero = 2; // one for the callback and another for the method that calls GetOverlappedResult

internal CallbackResetEvent(ThreadPoolBoundHandle threadPoolBoundHandle) : base(initialState: false, EventResetMode.ManualReset)
{
_threadPoolBoundHandle = threadPoolBoundHandle;
}

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.
// There can be a race condition between the call to GetOverlappedResult and the callback invocation,
// so we need to track the number of references, and when it drops to zero, then free the native overlapped.
if (Interlocked.Decrement(ref _freeWhenZero) == 0)
{
_threadPoolBoundHandle.FreeNativeOverlapped(pOverlapped);
}
}
}

// Abstracts away the type signature incompatibility between Memory and ReadOnlyMemory.
private interface IMemoryHandler<T>
{
Expand Down
Loading