File descriptors passed to Command::stdout/err/in are not closed after dup2

[i509VCB]

File descriptors passed to Command::stdout/err/in are not closed after dup2 to replace stdout/stderr/stdin. This means the file descriptors passed to Command::stdout/err/in are leaked in the child process.

Example:

use std::fs::File;
use std::process::{Command, Stdio};

fn main() -> std::io::Result<()> {
    let file = File::create("output.txt")?;

    let mut child = Command::new("echo")
        .arg("Hello from the child process!")
        .stdout(Stdio::from(file))
        .spawn()?;

    let status = child.wait()?;

    println!("Child exited with status: {}", status);

    Ok(())
}

strace -f output (full output is attached):

3903339 dup2(3, 1)                      = 1
3903339 rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
3903339 execve("/home/i509vcb/.cargo/bin/echo", ["echo", "Hello from the child process!"], 0x7fffc66d0a58 /* 75 vars */) = -1 ENOENT (No such file or directory)
3903339 execve("/home/i509vcb/.local/bin/echo", ["echo", "Hello from the child process!"], 0x7fffc66d0a58 /* 75 vars */) = -1 ENOENT (No such file or directory)
3903339 execve("/home/i509vcb/bin/echo", ["echo", "Hello from the child process!"], 0x7fffc66d0a58 /* 75 vars */) = -1 ENOENT (No such file or directory)
3903339 execve("/usr/lib64/ccache/echo", ["echo", "Hello from the child process!"], 0x7fffc66d0a58 /* 75 vars */) = -1 ENOENT (No such file or directory)
3903339 execve("/usr/local/bin/echo", ["echo", "Hello from the child process!"], 0x7fffc66d0a58 /* 75 vars */) = -1 ENOENT (No such file or directory)
3903339 execve("/usr/bin/echo", ["echo", "Hello from the child process!"], 0x7fffc66d0a58 /* 75 vars */ <unfinished ...>
3903338 <... clone3 resumed>)           = 3903339

This will leak fd 3 on the child process. Ideally do_exec would close the stdio file descriptors which were overriden after dup2 on the child:

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

Line 282 in 2300c2a

) -> Result<!, io::Error> {

strace.txt

[bjorn3]

I think there is just a drop(stdio) missing at

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

Line 294 in 2300c2a

[uellenberg]

There's an interesting comment over that method:

// As an example of not having malloc/free traffic, we don't close
// this file descriptor by dropping the FileDesc (which contains an
// allocation). Instead we just close it manually. This will never
// have the drop glue anyway because this code never returns (the
// child will either exec() or invoke libc::exit)

(full comment)

// And at this point we've reached a special time in the life of the
// child. The child must now be considered hamstrung and unable to
// do anything other than syscalls really. Consider the following
// scenario:
//
//      1. Thread A of process 1 grabs the malloc() mutex
//      2. Thread B of process 1 forks(), creating thread C
//      3. Thread C of process 2 then attempts to malloc()
//      4. The memory of process 2 is the same as the memory of
//         process 1, so the mutex is locked.
//
// This situation looks a lot like deadlock, right? It turns out
// that this is what pthread_atfork() takes care of, which is
// presumably implemented across platforms. The first thing that
// threads to *before* forking is to do things like grab the malloc
// mutex, and then after the fork they unlock it.
//
// Despite this information, libnative's spawn has been witnessed to
// deadlock on both macOS and FreeBSD. I'm not entirely sure why, but
// all collected backtraces point at malloc/free traffic in the
// child spawned process.
//
// For this reason, the block of code below should contain 0
// invocations of either malloc of free (or their related friends).
//
// As an example of not having malloc/free traffic, we don't close
// this file descriptor by dropping the FileDesc (which contains an
// allocation). Instead we just close it manually. This will never
// have the drop glue anyway because this code never returns (the
// child will either exec() or invoke libc::exit)

Which was true 10 years ago, but the manual closing was removed in 33a2191. The stdio struct doesn't actually contain any allocations, so is safe to drop here, but it makes me a bit uneasy to add it to a method which explicitly isn't allowed to free (on the probably unlikely chance that an allocation is added to stdio's type).

Some of this definitely should be re-evaluated - who knows whether the deadlocks are still an issue a decade later. There are also public-facing APIs like pre_exec which run under this context and *should* be beholden to the same rules that are documented here, but nothing in their safety requirements mentions that malloc/free are off-limits (just that they may be unavailable).

In any case, that doesn't affect this issue here. We can either call drop on it, or do something more explicit like. Not sure what's more appropriate given the above.

[chenyukang]

how about call close:

         if let Some(fd) = stdio.stdin.fd() {
             cvt_r(|| libc::dup2(fd, libc::STDIN_FILENO))?;
+            if fd != libc::STDIN_FILENO {
+                cvt_r(|| libc::close(fd))?;
+            }
         }
         if let Some(fd) = stdio.stdout.fd() {
             cvt_r(|| libc::dup2(fd, libc::STDOUT_FILENO))?;
+            if fd != libc::STDOUT_FILENO {
+                cvt_r(|| libc::close(fd))?;
+            }
         }
         if let Some(fd) = stdio.stderr.fd() {
             cvt_r(|| libc::dup2(fd, libc::STDERR_FILENO))?;
+            if fd != libc::STDERR_FILENO {
+                cvt_r(|| libc::close(fd))?;
+            }
         }

EDIT: this change will make this single test failed:

thread 'main' (412153) panicked at /home/gh-chenyukang/rust/tests/ui/command/command-pre-exec.rs:66:14:
called `Result::unwrap_err()` on an `Ok` value: Output { status: ExitStatus(unix_wait_status(6)), stdout: "", stderr: "fatal runtime error: IO Safety violation: owned file descriptor already closed, aborting\n" }

it's better to drop there.

[joboet]

All file descriptors created by std are marked with CLOEXEC, so the exec call will close them automatically. Thus closing e.g. the file descriptor for the file here just results in unnecessary additional syscalls.