Skip to content

Commit 38cf5d0

Browse files
Copilotadamsitnik
andauthored
Use cached ManualResetEvent + CancelIoEx for safe sync-over-async IO
- Cache ManualResetEvent (not raw nint) in SafeFileHandle using Interlocked rent/return pattern - Use public ManualResetEvent ctor, Reset() on reuse to ensure non-signaled state - Use WaitOne() to respect SynchronizationContext - When WaitOne throws, cancel pending IO with CancelIoEx + GetOverlappedResult(bWait:true) before freeing overlapped - Add test demonstrating SynchronizationContext.Wait exception scenario Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
1 parent ac128bb commit 38cf5d0

3 files changed

Lines changed: 94 additions & 44 deletions

File tree

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.OverlappedValueTaskSource.Windows.cs

Lines changed: 14 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -14,51 +14,36 @@ namespace Microsoft.Win32.SafeHandles
1414
public sealed partial class SafeFileHandle : SafeHandleZeroOrMinusOneIsInvalid
1515
{
1616
private OverlappedValueTaskSource? _reusableOverlappedValueTaskSource; // reusable OverlappedValueTaskSource that is currently NOT being used
17-
private nint _reusableWaitHandle = -1; // reusable native event handle for sync-over-async I/O; -1 means empty
17+
private ManualResetEvent? _reusableSyncWaitEvent; // reusable event for sync-over-async I/O
1818

1919
// Rent the reusable OverlappedValueTaskSource, or create a new one to use if we couldn't get one (which
2020
// should only happen on first use or if the SafeFileHandle is being used concurrently).
2121
internal OverlappedValueTaskSource GetOverlappedValueTaskSource() =>
2222
Interlocked.Exchange(ref _reusableOverlappedValueTaskSource, null) ?? new OverlappedValueTaskSource(this);
2323

24-
internal nint RentSyncWaitHandle()
24+
// Rent the reusable ManualResetEvent for sync-over-async I/O, or create a new one.
25+
// The returned event is guaranteed to be in non-signaled state.
26+
internal ManualResetEvent RentSyncWaitEvent()
2527
{
26-
nint handle = Interlocked.Exchange(ref _reusableWaitHandle, -1);
27-
if (handle != -1)
28+
ManualResetEvent? mre = Interlocked.Exchange(ref _reusableSyncWaitEvent, null);
29+
if (mre is not null)
2830
{
29-
return handle;
31+
mre.Reset();
32+
return mre;
3033
}
3134

32-
using SafeWaitHandle safeHandle = Interop.Kernel32.CreateEventEx(
33-
IntPtr.Zero,
34-
null,
35-
Interop.Kernel32.CREATE_EVENT_MANUAL_RESET,
36-
(uint)(Interop.Kernel32.SYNCHRONIZE | Interop.Kernel32.EVENT_MODIFY_STATE));
37-
38-
if (safeHandle.IsInvalid)
39-
{
40-
throw Win32Marshal.GetExceptionForLastWin32Error();
41-
}
42-
43-
handle = safeHandle.DangerousGetHandle();
44-
safeHandle.SetHandleAsInvalid();
45-
46-
return handle;
35+
return new ManualResetEvent(false);
4736
}
4837

49-
internal void ReturnSyncWaitHandle(nint waitHandle)
38+
internal void ReturnSyncWaitEvent(ManualResetEvent waitEvent)
5039
{
51-
if (Interlocked.CompareExchange(ref _reusableWaitHandle, waitHandle, -1) != -1)
40+
if (Interlocked.CompareExchange(ref _reusableSyncWaitEvent, waitEvent, null) is not null)
5241
{
53-
Interop.Kernel32.CloseHandle(waitHandle);
42+
waitEvent.Dispose();
5443
}
5544
else if (IsClosed)
5645
{
57-
nint h = Interlocked.Exchange(ref _reusableWaitHandle, -1);
58-
if (h != -1)
59-
{
60-
Interop.Kernel32.CloseHandle(h);
61-
}
46+
Interlocked.Exchange(ref _reusableSyncWaitEvent, null)?.Dispose();
6247
}
6348
}
6449

@@ -67,12 +52,7 @@ protected override bool ReleaseHandle()
6752
bool result = Interop.Kernel32.CloseHandle(handle);
6853

6954
Interlocked.Exchange(ref _reusableOverlappedValueTaskSource, null)?.Dispose();
70-
71-
nint waitHandle = Interlocked.Exchange(ref _reusableWaitHandle, -1);
72-
if (waitHandle != -1)
73-
{
74-
Interop.Kernel32.CloseHandle(waitHandle);
75-
}
55+
Interlocked.Exchange(ref _reusableSyncWaitEvent, null)?.Dispose();
7656

7757
return result;
7858
}

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,12 @@ _ when IsEndOfFile(errorCode, handle, fileOffset) => 0,
6767

