From 6f67fa0e575fd7c09cb0581cb648fc44169be54e Mon Sep 17 00:00:00 2001 From: Noa Date: Fri, 13 Sep 2024 11:51:37 -0400 Subject: [PATCH] Better comments for set_aux_fd --- src/tools/run-make-support/Cargo.toml | 1 - src/tools/run-make-support/src/command.rs | 35 ++++++++++++++++------- src/tools/run-make-support/src/macros.rs | 5 ++++ tests/run-make/jobserver-error/rmake.rs | 10 ++----- 4 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/tools/run-make-support/Cargo.toml b/src/tools/run-make-support/Cargo.toml index 105b3e2ed2239..cdc49d45c3d61 100644 --- a/src/tools/run-make-support/Cargo.toml +++ b/src/tools/run-make-support/Cargo.toml @@ -13,4 +13,3 @@ gimli = "0.31.0" libc = "0.2" build_helper = { path = "../build_helper" } serde_json = "1.0" -libc = "0.2" diff --git a/src/tools/run-make-support/src/command.rs b/src/tools/run-make-support/src/command.rs index ee900d3cac484..9125ec3d3e60e 100644 --- a/src/tools/run-make-support/src/command.rs +++ b/src/tools/run-make-support/src/command.rs @@ -152,6 +152,11 @@ impl Command { } /// Set an auxiliary stream passed to the process, besides the stdio streams. + /// + /// Use with caution - ideally, only set one aux fd; if there are multiple, + /// their old `fd` may overlap with another's `newfd`, and thus will break. + /// If you need more than 1 auxiliary file descriptor, rewrite this function + /// to be able to support that. #[cfg(unix)] pub fn set_aux_fd>( &mut self, @@ -161,18 +166,28 @@ impl Command { use std::os::fd::AsRawFd; use std::os::unix::process::CommandExt; + let cvt = |x| if x == -1 { Err(std::io::Error::last_os_error()) } else { Ok(()) }; + let fd = fd.into(); - unsafe { - self.cmd.pre_exec(move || { - let fd = fd.as_raw_fd(); - let ret = if fd == newfd { - libc::fcntl(fd, libc::F_SETFD, 0) - } else { - libc::dup2(fd, newfd) - }; - if ret == -1 { Err(std::io::Error::last_os_error()) } else { Ok(()) } - }); + if fd.as_raw_fd() == newfd { + // if fd is already where we want it, just turn off FD_CLOEXEC + // SAFETY: io-safe: we have ownership over fd + cvt(unsafe { libc::fcntl(fd.as_raw_fd(), libc::F_SETFD, 0) }) + .expect("disabling CLOEXEC failed"); + // we still unconditionally set the pre_exec function, since it captures + // `fd`, and we want to ensure that it stays open until the fork } + let pre_exec = move || { + if fd.as_raw_fd() != newfd { + // set newfd to point to the same file as fd + // SAFETY: io-"safe": newfd is. not necessarily an unused fd. + // however, we're + cvt(unsafe { libc::dup2(fd.as_raw_fd(), newfd) })?; + } + Ok(()) + }; + // SAFETY: dup2 is pre-exec-safe + unsafe { self.cmd.pre_exec(pre_exec) }; self } diff --git a/src/tools/run-make-support/src/macros.rs b/src/tools/run-make-support/src/macros.rs index 322ee5314e239..43844edf4127d 100644 --- a/src/tools/run-make-support/src/macros.rs +++ b/src/tools/run-make-support/src/macros.rs @@ -81,6 +81,11 @@ macro_rules! impl_common_helpers { } /// Set an auxiliary stream passed to the process, besides the stdio streams. + /// + /// Use with caution - ideally, only set one aux fd; if there are multiple, + /// their old `fd` may overlap with another's `newfd`, and thus will break. + /// If you need more than 1 auxiliary file descriptor, rewrite this function + /// to be able to support that. #[cfg(unix)] pub fn set_aux_fd>( &mut self, diff --git a/tests/run-make/jobserver-error/rmake.rs b/tests/run-make/jobserver-error/rmake.rs index c18c226285740..e99a94e4ce514 100644 --- a/tests/run-make/jobserver-error/rmake.rs +++ b/tests/run-make/jobserver-error/rmake.rs @@ -4,24 +4,20 @@ // and errors are printed instead in case of a wrong jobserver. // See https://github.com/rust-lang/rust/issues/46981 -// FIXME(Oneirical): The original test included this memo: -// # Note that by default, the compiler uses file descriptors 0 (stdin), 1 (stdout), 2 (stderr), -// # but also 3 and 4 for either end of the ctrl-c signal handler self-pipe. - // FIXME(Oneirical): only-linux ignore-cross-compile -use run_make_support::{diff, rfs, rustc}; +use run_make_support::{diff, rustc}; fn main() { let out = rustc() - .stdin("fn main() {}") + .stdin_buf(("fn main() {}").as_bytes()) .env("MAKEFLAGS", "--jobserver-auth=5,5") .run_fail() .stderr_utf8(); diff().expected_file("cannot_open_fd.stderr").actual_text("actual", out).run(); let out = rustc() - .stdin("fn main() {}") + .stdin_buf(("fn main() {}").as_bytes()) .input("-") .env("MAKEFLAGS", "--jobserver-auth=3,3") .set_aux_fd(3, std::fs::File::open("/dev/null").unwrap())