diff --git a/Cargo.toml b/Cargo.toml index 116d00b..27e9f18 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,3 +43,8 @@ harness = false [[test]] name = "helper" path = "tests/helper.rs" + +[[test]] +name = "spawn-after-drop" +path = "tests/spawn-after-drop.rs" +harness = false diff --git a/src/unix.rs b/src/unix.rs index cd05edd..1324f9b 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -47,7 +47,7 @@ pub struct Acquired { impl Client { pub fn new(mut limit: usize) -> io::Result { - let client = unsafe { Client::mk()? }; + let client = Client::mk()?; // I don't think the character written here matters, but I could be // wrong! @@ -55,7 +55,7 @@ impl Client { let mut write = &client.write; - set_nonblocking(write.as_raw_fd(), true)?; + set_nonblocking(write.as_fd(), true)?; while limit > 0 { let n = limit.min(BUFFER.len()); @@ -64,14 +64,12 @@ impl Client { limit -= n; } - set_nonblocking(write.as_raw_fd(), false)?; + set_nonblocking(write.as_fd(), false)?; Ok(client) } - unsafe fn mk() -> io::Result { - let mut pipes = [0; 2]; - + fn mk() -> io::Result { // Attempt atomically-create-with-cloexec if we can on Linux, // detected by using the `syscall` function in `libc` to try to work // with as many kernels/glibc implementations as possible. @@ -79,24 +77,39 @@ impl Client { { static PIPE2_AVAILABLE: AtomicBool = AtomicBool::new(true); if PIPE2_AVAILABLE.load(Ordering::SeqCst) { - match libc::syscall(libc::SYS_pipe2, pipes.as_mut_ptr(), libc::O_CLOEXEC) { - -1 => { - let err = io::Error::last_os_error(); - if err.raw_os_error() == Some(libc::ENOSYS) { - PIPE2_AVAILABLE.store(false, Ordering::SeqCst); - } else { - return Err(err); + unsafe { + let mut pipes = [0; 2]; + match libc::syscall(libc::SYS_pipe2, pipes.as_mut_ptr(), libc::O_CLOEXEC) { + -1 => { + let err = io::Error::last_os_error(); + if err.raw_os_error() == Some(libc::ENOSYS) { + PIPE2_AVAILABLE.store(false, Ordering::SeqCst); + } else { + return Err(err); + } + } + _ => { + return Ok(Client::from_fds( + OwnedFd::from_raw_fd(pipes[0]), + OwnedFd::from_raw_fd(pipes[1]), + )) } } - _ => return Ok(Client::from_fds(pipes[0], pipes[1])), } } } - cvt(libc::pipe(pipes.as_mut_ptr()))?; - drop(set_cloexec(pipes[0], true)); - drop(set_cloexec(pipes[1], true)); - Ok(Client::from_fds(pipes[0], pipes[1])) + let (read, write) = unsafe { + let mut pipes = [0; 2]; + cvt(libc::pipe(pipes.as_mut_ptr()))?; + ( + OwnedFd::from_raw_fd(pipes[0]), + OwnedFd::from_raw_fd(pipes[1]), + ) + }; + drop(set_cloexec(read.as_fd(), true)); + drop(set_cloexec(write.as_fd(), true)); + Ok(Client::from_fds(read, write)) } pub(crate) unsafe fn open(s: &str, check_pipe: bool) -> Result { @@ -211,18 +224,21 @@ impl Client { } Ok(Some(Client { - read: clone_fd_and_set_cloexec(read)?, - write: clone_fd_and_set_cloexec(write)?, + read: clone_fd_and_set_cloexec(BorrowedFd::borrow_raw(read))?, + write: clone_fd_and_set_cloexec(BorrowedFd::borrow_raw(write))?, creation_arg, is_non_blocking: None, })) } - unsafe fn from_fds(read: c_int, write: c_int) -> Client { + fn from_fds(read: OwnedFd, write: OwnedFd) -> Client { Client { - read: File::from_raw_fd(read), - write: File::from_raw_fd(write), - creation_arg: ClientCreationArg::Fds { read, write }, + creation_arg: ClientCreationArg::Fds { + read: read.as_raw_fd(), + write: write.as_raw_fd(), + }, + read: read.into(), + write: write.into(), is_non_blocking: None, } } @@ -304,7 +320,7 @@ impl Client { if let Some(is_non_blocking) = self.is_non_blocking.as_ref() { if !is_non_blocking.load(Ordering::Relaxed) { - set_nonblocking(fifo.as_raw_fd(), true)?; + set_nonblocking(fifo.as_fd(), true)?; is_non_blocking.store(true, Ordering::Relaxed); } } else { @@ -357,24 +373,30 @@ impl Client { Ok(unsafe { len.assume_init() } as usize) } - pub fn configure(&self, cmd: &mut Command) { - if matches!(self.creation_arg, ClientCreationArg::Fifo { .. }) { - // We `File::open`ed it when inheriting from environment, - // so no need to set cloexec for fifo. - return; - } - // Here we basically just want to say that in the child process - // we'll configure the read/write file descriptors to *not* be - // cloexec, so they're inherited across the exec and specified as - // integers through `string_arg` above. - let read = self.read.as_raw_fd(); - let write = self.write.as_raw_fd(); - unsafe { - cmd.pre_exec(move || { - set_cloexec(read, false)?; - set_cloexec(write, false)?; - Ok(()) - }); + pub fn configure(self: &Arc, cmd: &mut Command) { + match self.creation_arg { + ClientCreationArg::Fifo { .. } => { + // We `File::open`ed it when inheriting from environment, + // so no need to set cloexec for fifo. + } + ClientCreationArg::Fds { read, write } => { + // Here we basically just want to say that in the child process + // we'll configure the read/write file descriptors to *not* be + // cloexec, so they're inherited across the exec and specified as + // integers through `string_arg` above. + unsafe { + // Keep a reference to the jobserver alive in the closure so that + // the pipe FDs aren't closed, otherwise `set_cloexec` might end up + // targetting a completely unrelated file descriptor. + let arc = self.clone(); + cmd.pre_exec(move || { + let _ = &arc; + set_cloexec(BorrowedFd::borrow_raw(read), false)?; + set_cloexec(BorrowedFd::borrow_raw(write), false)?; + Ok(()) + }); + } + } } } } @@ -515,34 +537,32 @@ unsafe fn fd_check(fd: c_int, check_pipe: bool) -> Result<(), FromEnvErrorInner> } } -fn clone_fd_and_set_cloexec(fd: c_int) -> Result { - // Safety: fd is a valid fd dand it remains open until returns - unsafe { BorrowedFd::borrow_raw(fd) } - .try_clone_to_owned() +fn clone_fd_and_set_cloexec(fd: BorrowedFd<'_>) -> Result { + fd.try_clone_to_owned() .map(File::from) - .map_err(|err| FromEnvErrorInner::CannotOpenFd(fd, err)) + .map_err(|err| FromEnvErrorInner::CannotOpenFd(fd.as_raw_fd(), err)) } -fn set_cloexec(fd: c_int, set: bool) -> io::Result<()> { +fn set_cloexec(fd: BorrowedFd<'_>, set: bool) -> io::Result<()> { unsafe { - let previous = cvt(libc::fcntl(fd, libc::F_GETFD))?; + let previous = cvt(libc::fcntl(fd.as_raw_fd(), libc::F_GETFD))?; let new = if set { previous | libc::FD_CLOEXEC } else { previous & !libc::FD_CLOEXEC }; if new != previous { - cvt(libc::fcntl(fd, libc::F_SETFD, new))?; + cvt(libc::fcntl(fd.as_raw_fd(), libc::F_SETFD, new))?; } Ok(()) } } -fn set_nonblocking(fd: c_int, set: bool) -> io::Result<()> { +fn set_nonblocking(fd: BorrowedFd<'_>, set: bool) -> io::Result<()> { let status_flag = if set { libc::O_NONBLOCK } else { 0 }; unsafe { - cvt(libc::fcntl(fd, libc::F_SETFL, status_flag))?; + cvt(libc::fcntl(fd.as_raw_fd(), libc::F_SETFL, status_flag))?; } Ok(()) diff --git a/src/wasm.rs b/src/wasm.rs index 7ebdeab..90b9d1f 100644 --- a/src/wasm.rs +++ b/src/wasm.rs @@ -75,7 +75,7 @@ impl Client { Ok(*lock) } - pub fn configure(&self, _cmd: &mut Command) { + pub fn configure(self: &Arc, _cmd: &mut Command) { unreachable!(); } } diff --git a/src/windows.rs b/src/windows.rs index 7e6e6d4..e03a1c4 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -214,7 +214,7 @@ impl Client { } } - pub fn configure(&self, _cmd: &mut Command) { + pub fn configure(self: &Arc, _cmd: &mut Command) { // nothing to do here, we gave the name of our semaphore to the // child above } diff --git a/tests/spawn-after-drop.rs b/tests/spawn-after-drop.rs new file mode 100644 index 0000000..ba30f1e --- /dev/null +++ b/tests/spawn-after-drop.rs @@ -0,0 +1,48 @@ +use std::{env, process::Command}; + +use jobserver::{Client, FromEnvErrorKind}; + +macro_rules! t { + ($e:expr) => { + match $e { + Ok(e) => e, + Err(e) => panic!("{} failed with {}", stringify!($e), e), + } + }; +} + +fn main() { + match env::args().skip(1).next().unwrap_or_default().as_str() { + "" => { + let me = t!(env::current_exe()); + let mut cmd = Command::new(me); + let client = t!(Client::new(1)); + client.configure(&mut cmd); + drop(client); + let status = t!(cmd.arg("from_env").status()); + assert!(status.success()); + } + "from_env" => { + let me = t!(env::current_exe()); + let mut cmd = Command::new(me); + let client = unsafe { t!(match Client::from_env_ext(true).client { + // Its ok for a dropped jobservers path to no longer exist (e.g. on Windows). + Err(e) if matches!(e.kind(), FromEnvErrorKind::CannotOpenPath) => return, + res => res, + }) }; + client.configure(&mut cmd); + drop(client); + let status = t!(cmd.arg("use_it").status()); + assert!(status.success()); + } + "use_it" => { + let client = unsafe { t!(match Client::from_env_ext(true).client { + // See above. + Err(e) if matches!(e.kind(), FromEnvErrorKind::CannotOpenPath) => return, + res => res, + }) }; + t!(client.acquire()); + } + _ => unreachable!(), + } +}