Add ProcessStartInfo.InheritedHandles to restrict child process handle inheritance

[Copilot]

• Remove LPPROC_THREAD_ATTRIBUTE_LIST struct; use void* in all related P/Invoke signatures (InitializeProcThreadAttributeList, UpdateProcThreadAttribute, DeleteProcThreadAttributeList, STARTUPINFOEX.lpAttributeList)
• Change CreateProcess/CreateProcessWithLogonW P/Invokes to use STARTUPINFOEX*/PROCESS_INFORMATION* instead of ref
• Update SafeProcessHandle.Windows.cs: remove LPPROC_THREAD_ATTRIBUTE_LIST variable, simplify BuildProcThreadAttributeList to use single void* buffer, update call sites to use &startupInfoEx/&processInfo
• Fix XML doc: remove "without the close_range syscall" per jkotas suggestion
• Fix UseShellExecute validation: reject non-null InheritedHandles regardless of Count

[adamsitnik]

@copilot please address my feedback

[adamsitnik]

@copilot please address my feedback

[adamsitnik]

@copilot please address my feedback

[Copilot]

@copilot please address my feedback

All addressed in 6d4cbac. Summary of changes:

• Merged Interop.ProcThreadAttributeList.cs into Interop.CreateProcess.cs (deleted the former file, updated CreateProcess to accept STARTUPINFOEX)
• Fixed braces style in AddHandleToInheritList
• Simplified all restrictHandles test code to startInfo.InheritedHandles = restrictHandles ? [] : null
• Moved HandleNotInheritedExitCode const inside InheritedHandles_CanRestrictHandleInheritance
• Moved CLOSE_RANGE_CLOEXEC define to the includes section as #if (HAVE_CLOSE_RANGE || HAVE_CLOSE_RANGE_SYSCALL) && !defined(CLOSE_RANGE_CLOEXEC)
• Added Illumos/Solaris fdwalk support via check_function_exists in cmake, HAVE_FDWALK in pal_config.h.in, and a SetCloexecForFd callback + #elif HAVE_FDWALK branch in pal_process.c

[adamsitnik]

to other reviewers: the alternative would be to have a fallback (for both distros that don't support any of these and OSes where the above sys-calls have failed) and just iterate over all opened fd.

It would be executed in following cases (ofc when the feature would be enabled):

• Linux: seccomp sandbox or very old Linux that we don't officially support anyway (RHEL is fine, so is Azure Linux)
• macOS: case where username and password were provided
• FreeBSD: very old BSD? I am not sure if we support these
• Android: very old Android? I am not sure if we support these but anyway running processes on mobile is not a common thing

[adamsitnik]

@jkotas @stephentoub please let me know what do you think would be best

cc @tmds

[jkotas]

What is the use case that we are designing this API for? Should the docs say when people should use this API?

If the use case is correctness, it does not sound right to fail silently like the current implementation does.

[adamsitnik]

to other reviewers: we could support it on Unix, but it would not be consistent with Windows

[github-actions]

🤖 Copilot Code Review — PR #126318

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Well-justified. This addresses the long-standing issue #13943 (from 2019) and implements the API approved in #125838. Explicit handle inheritance control is a real need for security and resource management.

Approach: The overall approach is sound — using PROC_THREAD_ATTRIBUTE_HANDLE_LIST on Windows, POSIX_SPAWN_CLOEXEC_DEFAULT on macOS, and close_range()/fdwalk() on Linux/Solaris. The reader/writer lock strategy for concurrent process starts is well thought out. However, several correctness and safety concerns need attention.

Summary: ⚠️ Needs Human Review. The implementation is mostly well-structured, but I found a DangerousAddRef/DangerousRelease tracking bug on the Unix path, a concern about unconditionally passing EXTENDED_STARTUPINFO_PRESENT to CreateProcessWithLogonW, and a cleanup ordering issue. A human reviewer should verify the Windows API compatibility claim and assess the severity of the Unix ref-counting issue.

---

Detailed Findings

⚠️ Correctness — DangerousAddRef/DangerousRelease tracking bug in Unix interop

File: src/libraries/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs, lines 26–97

The code uses a single shared boolean inheritedHandleRefsAdded to track DangerousAddRef calls across all inherited handles:

bool inheritedHandleRefsAdded = false;
for (int i = 0; i < inheritedFdCount; i++)
{
    SafeHandle handle = inheritedHandles[i];
    handle.DangerousAddRef(ref inheritedHandleRefsAdded); // shared flag
    inheritedFds[i] = handle.DangerousGetHandle().ToInt32();
}

If handle[0].DangerousAddRef succeeds (sets flag to true) but handle[2].DangerousAddRef throws ObjectDisposedException, the finally block releases all handles (0 through Count-1), including handles 2+ that were never AddRef'd. This decrements their reference counts without a corresponding increment, which can cause premature handle disposal.

The Windows implementation (PrepareHandleAllowList at lines 403–463) does this correctly with per-handle tracking:

bool refAdded = false;
try {
    handle.DangerousAddRef(ref refAdded);
    // ...
    refAdded = false; // transfer ownership
}
finally {
    if (refAdded) handle.DangerousRelease();
}

Additionally, the Unix path does not check for null/invalid/closed handles before calling DangerousAddRef (unlike the Windows path which has if (handle is null || handle.IsInvalid || handle.IsClosed) continue;). A null handle in the list would throw NullReferenceException and then incorrectly release subsequent handles.

Suggested fix: Track AddRef'd handles individually (e.g., track the count of successfully AddRef'd handles and only release that many in finally), and add null/validity checks matching the Windows path.

---

