diff --git a/README.md b/README.md index 4b95d65..9dad18b 100644 --- a/README.md +++ b/README.md @@ -121,8 +121,8 @@ let lines = Exec::cmd("sqlite3") Create pipelines using the `|` operator: ```rust -let top_mem = (Exec::cmd("ps").args(&["aux"]) - | Exec::cmd("sort").args(&["-k4", "-rn"]) +let top_mem = (Exec::cmd("ps").args(["aux"]) + | Exec::cmd("sort").args(["-k4", "-rn"]) | Exec::cmd("head").arg("-5")) .capture()? .stdout_str(); diff --git a/examples/pipeline.rs b/examples/pipeline.rs index f2fff2b..39c6ec8 100644 --- a/examples/pipeline.rs +++ b/examples/pipeline.rs @@ -6,8 +6,8 @@ use subprocess::Exec; fn main() -> std::io::Result<()> { // Simple pipeline: generate data, transform it, capture output - let data = (Exec::cmd("echo").args(&["cherry", "apple", "banana"]) - | Exec::cmd("tr").args(&[" ", "\n"]) + let data = (Exec::cmd("echo").args(["cherry", "apple", "banana"]) + | Exec::cmd("tr").args([" ", "\n"]) | Exec::cmd("sort")) .capture()? .stdout_str(); diff --git a/src/communicate.rs b/src/communicate.rs index d29ed62..aa4a3a2 100644 --- a/src/communicate.rs +++ b/src/communicate.rs @@ -435,12 +435,13 @@ use win32::RawCommunicator; /// serially, our process could block waiting to write input while the subprocess blocks /// waiting for us to read its output, or vice versa. /// -/// Create a `Communicator` by calling [`Job::communicate`], then call [`read`] or -/// [`read_string`] to perform the data exchange. +/// Create a `Communicator` by calling [`Job::communicate`], then call [`read`], +/// [`read_string`], or [`read_to`] to perform the data exchange. /// /// [`Job::communicate`]: crate::Job::communicate /// [`read`]: #method.read /// [`read_string`]: #method.read_string +/// [`read_to`]: #method.read_to #[must_use] pub struct Communicator { inner: RawCommunicator, @@ -539,7 +540,7 @@ impl Communicator { /// /// Note that this method does not wait for the subprocess to finish, only to close its /// output/error streams. It is rare but possible for the program to continue running - /// after having closed the streams, in which case `Process::Drop` will wait for it + /// after having closed the streams, in which case `Process::drop()` will wait for it /// to finish. If such a wait is undesirable, it can be prevented by waiting /// explicitly using `wait()`, by detaching the process using `detach()`, or by /// terminating it with `terminate()`. @@ -564,7 +565,10 @@ impl Communicator { Ok((from_utf8_lossy(out), from_utf8_lossy(err))) } - /// Limit the amount of data the next `read()` will read from the subprocess. + /// Limit the amount of data each `read()` will read from the subprocess. + /// + /// The limit applies per call and persists across subsequent reads, so any data + /// beyond the limit is left buffered for the next `read()` to retrieve. /// /// On Windows, when capturing both stdout and stderr, the limit is approximate /// and may be exceeded by several kilobytes. @@ -573,7 +577,7 @@ impl Communicator { self } - /// Limit the amount of time the next `read()` will spend reading from the subprocess. + /// Limit the amount of time each `read()` will spend reading from the subprocess. pub fn limit_time(mut self, time: Duration) -> Communicator { self.time_limit = Some(time); self diff --git a/src/exec.rs b/src/exec.rs index 162a91b..973707c 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -213,7 +213,8 @@ impl Exec { /// code is prone to errors and, if `filename` comes from an untrusted source, to /// shell injection attacks. Instead, use `Exec::cmd("sort").arg(filename)`. pub fn shell(cmdstr: impl Into) -> Exec { - let cmd = Exec::cmd(SHELL[0]).args(&SHELL[1..]); + let [shell, flag] = SHELL; + let cmd = Exec::cmd(shell).arg(flag); #[cfg(not(windows))] { cmd.arg(cmdstr) @@ -331,9 +332,9 @@ impl Exec { /// /// * a [`Redirection`]; /// * a `File`, which is a shorthand for `Redirection::File(file)`; - /// * a `Vec`, `&str`, `&[u8]`, `Box<[u8]>`, or `[u8; N]`, which will set up a - /// `Redirection::Pipe` for stdin, feeding that data into the standard input of the - /// subprocess; + /// * a `Vec`, `&'static str`, `&'static [u8]`, `Box<[u8]>`, or `[u8; N]`, which + /// will set up a `Redirection::Pipe` for stdin, feeding that data into the standard + /// input of the subprocess; /// * an [`InputData`], which also sets up a pipe, but wraps any reader and feeds its /// content to the standard input of the subprocess. Use [`InputData::from_bytes`] /// for in-memory byte containers not covered by the above, like `bytes::Bytes` or @@ -543,7 +544,10 @@ impl Exec { self.start()?.capture() } - /// Show Exec as command-line string quoted in the Unix style. + /// Show `Exec` as a command-line string using POSIX shell quoting. + /// + /// The output uses POSIX shell quoting rules on all platforms, including Windows; + /// it is intended for display and debugging, not for re-execution by `cmd.exe`. pub fn to_cmdline_lossy(&self) -> String { let mut out = String::new(); if let Some(cmd_env) = &self.env { diff --git a/src/job.rs b/src/job.rs index c31c77f..a5a7e59 100644 --- a/src/job.rs +++ b/src/job.rs @@ -93,8 +93,8 @@ impl Job { /// Returns the PIDs of all processes in the pipeline, in pipeline order. /// - /// If the job was started by a single process, this will return its pid. It will be - /// empty for a job started by an empty pipeline. + /// If the job was started from a single command (`Exec`), this returns one PID. It + /// will be empty for a job started by an empty pipeline. pub fn pids(&self) -> Vec { self.processes.iter().map(|p| p.pid()).collect() } diff --git a/src/lib.rs b/src/lib.rs index a679336..bfc7f2f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -33,7 +33,7 @@ //! # } //! ``` //! -//! Use the [`Exec`] builder to execute a pipeline of commands and capture the output: +//! Combine [`Exec`] instances with `|` to build a [`Pipeline`] and capture its output: //! //! ```no_run //! # use subprocess::*; diff --git a/src/pipeline.rs b/src/pipeline.rs index 6ff670f..709908f 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -137,9 +137,9 @@ impl Pipeline { /// /// * a [`Redirection`]; /// * a `File`, which is a shorthand for `Redirection::File(file)`; - /// * a `Vec`, `&str`, `&[u8]`, `Box<[u8]>`, or `[u8; N]`, which will set up a - /// `Redirection::Pipe` for stdin, feeding that data into the standard input of the - /// subprocess; + /// * a `Vec`, `&'static str`, `&'static [u8]`, `Box<[u8]>`, or `[u8; N]`, which + /// will set up a `Redirection::Pipe` for stdin, feeding that data into the standard + /// input of the subprocess; /// * an [`InputData`], which also sets up a pipe, but wraps any reader and feeds its /// content to the standard input of the subprocess. Use [`InputData::from_bytes`] /// for in-memory byte containers not covered by the above, like `bytes::Bytes` or @@ -188,10 +188,9 @@ impl Pipeline { /// Specifies the sink for the standard error of all commands in the pipeline. /// /// Unlike `stdout()`, which only affects the last command in the pipeline, this - /// affects all commands. The difference is because standard output is piped from one - /// command to the next, so only the output of the last command is "free". In - /// contrast, the standard errors are not connected to each other and can be - /// configured *en masse*. + /// affects all commands. This is because standard output is piped from one command + /// to the next, so only the output of the last command is "free". In contrast, the + /// standard errors are not connected to each other and can be configured *en masse*. /// /// The sink can be: /// diff --git a/src/posix.rs b/src/posix.rs index 9d0fa9b..8f37fa3 100644 --- a/src/posix.rs +++ b/src/posix.rs @@ -54,14 +54,25 @@ pub fn pipe() -> Result<(File, File)> { pub fn pipe() -> Result<(File, File)> { let mut fds = [0; 2]; check_err(unsafe { libc::pipe(fds.as_mut_ptr()) })?; + // Wrap in File immediately so an fcntl failure below closes both fds via Drop + // instead of leaking them. + let (read, write) = unsafe { (File::from_raw_fd(fds[0]), File::from_raw_fd(fds[1])) }; // Set CLOEXEC on both ends. There is a small race window between pipe() and fcntl() // where another thread's fork()+exec() could inherit these fds - this matches what // Rust stdlib does on these platforms that lack pipe2(). unsafe { - check_err(libc::fcntl(fds[0], libc::F_SETFD, libc::FD_CLOEXEC))?; - check_err(libc::fcntl(fds[1], libc::F_SETFD, libc::FD_CLOEXEC))?; + check_err(libc::fcntl( + read.as_raw_fd(), + libc::F_SETFD, + libc::FD_CLOEXEC, + ))?; + check_err(libc::fcntl( + write.as_raw_fd(), + libc::F_SETFD, + libc::FD_CLOEXEC, + ))?; } - Ok(unsafe { (File::from_raw_fd(fds[0]), File::from_raw_fd(fds[1])) }) + Ok((read, write)) } // marked unsafe because the child must not allocate before exec-ing @@ -188,7 +199,10 @@ impl PrepExec { let mut exe = std::mem::ManuallyDrop::new(std::mem::take(&mut this.prealloc_exe)); if let Some(ref search_path) = this.search_path { - let mut err = Ok(()); + // Default to ENOENT so a PATH with no usable entries (e.g. ":::") yields + // a real error instead of Ok(()) - the caller would otherwise hit + // unreachable!() in the child after fork. + let mut err = Err(Error::from_raw_os_error(libc::ENOENT)); // POSIX requires execvp and execve, but not execvpe (although glibc provides // one), so we have to iterate over PATH ourselves for dir in split_path(search_path.as_os_str()) { @@ -297,6 +311,35 @@ pub fn waitpid(pid: u32, flags: i32) -> Result<(u32, ExitStatus)> { Ok((pid as u32, ExitStatus::from_raw(status))) } +/// Block until `pid` has exited, leaving it as a zombie (does not reap). +/// +/// The PID stays tied to the dead child until something calls `waitpid` to reap it, so a +/// signal sent in the window between this returning and the reap is delivered to the +/// original child (or to a zombie, which is a no-op) - never to a recycled PID. +/// +/// Restarted automatically on `EINTR`. +pub fn wait_no_reap(pid: u32) -> Result<()> { + loop { + let mut info: libc::siginfo_t = unsafe { mem::zeroed() }; + let r = unsafe { + libc::waitid( + libc::P_PID, + pid as libc::id_t, + &mut info, + libc::WEXITED | libc::WNOWAIT, + ) + }; + if r == 0 { + return Ok(()); + } + let err = Error::last_os_error(); + if err.raw_os_error() == Some(libc::EINTR) { + continue; + } + return Err(err); + } +} + #[cfg(target_os = "linux")] pub fn pidfd_open(pid: u32) -> Result { let fd = @@ -318,6 +361,7 @@ pub fn killpg(pgid: u32, signal: i32) -> Result<()> { pub const F_GETFD: i32 = libc::F_GETFD; pub const F_SETFD: i32 = libc::F_SETFD; +pub const F_DUPFD_CLOEXEC: i32 = libc::F_DUPFD_CLOEXEC; pub const FD_CLOEXEC: i32 = libc::FD_CLOEXEC; pub fn fcntl(fd: i32, cmd: i32, arg1: Option) -> Result { @@ -393,11 +437,12 @@ impl PollFd<'_> { pub use libc::{POLLHUP, POLLIN, POLLOUT}; pub fn poll(fds: &mut [PollFd<'_>], mut timeout: Option) -> Result { + // The loop handles two cases: + // - poll() accepts a maximum timeout of 2**31-1 ms (less than 25 days), and the + // caller can specify larger Durations. + // - poll() fails with EINTR when interrupted by a signal handler. let deadline = timeout.map(|timeout| Instant::now() + timeout); loop { - // poll() accepts a maximum timeout of 2**31-1 ms, which is less than 25 days. - // The caller can specify Durations much larger than that, so support them by - // waiting in a loop. let (timeout_ms, overflow) = timeout .map(|timeout| { let timeout = timeout.as_millis(); @@ -409,17 +454,28 @@ pub fn poll(fds: &mut [PollFd<'_>], mut timeout: Option) -> Result= 0 { + let cnt = raw as usize; + if cnt != 0 || !overflow { + return Ok(cnt); + } + // Timeout fired on the clamped value; loop to wait the rest. + } else { + let err = Error::last_os_error(); + if err.raw_os_error() != Some(libc::EINTR) { + return Err(err); + } + // Interrupted by a signal; loop and retry. } - let deadline = deadline.unwrap(); - let now = Instant::now(); - if now >= deadline { - return Ok(0); + if let Some(deadline) = deadline { + let now = Instant::now(); + if now >= deadline { + return Ok(0); + } + timeout = Some(deadline - now); } - timeout = Some(deadline - now); } } diff --git a/src/process.rs b/src/process.rs index 49a7bb6..a4de083 100644 --- a/src/process.rs +++ b/src/process.rs @@ -11,7 +11,9 @@ use std::time::Duration; /// representation. Use the provided methods to query the exit status. /// /// On Unix, the raw value is the status from `waitpid()`. On Windows, it is the exit code -/// from `GetExitCodeProcess()`. +/// from `GetExitCodeProcess()`. The exit status may also be undetermined, in which case +/// `code()` and `signal()` both return `None`; this happens when the child was reaped +/// outside of this library and its status is no longer available. #[derive(Eq, PartialEq, Hash, Copy, Clone)] pub struct ExitStatus(pub(crate) Option); @@ -50,12 +52,15 @@ impl ExitStatus { /// /// # Drop behavior /// -/// When the last clone of a `Process` is dropped, it waits for the child process to -/// finish unless [`detach`](Self::detach) has been called. Because `Process` does not own -/// any pipes to the child, callers must ensure that any pipes connected to the child's -/// stdin are dropped *before* the `Process` is dropped. Otherwise, the child may block -/// waiting for input while the `Process` drop waits for the child to exit, resulting in a -/// deadlock. [`Job`] handles this automatically via field declaration order. +/// When the last clone of a `Process` is dropped, it waits for the child to finish. +/// Call [`detach`](Self::detach) to skip the wait. +/// +/// `Process` does not own any pipes to the child, so callers must drop pipes to the +/// child's standard streams *before* the `Process` itself. Otherwise the child may +/// block on a full stdout/stderr pipe or on a stdin pipe that never sees EOF, while +/// the `Process` drop waits for the child to exit -- a deadlock. +/// +/// [`Job`] handles this automatically via field declaration order. /// /// [`Exec::start`]: crate::Exec::start /// [`Pipeline::start`]: crate::Pipeline::start @@ -248,8 +253,8 @@ mod os { impl ExitStatus { /// Returns the exit code if the process exited normally. /// - /// On Unix, this returns `Some` only if the process exited voluntarily (not - /// killed by a signal). + /// On Unix, this returns `Some` only if the process exited normally, as opposed + /// to being killed by a signal. pub fn code(&self) -> Option { let raw = self.0?; libc::WIFEXITED(raw).then(|| libc::WEXITSTATUS(raw) as u32) @@ -298,12 +303,87 @@ mod os { impl InnerProcess { pub(super) fn os_wait(&self) -> io::Result { - let mut state = self.state.lock().unwrap(); + // Fast path: status already known. + { + let state = self.state.lock().unwrap(); + if let Some(status) = state.exit_status { + return Ok(status); + } + } + + // On Linux with pidfd, poll(pidfd, INFINITE) is concurrent-safe and doesn't + // require holding any lock during the syscall. + #[cfg(target_os = "linux")] + { + let pidfd = self.state.lock().unwrap().pidfd.fd(self.pid); + if let Some(pidfd) = pidfd { + return self.wait_pidfd(pidfd); + } + } + + self.wait_blocking() + } + + /// Wait indefinitely via pidfd. Linux-only fast path. + #[cfg(target_os = "linux")] + fn wait_pidfd( + &self, + pidfd: std::sync::Arc, + ) -> io::Result { + use std::os::unix::io::AsFd; loop { + { + let state = self.state.lock().unwrap(); + if let Some(status) = state.exit_status { + return Ok(status); + } + } + let mut pfd = [posix::PollFd::new(Some(pidfd.as_fd()), posix::POLLIN)]; + posix::poll(&mut pfd, None)?; + let mut state = self.state.lock().unwrap(); + if state.exit_status.is_none() { + let result = posix::waitpid(self.pid, posix::WNOHANG); + Self::record_waitpid_result(&mut state, self.pid, result)?; + } + if let Some(status) = state.exit_status { + return Ok(status); + } + } + } + + /// Wait indefinitely via waitid()+waitpid(). Used when pidfd is unavailable + /// (non-Linux, or Linux with pidfd_open() failing). + /// + /// We block in waitid(WNOWAIT) rather than waitpid(0) so the kernel keeps the + /// child as a zombie until we reap under state lock. That closes the PID-reuse + /// window for a concurrent terminate()/kill(): while the zombie persists the PID + /// can't be recycled, and signals to a zombie are silently dropped by the kernel. + fn wait_blocking(&self) -> io::Result { + loop { + if let Some(status) = self.state.lock().unwrap().exit_status { + return Ok(status); + } + if let Err(e) = posix::wait_no_reap(self.pid) { + // ECHILD: someone else (an external waitpid) reaped before us. + // Record it as undetermined and exit. + if e.raw_os_error() == Some(posix::ECHILD) { + let mut state = self.state.lock().unwrap(); + if state.exit_status.is_none() { + state.exit_status = Some(ExitStatus(None)); + } + return Ok(state.exit_status.unwrap()); + } + return Err(e); + } + // Child is now a zombie. Reap and record under state lock. + let mut state = self.state.lock().unwrap(); + if state.exit_status.is_none() { + let result = posix::waitpid(self.pid, posix::WNOHANG); + Self::record_waitpid_result(&mut state, self.pid, result)?; + } if let Some(status) = state.exit_status { return Ok(status); } - Self::waitpid_into(&mut state, self.pid, true)?; } } @@ -340,7 +420,7 @@ mod os { let mut state = self.state.lock().unwrap(); if ready { - Self::waitpid_into(&mut state, self.pid, false)?; + self.try_reap(&mut state)?; } Ok(state.exit_status) } @@ -355,7 +435,7 @@ mod os { let mut delay = Duration::from_millis(1); loop { - Self::waitpid_into(&mut state, self.pid, false)?; + self.try_reap(&mut state)?; if state.exit_status.is_some() { return Ok(state.exit_status); } @@ -402,11 +482,24 @@ mod os { } } - fn waitpid_into(state: &mut WaitState, pid: u32, block: bool) -> io::Result<()> { + /// Try a non-blocking (WNOHANG) waitpid for this process. + fn try_reap(&self, state: &mut WaitState) -> io::Result<()> { + if state.exit_status.is_some() { + return Ok(()); + } + let result = posix::waitpid(self.pid, posix::WNOHANG); + Self::record_waitpid_result(state, self.pid, result) + } + + fn record_waitpid_result( + state: &mut WaitState, + pid: u32, + result: io::Result<(u32, ExitStatus)>, + ) -> io::Result<()> { if state.exit_status.is_some() { return Ok(()); } - match posix::waitpid(pid, if block { 0 } else { posix::WNOHANG }) { + match result { Ok((pid_out, exit_status)) if pid_out == pid => { state.exit_status = Some(exit_status); } @@ -433,8 +526,9 @@ mod os { pub trait ProcessExt { /// Send the specified signal to the child process. /// - /// If the child process is known to have finished (due to e.g. a previous - /// call to [`wait`] or [`poll`]), this will do nothing and return `Ok`. + /// If the process has already been reaped (e.g. by a previous call to + /// [`wait`] or [`poll`]), this is a no-op to avoid signaling a potentially + /// reused PID. /// /// [`poll`]: crate::Process::poll /// [`wait`]: crate::Process::wait @@ -447,8 +541,8 @@ mod os { /// [`ExecExt::setpgid`] set, which places the child in a new process group /// with PGID equal to its PID. /// - /// If the child process is known to have finished, this will do nothing and - /// return `Ok`. + /// If the process has already been reaped, this is a no-op to avoid signaling + /// an unrelated process group that may now exist under the reused PID. /// /// [`ExecExt::setpgid`]: crate::ExecExt::setpgid fn send_signal_group(&self, signal: i32) -> io::Result<()>; diff --git a/src/spawn.rs b/src/spawn.rs index fb5b807..bbbe890 100644 --- a/src/spawn.rs +++ b/src/spawn.rs @@ -124,27 +124,22 @@ fn prepare_child_stream( redir: Arc, is_input: bool, ) -> io::Result<(Option, Option>)> { - // File is the only variant holding a resource - handle specially to avoid dup() when - // the Arc is shared. - if matches!(&*redir, Redirection::File(_)) { - return match Arc::try_unwrap(redir) { - Ok(Redirection::File(f)) => Ok((None, Some(os::prepare_file(f)?))), - Err(arc) => Ok((None, Some(os::prepare_file_shared(arc)?))), - _ => unreachable!(), - }; - } - // Other variants are trivially cheap - just peek and handle. match &*redir { + Redirection::File(_) => Ok((None, Some(os::prepare_file(redir)?))), Redirection::Pipe => { let (parent, child) = prepare_pipe(is_input)?; Ok((Some(parent), Some(child))) } Redirection::Null => Ok((None, Some(prepare_null_file(is_input)?))), Redirection::None => Ok((None, None)), - _ => unreachable!(), + Redirection::Merge => unreachable!(), } } +fn wrap_file(file: File) -> Arc { + Arc::new(Redirection::File(file)) +} + fn prepare_pipe(parent_writes: bool) -> io::Result<(File, Arc)> { let (read, write) = os::make_pipe()?; let (parent_end, child_end) = if parent_writes { @@ -152,7 +147,7 @@ fn prepare_pipe(parent_writes: bool) -> io::Result<(File, Arc)> { } else { (read, write) }; - Ok((parent_end, os::prepare_file(child_end)?)) + Ok((parent_end, os::prepare_file(wrap_file(child_end))?)) } fn prepare_null_file(for_read: bool) -> io::Result> { @@ -161,7 +156,7 @@ fn prepare_null_file(for_read: bool) -> io::Result> { } else { OpenOptions::new().write(true).open(os::NULL_DEVICE)? }; - os::prepare_file(file) + os::prepare_file(wrap_file(file)) } // Share a child stream via Arc::clone - zero dup syscalls. @@ -303,7 +298,7 @@ pub(crate) mod os { use std::ffi::OsString; use std::fs::File; use std::io::{self, Read, Write}; - use std::os::fd::{AsRawFd, FromRawFd}; + use std::os::fd::{AsRawFd, RawFd}; pub use crate::posix::make_redirection_to_standard_stream; @@ -400,45 +395,69 @@ pub(crate) mod os { formatted } - fn install_child_fd(end: Option>, target_fd: i32) -> io::Result<()> { - // Called after fork - use ManuallyDrop to prevent deallocation on - // early return via ?. - let mut end = std::mem::ManuallyDrop::new(end); - if let Some(r) = &*end { - let fd = child_file(r).as_raw_fd(); - if fd != target_fd { - posix::dup2(fd, target_fd)?; - } else { - // dup2(fd, fd) is a no-op per POSIX and doesn't clear - // CLOEXEC. Clear it so the fd survives exec. - set_inheritable(child_file(r), true)?; + // Install all three child-end fds onto stdin/stdout/stderr in fixed + // [0, 1, 2] order. Pre-pass: for any source fd that another stream's + // dup2 would clobber, F_DUPFD_CLOEXEC it to a fresh fd >= 3. Cycles + // (stdout source = 2, stderr source = 1, etc.) fall out for free: + // every node in a cycle would be overwritten, so every node gets duped. + // + // The fixed install order is correct because, after the pre-pass, + // every slot i with source S satisfies one of: + // - S >= 3 (no slot's dup2 lands on S; targets are 0..=2 only), or + // - S == i (slot i does no dup2; the question doesn't arise), or + // - slots[S] does no dup2 either: it is None, or its source == S + // (so fd S is untouched by the time slot i reads it). + // The will_be_overwritten predicate below is the precise negation of these. + // + // `slots` is borrowed (not mutated): the source fds remain valid for the + // duration of this call because the caller keeps the underlying Arcs alive + // (in a ManuallyDrop, post-fork). + fn redirect_streams(slots: &[Option>; 3]) -> io::Result<()> { + let mut sources: [Option; 3] = [ + slots[0].as_ref().map(|a| child_file(a).as_raw_fd()), + slots[1].as_ref().map(|a| child_file(a).as_raw_fd()), + slots[2].as_ref().map(|a| child_file(a).as_raw_fd()), + ]; + + for i in 0..3 { + if let Some(fd) = sources[i] { + // Will be overwritten iff fd is a low fd (a possible dup2 + // target) other than this stream's own target, AND the stream + // owning fd as its target will actually dup2 to it (source != + // target there too). The normal case (sources > 2) skips this + // entirely. + let will_be_overwritten = (0..=2).contains(&fd) + && fd != i as i32 + && sources[fd as usize].is_some_and(|s| s != fd); + if will_be_overwritten { + sources[i] = Some(posix::fcntl(fd, posix::F_DUPFD_CLOEXEC, Some(3))?); + } } } - if let Some(end) = end.take() { - prevent_dealloc(end); + + for (i, &source) in sources.iter().enumerate() { + let target = i as i32; + let Some(fd) = source else { continue }; + if fd == target { + // dup2(fd, fd) is a no-op and doesn't clear CLOEXEC; clear it + // so the fd survives exec. + set_inheritable(fd, true)?; + } else { + posix::dup2(fd, target)?; + // Source fd is redundant after dup2; for fd > 2, set CLOEXEC + // so it closes at exec. (Pre-pass dups already have CLOEXEC, + // so this is a single F_GETFD no-op for them.) fd in 0..=2 is + // an inherited standard stream the child may still need - + // notably the standard-stream wrapper used to back a Merge + // against an unredirected stdout/stderr - so leave it. + if fd >= 3 { + let _ = set_inheritable(fd, false); + } + } } Ok(()) } - // Prevent deallocation of Arc while still closing the underlying resource if - // necessary. Needed because this runs after fork() - see prep_exec(). - // - // Note that this never closes system streams returned by - // get_redirection_to_standard_stream() because it returns "leaked" Arcs which are - // immortal. - fn prevent_dealloc(r: Arc) { - // If Arc we received is the last one, manually close the File - // inside. - if Arc::strong_count(&r) == 1 - && let Redirection::File(f) = &*r - { - // SAFETY: strong_count == 1 guarantees no other Arc clone will access or - // close the fd. std::mem::forget() prevents double-close. - let _ = unsafe { File::from_raw_fd(f.as_raw_fd()) }; - }; - std::mem::forget(r); - } - fn do_exec( just_exec: impl FnOnce() -> io::Result<()>, child_ends: ( @@ -452,9 +471,7 @@ pub(crate) mod os { // Called after fork - use ManuallyDrop to prevent deallocation on // early return via ?. let (stdin, stdout, stderr) = child_ends; - let mut stdin = std::mem::ManuallyDrop::new(stdin); - let mut stdout = std::mem::ManuallyDrop::new(stdout); - let mut stderr = std::mem::ManuallyDrop::new(stderr); + let slots = std::mem::ManuallyDrop::new([stdin, stdout, stderr]); let mut just_exec = std::mem::ManuallyDrop::new(just_exec); let mut os_options = std::mem::ManuallyDrop::new(os_options); @@ -462,9 +479,7 @@ pub(crate) mod os { chdir()?; } - install_child_fd(stdin.take(), 0)?; - install_child_fd(stdout.take(), 1)?; - install_child_fd(stderr.take(), 2)?; + redirect_streams(&slots)?; posix::reset_sigpipe()?; if let Some(gid) = os_options.setgid { @@ -485,8 +500,7 @@ pub(crate) mod os { unreachable!(); } - pub fn set_inheritable(f: &File, inheritable: bool) -> io::Result<()> { - let fd = f.as_raw_fd(); + pub fn set_inheritable(fd: RawFd, inheritable: bool) -> io::Result<()> { let old = posix::fcntl(fd, posix::F_GETFD, None)?; let new = if inheritable { old & !posix::FD_CLOEXEC @@ -499,21 +513,17 @@ pub(crate) mod os { Ok(()) } - pub fn prepare_file(file: File) -> io::Result> { + pub fn prepare_file(redir: Arc) -> io::Result> { // On Unix, child fds don't need CLOEXEC cleared before fork. dup2() in the child // atomically places them on fd 0/1/2 and clears CLOEXEC on the target. - Ok(Arc::new(Redirection::File(file))) - } - - pub fn prepare_file_shared(arc: Arc) -> io::Result> { - Ok(arc) + Ok(redir) } /// Create a pipe. /// /// Child processes won't inherit these fds across exec. To pass a pipe end to a /// child, dup2() it to a standard fd (which clears CLOEXEC), or call - /// `set_inheritable(f, true)`. + /// `set_inheritable(fd, true)`. pub fn make_pipe() -> io::Result<(File, File)> { posix::pipe() } @@ -590,20 +600,18 @@ pub(crate) mod os { } fn format_env_block(env: &[(OsString, OsString)]) -> Vec { - // Dedupe by env-var-name semantics, keeping the last occurrence of each - // key. Walk in reverse, retain entries whose key isn't already seen, then - // reverse back to restore original relative order. - let mut pruned: Vec<_> = { - let mut seen = std::collections::BTreeSet::::new(); - env.iter() - .rev() - .filter(|(k, _)| seen.insert(EnvKey::new(k))) - .collect() - }; - pruned.reverse(); + // Sort and dedup in one pass via BTreeMap. EnvKey orders entries by + // case-insensitive ordinal compare, which is exactly the order CreateProcessW + // requires for Unicode environment blocks. Walking input in reverse with + // or_insert means the latest occurrence of each key wins, both for value + // (matching "last value used" semantics) and for the key's case form. + let mut sorted = std::collections::BTreeMap::::new(); + for (k, v) in env.iter().rev() { + sorted.entry(EnvKey::new(k)).or_insert(v); + } let mut block = vec![]; - for (k, v) in pruned { - block.extend(k.encode_wide()); + for (k, v) in sorted { + block.extend(k.0); block.push('=' as u16); block.extend(v.encode_wide()); block.push(0); @@ -612,8 +620,8 @@ pub(crate) mod os { block } - /// `BTreeSet` key for env-block dedup. Caches the UTF-16 encoding so each - /// compare in the set hits the OS API directly without re-encoding - + /// `BTreeMap` key for env-block sort+dedup. Caches the UTF-16 encoding so + /// each compare in the map hits the OS API directly without re-encoding - /// matches the approach in `std::sys::process::windows::EnvKey`. struct EnvKey(Vec); @@ -655,23 +663,18 @@ pub(crate) mod os { pub fn set_inheritable(f: &File, inheritable: bool) -> io::Result<()> { win32::SetHandleInformation( - f, + f.as_raw_handle(), win32::HANDLE_FLAG_INHERIT, if inheritable { 1 } else { 0 }, )?; Ok(()) } - pub fn prepare_file(file: File) -> io::Result> { + pub fn prepare_file(redir: Arc) -> io::Result> { // On Windows, child handles must be marked inheritable for CreateProcess to pass // them to the child. - set_inheritable(&file, true)?; - Ok(Arc::new(Redirection::File(file))) - } - - pub fn prepare_file_shared(arc: Arc) -> io::Result> { - set_inheritable(child_file(&arc), true)?; - Ok(arc) + set_inheritable(child_file(&redir), true)?; + Ok(redir) } /// Create a pipe where both ends support overlapped I/O. @@ -797,7 +800,18 @@ pub(crate) mod os { let env = vec![pair("A", "1"), pair("B", "x"), pair("A", "2")]; assert_eq!( parse_block(&format_env_block(&env)), - vec![pair("B", "x"), pair("A", "2")] + vec![pair("A", "2"), pair("B", "x")] + ); + } + + #[test] + fn format_env_block_is_sorted() { + // CreateProcessW requires Unicode env blocks to be sorted by name + // with case-insensitive ordinal comparison. + let env = vec![pair("Z", "1"), pair("a", "2"), pair("M", "3")]; + assert_eq!( + parse_block(&format_env_block(&env)), + vec![pair("a", "2"), pair("M", "3"), pair("Z", "1")] ); } diff --git a/src/tests/communicate.rs b/src/tests/communicate.rs index 015122e..78e6064 100644 --- a/src/tests/communicate.rs +++ b/src/tests/communicate.rs @@ -28,7 +28,7 @@ fn communicate_input() { fn communicate_output() { // Capture both stdout and stderr from a command that writes to both. let mut handle = Exec::cmd("sh") - .args(&["-c", "echo foo; echo bar >&2"]) + .args(["-c", "echo foo; echo bar >&2"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -43,7 +43,7 @@ fn communicate_output() { fn communicate_input_output() { // Feed input data and capture both stdout and stderr. let mut handle = Exec::cmd("sh") - .args(&["-c", "cat; echo foo >&2"]) + .args(["-c", "cat; echo foo >&2"]) .stdin("hello world") .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) @@ -60,7 +60,7 @@ fn communicate_input_output_long() { // Large data in both directions with simultaneous stdout and stderr // output, testing deadlock prevention. let mut handle = Exec::cmd("sh") - .args(&["-c", "cat; printf '%100000s' '' >&2"]) + .args(["-c", "cat; printf '%100000s' '' >&2"]) .stdin(vec![65u8; 1_000_000]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) @@ -77,7 +77,7 @@ fn communicate_timeout() { // A command that produces partial output then sleeps should time out, // and the partial output should still be available. let mut job = Exec::cmd("sh") - .args(&["-c", "printf foo; sleep 1"]) + .args(["-c", "printf foo; sleep 1"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -99,7 +99,7 @@ fn communicate_timeout() { fn communicate_size_limit_small() { // Read with a small size limit, then continue reading in chunks. let mut job = Exec::cmd("sh") - .args(&["-c", "printf '%5s' a"]) + .args(["-c", "printf '%5s' a"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -120,7 +120,7 @@ fn check_vec(v: &[u8], size: usize, content: u8) { fn communicate_size_limit_large() { // Read large output in chunks using limit_size. let mut job = Exec::cmd("sh") - .args(&["-c", "printf '%20001s' a"]) + .args(["-c", "printf '%20001s' a"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -144,7 +144,7 @@ fn communicate_size_limit_different_sizes() { // Change the size limit between successive reads to verify that // the communicator respects the new limit each time. let mut job = Exec::cmd("sh") - .args(&["-c", "printf '%20001s' a"]) + .args(["-c", "printf '%20001s' a"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -181,7 +181,7 @@ fn communicate_stdout_only() { // Capture only stdout (no stderr pipe). Stderr output goes to the // parent's stderr and is not captured. let mut handle = Exec::cmd("sh") - .args(&["-c", "echo hello; echo ignored >&2"]) + .args(["-c", "echo hello; echo ignored >&2"]) .stdout(Redirection::Pipe) .start() .unwrap(); @@ -196,7 +196,7 @@ fn communicate_stderr_only() { // Capture only stderr (no stdout pipe). Stdout output goes to the // parent's stdout and is not captured. let mut handle = Exec::cmd("sh") - .args(&["-c", "echo ignored; echo error >&2"]) + .args(["-c", "echo ignored; echo error >&2"]) .stderr(Redirection::Pipe) .start() .unwrap(); @@ -249,7 +249,7 @@ fn communicate_empty_output() { fn communicate_large_stderr() { // Test large output on stderr specifically. let mut handle = Exec::cmd("sh") - .args(&["-c", "printf '%50000s' x >&2"]) + .args(["-c", "printf '%50000s' x >&2"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -266,7 +266,7 @@ fn communicate_interleaved_output() { // Test interleaved stdout/stderr - both should be captured correctly // in their respective buffers. let mut handle = Exec::cmd("sh") - .args(&["-c", "echo out1; echo err1 >&2; echo out2; echo err2 >&2"]) + .args(["-c", "echo out1; echo err1 >&2; echo out2; echo err2 >&2"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -281,7 +281,7 @@ fn communicate_interleaved_output() { fn communicate_quick_exit() { // Process exits immediately without producing output. let mut handle = Exec::cmd("sh") - .args(&["-c", "exit 0"]) + .args(["-c", "exit 0"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -297,7 +297,7 @@ fn communicate_process_fails() { // Process exits with non-zero status. Communicate should still // succeed and return the captured data. let mut handle = Exec::cmd("sh") - .args(&["-c", "echo output; echo error >&2; exit 42"]) + .args(["-c", "echo output; echo error >&2; exit 42"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -313,7 +313,7 @@ fn communicate_size_limit_zero() { // Size limit of 0 should return empty immediately; continue reading // with a larger limit to get the remaining data. let mut job = Exec::cmd("sh") - .args(&["-c", "printf 'data'"]) + .args(["-c", "printf 'data'"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -334,7 +334,7 @@ fn communicate_size_limit_zero() { fn communicate_size_limit_stderr() { // Size limit should apply to the combined total of stdout + stderr. let mut job = Exec::cmd("sh") - .args(&["-c", "printf out; printf err >&2"]) + .args(["-c", "printf out; printf err >&2"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -351,7 +351,7 @@ fn communicate_size_limit_stderr() { fn communicate_timeout_zero() { // Immediate timeout (zero duration) on a sleeping process. let mut job = Exec::cmd("sh") - .args(&["-c", "sleep 1; echo done"]) + .args(["-c", "sleep 1; echo done"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -414,7 +414,7 @@ fn communicate_partial_read_continue() { // Read with a size limit, then continue reading in multiple chunks // until all data is consumed. let mut job = Exec::cmd("sh") - .args(&["-c", "printf 'abcdefghij'"]) + .args(["-c", "printf 'abcdefghij'"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() @@ -455,7 +455,7 @@ fn communicate_no_streams() { fn communicate_very_long_lines() { // Test with very long output that contains no newlines. let mut handle = Exec::cmd("sh") - .args(&["-c", "printf '%100000s' x"]) + .args(["-c", "printf '%100000s' x"]) .stdout(Redirection::Pipe) .start() .unwrap(); @@ -470,7 +470,7 @@ fn communicate_timeout_with_partial_and_continue() { // Time out while reading, capture partial data, then continue // reading with a longer timeout to get the rest. let mut handle = Exec::cmd("sh") - .args(&["-c", "printf first; sleep 0.5; printf second"]) + .args(["-c", "printf first; sleep 0.5; printf second"]) .stdout(Redirection::Pipe) .stderr(Redirection::Pipe) .start() diff --git a/src/tests/exec.rs b/src/tests/exec.rs index 0ba11e8..e36fde9 100644 --- a/src/tests/exec.rs +++ b/src/tests/exec.rs @@ -42,7 +42,7 @@ fn reject_empty_argv() { #[test] fn err_exit() { let status = Exec::cmd("sh") - .args(&["-c", "exit 13"]) + .args(["-c", "exit 13"]) .start() .unwrap() .wait() @@ -114,7 +114,7 @@ fn stream_stdout() { #[test] fn stream_stderr() { let stream = Exec::cmd("sh") - .args(&["-c", "printf foo >&2"]) + .args(["-c", "printf foo >&2"]) .stream_stderr() .unwrap(); assert_eq!(io::read_to_string(stream).unwrap(), "foo"); @@ -226,7 +226,7 @@ fn reject_input_data_stream_stdin() { fn env_set() { assert!( Exec::cmd("sh") - .args(&["-c", r#"test "$SOMEVAR" = "foo""#]) + .args(["-c", r#"test "$SOMEVAR" = "foo""#]) .env("SOMEVAR", "foo") .join() .unwrap() @@ -238,7 +238,7 @@ fn env_set() { fn env_extend() { assert!( Exec::cmd("sh") - .args(&["-c", r#"test "$VAR1" = "foo" && test "$VAR2" = "bar""#]) + .args(["-c", r#"test "$VAR1" = "foo" && test "$VAR2" = "bar""#]) .env_extend([("VAR1", "foo"), ("VAR2", "bar")]) .join() .unwrap() @@ -282,7 +282,7 @@ fn tmp_env_var<'a>(varname: &'static str, tmp_value: &'static str) -> TmpEnvVar< #[test] fn env_add() { let status = Exec::cmd("sh") - .args(&["-c", r#"test "$SOMEVAR" = "foo""#]) + .args(["-c", r#"test "$SOMEVAR" = "foo""#]) .env("SOMEVAR", "foo") .start() .unwrap() @@ -294,7 +294,7 @@ fn env_add() { #[test] fn env_dup() { let status = Exec::cmd("sh") - .args(&["-c", r#"test "$SOMEVAR" = "bar""#]) + .args(["-c", r#"test "$SOMEVAR" = "bar""#]) .env_clear() .env("SOMEVAR", "foo") .env("SOMEVAR", "bar") @@ -313,7 +313,7 @@ fn env_inherit() { let _guard = tmp_env_var(varname, "inherited"); assert!( Exec::cmd("sh") - .args(&["-c", &format!(r#"test "${}" = "inherited""#, varname)]) + .args(["-c", &format!(r#"test "${}" = "inherited""#, varname)]) .join() .unwrap() .success() @@ -327,7 +327,7 @@ fn env_inherit_set() { let _guard = tmp_env_var(varname, "inherited"); assert!( Exec::cmd("sh") - .args(&["-c", &format!(r#"test "${}" = "new""#, varname)]) + .args(["-c", &format!(r#"test "${}" = "new""#, varname)]) .env(varname, "new") .join() .unwrap() @@ -377,7 +377,7 @@ fn exec_capture_auto_stdout_when_stderr_set() { // Exec::capture() auto-pipes stdout and stderr independently. // Setting stderr(Pipe) does not suppress stdout auto-piping. let c = Exec::cmd("sh") - .args(&["-c", "echo out; echo err >&2"]) + .args(["-c", "echo out; echo err >&2"]) .stderr(Redirection::Pipe) .capture() .unwrap(); @@ -389,7 +389,7 @@ fn exec_capture_auto_stdout_when_stderr_set() { fn exec_capture_auto_pipes_both() { // Bare Exec::cmd(...).capture() auto-pipes both stdout and stderr. let c = Exec::cmd("sh") - .args(&["-c", "echo out; echo err >&2"]) + .args(["-c", "echo out; echo err >&2"]) .capture() .unwrap(); assert_eq!(c.stdout_str().trim(), "out"); @@ -401,7 +401,7 @@ fn exec_communicate_auto_stdout_when_stderr_set() { // Exec::communicate() auto-pipes stdout and stderr independently. // Setting stderr(Pipe) does not suppress stdout auto-piping. let mut comm = Exec::cmd("sh") - .args(&["-c", "echo out; echo err >&2"]) + .args(["-c", "echo out; echo err >&2"]) .stderr(Redirection::Pipe) .communicate() .unwrap(); diff --git a/src/tests/job.rs b/src/tests/job.rs index b07fa36..c8e0c0a 100644 --- a/src/tests/job.rs +++ b/src/tests/job.rs @@ -62,7 +62,7 @@ fn exec_start_stdin_write() { #[test] fn exec_start_stderr() { let mut handle = Exec::cmd("sh") - .args(&["-c", "echo err-output >&2"]) + .args(["-c", "echo err-output >&2"]) .stderr(Redirection::Pipe) .start() .unwrap(); @@ -177,7 +177,7 @@ fn write_to_subprocess() { #[test] fn merge_err_to_out_pipe() { let mut handle = Exec::cmd("sh") - .args(&["-c", "echo foo; echo bar >&2"]) + .args(["-c", "echo foo; echo bar >&2"]) .stdout(Redirection::Pipe) .stderr(Redirection::Merge) .start() @@ -191,7 +191,7 @@ fn merge_err_to_out_pipe() { #[test] fn merge_out_to_err_pipe() { let mut handle = Exec::cmd("sh") - .args(&["-c", "echo foo; echo bar >&2"]) + .args(["-c", "echo foo; echo bar >&2"]) .stdout(Redirection::Merge) .stderr(Redirection::Pipe) .start() @@ -207,7 +207,7 @@ fn merge_err_to_out_file() { let tmpdir = TempDir::new().unwrap(); let tmpname = tmpdir.path().join("output"); let status = Exec::cmd("sh") - .args(&["-c", "printf foo; printf bar >&2"]) + .args(["-c", "printf foo; printf bar >&2"]) .stdout(File::create(&tmpname).unwrap()) .stderr(Redirection::Merge) .start() @@ -320,7 +320,7 @@ fn poll_finished_process() { #[test] fn wait_multiple_times() { - let job = Exec::cmd("sh").args(&["-c", "exit 42"]).start().unwrap(); + let job = Exec::cmd("sh").args(["-c", "exit 42"]).start().unwrap(); let s1 = job.wait().unwrap(); let s2 = job.wait().unwrap(); let s3 = job.wait().unwrap(); @@ -391,14 +391,14 @@ fn detach_does_not_wait_on_drop() { #[test] fn capture_timeout() { match Exec::cmd("sleep") - .args(&["0.5"]) + .args(["0.5"]) .start() .unwrap() .capture_timeout(Duration::from_millis(100)) { Ok(_) => panic!("expected timeout return"), Err(e) => match e.kind() { - ErrorKind::TimedOut => assert!(true), + ErrorKind::TimedOut => {} _ => panic!("expected timeout return"), }, } diff --git a/src/tests/pipeline.rs b/src/tests/pipeline.rs index 81c642b..d9912a0 100644 --- a/src/tests/pipeline.rs +++ b/src/tests/pipeline.rs @@ -38,7 +38,7 @@ fn pipeline_stream_in() { #[test] fn pipeline_stream_err() { - let stream = { Exec::cmd("sh").args(&["-c", "printf foo >&2"]) | Exec::cmd("true") } + let stream = { Exec::cmd("sh").args(["-c", "printf foo >&2"]) | Exec::cmd("true") } .stream_stderr_all() .unwrap(); assert_eq!(io::read_to_string(stream).unwrap(), "foo"); diff --git a/src/tests/posix.rs b/src/tests/posix.rs index 3fa219e..d5ca289 100644 --- a/src/tests/posix.rs +++ b/src/tests/posix.rs @@ -4,6 +4,54 @@ use super::exec_signal_delay; use crate::unix::{ExitStatusExt, JobExt, PipelineExt}; use crate::{Exec, ExecExt, ExitStatus, Redirection}; +// Tests that close fds 0/1/2 in the parent and let `File::create` reclaim the +// freed slot run their body in a fresh single-test child process. While the +// parent has fd 0/1/2 pointed at a tempfile, any sibling thread writing to +// that fd (notably libtest's "test ... ok" status line) would leak into the +// file and corrupt the test's assertions. Re-execing isolates each such test +// from the rest of the run. +// +// The env var name embeds the parent's PID. The child recognizes itself by +// looking up SUBPROCESS_ISOLATED_FD_TEST_ - a stray variable the +// user happens to have set can't match a PID assigned to a not-yet-running +// `cargo test`, so a generic environment leak can't silently disable the +// isolation. +const ISOLATED_TEST_PREFIX: &str = "SUBPROCESS_ISOLATED_FD_TEST_"; + +fn run_isolated(name: &str, body: impl FnOnce()) { + let parent_var = format!("{ISOLATED_TEST_PREFIX}{}", unsafe { libc::getppid() }); + if std::env::var_os(&parent_var).is_some() { + body(); + return; + } + let exe = std::env::current_exe().expect("current_exe"); + let test_path = format!("tests::posix::{name}"); + let child_var = format!("{ISOLATED_TEST_PREFIX}{}", std::process::id()); + let output = std::process::Command::new(&exe) + .args(["--exact", &test_path]) + .env(&child_var, "1") + .output() + .expect("spawning isolated test child"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "isolated child for {test_path} failed: {status}\n\ + --- child stdout ---\n{stdout}\ + --- child stderr ---\n{stderr}", + status = output.status, + ); + // Defense against `name` not matching the surrounding fn name: libtest + // exits 0 when --exact matches no tests, so the call would silently pass + // without running the body. "1 passed" only appears when exactly one + // matching test ran successfully. + assert!( + stdout.contains("1 passed"), + "isolated child for {test_path} matched no tests (typo in name?):\n\ + --- child stdout ---\n{stdout}", + ); +} + #[test] fn err_terminate() { let job = Exec::cmd("sleep").arg("5").start().unwrap(); @@ -68,7 +116,7 @@ fn exec_setpgid() { // child. Signaling the group should terminate both the shell and // its child. let job = Exec::cmd("sh") - .args(&["-c", "sleep 10 & wait"]) + .args(["-c", "sleep 10 & wait"]) .setpgid() .start() .unwrap(); @@ -83,7 +131,7 @@ fn send_signal_group() { // child. Signaling the group should terminate both the shell and // its child. let job = Exec::cmd("sh") - .args(&["-c", "sleep 10 & wait"]) + .args(["-c", "sleep 10 & wait"]) .setpgid() .start() .unwrap(); @@ -234,7 +282,7 @@ fn pre_exec_multiple() { fn arg0_override() { let out = Exec::cmd("sh") .arg0("custom-name") - .args(&["-c", "echo $0"]) + .args(["-c", "echo $0"]) .capture() .unwrap() .stdout_str(); @@ -255,7 +303,7 @@ fn started_send_signal() { #[test] fn started_send_signal_group() { let job = Exec::cmd("sh") - .args(&["-c", "sleep 10 & wait"]) + .args(["-c", "sleep 10 & wait"]) .setpgid() .start() .unwrap(); @@ -294,6 +342,370 @@ fn pipeline_setpgid_rejects_exec_setpgid() { assert!(err.to_string().contains("setpgid")); } +#[test] +fn user_file_at_target_fd_survives_exec() { + // A File passed as redirection whose raw fd already equals the target + // stream fd must remain open in the child after exec. Set up by closing + // fd 0 in the parent and opening a file so it lands on fd 0. + run_isolated("user_file_at_target_fd_survives_exec", || { + use std::fs::File; + use std::os::fd::AsRawFd; + use tempfile::TempDir; + + let tmpdir = TempDir::new().unwrap(); + let tmpname = tmpdir.path().join("input"); + std::fs::write(&tmpname, "stdin-payload").unwrap(); + + let saved = unsafe { libc::dup(0) }; + assert!(saved >= 0); + let close_rc = unsafe { libc::close(0) }; + assert_eq!(close_rc, 0); + let f = File::open(&tmpname).unwrap(); + assert_eq!(f.as_raw_fd(), 0, "test setup: file did not land at fd 0"); + // Park the parent's original stdin at fd 100 until we restore it. + let dup_rc = unsafe { libc::dup2(saved, 100) }; + assert!(dup_rc >= 0); + unsafe { + libc::close(saved); + } + + let result = Exec::cmd("cat") + .stdin(f) + .stdout(Redirection::Pipe) + .stderr(Redirection::Pipe) + .capture(); + + // Restore the parent's stdin. + unsafe { + libc::dup2(100, 0); + libc::close(100); + } + + let c = result.expect("capture failed"); + assert_eq!( + c.stdout_str(), + "stdin-payload", + "stderr was: {:?}", + c.stderr_str() + ); + assert!(c.exit_status.success()); + }); +} + +#[test] +fn user_file_at_other_standard_fd_preserves_inherited_stream() { + // A File passed as redirection whose raw fd is a standard fd *other than* + // its target_fd (e.g., file at fd 2 used as stdout) must not have that + // standard fd closed by install_child_fd. Otherwise the child loses its + // inherited standard stream that was at that slot. + // + // Set up by closing fd 2 and opening a file so it lands on fd 2, then use + // it as stdout. fd 2 must still be open in the child when pre_exec runs. + run_isolated( + "user_file_at_other_standard_fd_preserves_inherited_stream", + || { + use std::fs::File; + use std::io::Read; + use std::os::fd::AsRawFd; + use tempfile::TempDir; + + let tmpdir = TempDir::new().unwrap(); + let tmpname = tmpdir.path().join("output"); + + let saved = unsafe { libc::dup(2) }; + assert!(saved >= 0); + let close_rc = unsafe { libc::close(2) }; + assert_eq!(close_rc, 0); + let f = File::create(&tmpname).unwrap(); + assert_eq!(f.as_raw_fd(), 2, "test setup: file did not land at fd 2"); + // Park the parent's original stderr at fd 100 until we restore it. + let dup_rc = unsafe { libc::dup2(saved, 100) }; + assert!(dup_rc >= 0); + unsafe { + libc::close(saved); + } + + // Pipe to receive the child's report on whether fd 2 is still open + // after install_child_fd has run. The pipe ends are CLOEXEC, so the + // write end closes at exec without us needing to set anything up + // here. + let (mut read_end, write_end) = crate::posix::pipe().unwrap(); + let report_fd = write_end.as_raw_fd(); + + let result = unsafe { + Exec::cmd("true") + .stdout(f) + .pre_exec(move || { + let r = libc::fcntl(2, libc::F_GETFD); + let msg: &[u8] = if r >= 0 { b"open" } else { b"clsd" }; + libc::write(report_fd, msg.as_ptr().cast(), msg.len()); + Ok(()) + }) + .start() + }; + + // Restore the parent's stderr. + unsafe { + libc::dup2(100, 2); + libc::close(100); + } + drop(write_end); + + let job = result.expect("start failed"); + let mut buf = [0u8; 4]; + read_end.read_exact(&mut buf).unwrap(); + let _ = job.wait(); + assert_eq!( + &buf, b"open", + "fd 2 was closed in the child by install_child_fd" + ); + }, + ); +} + +#[test] +fn stdin_pipe_with_user_stdout_at_fd_0() { + // A user-supplied File whose raw fd is 0, used as stdout, must not be + // clobbered by the install_child_fd call that places stdin onto fd 0. + // redirect_streams must install stdout (which dup2s from fd 0) before + // stdin (which overwrites fd 0). + run_isolated("stdin_pipe_with_user_stdout_at_fd_0", || { + use std::fs::File; + use std::os::fd::AsRawFd; + use tempfile::TempDir; + + let tmpdir = TempDir::new().unwrap(); + let outfile = tmpdir.path().join("output"); + + let saved = unsafe { libc::dup(0) }; + assert!(saved >= 0); + assert_eq!(unsafe { libc::close(0) }, 0); + let f = File::create(&outfile).unwrap(); + assert_eq!(f.as_raw_fd(), 0, "test setup: file did not land at fd 0"); + assert!(unsafe { libc::dup2(saved, 100) } >= 0); + unsafe { + libc::close(saved); + } + + let result = Exec::cmd("printf") + .args(["%s", "hello"]) + .stdin(Redirection::Pipe) + .stdout(f) + .stderr(Redirection::Pipe) + .capture(); + + unsafe { + libc::dup2(100, 0); + libc::close(100); + } + + let c = result.expect("capture failed"); + assert!( + c.exit_status.success(), + "printf failed; stderr: {:?}", + c.stderr_str() + ); + let content = std::fs::read_to_string(&outfile).unwrap(); + assert_eq!(content, "hello"); + }); +} + +#[test] +fn stdout_pipe_with_user_stderr_at_fd_1() { + // A user-supplied File whose raw fd is 1, used as stderr, must not be + // clobbered by the install_child_fd call that places stdout onto fd 1. + // redirect_streams must install stderr (which dup2s from fd 1) before + // stdout (which overwrites fd 1). + run_isolated("stdout_pipe_with_user_stderr_at_fd_1", || { + use std::fs::File; + use std::os::fd::AsRawFd; + use tempfile::TempDir; + + let tmpdir = TempDir::new().unwrap(); + let errfile = tmpdir.path().join("err"); + + let saved = unsafe { libc::dup(1) }; + assert!(saved >= 0); + assert_eq!(unsafe { libc::close(1) }, 0); + let f = File::create(&errfile).unwrap(); + assert_eq!(f.as_raw_fd(), 1, "test setup: file did not land at fd 1"); + assert!(unsafe { libc::dup2(saved, 100) } >= 0); + unsafe { + libc::close(saved); + } + + let result = Exec::cmd("sh") + .args(["-c", "echo to-stdout; echo to-stderr >&2"]) + .stdout(Redirection::Pipe) + .stderr(f) + .capture(); + + unsafe { + libc::dup2(100, 1); + libc::close(100); + } + + let c = result.expect("capture failed"); + assert!(c.exit_status.success()); + assert_eq!(c.stdout_str().trim(), "to-stdout"); + let stderr_content = std::fs::read_to_string(&errfile).unwrap(); + assert_eq!(stderr_content.trim(), "to-stderr"); + }); +} + +#[test] +fn stdin_pipe_with_user_stderr_at_fd_0() { + // A user-supplied File whose raw fd is 0, used as stderr, must not be + // clobbered by the install_child_fd call that places stdin onto fd 0. + run_isolated("stdin_pipe_with_user_stderr_at_fd_0", || { + use std::fs::File; + use std::os::fd::AsRawFd; + use tempfile::TempDir; + + let tmpdir = TempDir::new().unwrap(); + let errfile = tmpdir.path().join("err"); + + let saved = unsafe { libc::dup(0) }; + assert!(saved >= 0); + assert_eq!(unsafe { libc::close(0) }, 0); + let f = File::create(&errfile).unwrap(); + assert_eq!(f.as_raw_fd(), 0, "test setup: file did not land at fd 0"); + assert!(unsafe { libc::dup2(saved, 100) } >= 0); + unsafe { + libc::close(saved); + } + + let result = Exec::cmd("sh") + .args(["-c", "echo to-stderr >&2"]) + .stdin(Redirection::Pipe) + .stdout(Redirection::Pipe) + .stderr(f) + .capture(); + + unsafe { + libc::dup2(100, 0); + libc::close(100); + } + + let c = result.expect("capture failed"); + assert!(c.exit_status.success()); + let stderr_content = std::fs::read_to_string(&errfile).unwrap(); + assert_eq!(stderr_content.trim(), "to-stderr"); + }); +} + +#[test] +fn user_files_with_swapped_fds_resolve_cycle() { + // The cyclic case: stdout's source fd is stderr's target, and stderr's + // source fd is stdout's target. No reorder alone can install both + // correctly; redirect_streams must dup one source via F_DUPFD_CLOEXEC to + // break the cycle. + run_isolated("user_files_with_swapped_fds_resolve_cycle", || { + use std::fs::File; + use std::os::fd::AsRawFd; + use tempfile::TempDir; + + let tmpdir = TempDir::new().unwrap(); + let path_at_1 = tmpdir.path().join("at_fd_1"); + let path_at_2 = tmpdir.path().join("at_fd_2"); + + let saved_1 = unsafe { libc::dup(1) }; + let saved_2 = unsafe { libc::dup(2) }; + assert!(saved_1 >= 0 && saved_2 >= 0); + assert_eq!(unsafe { libc::close(1) }, 0); + assert_eq!(unsafe { libc::close(2) }, 0); + + let file_at_1 = File::create(&path_at_1).unwrap(); + assert_eq!(file_at_1.as_raw_fd(), 1); + let file_at_2 = File::create(&path_at_2).unwrap(); + assert_eq!(file_at_2.as_raw_fd(), 2); + + assert!(unsafe { libc::dup2(saved_1, 100) } >= 0); + assert!(unsafe { libc::dup2(saved_2, 101) } >= 0); + unsafe { + libc::close(saved_1); + libc::close(saved_2); + } + + // file_at_2 (fd=2) used as stdout, file_at_1 (fd=1) used as stderr -> cycle. + let result = Exec::cmd("sh") + .args(["-c", "echo out; echo err >&2"]) + .stdout(file_at_2) + .stderr(file_at_1) + .join(); + + unsafe { + libc::dup2(100, 1); + libc::dup2(101, 2); + libc::close(100); + libc::close(101); + } + + let status = result.expect("spawn failed"); + assert!(status.success()); + + let content_at_1 = std::fs::read_to_string(&path_at_1).unwrap(); + let content_at_2 = std::fs::read_to_string(&path_at_2).unwrap(); + assert_eq!( + content_at_1.trim(), + "err", + "stderr file should contain 'err'" + ); + assert_eq!( + content_at_2.trim(), + "out", + "stdout file should contain 'out'" + ); + }); +} + +#[cfg(target_os = "linux")] +#[test] +fn pipeline_stderr_all_non_cloexec_file_does_not_leak() { + // Regression test: a Pipeline with stderr_all(File) shares one Arc across + // all commands. Non-last commands see Arc::strong_count > 1 in the child, + // which used to short-circuit prevent_dealloc and leave the source fd open + // in the child. If the user's File lacks CLOEXEC, the fd survived into the + // exec'd binary. + use std::fs::File; + use std::os::fd::AsRawFd; + use tempfile::TempDir; + + let tmpdir = TempDir::new().unwrap(); + let errfile = tmpdir.path().join("err"); + let report = tmpdir.path().join("report"); + + let f = File::create(&errfile).unwrap(); + let raw = f.as_raw_fd(); + // Clear CLOEXEC so a leaked fd would otherwise survive exec. + unsafe { + let flags = libc::fcntl(raw, libc::F_GETFD); + assert!(flags >= 0); + let r = libc::fcntl(raw, libc::F_SETFD, flags & !libc::FD_CLOEXEC); + assert_eq!(r, 0); + } + + // First (non-last) child checks whether fd `raw` is open in its address + // space after exec. With the bug present the file is still mapped at fd + // `raw`; with the fix CLOEXEC has been set so /proc/self/fd/ is gone. + let check_cmd = format!( + "if [ -e /proc/self/fd/{} ]; then echo LEAK > {}; \ + else echo CLEAR > {}; fi", + raw, + report.display(), + report.display(), + ); + let p = Exec::shell(check_cmd) | Exec::cmd("true"); + p.stderr_all(f).join().unwrap(); + + let content = std::fs::read_to_string(&report).unwrap(); + assert_eq!( + content.trim(), + "CLEAR", + "user File fd was leaked into a non-last pipeline child" + ); +} + #[test] fn null_redirect_does_not_leak_fd() { // Regression test for issue #81. When bash spawns a background process ("sleep 10 @@ -302,7 +714,7 @@ fn null_redirect_does_not_leak_fd() { // backgrounded sleep keeps them open and join() hangs. let start = Instant::now(); let status = Exec::cmd("sh") - .args(&["-c", "sleep 10 &"]) + .args(["-c", "sleep 10 &"]) .stdout(Redirection::Null) .stderr(Redirection::Null) .join() @@ -313,3 +725,63 @@ fn null_redirect_does_not_leak_fd() { "join() took too long, /dev/null fds may have leaked" ); } + +#[test] +fn poll_does_not_block_during_wait() { + // Process::poll() is documented as non-blocking. Verify that a poll() in one thread + // is not serialized behind a blocking wait() in another, even though both touch the + // shared exit-status state. + use std::thread; + + let job = Exec::cmd("sleep").arg("5").start().unwrap(); + let process = job.processes[0].clone(); + + // Park a thread in wait(). + let waiter_proc = process.clone(); + let waiter = thread::spawn(move || waiter_proc.wait().unwrap()); + + // Give the waiter time to actually enter the blocking syscall. + thread::sleep(Duration::from_millis(100)); + + let start = Instant::now(); + let status = process.poll(); + let elapsed = start.elapsed(); + assert!(status.is_none(), "child should still be running"); + assert!( + elapsed < Duration::from_millis(200), + "poll() took {:?}, expected to return immediately while wait() blocks", + elapsed + ); + + // Unblock the waiter so the test doesn't sit out the child's full sleep. + process.terminate().unwrap(); + let _ = waiter.join().unwrap(); +} + +#[test] +fn terminate_during_wait() { + // terminate() from one thread must reach the child while another thread is blocked + // in wait(), and must not signal a recycled PID after the child has been reaped. + use std::thread; + + let job = Exec::cmd("sleep").arg("10").start().unwrap(); + let process = job.processes[0].clone(); + + let waiter_proc = process.clone(); + let waiter = thread::spawn(move || waiter_proc.wait().unwrap()); + + // Let the waiter reach its blocking syscall before we signal. + thread::sleep(Duration::from_millis(100)); + + let start = Instant::now(); + process.terminate().unwrap(); + let term_elapsed = start.elapsed(); + assert!( + term_elapsed < Duration::from_millis(200), + "terminate() took {:?}, expected to return immediately while wait() blocks", + term_elapsed + ); + + let status = waiter.join().unwrap(); + assert!(status.is_killed_by(libc::SIGTERM)); +} diff --git a/src/win32.rs b/src/win32.rs index dba8e01..0f96a7a 100644 --- a/src/win32.rs +++ b/src/win32.rs @@ -122,6 +122,9 @@ pub fn make_pipe() -> Result<(File, File)> { &mut sa as LPSECURITY_ATTRIBUTES, ) })?; + // Take ownership immediately so a CreateFileW failure below closes the named pipe + // via File::Drop instead of leaking the handle. + let write_file = unsafe { File::from_raw_handle(write_handle) }; let read_handle = check_handle(unsafe { CreateFileW( pipe_name.as_ptr(), @@ -133,12 +136,7 @@ pub fn make_pipe() -> Result<(File, File)> { ptr::null_mut(), ) })?; - Ok(unsafe { - ( - File::from_raw_handle(read_handle), - File::from_raw_handle(write_handle), - ) - }) + Ok((unsafe { File::from_raw_handle(read_handle) }, write_file)) } /// Create a manual-reset event object for use with overlapped I/O. @@ -479,8 +477,8 @@ pub fn WaitForMultipleObjects( } } -pub fn SetHandleInformation(handle: &File, dwMask: u32, dwFlags: u32) -> Result<()> { - check(unsafe { handleapi::SetHandleInformation(handle.as_raw_handle(), dwMask, dwFlags) })?; +pub fn SetHandleInformation(handle: RawHandle, mask: u32, flags: u32) -> Result<()> { + check(unsafe { handleapi::SetHandleInformation(handle, mask, flags) })?; Ok(()) }