Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std::process::Command::spawn() leaks file descriptors on unix platforms that don't have pipe2() #95584

Open
nico opened this issue Apr 2, 2022 · 4 comments
Labels
A-process Area: std::process and std::env C-bug Category: This is a bug. O-unix Operating system: Unix-like T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@nico
Copy link

nico commented Apr 2, 2022

The n2 project is a reimplementation of the ninja build system. As such, it launches many subprocesses. For every subprocess, it spawns a thread that runs std::process::Command::spawn().

On macOS, ninja -j250 runs fine, while n2 -j250 runs out of file descriptors (n2 bug report: evmar/n2#14). It looks like this is due to an FD leak in rust's standard library.

Command::spawn() in the rust stdlib unconditionally calls anon_pipe here:

let (input, output) = sys::pipe::anon_pipe()?;

anon_pipe on Linux calls pipe2 to set CLOEXEC on the pipe atomically:

// to use the `pipe2` syscall. This was added to Linux in 2.6.27, glibc 2.9

But macOS has no pipe2, so here the stdlib instead calls pipe() followed by set_cloexec:

cvt(libc::pipe(fds.as_mut_ptr()))?;

This means there's a window where the pipe is created but cloexec isn't set on the pipe's FDs yet. If a different thread forks in that window, the pipe's fds get leaked.

The FD leak went away when putting std::process::Command::spawn() behind a mutex, so it does seem like this race is in fact the cause.


On the n2 issue, @bnoordhuis remarks "Just throwing it out there: libstd uses posix_spawn() under the right conditions (instead of fork + execve) and macOS has a POSIX_SPAWN_CLOEXEC_DEFAULT attribute that does what its name suggests. Teaching libstd about it probably isn't too hard." This might be a possible venue for a fix on macOS, but it's possible to imagine a program that depends on some FDs staying open, and I don't know if there's a way to make POSIX_SPAWN_CLOEXEC_DEFAULT apply only to the 2 fds returned by pipe().

@nico nico added the C-bug Category: This is a bug. label Apr 2, 2022
@ehuss
Copy link
Contributor

ehuss commented Apr 2, 2022

We ran into this issue with Cargo. There's more discussion at rust-lang/cargo#7858 (comment) and rust-lang/cargo#6344 and in #24237 where O_CLOEXEC support was added. Unfortunately I was never able to find a solution.

@cuviper
Copy link
Member

cuviper commented Apr 8, 2022

Command::spawn() in the rust stdlib unconditionally calls anon_pipe here:

let (input, output) = sys::pipe::anon_pipe()?;

Are you sure it's that one? This is right after the attempt to use posix_spawn, only for when we need the manual fork/exec. So if that's your leaking pipe, POSIX_SPAWN_CLOEXEC_DEFAULT would already be out of the picture.

The other place that calls anon_pipe is Stdio::to_child_stdio() for the MakePipe variant:

Stdio::MakePipe => {
let (reader, writer) = pipe::anon_pipe()?;
let (ours, theirs) = if readable { (writer, reader) } else { (reader, writer) };
Ok((ChildStdio::Owned(theirs.into_inner()), Some(ours)))
}

That's created by Stdio::piped() passed to one of the Command handles, or by Command::output(). It looks like n2 does use Stdio::piped(), now guarded by its TASK_MUTEX.lock().

@nico
Copy link
Author

nico commented Apr 9, 2022

It's possible it's that other call.

When I filed this bug, n2 used .output(), but that internally probably uses piped(): https://github.com/evmar/n2/blame/5aa3a466c30b97fe40718882041440ff5455caeb/src/task.rs#L91

@ChrisDenton ChrisDenton added the needs-triage-legacy Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged. label Jul 16, 2023
@workingjubilee workingjubilee added the A-process Area: std::process and std::env label Jul 22, 2023
@ChrisDenton ChrisDenton added T-libs Relevant to the library team, which will review and decide on the PR/issue. O-unix Operating system: Unix-like and removed needs-triage-legacy Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged. labels Nov 6, 2023
@evmar
Copy link

evmar commented Jan 29, 2024

BTW, I think I was able to work around this in my application by using a MacOS-specific flag on MacOS and pipe2 elsewhere:

evmar/n2@12652a1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-process Area: std::process and std::env C-bug Category: This is a bug. O-unix Operating system: Unix-like T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants