Process: SafeChildProcessHandle and option bag

[adamsitnik]

Background and motivation

System.Diagnostics.Process has plenty of design, usability, performance and reliablity issues. The API can not be 100% fixed without breaking changes, so I would like to introduce a set of new types related to process execution.

The foundation of the new process APIs is a low-level API that represents SafeHandle for child processes. It allow users to start processes with fine-grained control over STD input/output/error, environment, working directory, and other parameters that can be implemented on every platform supported by .NET.

API Proposal

The first part of the proposal is an option bag type that represents process start options. It's used by the low-level process creation API. It will be also used by higher-level APIs (proposed in #123959) that provide more user-friendly abstractions over typical process scenarios (execute, discard output, get output lines etc). Those high level APIs are currently WIP and will be proposed later.

namespace System.INeedName;

public sealed class ProcessStartOptions
{
    public string FileName { get; }
    public IList<string> Arguments { get; } // if it was List, users  could use .AddRange
    public IDictionary<string, string?> Environment { get; }
    public DirectoryInfo? WorkingDirectory { get; set; }

    public IList<SafeHandle> InheritedHandles { get; } // NEW (https://github.com/dotnet/runtime/issues/13943)
    public bool KillOnParentDeath { get; set; } // NEW (https://github.com/dotnet/runtime/issues/101985)
    public bool CreateNewProcessGroup { get; set; } // NEW for Unix, so far we had it only for Windows

    public ProcessStartOptions(string fileName);

    /// <summary>
    /// Resolves the given file name to an absolute path and creates a new ProcessStartOptions instance.
    /// </summary>
    public static ProcessStartOptions ResolvePath(string fileName); // NEW
}

The second part of the proposal is a low-level API that allows starting a child process with the given options and STD input/output/error handles.

The most important aspects:

