Refactor Process.StartCore to accept SafeFileHandle parameters for stdin/stdout/stderr by Copilot · Pull Request #125766 · dotnet/runtime

Copilot · 2026-03-19T08:01:37Z

Description

Refactors StartCore on both Windows and Unix to accept SafeFileHandle instances for child process standard streams, enabling future scenarios where callers provide pre-existing handles. Pipe creation and stream setup are lifted into the shared Process.cs:Start() method, with platform-specific OpenStream and encoding helper methods defined directly in each platform file (no partial methods).

Native layer (`pal_process.c`, `pal_process.h`, `pal_process_wasi.c`)

stdinFd/stdoutFd/stderrFd changed from int32_t* (out) to int32_t (in) — caller now owns pipe creation
Removed internal pipe creation (SystemNative_Pipe calls) and associated ProcessCreateLock
dup2 of the provided fds onto STDIN_FILENO/STDOUT_FILENO/STDERR_FILENO is now unconditional (CLOEXEC flags are stripped by dup2 itself since it doesn't copy descriptor flags)
Removed redirectStdin/redirectStdout/redirectStderr parameters from native signature

New native entry point (`pal_io.c`, `pal_io.h`)

Added SystemNative_IsAtomicNonInheritablePipeCreationSupported — returns 1 when HAVE_PIPE2 is defined, 0 otherwise; used by managed code to decide whether pipe creation requires a write lock

Interop layer (`Interop.ForkAndExecProcess.cs`)

P/Invoke signature changed from out int stdinFd/stdoutFd/stderrFd to SafeFileHandle parameters (marshalled as int32_t values)
Removed redirectStdin/redirectStdout/redirectStderr boolean parameters

New interop declarations

Interop.IsATty.cs: Added SafeFileHandle overload of IsATty
Interop.IsAtomicNonInheritablePipeCreationSupported.cs: New P/Invoke with NullableBool caching for SystemNative_IsAtomicNonInheritablePipeCreationSupported

Managed layer

Process.cs: Start() now creates pipes via SafeFileHandle.CreateAnonymousPipe directly (with Console.OpenStandard*Handle() fallbacks for non-redirected streams). Pipe creation is guarded by !startInfo.UseShellExecute || !OperatingSystem.IsWindows(). On platforms without pipe2, a WriteLock wraps pipe creation to prevent accidental handle inheritance. After StartCore, sets up _standardInput/_standardOutput/_standardError streams via platform-specific OpenStream. The CantRedirectStreams validation for UseShellExecute is moved here from platform files.
Process.Unix.cs: StartCore takes SafeFileHandle? parameters (asserted non-null). Terminal detection (usesTerminal) changed from checking redirect booleans to IsATty() calls on the actual handles. OpenStream transfers ownership from SafeFileHandle to SafePipeHandle and wraps in AnonymousPipeClientStream. GetStandardInputEncoding/GetStandardOutputEncoding return Encoding.Default. s_processStartLock moved to shared ProcessUtils.
Process.Windows.cs: StartWithCreateProcess takes non-nullable SafeFileHandle parameters. Handles are duplicated as inheritable via DuplicateAsInheritable. The old CreatePipe helper and s_createProcessLock are removed. The lock is replaced by ProcessUtils.s_processStartLock.EnterWriteLock(). OpenStream wraps handle in FileStream. GetStandardInputEncoding/GetStandardOutputEncoding use console code pages.
Process.Win32.cs: StartCore signature updated; asserts that UseShellExecute implies all-null handles. CantRedirectStreams check removed (moved to shared Process.cs).
ProcessUtils.cs: Now hosts s_processStartLock (shared across platforms).
Process.ConfigureTerminalForChildProcesses.Unix.cs: References ProcessUtils.s_processStartLock instead of the removed instance field.

Console changes (Android support)

ConsolePal.Android.cs: OpenStandardInputHandle/OpenStandardOutputHandle/OpenStandardErrorHandle now return SafeFileHandle(0/1/2, ownsHandle: false) instead of throwing PlatformNotSupportedException, matching the existing Unix/Wasi behavior.
Console.cs: Removed [UnsupportedOSPlatform("android")] from all three OpenStandard*Handle methods.
Ref assembly (System.Console.cs): Removed [UnsupportedOSPlatformAttribute("android")] from all three methods.
Tests (ConsoleHandles.cs): Updated platform filters to include Android in valid-handle tests and exclude it from throws-on-unsupported tests.

Build verification

Managed build passes for all target frameworks (including Android)
Native build passes
Tests pass (372/373, 1 unrelated flaky failure in TestGetProcesses)

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…ation and stream setup to Process.cs Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

…nterop.Pipe.cs to csproj Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

dotnet-policy-service · 2026-03-19T08:07:45Z

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

adamsitnik

@copilot address my feedback

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

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

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

src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj

…eAnonymousPipe directly, simplify Windows StartWithCreateProcess Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

Copilot · 2026-03-19T08:58:04Z

@copilot address my feedback

All feedback addressed in commit 07d9369:

Removed partial method declarations; OpenStream/encoding methods defined directly in platform files
Pipe creation uses SafeFileHandle.CreateAnonymousPipe directly in Process.cs:Start() with !startInfo.UseShellExecute || !OperatingSystem.IsWindows() condition
Console.OpenStandard*Handle() called from Start() for non-redirected streams
Windows StartWithCreateProcess just calls DuplicateAsInheritable for all 3 handles
Removed CreatePipe from both platforms, s_invalidSafeFileHandle from Unix, Interop.Pipe.cs from csproj
Removed XML docs from non-public methods
Build passes all targets; 372/373 tests pass (1 flaky TestGetProcesses failure unrelated to changes)

adamsitnik · 2026-03-19T12:48:45Z

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

To review this file I recommend hiding whitespace changes:

adamsitnik · 2026-03-19T13:28:36Z

@copilot please update the PR description based on my commits and then invoke the code-review skill and post your analysis/results as a comment on this PR

Copilot · 2026-03-19T13:40:48Z

@copilot please update the PR description based on my commits and then invoke the code-review skill and post your analysis/results as a comment on this PR

PR description updated. Code review follows below.

🤖 Copilot Code Review

Holistic Assessment

Motivation: This refactoring lifts pipe creation out of platform-specific code into the shared Process.cs:Start() method, making StartCore accept pre-created SafeFileHandle parameters. This enables future scenarios where callers provide pre-existing handles and unifies the pipe creation logic across platforms.

Approach: The approach is sound — pipes are created uniformly via SafeFileHandle.CreateAnonymousPipe in shared code, with Console.OpenStandard*Handle() fallbacks for non-redirected streams. Platform-specific OpenStream and encoding methods are defined directly in each platform file without partial methods. The locking strategy is carefully designed to prevent accidental handle inheritance.

Summary: ✅ LGTM. The refactoring is correct, handles resource management properly, and the locking scheme is well-designed. Two models (Gemini 2.5 Pro and Claude Opus 4.5) reviewed this independently; their findings converge. No blocking issues found.

Detailed Findings

✅ Resource management — Console handles are safe to dispose

The finally block in Process.cs:Start() disposes childInputPipeHandle, childOutputPipeHandle, childErrorPipeHandle. When not redirecting, these are assigned from Console.OpenStandard*Handle(). Verified that both Unix and Windows implementations create these with ownsHandle: false:

Unix: new SafeFileHandle(fd, ownsHandle: false) (ConsolePal.Unix.cs)
Windows: new SafeFileHandle(handle, ownsHandle: false) (ConsolePal.Windows.cs)

Disposing a non-owning SafeFileHandle is a no-op — no risk of closing actual stdin/stdout/stderr descriptors.

✅ Handle ownership transfer in Unix OpenStream — No double-dispose

The Unix OpenStream method transfers ownership from SafeFileHandle to SafePipeHandle:

SafePipeHandle safePipeHandle = new(handle.DangerousGetHandle(), ownsHandle: true);
handle.SetHandleAsInvalid();
handle.Dispose();

The parent pipe handles (parentInputPipeHandle etc.) are only disposed in the catch block on exception, which runs before OpenStream is called. On success, OpenStream takes ownership and no double-dispose occurs.

✅ Platform-conditional handle creation — Assertions are correct

The condition !startInfo.UseShellExecute || !OperatingSystem.IsWindows():

On Unix: Always true → handles are always created. The assertion stdinHandle is not null && ... is correct.
On Windows: true only when !UseShellExecute → handles are null for ShellExecute. The assertion UseShellExecute == (handles are null) is correct.

✅ Native dup2 now unconditional — Safe

The native code now always calls dup2(stdinFd, STDIN_FILENO) etc. When not redirecting, the caller passes the actual stdin fd (from Console.OpenStandardInputHandle()), so stdinFd == STDIN_FILENO. Per POSIX, dup2(fd, fd) is a no-op that returns fd without error.

✅ Locking scheme — Correctly prevents handle inheritance leaks

On platforms without pipe2: WriteLock wraps pipe creation (preventing other processes from inheriting pipes during the non-atomic create+set-CLOEXEC window).
On Windows: WriteLock wraps DuplicateAsInheritable + CreateProcess (preventing concurrent processes from inheriting each other's inheritable handles).
On Unix: ReadLock during ForkAndExecProcess allows concurrent process starts while being mutually exclusive with WriteLock operations.

The lock is shared across platforms via ProcessUtils.s_processStartLock, which is correct since the same ReaderWriterLockSlim instance must be used for read/write coordination.

💡 Minor — Comment could be more precise

The comment at line 1372 ("We MUST close the parent copies of the child handles") is slightly misleading for the non-redirect case where handles came from Console.OpenStandard*Handle() — these aren't "parent copies of child handles" but non-owning wrappers. This is a cosmetic observation, not a blocker.

Models contributing to this review: Claude Sonnet 4 (primary), Gemini 2.5 Pro, Claude Opus 4.5

- STARTF_USESTDHANDLES is always provided now - we may use more flags in the future - so ProcessWindowStyle tests should check only if STARTF_USESHOWWINDOW (1) was applied

adamsitnik · 2026-03-19T18:23:23Z

src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.Windows.cs

                short expectedWindowFlag = short.Parse(argSplit[1]);

-                Assert.Equal(expectedDwFlag, si.dwFlags);
+                Assert.Equal(expectUsesShowWindow, (si.dwFlags & 0x1) != 0); // STARTF_USESHOWWINDOW is 0x1


To other reviewers: This test was checking exact flag value. Now we just check if the flag is applied, allowing other flags like STARTF_USESTDHANDLES

adamsitnik · 2026-03-19T18:23:51Z

src/native/libs/System.Native/pal_process.c

 c_static_assert(PAL_PRIO_USER == (int)PRIO_USER);

-#if !HAVE_PIPE2
-static pthread_mutex_t ProcessCreateLock = PTHREAD_MUTEX_INITIALIZER;


To other reviewers: now this lock is taken in managed code, only for the time of creating the pipes.

adamsitnik · 2026-03-19T18:24:56Z

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

-            bool usesTerminal = !(startInfo.RedirectStandardInput &&
-                                  startInfo.RedirectStandardOutput &&
-                                  startInfo.RedirectStandardError);
+            bool usesTerminal = Interop.Sys.IsATty(stdinHandle) || Interop.Sys.IsATty(stdoutHandle) || Interop.Sys.IsATty(stderrHandle);


To other reviewers: each of these handles could point to a terminal. So we perform a detailed check.

jkotas · 2026-03-19T18:40:56Z

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

+                        }
+                        else
+                        {
+                            childOutputPipeHandle = Console.OpenStandardOutputHandle();


This and the other logic in this PR assume that stdin/stdout/stderr are distinct handles. The earlier versions had code to deal with the situation where these handle overlap. Do we need to worry about that?

This and the other logic in this PR assume that stdin/stdout/stderr are distinct handles. The earlier versions had code to deal with the situation where these handle overlap. Do we need to worry about that?

That version was using an allow list which prohibits duplicates. It will be needed for #13943

In case of this PR, in the worst case (for example a process started with all handles redirected to NUL), we are going to perform 2 extra handle duplicates (it will be neglible when compared to the cost of starting a new process)

It is the problem that Tom commented on. It is not clear what exact API shapes is this refactoring expected to support.

jkotas · 2026-03-19T18:49:58Z

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

+                        {
+                            childInputPipeHandle = Console.OpenStandardInputHandle();
+                        }
+
+                        if (startInfo.RedirectStandardOutput)
+                        {
+                            SafeFileHandle.CreateAnonymousPipe(out parentOutputPipeHandle, out childOutputPipeHandle, asyncRead: OperatingSystem.IsWindows());
+                        }
+                        else
+                        {
+                            childOutputPipeHandle = Console.OpenStandardOutputHandle();
+                        }
+
+                        if (startInfo.RedirectStandardError)
+                        {
+                            SafeFileHandle.CreateAnonymousPipe(out parentErrorPipeHandle, out childErrorPipeHandle, asyncRead: OperatingSystem.IsWindows());
+                        }
+                        else
+                        {
+                            childErrorPipeHandle = Console.OpenStandardErrorHandle();
+                        }


I do not know what works on mobile wrt. Process.Start today. I agree with copilot that we should avoid regressing it.

tmds · 2026-03-19T20:05:06Z

The most recent conclusion from API Review is that we don't want a new option bag at all (#124334 (comment)).

I think ProcessStartOptions stranded while trying to figure out how to make it both a simple API that avoids the "pit of failure" of current Process API, and an API that enables advanced features which require some "process domain knowledge" which makes it more complex.

Deciding to no longer do ProcessStartOptions doesn't mean we know how to solve this problem with Process and ProcessStartInfo.

It will allow me to implement #123959 as set of Process methods (yes, no ChildProcess) without introducing a new public API. It's not like I don't want to introduce such APIs (I still want to introduce SafeChildProcessHandle), but I can simply do it later, after providing the high-level APIs.

The API this is refactoring for is not worked out. It depends on what the API is whether it is useful to refactor this.

…n Android (it's not supported)

adamsitnik · 2026-03-19T21:34:53Z

Deciding to no longer do ProcessStartOptions doesn't mean we know how to solve this problem with Process and ProcessStartInfo.

The conclusion was that having more than one option bag would cause too much confusion (which one to use? is the old one deprecated? etc). So even if we come up with a better ProcessStartOptions design (like 4 simple things in base class and all other things in AdvancedProcessStartOptions) it won't be allowed.

The API this is refactoring for is not worked out. It depends on what the API is whether it is useful to refactor this.

We have at least two options:

extending ProcessStartInfo with 3 new SafeFileHandle properties
extending Process with a new Start overload that takes ProcessStartInfo instance and 3 SafeFileHandle arguments

If everything goes right, I am going to propose this API next Tuesday. With this refactor, exposing it publicly will be trivial. Another simple refactor is going to make it possible to move the StartCore as is to SafeProcessHandle Start method.

Moreover, this refactor is going to make #13943 easier (and this API was already approved).

@tmds please let me know if you need any more details. Thanks!

jkotas · 2026-03-19T22:59:13Z

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

+                        {
+                            SafeFileHandle.CreateAnonymousPipe(out childInputPipeHandle, out parentInputPipeHandle);
+                        }
+                        else if (OperatingSystem.IsAndroid())


This looks odd. Should we make OpenStandardInputHandle work on Android instead of this?

@copilot address this feedback, make Console.OpenStandard*Handle APIs work on android by returning 0/1/2 values. Update the implementation, ref and tests. And then remove the Android checks from this PR

Done in bba0144. Changed ConsolePal.Android.cs to return new SafeFileHandle(0/1/2, ownsHandle: false) (matching Unix/Wasi pattern), removed [UnsupportedOSPlatform("android")] from all three methods in both Console.cs and the ref assembly, updated tests, and removed the Android workarounds from Process.cs. All builds pass (ref, src including Android target, and Process).

jkotas · 2026-03-19T23:04:25Z

extending Process with a new Start overload that takes ProcessStartInfo instance and 3 SafeFileHandle arguments

I am not sure this API shape is going to fly well - it is inconsistent with what's there today. I think the SafeFileHandles should be members on ProcessStartInfo and be mutually exclusive with the other ways to redirect input/output.

… 0/1/2; remove Android checks from Process.cs Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

jkotas · 2026-03-20T00:16:44Z

src/libraries/System.Console/src/System/ConsolePal.Android.cs

        public static Stream OpenStandardError() => new LogcatStream(OutputEncoding);

-        public static SafeFileHandle OpenStandardInputHandle() => throw new PlatformNotSupportedException();
+        public static SafeFileHandle OpenStandardInputHandle() => new SafeFileHandle(0, ownsHandle: false);


Hmm, this is inconsistent with what OpenStandardInput/OpenStandardOutput/OpenStandardError does now.

Can we just leave the standard handles alone when there is nothing to redirect?

tmds · 2026-03-20T08:01:46Z

We should start from the use-cases/issues, propose the API and design (which may involve some prototyping/derisking) and then the actual implementation follows.

You may have a picture about the API and design. I don't think it is written down so others can understand what way we're heading and how it solves the user issues.

I can technically review if this refactoring isn't breaking anything, but currently I can't say why we need it, which is the more important thing.

Personally, I prefer we capture such information in an issue. PRs are already implementation centric and the coding agent workflows make it even more focused on code and code changes.

…rning fd 0/1/2; remove Android checks from Process.cs" This reverts commit bba0144.

Copilot AI and others added 2 commits March 18, 2026 22:14

Refactor StartCore to accept SafeFileHandle parameters; move pipe cre…

f47b2fd

…ation and stream setup to Process.cs Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

Fix build errors: XML doc comments and unsafe partial mismatch; add I…

fea2a80

…nterop.Pipe.cs to csproj Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

Copilot AI assigned Copilot and adamsitnik Mar 19, 2026

Copilot created this pull request from a session on behalf of adamsitnik March 19, 2026 08:01 View session

adamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 19, 2026

github-actions bot added the area-System.Diagnostics.Process label Mar 19, 2026

adamsitnik requested changes Mar 19, 2026

View reviewed changes

Copilot started work on behalf of adamsitnik March 19, 2026 08:16 View session

Address PR feedback: remove partial methods, use SafeFileHandle.Creat…

07d9369

…eAnonymousPipe directly, simplify Windows StartWithCreateProcess Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

Copilot AI requested a review from adamsitnik March 19, 2026 09:02

Copilot finished work on behalf of adamsitnik March 19, 2026 09:02

address my own feedback

b65842b

adamsitnik reviewed Mar 19, 2026

View reviewed changes

more polishing after reading the code again

43b6301

Copilot started work on behalf of adamsitnik March 19, 2026 13:28 View session

Copilot AI requested a review from adamsitnik March 19, 2026 13:42

Copilot finished work on behalf of adamsitnik March 19, 2026 13:42

adamsitnik added 3 commits March 19, 2026 14:45

fix Unix build

dc85b03

trigger the CI as it seems to got stuck

9ee1881

fix the tests:

17bc483

- STARTF_USESTDHANDLES is always provided now - we may use more flags in the future - so ProcessWindowStyle tests should check only if STARTF_USESHOWWINDOW (1) was applied

adamsitnik removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 19, 2026

adamsitnik mentioned this pull request Mar 19, 2026

System.Diagnostics.Tests.ProcessStartInfoTests.TestWindowStyle failing in CI #119533

Open

adamsitnik reviewed Mar 19, 2026

View reviewed changes

adamsitnik requested a review from tmds March 19, 2026 18:25

handle INVALID_HANDLE_VALUE on Windows

7cf7f0c

jkotas reviewed Mar 19, 2026

View reviewed changes

address code review feedback: don't use Console.OpenStandard*Handle o…

2c8c444

…n Android (it's not supported)

adamsitnik requested a review from jkotas March 19, 2026 21:43

jkotas reviewed Mar 19, 2026

View reviewed changes

Copilot started work on behalf of adamsitnik March 19, 2026 23:13 View session

Make Console.OpenStandard*Handle APIs work on Android by returning fd…

bba0144

… 0/1/2; remove Android checks from Process.cs Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

Copilot AI requested a review from adamsitnik March 19, 2026 23:40

Copilot finished work on behalf of adamsitnik March 19, 2026 23:40

jkotas reviewed Mar 20, 2026

View reviewed changes

adamsitnik added 2 commits March 20, 2026 12:00

Revert "Make Console.OpenStandard*Handle APIs work on Android by retu…

7a345e2

…rning fd 0/1/2; remove Android checks from Process.cs" This reverts commit bba0144.

address code review feedback: don't dup 0/1/2 on Unix

b362a72

Conversation

Copilot AI commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Native layer (pal_process.c, pal_process.h, pal_process_wasi.c)

New native entry point (pal_io.c, pal_io.h)

Interop layer (Interop.ForkAndExecProcess.cs)

New interop declarations

Managed layer

Console changes (Android support)

Build verification

Uh oh!

dotnet-policy-service bot commented Mar 19, 2026

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Mar 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Mar 19, 2026

Uh oh!

Copilot AI commented Mar 19, 2026

🤖 Copilot Code Review

Holistic Assessment

Detailed Findings

✅ Resource management — Console handles are safe to dispose

✅ Handle ownership transfer in Unix OpenStream — No double-dispose

✅ Platform-conditional handle creation — Assertions are correct

✅ Native dup2 now unconditional — Safe

✅ Locking scheme — Correctly prevents handle inheritance leaks

💡 Minor — Comment could be more precise

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmds commented Mar 19, 2026

Uh oh!

adamsitnik commented Mar 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas commented Mar 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmds commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Copilot AI commented Mar 19, 2026 •

edited

Loading

Native layer (`pal_process.c`, `pal_process.h`, `pal_process_wasi.c`)

New native entry point (`pal_io.c`, `pal_io.h`)

Interop layer (`Interop.ForkAndExecProcess.cs`)