Skip to content

Commit 95247af

Browse files
adamsitnikadamsitnik
committed
address my own feedback:
- add more tests - fix bugs - handle edge cases
1 parent 017f286 commit 95247af

5 files changed

Lines changed: 76 additions & 38 deletions

File tree

‎src/libraries/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,26 +20,26 @@ internal static unsafe int ForkAndExecProcess(
2020
byte** argvPtr = null, envpPtr = null;
2121
int result = -1;
2222

23-
bool stdinRefAdded = false, stdoutRefAdded = false, stderrRefAdded = false;
23+
bool ignore = false;
2424
try
2525
{
2626
int stdinRawFd = -1, stdoutRawFd = -1, stderrRawFd = -1;
2727

2828
if (stdinFd is not null)
2929
{
30-
stdinFd.DangerousAddRef(ref stdinRefAdded);
30+
stdinFd.DangerousAddRef(ref ignore);
3131
stdinRawFd = stdinFd.DangerousGetHandle().ToInt32();
3232
}
3333

3434
if (stdoutFd is not null)
3535
{
36-
stdoutFd.DangerousAddRef(ref stdoutRefAdded);
36+
stdoutFd.DangerousAddRef(ref ignore);
3737
stdoutRawFd = stdoutFd.DangerousGetHandle().ToInt32();
3838
}
3939

4040
if (stderrFd is not null)
4141
{
42-
stderrFd.DangerousAddRef(ref stderrRefAdded);
42+
stderrFd.DangerousAddRef(ref ignore);
4343
stderrRawFd = stderrFd.DangerousGetHandle().ToInt32();
4444
}
4545

@@ -59,12 +59,12 @@ internal static unsafe int ForkAndExecProcess(
5959
FreeArray(envpPtr, envp.Length);
6060
FreeArray(argvPtr, argv.Length);
6161

62-
if (stdinRefAdded)
63-
stdinFd!.DangerousRelease();
64-
if (stdoutRefAdded)
65-
stdoutFd!.DangerousRelease();
66-
if (stderrRefAdded)
67-
stderrFd!.DangerousRelease();
62+
if (stdinFd is not null)
63+
stdinFd.DangerousRelease();
64+
if (stdoutFd is not null)
65+
stdoutFd.DangerousRelease();
66+
if (stderrFd is not null)
67+
stderrFd.DangerousRelease();
6868
}
6969
}
7070

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -387,9 +387,9 @@ private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle,
387387
// To support processes that interact with the terminal (e.g. 'vi'), we need to configure the
388388
// terminal to echo. We keep this configuration as long as there are children possibly using the terminal.
389389
// When a handle is null, the child inherits the parent's standard stream which could be a terminal.
390-
bool usesTerminal = (stdinHandle is null || Interop.Sys.IsATty(stdinHandle))
391-
|| (stdoutHandle is null || Interop.Sys.IsATty(stdoutHandle))
392-
|| (stderrHandle is null || Interop.Sys.IsATty(stderrHandle));
390+
bool usesTerminal = (stdinHandle is not null && Interop.Sys.IsATty(stdinHandle))
391+
|| (stdoutHandle is not null && Interop.Sys.IsATty(stdoutHandle))
392+
|| (stderrHandle is not null && Interop.Sys.IsATty(stderrHandle));
393393

394394
if (startInfo.UseShellExecute)
395395
{

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

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -451,26 +451,23 @@ private unsafe bool StartWithCreateProcess(ProcessStartInfo startInfo, SafeFileH
451451
{
452452
startupInfo.cb = sizeof(Interop.Kernel32.STARTUPINFO);
453453

454-
if (stdinHandle is not null && !stdinHandle.IsInvalid)
454+
if (stdinHandle is not null || stdoutHandle is not null || stderrHandle is not null)
455455
{
456-
inheritableStdinHandle = DuplicateAsInheritable(stdinHandle);
457-
startupInfo.hStdInput = inheritableStdinHandle.DangerousGetHandle();
458-
}
456+
Debug.Assert(stdinHandle is not null && stdoutHandle is not null && stderrHandle is not null, "All or none of the standard handles must be provided.");
459457

460-
if (stdoutHandle is not null && !stdoutHandle.IsInvalid)
461-
{
462-
inheritableStdoutHandle = DuplicateAsInheritable(stdoutHandle);
463-
startupInfo.hStdOutput = inheritableStdoutHandle.DangerousGetHandle();
464-
}
458+
// The user can't specify invalid handle via ProcessStartInfo.Standar*Handle APIs.
459+
// However, Console.OpenStandard*Handle() can return INVALID_HANDLE_VALUE for a process
460+
// that was started with INVALID_HANDLE_VALUE as given standard handle.
461+
// As soon as SafeFileHandle.IsInheritabe() is added, we can use it here to avoid unnecessary duplication.
462+
inheritableStdinHandle = !stdinHandle.IsInvalid ? DuplicateAsInheritable(stdinHandle) : stdinHandle;
463+
inheritableStdoutHandle = !stdoutHandle.IsInvalid ? DuplicateAsInheritable(stdoutHandle) : stdoutHandle;
464+
inheritableStderrHandle = !stderrHandle.IsInvalid ? DuplicateAsInheritable(stderrHandle) : stderrHandle;
465465

466-
if (stderrHandle is not null && !stderrHandle.IsInvalid)
467-
{
468-
inheritableStderrHandle = DuplicateAsInheritable(stderrHandle);
466+
startupInfo.hStdInput = inheritableStdinHandle.DangerousGetHandle();
467+
startupInfo.hStdOutput = inheritableStdoutHandle.DangerousGetHandle();
469468
startupInfo.hStdError = inheritableStderrHandle.DangerousGetHandle();
470-
}
471469

472-
if (stdinHandle is not null || stdoutHandle is not null || stderrHandle is not null)
473-
{
470+
// If STARTF_USESTDHANDLES is not set, the new process will inherit the standard handles.
474471
startupInfo.dwFlags = Interop.Advapi32.StartupInfoOptions.STARTF_USESTDHANDLES;
475472
}
476473

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

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1278,19 +1278,16 @@ public bool Start()
12781278
}
12791279
}
12801280
}
1281+
12811282
bool anyRedirection = startInfo.RedirectStandardInput || startInfo.RedirectStandardOutput || startInfo.RedirectStandardError;
1282-
if (startInfo.UseShellExecute && anyRedirection)
1283+
bool anyHandle = startInfo.StandardInput is not null || startInfo.StandardOutput is not null || startInfo.StandardError is not null;
1284+
if (startInfo.UseShellExecute && (anyRedirection || anyHandle))
12831285
{
12841286
throw new InvalidOperationException(SR.CantRedirectStreams);
12851287
}
12861288

1287-
bool anyHandle = startInfo.StandardInput is not null || startInfo.StandardOutput is not null || startInfo.StandardError is not null;
12881289
if (anyHandle)
12891290
{
1290-
if (startInfo.UseShellExecute)
1291-
{
1292-
throw new InvalidOperationException(SR.CantRedirectStreams);
1293-
}
12941291
if (startInfo.StandardInput is not null && startInfo.RedirectStandardInput)
12951292
{
12961293
throw new InvalidOperationException(SR.CantSetHandleAndRedirect);
@@ -1320,15 +1317,15 @@ public bool Start()
13201317

13211318
try
13221319
{
1323-
if (anyRedirection)
1320+
if (anyRedirection || anyHandle)
13241321
{
13251322
// Windows supports creating non-inheritable pipe in atomic way.
13261323
// When it comes to Unixes, it depends whether they support pipe2 sys-call or not.
13271324
// If they don't, the pipe is created as inheritable and made non-inheritable with another sys-call.
13281325
// Some process could be started in the meantime, so in order to prevent accidental handle inheritance,
1329-
// a WriterLock is used around the pipe creation code.
1326+
// a writer lock is used around the pipe creation code.
13301327

1331-
bool requiresLock = !SupportsAtomicNonInheritablePipeCreation;
1328+
bool requiresLock = anyRedirection && !SupportsAtomicNonInheritablePipeCreation;
13321329

13331330
if (requiresLock)
13341331
{
@@ -1345,6 +1342,10 @@ public bool Start()
13451342
{
13461343
SafeFileHandle.CreateAnonymousPipe(out childInputPipeHandle, out parentInputPipeHandle);
13471344
}
1345+
else if (!OperatingSystem.IsAndroid())
1346+
{
1347+
childInputPipeHandle = Console.OpenStandardInputHandle();
1348+
}
13481349

13491350
if (startInfo.StandardOutput is not null)
13501351
{
@@ -1354,6 +1355,10 @@ public bool Start()
13541355
{
13551356
SafeFileHandle.CreateAnonymousPipe(out parentOutputPipeHandle, out childOutputPipeHandle, asyncRead: OperatingSystem.IsWindows());
13561357
}
1358+
else if (!OperatingSystem.IsAndroid())
1359+
{
1360+
childOutputPipeHandle = Console.OpenStandardOutputHandle();
1361+
}
13571362

13581363
if (startInfo.StandardError is not null)
13591364
{
@@ -1363,6 +1368,10 @@ public bool Start()
13631368
{
13641369
SafeFileHandle.CreateAnonymousPipe(out parentErrorPipeHandle, out childErrorPipeHandle, asyncRead: OperatingSystem.IsWindows());
13651370
}
1371+
else if (!OperatingSystem.IsAndroid())
1372+
{
1373+
childErrorPipeHandle = Console.OpenStandardErrorHandle();
1374+
}
13661375
}
13671376
finally
13681377
{

‎src/libraries/System.Diagnostics.Process/tests/ProcessHandlesTests.cs‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.IO;
5-
using System.Text;
65
using System.Threading.Tasks;
76
using Microsoft.Win32.SafeHandles;
87
using Xunit;
@@ -81,6 +80,39 @@ public async Task CanRedirectOutputAndErrorToDifferentPipes(bool readAsync)
8180
}
8281
}
8382

83+
[Theory]
84+
[InlineData(true)]
85+
[InlineData(false)]
86+
public async Task CanRedirectOutputAndErrorToSamePipe(bool readAsync)
87+
{
88+
ProcessStartInfo startInfo = OperatingSystem.IsWindows()
89+
? new("cmd") { ArgumentList = { "/c", "echo Hello from stdout && echo Error from stderr 1>&2" } }
90+
: new("sh") { ArgumentList = { "-c", "echo 'Hello from stdout' && echo 'Error from stderr' >&2" } };
91+
92+
string expectedOutput = OperatingSystem.IsWindows()
93+
? "Hello from stdout \r\nError from stderr \r\n"
94+
: "Hello from stdout\nError from stderr\n";
95+
96+
SafeFileHandle.CreateAnonymousPipe(out SafeFileHandle readPipe, out SafeFileHandle writePipe, asyncRead: readAsync);
97+
98+
startInfo.StandardOutput = writePipe;
99+
startInfo.StandardError = writePipe;
100+
101+
using (readPipe)
102+
using (writePipe)
103+
{
104+
using Process process = Process.Start(startInfo)!;
105+
106+
using FileStream combinedStream = new(readPipe, FileAccess.Read, bufferSize: 1, isAsync: readAsync);
107+
using StreamReader combinedReader = new(combinedStream);
108+
109+
Assert.Equal(expectedOutput, await combinedReader.ReadToEndAsync());
110+
111+
process.WaitForExit();
112+
Assert.Equal(0, process.ExitCode);
113+
}
114+
}
115+
84116
[Theory]
85117
[InlineData(true)]
86118
[InlineData(false)]

0 commit comments

Comments
 (0)