• WaitForExit and WaitForExitAsync do not depend on reading STD OUT/ERR until EOF (this is very error prone, see #51277).
• Every new process inherits only STD IN/OUT/ERR. Users can specify additional handles to inherit via an allow-list.

namespace Microsoft.Win32.SafeHandles; // should we still follow that convention?

public class SafeChildProcessHandle : SafeHandle
{
    public int ProcessId { get; }

    public SafeChildProcessHandle(IntPtr existingHandle, int processId, bool ownsHandle);

    public static SafeChildProcessHandle Open(int processId);

    public static SafeChildProcessHandle Start(ProcessStartOptions options, SafeFileHandle? input, SafeFileHandle? output, SafeFileHandle? error); // NEW (https://github.com/dotnet/runtime/issues/28838)
    public static SafeChildProcessHandle StartSuspended(ProcessStartOptions options, SafeFileHandle? input, SafeFileHandle? output, SafeFileHandle? error); // NEW (https://github.com/dotnet/runtime/issues/94127)

    public bool Kill(bool entireProcessGroup = false);
    public void Resume(); // NEW (https://github.com/dotnet/runtime/issues/94127)

    public void SendSignal(PosixSignal signal, bool toEntireProcessGroup = false);  // NEW (https://github.com/dotnet/runtime/issues/59746)

    public ProcessExitStatus WaitForExit();
    public bool TryWaitForExit(TimeSpan timeout, out ProcessExitStatus exitStatus);
    public ProcessExitStatus WaitForExitOrKillOnTimeout(TimeSpan timeout);

    public Task<ProcessExitStatus> WaitForExitAsync(CancellationToken cancellationToken = default)
    public Task<ProcessExitStatus> WaitForExitOrKillOnCancellationAsync(CancellationToken cancellationToken)
}

We need an ability to send SIGKILL and SIGSTOP which so far were not part of the PosixSignal enum:

namespace System.Runtime.InteropServices;

public enum PosixSignal
{
    SIGTSTP = -10, // polite request to stop
+   SIGKILL = -11, // force termination
+   SIGSTOP = -12 // unconditional stop
}

And exit status to express processes that were signaled and/or terminated due to timeout or cancellation:

namespace System.INeedName;

public readonly struct ProcessExitStatus : IEquatable<ProcessExitStatus>
{
    internal ProcessExitStatus(int exitCode, bool cancelled, PosixSignal? signal = null);

    public int ExitCode { get; }
    public PosixSignal? Signal { get; }

    /// <summary>
    /// Gets a value indicating whether the process has been terminated due to timeout or cancellation.
    /// </summary>
    public bool Canceled { get; }

    public bool Equals(ProcessExitStatus other);
    public override bool Equals([NotNullWhen(true)] object? obj);
    public static bool operator ==(ProcessExitStatus left, ProcessExitStatus right);
    public static bool operator !=(ProcessExitStatus left, ProcessExitStatus right);
    public override int GetHashCode();
}

API Usage

This example demonstrates piping output from one process to another using anonymous pipes, providing empty input, redirecting to file and discarding error:

File.CreateAnonymousPipe(out SafeFileHandle readPipe, out SafeFileHandle writePipe);

using (readPipe)
using (writePipe)
{
    ProcessStartOptions producer = new("cmd")
    {
        Arguments = { "/c", "echo hello world & echo test line & echo another test" }
    };
    ProcessStartOptions consumer = new("findstr")
    {
        Arguments = { "test" }
    };

    // Start producer with output redirected to the write end of the pipe, no input and discarding error.
    using SafeChildProcessHandle producerHandle = SafeChildProcessHandle.Start(producer, input: null, output: writePipe, error: null);

    // Start consumer with input from the read end of the pipe, writing output to file and discarding error.
    using SafeFileHandle outputHandle = File.OpenHandle("output.txt", FileMode.Create, FileAccess.Write, FileShare.ReadWrite);
    using SafeProcessHandle consumerHandle = SafeProcessHandle.Start(consumer, readPipe, outputHandle, error: null);

    // Wait for both processes to complete
    await producerHandle.WaitForExitAsync();
    await consumerHandle.WaitForExitAsync();
}

Alternative Designs

Moving InheritedHandles out of the option bag

InheritedHandles provides a list of additional handles to be inherited by the child process. Each of these handles implements IDisposable, so it affects the lifetime management of the option bag. Since providing such handles is a niche scenario that we need for completness, we could move it out of the option bag and into the Start method:

public sealed class ProcessStartOptions
{
-   public IList<SafeHandle> InheritedHandles { get; } 
}

public class SafeChildProcessHandle : SafeHandle
{
-   public static SafeChildProcessHandle Start(ProcessStartOptions options, SafeFileHandle? input, SafeFileHandle? output, SafeFileHandle? error);
+   public static SafeChildProcessHandle Start(ProcessStartOptions options, SafeFileHandle? input, SafeFileHandle? output, SafeFileHandle? error, IList<SafeHandle>? inheritedHandles = null);
}

It would allow the users to cache the option bag (with a resolved exe path for example) without worrying about the lifetime of the inherited handles.

Risks

To be 100% safe from accidental handle inheritance, both old and new APIs need to share the global reader-writer lock on Windows. See the details for more:

Details

From https://learn.microsoft.com/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute:

PROC_THREAD_ATTRIBUTE_HANDLE_LIST: "These handles must be created as inheritable handles and must not include pseudo handles".

So even if we want to provide an explicit list of handles to inherit, we still need to create them as inheritable. It's fine as long as everyone uses PROC_THREAD_ATTRIBUTE_HANDLE_LIST to spawn processes, but if some code spawns a process without it, the inheritable handles may be inherited by accident. And if we introduce the new Process API, there is nothing that would stop the users from using the old API to spawn processes without PROC_THREAD_ATTRIBUTE_HANDLE_LIST.

To be 100% safe without breaking changes, the new and old API need to share the global reader-writer lock.

Due to explicit executable path resolution, if CreateProcess sys-call is one day changed to handle path resolution in a different way for security reasons, the managed layer will need to be updated as well.

[dotnet-policy-service]

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

[tmds]

WaitForExit and WaitForExitAsync do not depend on reading STD OUT/ERR until EOF (this is very error prone, see #51277).

When a process produces more output than the size of the pipe, wait for exit depends on reading that output because the writing process will block when the pipe is full (which causes it not to exit).

SafeFileHandle? input, SafeFileHandle? output, SafeFileHandle? error, IList? inheritedHandles = null

On Linux/Unix, sometimes applications use conventions about the file descriptors that are passed. For example, systemd starts passing additional file descriptors at 3 (see https://www.freedesktop.org/software/systemd/man/latest/sd_listen_fds.html). This API doesn't support this.

I want to make explicit it doesn't. I don't mean to say it must.

[tmds]

Task WaitForExitAsync(CancellationToken cancellationToken = default)

Combining waiting for the process exit, returning the exit code and not depending it on EOF make a good API.

I've implemented the same behavior in a RemoteProces.GetExitCodeAsync for Tmds.Ssh. That method also actively discards the output so that the process may complete. In this case SafeChildProcessHandle doesn't concern itself with the output streams, so killing the child when the CancellationToken triggers is a good way to go about it.

[tmds]

so killing the child when the CancellationToken triggers is a good way to go about it.

Or you might leave it to the user when the method throws OperationCanceledException. The user can then call Kill and may call WaitForExitAsync once more.

[tmds]

Task WaitForExitAsync(CancellationToken cancellationToken = default)

Adding a separate timeout arg can help encourage users to set a timeout (esp when they'd need to create a linked CancellationToken).

[adamsitnik]

This API doesn't support this.

Yes, the handles are inherited with their current values. The reason why I don't allow to provide new value like with dup2 on Linux is that Windows does not support it.

The DuplicateHandle sys-call just returns the new value.

The exact code that I am using:

https://github.com/adamsitnik/ProcessPlayground/blob/3191f94dde3a6c6da3a64857c8341b74e4eaee73/Library/SafeChildProcessHandle.Windows.cs#L276-L289

[jkotas]

The reason why I don't allow to provide new value like with dup2 on Linux is that Windows does not support it.

The typical pattern of passing the extra handles on Windows vs. Unix seems to be different. On Windows, one cannot depend on fixed handle value and the current handle value must be passed to the child process via some channel (e.g. via the command line). On Unix, it is common to depend on the fixed handle value, we have seen multiple instances of that.

Emulating the Windows pattern on Unix does not make the API super useful on Unix. Should we have Unix-specific API to support the typical Unix pattern?

[KalleOlaviNiemitalo]

On Windows, the Universal C Runtime Library (UCRT) has a convention for mapping file descriptor numbers to handles, via the WORD cbReserved2 and LPBYTE lpReserved2 members of STARTUPINFOW. This is implemented in exec\spawnv.cpp and lowio\ioinit.cpp. So if the .NET Runtime implemented the same convention, then it would be possible for a .NET program to start a C program as a child process and pass a specific handle to it as file descriptor 3, for example. However, further API design would be needed if you wanted .NET programs to be able to receive this kind of information from their parent processes.