Skip to content

Commit

Permalink
Close non-stdio file handles when daemonizing
Browse files Browse the repository at this point in the history
Otherwise, when the compiler wrapper spawns the sccache server, the
server may end up with unintended file descriptors, which can lead to
unexpected problems.

This is particularly problematic if any of those file descriptors that
accidentally end up in the server process is a pipe, as the pipe will
only be closed when all the processes with that file descriptor close it
or exit.

This was causing sccache to hang ninja, as ninja uses the EOF of a pipe
given to the subprocess to detect when that subprocess has exited:
ninja-build/ninja#2444 (comment)

This patch adds a dependency on the
[close_fds](https://crates.io/crates/close_fds) crate, which
automatically chooses an appropriate mechanism to close a range of file
descriptors. On Linux 5.9+ that mechanism will be libc::close_range().

Fixes mozilla#2313
  • Loading branch information
ntrrgc committed Jan 9, 2025
1 parent 9ca8beb commit 2c22e92
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 23 deletions.
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ rouille = { version = "3.6", optional = true, default-features = false, features
] }
syslog = { version = "6", optional = true }
version-compare = { version = "0.1.1", optional = true }
close_fds = "0.3.2"

[dev-dependencies]
assert_cmd = "2.0.13"
Expand Down
2 changes: 1 addition & 1 deletion src/bin/sccache-dist/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ fn run(command: Command) -> Result<i32> {
}
};

daemonize()?;
daemonize(None)?;
let scheduler = Scheduler::new();
let http_scheduler = dist::http::Scheduler::new(
public_addr,
Expand Down
24 changes: 4 additions & 20 deletions src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,16 +126,6 @@ fn run_server_process(startup_timeout: Option<Duration>) -> Result<ServerStartup
})
}

#[cfg(not(windows))]
fn redirect_stderr(f: File) {
use libc::dup2;
use std::os::unix::io::IntoRawFd;
// Ignore errors here.
unsafe {
dup2(f.into_raw_fd(), 2);
}
}

#[cfg(windows)]
fn redirect_stderr(f: File) {
use std::os::windows::io::IntoRawHandle;
Expand Down Expand Up @@ -165,13 +155,6 @@ fn create_error_log() -> Result<File> {
Ok(f)
}

/// If `SCCACHE_ERROR_LOG` is set, redirect stderr to it.
fn redirect_error_log(f: File) -> Result<()> {
debug!("redirecting stderr into {:?}", f);
redirect_stderr(f);
Ok(())
}

/// Re-execute the current executable as a background server.
#[cfg(windows)]
fn run_server_process(startup_timeout: Option<Duration>) -> Result<ServerStartup> {
Expand Down Expand Up @@ -654,14 +637,15 @@ pub fn run_command(cmd: Command) -> Result<i32> {
}
Command::InternalStartServer => {
trace!("Command::InternalStartServer");
// If `SCCACHE_ERROR_LOG` is set, redirect stderr to it.
if env::var("SCCACHE_ERROR_LOG").is_ok() {
let f = create_error_log()?;
debug!("redirecting stderr into {:?}", f);
// Can't report failure here, we're already daemonized.
daemonize()?;
redirect_error_log(f)?;
daemonize(Some(f))?;
} else {
// We aren't asking for a log file
daemonize()?;
daemonize(None)?;
}
server::start_server(config, &get_addr())?;
}
Expand Down
13 changes: 11 additions & 2 deletions src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ impl<'a> Hasher for HashToDigest<'a> {

/// Pipe `cmd`'s stdio to `/dev/null`, unless a specific env var is set.
#[cfg(not(windows))]
pub fn daemonize() -> Result<()> {
pub fn daemonize(log_file: Option<File>) -> Result<()> {
use crate::jobserver::discard_inherited_jobserver;
use daemonize::Daemonize;
use std::env;
Expand All @@ -850,7 +850,16 @@ pub fn daemonize() -> Result<()> {
match env::var("SCCACHE_NO_DAEMON") {
Ok(ref val) if val == "1" => {}
_ => {
Daemonize::new().start().context("failed to daemonize")?;
Daemonize::new()
.stderr(log_file
.map(|f| f.into_parts().0.into())
.unwrap_or_else(|| daemonize::Stdio::devnull())
)
.start().context("failed to daemonize")?;
// Be extra-zealous and clase all non-stdio file descriptors.
unsafe {
close_fds::close_open_fds(3, &[]);
}
}
}

Expand Down

0 comments on commit 2c22e92

Please sign in to comment.