Skip to content

Commit df0e65a

Browse files
Copilotadamsitnik
Copilot
and
adamsitnik
authored
Address round 2 feedback: extract ValidateReadAllState helper, make Windows ReadPipes fully unsafe with proper cleanup, use is not null
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/451cf437-2de0-409a-937a-0031c3fa69d4 Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
1 parent 0986690 commit df0e65a

2 files changed

Lines changed: 117 additions & 104 deletions

File tree

‎src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Multiplexing.Windows.cs‎

Lines changed: 95 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ public partial class Process
1515
/// Reads from both standard output and standard error pipes using Windows overlapped IO
1616
/// with wait handles for single-threaded synchronous multiplexing.
1717
/// </summary>
18-
private static void ReadPipes(
18+
private static unsafe void ReadPipes(
1919
SafeFileHandle outputHandle,
2020
SafeFileHandle errorHandle,
2121
int timeoutMs,
@@ -24,131 +24,132 @@ private static void ReadPipes(
2424
ref byte[] errorBuffer,
2525
ref int errorBytesRead)
2626
{
27-
MemoryHandle outputPin = outputBuffer.AsMemory().Pin();
28-
MemoryHandle errorPin = errorBuffer.AsMemory().Pin();
29-
30-
unsafe
27+
MemoryHandle outputPin = default;
28+
MemoryHandle errorPin = default;
29+
NativeOverlapped* outputOverlapped = null;
30+
NativeOverlapped* errorOverlapped = null;
31+
EventWaitHandle? outputEvent = null;
32+
EventWaitHandle? errorEvent = null;
33+
34+
try
3135
{
32-
NativeOverlapped* outputOverlapped = null;
33-
NativeOverlapped* errorOverlapped = null;
36+
outputPin = outputBuffer.AsMemory().Pin();
37+
errorPin = errorBuffer.AsMemory().Pin();
3438

35-
EventWaitHandle outputEvent = new(initialState: false, EventResetMode.ManualReset);
36-
EventWaitHandle errorEvent = new(initialState: false, EventResetMode.ManualReset);
39+
outputEvent = new EventWaitHandle(initialState: false, EventResetMode.ManualReset);
40+
errorEvent = new EventWaitHandle(initialState: false, EventResetMode.ManualReset);
3741

38-
try
39-
{
40-
outputOverlapped = AllocateOverlapped(outputEvent);
41-
errorOverlapped = AllocateOverlapped(errorEvent);
42+
outputOverlapped = AllocateOverlapped(outputEvent);
43+
errorOverlapped = AllocateOverlapped(errorEvent);
4244

43-
bool outputDone = false;
44-
bool errorDone = false;
45+
bool outputDone = false;
46+
bool errorDone = false;
4547

46-
WaitHandle[] waitHandles = [outputEvent, errorEvent];
48+
WaitHandle[] waitHandles = [outputEvent, errorEvent];
4749

48-
// Issue initial reads.
49-
Interop.Kernel32.ReadFile(outputHandle, (byte*)outputPin.Pointer + outputBytesRead,
50-
outputBuffer.Length - outputBytesRead, IntPtr.Zero, outputOverlapped);
50+
// Issue initial reads.
51+
Interop.Kernel32.ReadFile(outputHandle, (byte*)outputPin.Pointer + outputBytesRead,
52+
outputBuffer.Length - outputBytesRead, IntPtr.Zero, outputOverlapped);
5153

52-
Interop.Kernel32.ReadFile(errorHandle, (byte*)errorPin.Pointer + errorBytesRead,
53-
errorBuffer.Length - errorBytesRead, IntPtr.Zero, errorOverlapped);
54+
Interop.Kernel32.ReadFile(errorHandle, (byte*)errorPin.Pointer + errorBytesRead,
55+
errorBuffer.Length - errorBytesRead, IntPtr.Zero, errorOverlapped);
5456

55-
long deadline = timeoutMs >= 0
56-
? Environment.TickCount64 + timeoutMs
57-
: long.MaxValue;
57+
long deadline = timeoutMs >= 0
58+
? Environment.TickCount64 + timeoutMs
59+
: long.MaxValue;
5860

59-
while (!outputDone || !errorDone)
61+
while (!outputDone || !errorDone)
62+
{
63+
int waitTimeout;
64+
if (timeoutMs >= 0)
6065
{
61-
int waitTimeout;
62-
if (timeoutMs >= 0)
63-
{
64-
long remaining = deadline - Environment.TickCount64;
65-
if (remaining <= 0)
66-
{
67-
CancelPendingIOIfNeeded(outputHandle, outputDone, outputOverlapped);
68-
CancelPendingIOIfNeeded(errorHandle, errorDone, errorOverlapped);
69-
throw new TimeoutException();
70-
}
71-
72-
waitTimeout = (int)Math.Min(remaining, int.MaxValue);
73-
}
74-
else
75-
{
76-
waitTimeout = Timeout.Infinite;
77-
}
78-
79-
int waitResult = WaitHandle.WaitAny(waitHandles, waitTimeout);
80-
81-
if (waitResult == WaitHandle.WaitTimeout)
66+
long remaining = deadline - Environment.TickCount64;
67+
if (remaining <= 0)
8268
{
8369
CancelPendingIOIfNeeded(outputHandle, outputDone, outputOverlapped);
8470
CancelPendingIOIfNeeded(errorHandle, errorDone, errorOverlapped);
8571
throw new TimeoutException();
8672
}
8773

88-
bool isError = waitResult == 1;
89-
NativeOverlapped* currentOverlapped = isError ? errorOverlapped : outputOverlapped;
90-
SafeFileHandle currentHandle = isError ? errorHandle : outputHandle;
91-
ref int totalBytesRead = ref (isError ? ref errorBytesRead : ref outputBytesRead);
92-
ref byte[] currentBuffer = ref (isError ? ref errorBuffer : ref outputBuffer);
93-
EventWaitHandle currentEvent = isError ? errorEvent : outputEvent;
74+
waitTimeout = (int)Math.Min(remaining, int.MaxValue);
75+
}
76+
else
77+
{
78+
waitTimeout = Timeout.Infinite;
79+
}
9480

95-
int bytesRead = GetOverlappedResultForPipe(currentHandle, currentOverlapped);
81+
int waitResult = WaitHandle.WaitAny(waitHandles, waitTimeout);
9682

97-
if (bytesRead > 0)
98-
{
99-
totalBytesRead += bytesRead;
83+
if (waitResult == WaitHandle.WaitTimeout)
84+
{
85+
CancelPendingIOIfNeeded(outputHandle, outputDone, outputOverlapped);
86+
CancelPendingIOIfNeeded(errorHandle, errorDone, errorOverlapped);
87+
throw new TimeoutException();
88+
}
89+
90+
bool isError = waitResult == 1;
91+
NativeOverlapped* currentOverlapped = isError ? errorOverlapped : outputOverlapped;
92+
SafeFileHandle currentHandle = isError ? errorHandle : outputHandle;
93+
ref int totalBytesRead = ref (isError ? ref errorBytesRead : ref outputBytesRead);
94+
ref byte[] currentBuffer = ref (isError ? ref errorBuffer : ref outputBuffer);
95+
EventWaitHandle currentEvent = isError ? errorEvent : outputEvent;
96+
97+
int bytesRead = GetOverlappedResultForPipe(currentHandle, currentOverlapped);
10098

101-
if (totalBytesRead == currentBuffer.Length)
102-
{
103-
ref MemoryHandle currentPin = ref (isError ? ref errorPin : ref outputPin);
104-
currentPin.Dispose();
99+
if (bytesRead > 0)
100+
{
101+
totalBytesRead += bytesRead;
105102

106-
RentLargerBuffer(ref currentBuffer, totalBytesRead);
103+
if (totalBytesRead == currentBuffer.Length)
104+
{
105+
ref MemoryHandle currentPin = ref (isError ? ref errorPin : ref outputPin);
106+
currentPin.Dispose();
107107

108-
currentPin = currentBuffer.AsMemory().Pin();
109-
}
108+
RentLargerBuffer(ref currentBuffer, totalBytesRead);
110109

111-
// Reset the event and overlapped for next read.
112-
ResetOverlapped(currentEvent, currentOverlapped);
110+
currentPin = currentBuffer.AsMemory().Pin();
111+
}
113112

114-
byte* pinPointer = isError ? (byte*)errorPin.Pointer : (byte*)outputPin.Pointer;
115-
Interop.Kernel32.ReadFile(currentHandle, pinPointer + totalBytesRead,
116-
currentBuffer.Length - totalBytesRead, IntPtr.Zero, currentOverlapped);
113+
// Reset the event and overlapped for next read.
114+
ResetOverlapped(currentEvent, currentOverlapped);
115+
116+
byte* pinPointer = isError ? (byte*)errorPin.Pointer : (byte*)outputPin.Pointer;
117+
Interop.Kernel32.ReadFile(currentHandle, pinPointer + totalBytesRead,
118+
currentBuffer.Length - totalBytesRead, IntPtr.Zero, currentOverlapped);
119+
}
120+
else
121+
{
122+
// EOF: pipe write end was closed.
123+
if (isError)
124+
{
125+
errorDone = true;
117126
}
118127
else
119128
{
120-
// EOF: pipe write end was closed.
121-
if (isError)
122-
{
123-
errorDone = true;
124-
}
125-
else
126-
{
127-
outputDone = true;
128-
}
129-
130-
// Reset the event so WaitAny won't trigger on this stale handle.
131-
currentEvent.Reset();
129+
outputDone = true;
132130
}
131+
132+
// Reset the event so WaitAny won't trigger on this stale handle.
133+
currentEvent.Reset();
133134
}
134135
}
135-
finally
136+
}
137+
finally
138+
{
139+
if (outputOverlapped is not null)
136140
{
137-
if (outputOverlapped != null)
138-
{
139-
NativeMemory.Free(outputOverlapped);
140-
}
141-
142-
if (errorOverlapped != null)
143-
{
144-
NativeMemory.Free(errorOverlapped);
145-
}
141+
NativeMemory.Free(outputOverlapped);
142+
}
146143

147-
outputEvent.Dispose();
148-
errorEvent.Dispose();
149-
outputPin.Dispose();
150-
errorPin.Dispose();
144+
if (errorOverlapped is not null)
145+
{
146+
NativeMemory.Free(errorOverlapped);
151147
}
148+
149+
outputEvent?.Dispose();
150+
errorEvent?.Dispose();
151+
outputPin.Dispose();
152+
errorPin.Dispose();
152153
}
153154
}
154155

‎src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Multiplexing.cs‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ public partial class Process
3737
/// </exception>
3838
public (string StandardOutput, string StandardError) ReadAllText(TimeSpan? timeout = default)
3939
{
40+
ValidateReadAllState();
41+
4042
byte[] outputBuffer = ArrayPool<byte>.Shared.Rent(InitialReadAllBufferSize);
4143
byte[] errorBuffer = ArrayPool<byte>.Shared.Rent(InitialReadAllBufferSize);
4244
int outputBytesRead = 0;
@@ -89,6 +91,8 @@ public partial class Process
8991
/// </exception>
9092
public (byte[] StandardOutput, byte[] StandardError) ReadAllBytes(TimeSpan? timeout = default)
9193
{
94+
ValidateReadAllState();
95+
9296
byte[] outputBuffer = ArrayPool<byte>.Shared.Rent(InitialReadAllBufferSize);
9397
byte[] errorBuffer = ArrayPool<byte>.Shared.Rent(InitialReadAllBufferSize);
9498
int outputBytesRead = 0;
@@ -116,15 +120,10 @@ public partial class Process
116120
}
117121

118122
/// <summary>
119-
/// Validates state, obtains handles, and reads both stdout and stderr pipes into the provided buffers.
120-
/// The caller is responsible for renting and returning the buffers.
123+
/// Validates that the process is not disposed, both stdout and stderr are redirected,
124+
/// and neither stream is in async mode. Sets both streams to sync mode.
121125
/// </summary>
122-
private void ReadPipesToBuffers(
123-
TimeSpan? timeout,
124-
ref byte[] outputBuffer,
125-
ref int outputBytesRead,
126-
ref byte[] errorBuffer,
127-
ref int errorBytesRead)
126+
private void ValidateReadAllState()
128127
{
129128
CheckDisposed();
130129

@@ -150,13 +149,26 @@ private void ReadPipesToBuffers(
150149

151150
_outputStreamReadMode = StreamReadMode.SyncMode;
152151
_errorStreamReadMode = StreamReadMode.SyncMode;
152+
}
153153

154+
/// <summary>
155+
/// Obtains handles and reads both stdout and stderr pipes into the provided buffers.
156+
/// The caller is responsible for calling <see cref="ValidateReadAllState"/> before renting buffers,
157+
/// and for renting and returning the buffers.
158+
/// </summary>
159+
private void ReadPipesToBuffers(
160+
TimeSpan? timeout,
161+
ref byte[] outputBuffer,
162+
ref int outputBytesRead,
163+
ref byte[] errorBuffer,
164+
ref int errorBytesRead)
165+
{
154166
int timeoutMs = timeout.HasValue
155167
? (int)timeout.Value.TotalMilliseconds
156168
: -1; // Infinite
157169

158-
SafeFileHandle outputHandle = GetSafeFileHandleFromStreamReader(_standardOutput, out SafeHandle outputOwner);
159-
SafeFileHandle errorHandle = GetSafeFileHandleFromStreamReader(_standardError, out SafeHandle errorOwner);
170+
SafeFileHandle outputHandle = GetSafeFileHandleFromStreamReader(_standardOutput!, out SafeHandle outputOwner);
171+
SafeFileHandle errorHandle = GetSafeFileHandleFromStreamReader(_standardError!, out SafeHandle errorOwner);
160172

161173
bool outputRefAdded = false;
162174
bool errorRefAdded = false;

0 commit comments

Comments
 (0)