6868
private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span<byte> buffer, long fileOffset)
6969
{
70-
IntPtr eventHandle = handle.RentSyncWaitHandle();
70+
ManualResetEvent waitEvent = handle.RentSyncWaitEvent();
7171
NativeOverlapped* overlapped = null;
7272

7373
try
7474
{
75-
overlapped = AllocNativeOverlappedWithEventHandle(handle, fileOffset, eventHandle);
75+
overlapped = AllocNativeOverlappedWithEventHandle(handle, fileOffset, waitEvent);
7676

7777
fixed (byte* pinned = &MemoryMarshal.GetReference(buffer))
7878
{
@@ -81,8 +81,22 @@ private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span<b
8181
int errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle);
8282
if (errorCode is Interop.Errors.ERROR_IO_PENDING or Interop.Errors.ERROR_SUCCESS)
8383
{
84+
try
85+
{
86+
waitEvent.WaitOne();
87+
}
88+
catch
89+
{
90+
// WaitOne can throw arbitrary exceptions (e.g., via SynchronizationContext).
91+
// Cancel the pending IO and wait for completion before freeing the overlapped.
92+
Interop.Kernel32.CancelIoEx(handle, overlapped);
93+
int canceledBytes = 0;
94+
Interop.Kernel32.GetOverlappedResult(handle, overlapped, ref canceledBytes, bWait: true);
95+
throw;
96+
}
97+
8498
int result = 0;
85-
if (Interop.Kernel32.GetOverlappedResult(handle, overlapped, ref result, bWait: true))
99+
if (Interop.Kernel32.GetOverlappedResult(handle, overlapped, ref result, bWait: false))
86100
{
87101
Debug.Assert(result >= 0 && result <= buffer.Length, $"GetOverlappedResult returned {result} for {buffer.Length} bytes request");
88102
return result;
@@ -106,7 +120,7 @@ private static unsafe int ReadSyncUsingAsyncHandle(SafeFileHandle handle, Span<b
106120
NativeMemory.Free(overlapped);
107121
}
108122

109-
handle.ReturnSyncWaitHandle(eventHandle);
123+
handle.ReturnSyncWaitEvent(waitEvent);
110124
}
111125
}
112126

@@ -144,12 +158,12 @@ private static unsafe void WriteSyncUsingAsyncHandle(SafeFileHandle handle, Read
144158
return;
145159
}
146160

147-
IntPtr eventHandle = handle.RentSyncWaitHandle();
161+
ManualResetEvent waitEvent = handle.RentSyncWaitEvent();
148162
NativeOverlapped* overlapped = null;
149163

150164
try
151165
{
152-
overlapped = AllocNativeOverlappedWithEventHandle(handle, fileOffset, eventHandle);
166+
overlapped = AllocNativeOverlappedWithEventHandle(handle, fileOffset, waitEvent);
153167

154168
fixed (byte* pinned = &MemoryMarshal.GetReference(buffer))
155169
{
@@ -158,8 +172,22 @@ private static unsafe void WriteSyncUsingAsyncHandle(SafeFileHandle handle, Read
158172
int errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle);
159173
if (errorCode is Interop.Errors.ERROR_IO_PENDING or Interop.Errors.ERROR_SUCCESS)
160174
{
175+
try
176+
{
177+
waitEvent.WaitOne();
178+
}
179+
catch
180+
{
181+
// WaitOne can throw arbitrary exceptions (e.g., via SynchronizationContext).
182+
// Cancel the pending IO and wait for completion before freeing the overlapped.
183+
Interop.Kernel32.CancelIoEx(handle, overlapped);
184+
int canceledBytes = 0;
185+
Interop.Kernel32.GetOverlappedResult(handle, overlapped, ref canceledBytes, bWait: true);
186+
throw;
187+
}
188+
161189
int result = 0;
162-
if (Interop.Kernel32.GetOverlappedResult(handle, overlapped, ref result, bWait: true))
190+
if (Interop.Kernel32.GetOverlappedResult(handle, overlapped, ref result, bWait: false))
163191
{
164192
Debug.Assert(result == buffer.Length, $"GetOverlappedResult returned {result} for {buffer.Length} bytes request");
165193
return;
@@ -186,7 +214,7 @@ private static unsafe void WriteSyncUsingAsyncHandle(SafeFileHandle handle, Read
186214
NativeMemory.Free(overlapped);
187215
}
188216

189-
handle.ReturnSyncWaitHandle(eventHandle);
217+
handle.ReturnSyncWaitEvent(waitEvent);
190218
}
191219
}
192220

@@ -686,7 +714,7 @@ private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFile
686714
}
687715
}
688716

689-
private static unsafe NativeOverlapped* AllocNativeOverlappedWithEventHandle(SafeFileHandle handle, long fileOffset, IntPtr eventHandle)
717+
private static unsafe NativeOverlapped* AllocNativeOverlappedWithEventHandle(SafeFileHandle handle, long fileOffset, EventWaitHandle waitHandle)
690718
{
691719
NativeOverlapped* result = (NativeOverlapped*)NativeMemory.AllocZeroed((nuint)sizeof(NativeOverlapped));
692720

@@ -707,7 +735,7 @@ private static async ValueTask WriteGatherAtOffsetMultipleSyscallsAsync(SafeFile
707735
// "If the file handle associated with the completion packet was previously associated with an I/O completion port
708736
// [...] setting the low-order bit of hEvent in the OVERLAPPED structure prevents the I/O completion
709737
// from being queued to a completion port."
710-
result->EventHandle = eventHandle | 1;
738+
result->EventHandle = waitHandle.SafeWaitHandle.DangerousGetHandle() | 1;
711739

712740
return result;
713741
}

src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/RandomAccess/Mixed.Windows.cs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System.Collections.Generic;
55
using System.Runtime.InteropServices;
6+
using System.Threading;
67
using System.Threading.Tasks;
78
using Microsoft.Win32.SafeHandles;
89
using Xunit;
@@ -143,5 +144,46 @@ static async Task Validate(SafeFileHandle handle, FileOptions options, bool[] sy
143144
}
144145
}
145146
}
147+
148+
[Fact]
149+
public void SyncIOOnAsyncHandle_DoesNotCorruptMemory_WhenSynchronizationContextThrows()
150+
{
151+
// This test verifies that when WaitOne() throws via SynchronizationContext,
152+
// the pending IO is properly canceled before freeing the NativeOverlapped,
153+
// preventing use-after-free / heap corruption.
154+
string filePath = GetTestFilePath();
155+
File.WriteAllBytes(filePath, new byte[1024]);
156+
157+
SynchronizationContext previous = SynchronizationContext.Current;
158+
try
159+
{
160+
var throwingContext = new ThrowingSynchronizationContext();
161+
SynchronizationContext.SetSynchronizationContext(throwingContext);
162+
163+
using SafeFileHandle handle = File.OpenHandle(filePath, FileMode.Open, FileAccess.Read, options: FileOptions.Asynchronous);
164+
165+
byte[] buffer = new byte[512];
166+
167+
// The ThrowingSynchronizationContext.Wait throws, which should be caught
168+
// and the IO should be canceled gracefully.
169+
Assert.Throws<InvalidOperationException>(() => RandomAccess.Read(handle, buffer, 0));
170+
}
171+
finally
172+
{
173+
SynchronizationContext.SetSynchronizationContext(previous);
174+
}
175+
}
176+
177+
/// <summary>
178+
/// A SynchronizationContext that throws from Wait to simulate the scenario
179+
/// where WaitOne() can throw arbitrary exceptions via user code.
180+
/// </summary>
181+
private sealed class ThrowingSynchronizationContext : SynchronizationContext
182+
{
183+
public override int Wait(IntPtr[] waitHandles, bool waitAll, int millisecondsTimeout)
184+
{
185+
throw new InvalidOperationException("SynchronizationContext.Wait threw an exception");
186+
}
187+
}
146188
}
147189
}

0 commit comments

Comments
 (0)