windows: when creating process with redirected stdio, make parent's std handles not inheritable

[ikostia]

Currently, if this happens:

use std::process::Command;
...
Command::new("child.exe")
        .stdin(Stdio::null())
        .stdout(Stdio::null())
        .stderr(Stdio::null())
        .spawn()

the child.exe accidentally inherits stdin, stdout and stderr handles of the parent process. This can be bad, because the objects these handles point to (say pipes), may be watched by some third process.

Since at the point of creating a child process we know that it does not need our std handles (because it's explicitly being created with Stdio::null()), we can temporarily disable handle inheritance on these handles.

[retep998]

Mucking about with enabling and disabling inheritance on handles is inherently racy and a bad idea.

We really just need to implement the solution in https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873

[ikostia]

@retep998: you are talking about #38227, right?

The problem I personally have with that approach is that it may be very hard to know which handles are intended to be inheritable and thus need to be passed in STARTUPINFOEX.

I will ask a question under that issue to keep the discussion centralized.

[DemiMarie]

@ikostia Only handles expressly passed to a child process should be inherited. The rest should not. This also applies to other platforms.

[retep998]

This can be trivially solved if #38227 were implemented.

[JamesMc86]

I wanted to add a failure mode which made this important to me.

I was trying to use command::spawn to start a process that outlives mine. It works from console but if stdout is piped then my process is held open until the spawned process is complete - breaking the expected behaviour of spawn.

Calling the following function operates as a workaround for now:

#[cfg(target_os = "windows")]
/// Disables inheritance on the inout pipe handles.
fn disable_handle_inheritance() {
    use windows::Win32::Foundation::{SetHandleInformation, HANDLE_FLAGS, HANDLE_FLAG_INHERIT};
    use windows::Win32::System::Console::{
        GetStdHandle, STD_ERROR_HANDLE, STD_INPUT_HANDLE, STD_OUTPUT_HANDLE,
    };

    unsafe {
        let std_err = GetStdHandle(STD_ERROR_HANDLE);
        let std_in = GetStdHandle(STD_INPUT_HANDLE);
        let std_out = GetStdHandle(STD_OUTPUT_HANDLE);

        for handle in [std_err, std_in, std_out] {
            SetHandleInformation(handle, HANDLE_FLAG_INHERIT.0, HANDLE_FLAGS(0));
        }
    }
}

[jsturtevant]

I was trying to use command::spawn to start a process that outlives mine. It works from console but if stdout is piped then my process is held open until the spawned process is complete - breaking the expected behaviour of spawn.

I ran into this as well. Adding another example where this is an issue.

In containerd a shim process is created and run to completion listening to the stdout. That process is often implemented to be short lived and responsible for some set up, and then it should create a new long lived process. Because containerd is listening to STDOUT this caused containerd to hang when spawning the long lived process. Even though the short live process completed, the inherited handle was causing a hang until the new child process completed.

The work around above did help. The issue seems to come from always passing true for bInheritHandles to createprocess:

rust/library/std/src/sys/windows/process.rs

Lines 326 to 331 in 439292b

	cvt(c::CreateProcessW(
	program.as_ptr(),
	cmd_str.as_mut_ptr(),
	ptr::null_mut(),
	ptr::null_mut(),
	c::TRUE,