⚠️ Correctness — EXTENDED_STARTUPINFO_PRESENT unconditionally set, including for CreateProcessWithLogonW

File: src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs, line 169

The code unconditionally sets EXTENDED_STARTUPINFO_PRESENT in creationFlags:

int creationFlags = Interop.Kernel32.EXTENDED_STARTUPINFO_PRESENT;

This flag and the STARTUPINFOEX struct (with cb = sizeof(STARTUPINFOEX)) are now passed to all CreateProcess calls, including CreateProcessWithLogonW (line 245–257). Per [Microsoft documentation]((learn.microsoft.com/redacted), CreateProcessWithLogonW does not support STARTUPINFOEX.

The PR review comments indicate this was "confirmed to work," but this is a behavioral change for all process starts with UserName set (not just those using InheritedHandles). If CreateProcessWithLogonW silently ignores the flag on current Windows versions but rejects it on older or future versions, this would be a regression.

Additionally, there is no validation preventing UserName + InheritedHandles together. If CreateProcessWithLogonW ignores the attribute list, handle restriction would silently not work — violating user expectations.

Questions for human reviewer:

1. Has CreateProcessWithLogonW with EXTENDED_STARTUPINFO_PRESENT been tested on multiple Windows versions (including Windows Server 2016/2019)?
2. Should there be validation to either reject or warn when UserName and InheritedHandles are both set?

---

⚠️ Safety — CleanupHandles releases ref before using handle

File: src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs, lines 465–486

In CleanupHandles, DangerousRelease() is called before SetHandleInformation():

safeHandle.DangerousRelease();  // releases our extra ref
if (!Interop.Kernel32.SetHandleInformation(safeHandle, ...))  // uses the handle

After DangerousRelease(), the extra reference keeping the handle alive is gone. If another thread concurrently disposes the SafeHandle between these two calls, SetHandleInformation would operate on a closed handle. The safer order is to call SetHandleInformation first (while the extra ref is still held), then DangerousRelease().

---

⚠️ Edge Case — UpdateProcThreadAttribute with zero handles

File: src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs, lines 189–206

When InheritedHandles is an empty list and no stdio handles are provided, handleCount would be 0. The code calls UpdateProcThreadAttribute with cbSize = 0:

(nuint)(handleCount * sizeof(IntPtr))  // = 0 when handleCount = 0

Passing a zero-size attribute value to UpdateProcThreadAttribute may fail or produce undefined behavior. Consider either skipping the attribute list construction when handleCount == 0, or validating this edge case.

---

💡 Style — Ref assembly member ordering

File: src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs, line 266

InheritedHandles is placed between StandardOutputEncoding and UserName. Per dotnet/runtime convention, ref assembly members should be in alphabetical order. InheritedHandles (starting with 'I') should appear before the Standard* properties (starting with 'S').

---

💡 Validation — Empty InheritedHandles with UseShellExecute

File: src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs, lines 378–381

The validation only rejects InheritedHandles with UseShellExecute when Count > 0:

if (UseShellExecute && InheritedHandles is not null && InheritedHandles.Count > 0)

An empty non-null list (InheritedHandles = []) is allowed with UseShellExecute = true. Semantically, an empty list means "restrict inheritance to only stdio handles" — a restriction that cannot be enforced through ShellExecute. Consider rejecting any non-null InheritedHandles when UseShellExecute is true.

---

✅ Native Code — close_range/fdwalk implementation is well-structured

The three-tier fallback in pal_process.c (close_range function → syscall → fdwalk) with correct placement after dup2 calls is well implemented. The configure.cmake checks follow existing patterns and the CLOSE_RANGE_CLOEXEC macro is properly scoped inside #if blocks.

---

✅ Windows — PROC_THREAD_ATTRIBUTE_HANDLE_LIST implementation

The PrepareHandleAllowList method correctly uses per-handle DangerousAddRef/DangerousRelease tracking, deduplication, and inheritance flag management. The BuildProcThreadAttributeList follows the standard two-call pattern. The reader/writer lock strategy (read lock for explicit handle lists, write lock for implicit inheritance) is a smart design.

---

✅ Tests — Good coverage with appropriate patterns

The tests use RemoteExecutor, [ConditionalTheory] with [InlineData], and AnonymousPipeServerStream with specific exit codes. The three test cases (handle in list → inherited, empty list → not inherited, null → default inherited) cover the key scenarios.

Generated by Code Review for issue #126318 · ◷

[jkotas]

Are either of these two paths compiled into our official builds?

(We are compiling against glibc 2.27 for .NET 11 - #120826 - so I assume that the above path is not taken.)

[tmds]

From CI log https://dev.azure.com/dnceng-public/public/_build/results?buildId=1358879&view=logs&j=826908f3-d824-5a25-6bc6-d6f6821a85ed&t=928c92d7-808f-5c75-12c9-d31e9c1b8f40:

  -- Looking for close_range
  -- Looking for close_range - not found
  -- Looking for __NR_close_range
  -- Looking for __NR_close_range - not found

This is flagged by failing InheritedHandles_CanRestrictHandleInheritance.

[jkotas]

It means you need a local copy to make this work. Look for __NR_copy_file_range for existing art.

[tmds]

Do we need to pass the CLOSE_RANGE_CLOEXEC flag?

[tmds]

Ah yes, we're adding CLOEXEC here and we'll remove it from the inherited handles later.

Might be useful to put this in a comment.

[jkotas]

Suggested change

	check_symbol_exists(
	__NR_close_range
	"sys/syscall.h"
	HAVE_CLOSE_RANGE_SYSCALL)

This should not be needed.