Add new process management APIs to SafeProcessHandle#124375
Add new process management APIs to SafeProcessHandle#124375
Conversation
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…, Theory for utils Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…h for sh Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…syntax Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
… remove dead code Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…names Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…s, and Windows implementation Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address the feedback
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
...ries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs
Outdated
Show resolved
Hide resolved
| { | ||
| public partial class SafeProcessHandleTests | ||
| { | ||
| [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))] |
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.Windows.cs
Outdated
Show resolved
Hide resolved
It would be a surprise to me. This is the implementation of the condition: https://github.com/dotnet/arcade/blob/55c97a45af8fd75bc87e9e4b4f61aef3fead254d/src/Microsoft.DotNet.RemoteExecutor/src/RemoteExecutor.cs#L110-L118 . I do not see anything that would disable it on Nano. There are subtle behavior difference between full Windows and Nano in how to signals behave (see #115206 (comment)), but as far as I know the signals generally work just fine on Nano. If I had to guess, it may be a problem with the process group emulator via Windows jobs. |
The other test (CreateNewProcessGroup_CanBeSetToTrue) uses process group and works fine on Nano. The emulation is very simple, it's just one Job created and assigned to process. When I tried enabling Windows signals on Nano, I was getting And the test that you have referenced is silently swallowing this error: |
…PinnableReference, fix CreateProcess braces Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
- merge SafeProcessHandleTests.Windows.cs into SafeProcessHandleTests as these tests are not really Windows specific - add WaitForExitOrKil test that covers ProcessGroup code path
…to-safeprocesshandle
| /// </remarks> | ||
| public void Signal(PosixSignal signal) | ||
| { | ||
| if (!Enum.IsDefined(signal)) |
There was a problem hiding this comment.
Allow providing raw signal value, similar to what the existing https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.posixsignalregistration.create?view=net-10.0#remarks does?
| /// <remarks> | ||
| /// On Windows, only SIGINT (mapped to CTRL_C_EVENT), SIGQUIT (mapped to CTRL_BREAK_EVENT), and SIGKILL are supported. | ||
| /// The process must have been started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/> set to true for signals to work properly. | ||
| /// On Windows, signals are always sent to the entire process group, not just the single process. |
There was a problem hiding this comment.
This does not match the implementation. SIGKILL emulation is sent to specific process on Windows.
| /// On Windows, signals are always sent to the entire process group. | ||
| /// On Unix/Linux, all signals defined in PosixSignal are supported, and the signal is sent to all processes in the process group. |
There was a problem hiding this comment.
| /// On Windows, signals are always sent to the entire process group. | |
| /// On Unix/Linux, all signals defined in PosixSignal are supported, and the signal is sent to all processes in the process group. | |
| /// On Unix/Linux, all signals defined in PosixSignal are supported. |
Saying that SignalProcessGroup API sends signals to entire process group is redundant.
|
|
||
| if (!isDuplicate) | ||
| { | ||
| if (!Interop.Kernel32.GetHandleInformation(handlePtr, out int flags)) |
There was a problem hiding this comment.
handlePtr is acquired by DangerousGetHandle. What makes it safe?
This should do regular SafeHandle marshalling to avoid handle use-after-free bugs in the presence of race conditions.
| // Thread handle for suspended processes (only used on Windows) | ||
| private IntPtr _threadHandle; | ||
|
|
||
| // Job handle for CreateNewProcessGroup functionality (only used on Windows) |
There was a problem hiding this comment.
| // Thread handle for suspended processes (only used on Windows) | |
| private IntPtr _threadHandle; | |
| // Job handle for CreateNewProcessGroup functionality (only used on Windows) | |
| // Thread handle for suspended processes | |
| private IntPtr _threadHandle; | |
| // Job handle for CreateNewProcessGroup functionality |
We do not need to say that stuff defined in Windows specific file is only used on Windows
| private ProcessExitStatus WaitForExitOrKillOnTimeoutCore(int milliseconds) | ||
| { | ||
| bool wasKilledOnTimeout = false; | ||
| using Interop.Kernel32.ProcessWaitHandle processWaitHandle = new(this); |
There was a problem hiding this comment.
ProcessWaitHandle duplicates the OS process handle. I am not sure why it does all that, but it is not needed here - this can wait on the process handle directly without duplication if you would like to make this leaner.
There was a problem hiding this comment.
Good catch, it allowed me to remove some allocations ;)
| /// <remarks> | ||
| /// On Windows, only SIGINT (mapped to CTRL_C_EVENT), SIGQUIT (mapped to CTRL_BREAK_EVENT), and SIGKILL are supported. | ||
| /// The process must have been started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/> set to true for signals to work properly. | ||
| /// On Windows, signals are always sent to the entire process group, not just the single process. |
There was a problem hiding this comment.
Mention that signals can be only sent to processes with console attached on Windows.
| /// <remarks> | ||
| /// On Unix, sends SIGKILL to all processes in the process group. | ||
| /// On Windows, requires the process to have been started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/>=true. | ||
| /// Terminates all processes in the job object. If the process was not started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/>=true, |
There was a problem hiding this comment.
This talks about job object out of the blue without explaining where the job object came from.
Also, this should mention that this is best-effort emulation of Unix process groups and that the set of processes in the job object may not be the same as the set of process in the Windows OS maintained process group.
For example, let's say a process starts a child process with CreateNewProcessGroup, and the child process starts grand-child process with CreateNewProcessGroup, I assume that this API when called on child process is going to kill both child process and grand-child process on Windows even though they belong to different Windows process groups. This is different from how it is works on Unix and it is different from how regular signals work (Ctrl+C is going to be only delivered to the child process).
Is this correct, or is there some detail that I am missing that makes it work 100%?
There was a problem hiding this comment.
We should have tests that cover all corner cases of the emulator.
There was a problem hiding this comment.
I was not able to provide unit test coverage for the KillProcessGroup differences. I wanted to start a child process that would start another process with CreateNewProcessGroup = true:
RemoteExecutordoes not support the new APIs yet, so I can't use it.- I can't use
ProcessStartInfor.CreateNewProcessGroupfrom PowerShell (compile error)
const string script = """
$psi = New-Object System.Diagnostics.ProcessStartInfo
$psi.FileName = "ping.exe"
$psi.Arguments = "127.0.0.1 -n 11"
$psi.CreateNewProcessGroup = $true
$process = New-Object System.Diagnostics.Process
$process.StartInfo = $psi
$process.Start() | Out-Null
$process.WaitForExit()
""";
ProcessStartOptions options = OperatingSystem.IsWindows()
? new("powershell.exe")
{
Arguments = { "-NoProfile", "-Command", script }
}
: new("sh")
{
Arguments = { "-c", "setsid sleep 30" }, // grand child starts new session (which also creates new process group)
};
options.CreateNewProcessGroup = true; // the child creates its own process groupI was able to use "sh -c setsid sleep 3" on Unix, but I can't add it in this PR as the Unix implementation throws not implemented for now.
There was a problem hiding this comment.
Remote executor allows you to start the process yourself using new RemoteInvokeOptions { Start = false; }. Number of the existing System.Diagnostics.Process tests do that. Would that work?
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessUtils.Windows.cs
Outdated
Show resolved
Hide resolved
| // - powershell is not available on Nano. We can't always use it. | ||
| // - ping seems to be a workaround, but it's simple and work everywhere. The arguments are set to make it sleep for approximately 10 seconds. | ||
| private static ProcessStartOptions CreateTenSecondSleep() => OperatingSystem.IsWindows() | ||
| ? new("ping") { Arguments = { "127.0.0.1", "-n", "11" } } |
There was a problem hiding this comment.
Great catch. The test was not re-enabling the Ctrl+C handler:
Since this logic needs to be performed inside the child process and RemoteExecutor does not support SafeProcessHandle.Start yet, I used PWS script to do it.
There was a problem hiding this comment.
0xc0000142 is STATUS_DLL_INIT_FAILED.
How does missing ReEnableCtrlHandlerIfNeeded lead to this error?
I would expect that missing ReEnableCtrlHandlerIfNeeded will make it impossible to send the signal to the process. I do not see how it can lead to STATUS_DLL_INIT_FAILED.
…ct directly, allow raw signal values, fix docs Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
@adamsitnik Let me know once you believe my feedback is addressed properly, and this is ready for another review |
|
@copilot please invoke the code-review skill and post the analysis/comments as a comment on this PR |
- use SafeHandles instead of raw handles - use DangerousAddRef and DangerousRelease for handles we don't control - simplify comments - call SetConsoleCtrlHandler in the test process to avoid weird pop up window - make ProcessId public - add tests for KillOnParentExit
|
|
||
| private ProcessExitStatus WaitForExitCore() | ||
| { | ||
| Interop.Kernel32.WaitForSingleObject(this, Timeout.Infinite); |
There was a problem hiding this comment.
We do not want to wait by calling WaitForSingleObject directly. It does not for interop with other subsystems: COM apartment support, SynchronizationContext, .... .
My earlier feedback was about avoiding the Windows OS handle duplication. I did not mean to switch to calling WaitForSingleObject directly. If it is too hard to avoid Windows OS handle duplication for some reason while still calling the managed APIs to avoid, we should keep duplicating the handle. It would be nice to have a comment that explains why we have opted for seemingly unnecessary handle duplication.
There was a problem hiding this comment.
The Windows OS handle duplication should be avoidable in both sync and async variants.
It is ok to leave this small optimization for later.
There was a problem hiding this comment.
@jkotas could you please tell me more about why using WaitForSingleObject is a bad idea? It was something that LLM came up with, but I am very curious why it's bad. Thanks!
There was a problem hiding this comment.
It is what I have said: WaitHandle.Wait does extra stuff for COM interop, SynchronizationContext, tracing, ... .
If you call WaitForSingleObject directly:
- SynchronizationContext Wait overrides won't be called:
- Wait tracing won't work
- Special COM STA apartment waits won't work (you may see hangs on other threads in the process if it is Windows GUI app).
runtime/src/coreclr/vm/threads.cpp
Lines 2965 to 2969 in 8ffb906
| /// <remarks> | ||
| /// On Windows, this method uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION, SYNCHRONIZE, and PROCESS_TERMINATE permissions. | ||
| /// On Linux with pidfd support, this method uses the pidfd_open syscall. | ||
| /// On other Unix systems, this method uses kill(pid, 0) to verify the process exists and the caller has permission to signal it. |
There was a problem hiding this comment.
| /// On other Unix systems, this method uses kill(pid, 0) to verify the process exists and the caller has permission to signal it. | |
| /// On other Unix systems, this method uses kill(pid, 0) to verify the process exists and the caller has permission to signal it. The returned handle is prone to process ID reuse issues in this case. |
Include a warning about PID reuse?
There was a problem hiding this comment.
This method accepts a number. There is nothing that prevents it from being reused.
So even with pidfd (and also on Windows) there is a potential pid reuse issue.
There was a problem hiding this comment.
Right, the method call itself has potential PID reuse issue in all cases.
The returned handle has the PID reuse issue only in the last case. It is non-intuitive that an instance of type called SafeProcessHandle can end up pointing to completely different process than it was pointing to originally. Maybe this should be documented on the type and not here.
| // - powershell is not available on Nano. We can't always use it. | ||
| // - ping seems to be a workaround, but it's simple and work everywhere. The arguments are set to make it sleep for approximately 10 seconds. | ||
| private static ProcessStartOptions CreateTenSecondSleep() => OperatingSystem.IsWindows() | ||
| ? new("ping") { Arguments = { "127.0.0.1", "-n", "11" } } |
There was a problem hiding this comment.
0xc0000142 is STATUS_DLL_INIT_FAILED.
How does missing ReEnableCtrlHandlerIfNeeded lead to this error?
I would expect that missing ReEnableCtrlHandlerIfNeeded will make it impossible to send the signal to the process. I do not see how it can lead to STATUS_DLL_INIT_FAILED.
| /// <remarks> | ||
| /// On Unix, sends SIGKILL to all processes in the process group. | ||
| /// On Windows, requires the process to have been started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/>=true. | ||
| /// Terminates all processes in the job object. If the process was not started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/>=true, |
There was a problem hiding this comment.
Remote executor allows you to start the process yourself using new RemoteInvokeOptions { Start = false; }. Number of the existing System.Diagnostics.Process tests do that. Would that work?
| if (jobHandle.IsInvalid) | ||
| { | ||
| jobHandle.Dispose(); | ||
| throw new Win32Exception(); |
There was a problem hiding this comment.
This constructor uses Marshal.GetLastWin32Error to fetch the last error. The last error can be overwritten by jobHandle.Dispose call that often leads to the famous "Error: Operation completed successfully.".
This needs to save the last error before calling Dispose.
| ref limitInfo, | ||
| (uint)sizeof(Interop.Kernel32.JOBOBJECT_EXTENDED_LIMIT_INFORMATION))) | ||
| { | ||
| jobHandle.Dispose(); |
| if (addedRef) | ||
| { | ||
| handlesToRelease[handleIndex++] = handle; | ||
| } |
There was a problem hiding this comment.
| if (addedRef) | |
| { | |
| handlesToRelease[handleIndex++] = handle; | |
| } | |
| handlesToRelease[handleIndex++] = handle; |
Unnecessary. addedRef is always true on return from DangerousAddRef (the argument is left over from thread abort hardeing that we do not care about anymore)
| { | ||
| if (!Interop.Kernel32.SetHandleInformation( | ||
| handle, | ||
| Interop.Kernel32.HandleFlags.HANDLE_FLAG_INHERIT, |
There was a problem hiding this comment.
This will make these handles irreversibly inheritable that has race condition with Process.Start inheriting all handles by default. How are people expected to write reliable code against InheritedHandles on Windows if they do not own everything that can possibly launch new processes in the process? Do we need to document this gotcha?
| /// </summary> | ||
| /// <param name="signal">The signal to send.</param> | ||
| /// <exception cref="InvalidOperationException">Thrown when the handle is invalid.</exception> | ||
| /// <exception cref="ArgumentException">Thrown when the signal is not supported on the current platform.</exception> |
There was a problem hiding this comment.
PosixSignalRegistration.Register throws PlatformNotSupportedException for signals not supported on the current platform. Should this do the same?
| /// <para> | ||
| /// On Windows, signals can only be sent to processes that have a console attached | ||
| /// and were started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/> set to <see langword="true"/>. | ||
| /// The following signal mappings are used: | ||
| /// </para> | ||
| /// <list type="bullet"> | ||
| /// <item><description><see cref="PosixSignal.SIGINT"/> is mapped to <c>GenerateConsoleCtrlEvent(CTRL_C_EVENT)</c>.</description></item> | ||
| /// <item><description><see cref="PosixSignal.SIGQUIT"/> is mapped to <c>GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT)</c>.</description></item> | ||
| /// <item><description><see cref="PosixSignal.SIGKILL"/> is mapped to <see cref="Kill"/>.</description></item> | ||
| /// </list> | ||
| /// <para> | ||
| /// On Unix/Linux, all signals defined in PosixSignal and raw signal numbers are supported. | ||
| /// </para> |
There was a problem hiding this comment.
| /// <para> | |
| /// On Windows, signals can only be sent to processes that have a console attached | |
| /// and were started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/> set to <see langword="true"/>. | |
| /// The following signal mappings are used: | |
| /// </para> | |
| /// <list type="bullet"> | |
| /// <item><description><see cref="PosixSignal.SIGINT"/> is mapped to <c>GenerateConsoleCtrlEvent(CTRL_C_EVENT)</c>.</description></item> | |
| /// <item><description><see cref="PosixSignal.SIGQUIT"/> is mapped to <c>GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT)</c>.</description></item> | |
| /// <item><description><see cref="PosixSignal.SIGKILL"/> is mapped to <see cref="Kill"/>.</description></item> | |
| /// </list> | |
| /// <para> | |
| /// On Unix/Linux, all signals defined in PosixSignal and raw signal numbers are supported. | |
| /// </para> | |
| /// <para> | |
| /// On Unix, all signals defined in PosixSignal are supported. Raw values can be provided for signal by casting them to PosixSignal. | |
| /// </para> | |
| /// <para> | |
| /// On Windows, this API provides best-effort simulation of Unix signals. | |
| /// <see cref="PosixSignal.SIGINT"/> and <see cref="PosixSignal.SIGQUIT"/> can only be sent to a process | |
| /// that is a root process of a process group, for example started with <see cref="ProcessStartOptions.CreateNewProcessGroup"/> or <see cref="ProcessStartInfo.CreateNewProcessGroup"/> options. All processes in the group that share the same console as the process group root process receive the signal. | |
| /// The following signal mappings are used: | |
| /// </para> | |
| /// <list type="bullet"> | |
| /// <item><description><see cref="PosixSignal.SIGINT"/> is mapped to <c>GenerateConsoleCtrlEvent(CTRL_C_EVENT)</c>.</description> The root process of the process group must <see href="https://learn.microsoft.com/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw#remarks">re-enable of this handler in order to receinve this signal.</see>. | |
| </item> | |
| /// <item><description><see cref="PosixSignal.SIGQUIT"/> is mapped to <c>GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT)</c>.</description></item> | |
| /// <item><description><see cref="PosixSignal.SIGKILL"/> is mapped to <see cref="Kill"/>.</description></item> | |
| /// </list> |
- We should lead with the Unix description since it is much simpler.
- Describe how to pass in raw values
- Describe more of the limitations
Similar for SignalProcessGroup
| } | ||
| finally | ||
| { | ||
| nullHandle?.Dispose(); |
There was a problem hiding this comment.
I wonder if it wouldn't make sense to dispose all handles in this finally block that are intended for the child process?
I think it helps on the caller side since usually you won't have a block that sits around just the Start call, so with a using these handles will be open longer than they should be.
@adamsitnik thoughts?


Description
Implements the approved API surface for
SafeProcessHandle(#123380): process lifecycle management viaStart,Open,Kill,Signal,SignalProcessGroup, andWaitForExitoverloads. Windows implementation complete; Unix stubs throwNotImplementedException(separate PR).Public API
Open(int processId)— opens existing process handleStart(ProcessStartOptions, SafeFileHandle?, SafeFileHandle?, SafeFileHandle?)— create process with explicit stdio handlesKill()— terminate processSignal(PosixSignal)/SignalProcessGroup(PosixSignal)— send signal; accepts raw signal numbers likePosixSignalRegistration.CreateWaitForExit(),TryWaitForExit(TimeSpan, out ProcessExitStatus?),WaitForExitOrKillOnTimeout(TimeSpan)— synchronous waitWaitForExitAsync(CancellationToken),WaitForExitOrKillOnCancellationAsync(CancellationToken)— async waitInternal for now (pending follow-up):
ProcessId,StartSuspended,Resume,KillProcessGroup.Windows implementation
STARTUPINFOEX+PROC_THREAD_ATTRIBUTE_HANDLE_LISTfor explicit handle inheritanceKillOnParentExitand process group termination (best-effort emulation of Unix process groups)DangerousAddRef/DangerousReleaseonInheritedHandlesinPrepareHandleAllowListand onthisinKillCoreto prevent use-after-free races with concurrentDisposeInterlocked.Exchangefor_threadHandleand_processGroupJobHandleinReleaseHandle;Volatile.Readfor_processGroupJobHandleaccess elsewhereWaitForSingleObject(SafeProcessHandle, ...)directly — no handle duplication viaProcessWaitHandleProcessWaitHandleforRegisterWaitForSingleObjectNativeMemory.Allocwith two-arg overflow-safe version; typed pointers (IntPtr*,void*)finally; error check immediately afterCreateProcessArchitecture —
ProcessUtilsas shared dependencyProcessUtils.csholdss_processStartLock(ReaderWriterLockSlim) — avoidsProcess↔SafeProcessHandledependency cycleProcessUtils.Windows.csholdsBuildArgs(string, IList<string>?)andGetEnvironmentVariablesBlockProcess.Windows.csacquires write lock;SafeProcessHandle.Windows.csacquires read lockInterop additions
Interop.JobObjects.cs,Interop.ProcThreadAttributeList.cs,Interop.ConsoleCtrl.cs(consolidated),Interop.HandleInformation.cs(extended),Interop.ResumeThread_IntPtr.cs,Interop.WaitForSingleObject_SafeProcessHandle.csTests
SafeProcessHandleTests.cs— start/kill/wait/timeout/signal tests with Nano Server support (pingfallback); signal tests as[Theory]with[InlineData]💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.