Skip to content

Commit 8125bae

Browse files
ntrrgcsylvestre
authored andcommitted
Close non-stdio file handles when daemonizing
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 #2313
1 parent 733688f commit 8125bae

File tree

5 files changed

+46
-35
lines changed

5 files changed

+46
-35
lines changed

Cargo.lock

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@ test-case = "3.3.1"
134134
thirtyfour_sync = "0.27"
135135

136136
[target.'cfg(unix)'.dependencies]
137+
close_fds = "0.3.2"
137138
daemonize = "0.5"
138139

139140
[target.'cfg(not(target_os = "freebsd"))'.dependencies.libmount]

src/bin/sccache-dist/main.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ fn run(command: Command) -> Result<i32> {
212212
}
213213
};
214214

215-
daemonize()?;
215+
daemonize(None)?;
216216
let scheduler = Scheduler::new();
217217
let http_scheduler = dist::http::Scheduler::new(
218218
public_addr,

src/commands.rs

Lines changed: 4 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -126,26 +126,6 @@ fn run_server_process(startup_timeout: Option<Duration>) -> Result<ServerStartup
126126
})
127127
}
128128

129-
#[cfg(not(windows))]
130-
fn redirect_stderr(f: File) {
131-
use libc::dup2;
132-
use std::os::unix::io::IntoRawFd;
133-
// Ignore errors here.
134-
unsafe {
135-
dup2(f.into_raw_fd(), 2);
136-
}
137-
}
138-
139-
#[cfg(windows)]
140-
fn redirect_stderr(f: File) {
141-
use std::os::windows::io::IntoRawHandle;
142-
use windows_sys::Win32::System::Console::{SetStdHandle, STD_ERROR_HANDLE};
143-
// Ignore errors here.
144-
unsafe {
145-
SetStdHandle(STD_ERROR_HANDLE, f.into_raw_handle() as _);
146-
}
147-
}
148-
149129
/// Create the log file and return an error if cannot be created
150130
fn create_error_log() -> Result<File> {
151131
trace!("Create the log file");
@@ -165,13 +145,6 @@ fn create_error_log() -> Result<File> {
165145
Ok(f)
166146
}
167147

168-
/// If `SCCACHE_ERROR_LOG` is set, redirect stderr to it.
169-
fn redirect_error_log(f: File) -> Result<()> {
170-
debug!("redirecting stderr into {:?}", f);
171-
redirect_stderr(f);
172-
Ok(())
173-
}
174-
175148
/// Re-execute the current executable as a background server.
176149
#[cfg(windows)]
177150
fn run_server_process(startup_timeout: Option<Duration>) -> Result<ServerStartup> {
@@ -654,14 +627,15 @@ pub fn run_command(cmd: Command) -> Result<i32> {
654627
}
655628
Command::InternalStartServer => {
656629
trace!("Command::InternalStartServer");
630+
// If `SCCACHE_ERROR_LOG` is set, redirect stderr to it.
657631
if env::var("SCCACHE_ERROR_LOG").is_ok() {
658632
let f = create_error_log()?;
633+
debug!("redirecting stderr into {:?}", f);
659634
// Can't report failure here, we're already daemonized.
660-
daemonize()?;
661-
redirect_error_log(f)?;
635+
daemonize(Some(f))?;
662636
} else {
663637
// We aren't asking for a log file
664-
daemonize()?;
638+
daemonize(None)?;
665639
}
666640
server::start_server(config, &get_addr())?;
667641
}

src/util.rs

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -841,7 +841,7 @@ impl Hasher for HashToDigest<'_> {
841841

842842
/// Pipe `cmd`'s stdio to `/dev/null`, unless a specific env var is set.
843843
#[cfg(not(windows))]
844-
pub fn daemonize() -> Result<()> {
844+
pub fn daemonize(log_file: Option<File>) -> Result<()> {
845845
use crate::jobserver::discard_inherited_jobserver;
846846
use daemonize::Daemonize;
847847
use std::env;
@@ -850,7 +850,18 @@ pub fn daemonize() -> Result<()> {
850850
match env::var("SCCACHE_NO_DAEMON") {
851851
Ok(ref val) if val == "1" => {}
852852
_ => {
853-
Daemonize::new().start().context("failed to daemonize")?;
853+
Daemonize::new()
854+
.stderr(
855+
log_file
856+
.map(|f| f.into_parts().0.into())
857+
.unwrap_or_else(daemonize::Stdio::devnull),
858+
)
859+
.start()
860+
.context("failed to daemonize")?;
861+
// Be extra-zealous and clase all non-stdio file descriptors.
862+
unsafe {
863+
close_fds::close_open_fds(3, &[]);
864+
}
854865
}
855866
}
856867

@@ -926,12 +937,26 @@ pub fn daemonize() -> Result<()> {
926937
}
927938
}
928939

929-
/// This is a no-op on Windows.
940+
/// Daemonizing is a no-op on Windows, but we still must set the log file as
941+
/// stderr.
930942
#[cfg(windows)]
931-
pub fn daemonize() -> Result<()> {
943+
pub fn daemonize(log_file: Option<File>) -> Result<()> {
944+
if let Some(log_file) = log_file {
945+
redirect_stderr(log_file);
946+
}
932947
Ok(())
933948
}
934949

950+
#[cfg(windows)]
951+
fn redirect_stderr(f: File) {
952+
use std::os::windows::io::IntoRawHandle;
953+
use windows_sys::Win32::System::Console::{SetStdHandle, STD_ERROR_HANDLE};
954+
// Ignore errors here.
955+
unsafe {
956+
SetStdHandle(STD_ERROR_HANDLE, f.into_raw_handle() as _);
957+
}
958+
}
959+
935960
/// Disable connection pool to avoid broken connection between runtime
936961
///
937962
/// # TODO

0 commit comments

Comments
 (0)