From 10cb7cb0690d64630629257f25c23966f8758d33 Mon Sep 17 00:00:00 2001 From: Simon Chopin Date: Tue, 8 Sep 2026 12:05:11 +0200 Subject: [PATCH] daemonize: use an allow-list approach to inherited FDs Our current approach is to try and close the jobserver file descriptors inherited from make, using heuristics based on parsing the MAKEFLAGS searching for explicit FD numbers. However, in some circumstances, make will dup() those file descriptors internally, and we'll inherit and leak those duplicated FDs, which can lead to build script deadlocking. This happens to me frequently when working on Firefox on my macOS machine: when I run `./mach lint --outgoing` with substantial Rust work, once the clippy run ends, `make` idles waiting for job tokens to be handed back through those open file descriptors, until the daemonized server finally finishes through its idle timeout, closing their end of the pipes. So, instead of trying to pinpoint what must absolutely be closed, we take the opposite approach, and assume that most file descriptors don't need to be inherited when spawning a daemon. --- src/bin/sccache-dist/main.rs | 2 +- src/commands.rs | 12 ++++++-- src/jobserver.rs | 41 ++------------------------- src/util.rs | 55 ++++++++++++++++++++++++++++++++---- 4 files changed, 63 insertions(+), 47 deletions(-) diff --git a/src/bin/sccache-dist/main.rs b/src/bin/sccache-dist/main.rs index eb85d3125d..8a19d3362c 100644 --- a/src/bin/sccache-dist/main.rs +++ b/src/bin/sccache-dist/main.rs @@ -219,7 +219,7 @@ fn run(command: Command) -> Result { } }; - daemonize()?; + daemonize(&[])?; let scheduler = Scheduler::new(); let http_scheduler = dist::http::Scheduler::new( public_addr, diff --git a/src/commands.rs b/src/commands.rs index 5ea8b949ce..58c04cccdd 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -29,8 +29,12 @@ use log::Level::Trace; use std::env; use std::ffi::{OsStr, OsString}; use std::io::{self, IsTerminal, Write}; +#[cfg(not(windows))] +use std::os::fd::AsRawFd; #[cfg(unix)] use std::os::unix::process::ExitStatusExt; +#[cfg(windows)] +use std::os::windows::io::AsRawHandle; use std::path::Path; use std::process; use std::sync::Arc; @@ -758,12 +762,16 @@ pub fn run_command(cmd: Command) -> Result { trace!("Command::InternalStartServer"); if env::var("SCCACHE_ERROR_LOG").is_ok() { let f = create_error_log()?; + #[cfg(not(windows))] + let preserve = [f.as_raw_fd()]; + #[cfg(windows)] + let preserve = [f.as_raw_handle()]; // Can't report failure here, we're already daemonized. - daemonize()?; + daemonize(&preserve)?; redirect_error_log(f)?; } else { // We aren't asking for a log file - daemonize()?; + daemonize(&[])?; } server::start_server(config, &get_addr())?; } diff --git a/src/jobserver.rs b/src/jobserver.rs index a0cb472ef4..65ac3848f1 100644 --- a/src/jobserver.rs +++ b/src/jobserver.rs @@ -29,44 +29,9 @@ use crate::errors::*; // What we do instead is to arbitrary use our own jobserver. // Unfortunately, that doesn't absolve us from having to deal with the original // jobserver, because make may give us file descriptors to its pipes, and the -// simple fact of keeping them open can block it. -// So if it does give us those file descriptors, close the preemptively. -// -// unsafe because it can use the wrong fds. -#[cfg(not(windows))] -pub unsafe fn discard_inherited_jobserver() { - if let Some(value) = ["CARGO_MAKEFLAGS", "MAKEFLAGS", "MFLAGS"] - .into_iter() - .find_map(|env| std::env::var(env).ok()) - && let Some(auth) = value.rsplit(' ').find_map(|arg| { - arg.strip_prefix("--jobserver-auth=") - .or_else(|| arg.strip_prefix("--jobserver-fds=")) - }) - && !auth.starts_with("fifo:") - { - let mut parts = auth.splitn(2, ','); - let read = parts.next().unwrap(); - let write = match parts.next() { - Some(w) => w, - None => return, - }; - let read = read.parse().unwrap(); - let write = write.parse().unwrap(); - if read < 0 || write < 0 { - return; - } - unsafe { - if libc::fcntl(read, libc::F_GETFD) == -1 { - return; - } - if libc::fcntl(write, libc::F_GETFD) == -1 { - return; - } - libc::close(read); - libc::close(write); - } - } -} +// simple fact of keeping them open can block it. That is handled by closing +// every inherited descriptor when the server detaches; see +// `util::close_inherited_fds`. #[derive(Clone)] pub struct Client { diff --git a/src/util.rs b/src/util.rs index 408ae8a553..c359e4258e 100644 --- a/src/util.rs +++ b/src/util.rs @@ -897,10 +897,55 @@ impl Hasher for HashToDigest<'_> { } } +/// Close every file descriptor we inherited from whoever spawned us, keeping +/// stdin/out/err and anything in `preserve`. +#[cfg(not(windows))] +fn close_inherited_fds(preserve: &[std::os::unix::io::RawFd]) { + use std::os::unix::io::RawFd; + + let keep = |fd: RawFd| fd <= libc::STDERR_FILENO || preserve.contains(&fd); + + // macOS/BSD: /dev/fd; Linux: /proc/self/fd + let listing = std::fs::read_dir("/dev/fd").or_else(|_| std::fs::read_dir("/proc/self/fd")); + let victims: Option> = listing.ok().map(|entries| { + entries + .flatten() + .filter_map(|e| e.file_name().to_str()?.parse::().ok()) + .filter(|fd| !keep(*fd)) + .collect() + }); + + match victims { + Some(fds) => { + for fd in fds { + unsafe { libc::close(fd) }; + } + } + // No fd directory to enumerate, so fall back to sweeping the whole + // range. Bounded by the soft limit rather than the hard one to keep + // this from turning into a million syscalls. + None => { + let max = unsafe { libc::sysconf(libc::_SC_OPEN_MAX) }; + let max = if max < 0 { + 4096 + } else { + max.min(65536) as RawFd + }; + for fd in (libc::STDERR_FILENO + 1)..max { + if !keep(fd) { + unsafe { libc::close(fd) }; + } + } + } + } +} + /// Pipe `cmd`'s stdio to `/dev/null`, unless a specific env var is set. +/// +/// `preserve_fds` lists descriptors the caller opened before daemonizing and +/// still needs afterwards; everything else inherited is closed. #[cfg(not(windows))] -pub fn daemonize() -> Result<()> { - use crate::jobserver::discard_inherited_jobserver; +pub fn daemonize(preserve_fds: &[std::os::unix::io::RawFd]) -> Result<()> { use daemonix::Daemonize; use std::env; use std::mem; @@ -912,9 +957,7 @@ pub fn daemonize() -> Result<()> { } } - unsafe { - discard_inherited_jobserver(); - } + close_inherited_fds(preserve_fds); static mut PREV_SIGSEGV: *mut libc::sigaction = std::ptr::null_mut(); static mut PREV_SIGBUS: *mut libc::sigaction = std::ptr::null_mut(); @@ -986,7 +1029,7 @@ pub fn daemonize() -> Result<()> { /// This is a no-op on Windows. #[cfg(windows)] -pub fn daemonize() -> Result<()> { +pub fn daemonize(_preserve_fds: &[std::os::windows::io::RawHandle]) -> Result<()> { Ok(()) }