From 8563627f0c4a16156e389d4e32e4a6cfb9951d21 Mon Sep 17 00:00:00 2001 From: "Guillem L. Jara" <4lon3ly0@tutanota.com> Date: Wed, 1 Jul 2026 17:33:19 +0200 Subject: [PATCH] fix(timeout, uucore): use sigwait and timers for waiting --- src/uu/timeout/src/timeout.rs | 121 +++++------- src/uucore/src/lib/features/process.rs | 263 ++++++++++++++++++++++--- tests/by-util/test_timeout.rs | 4 + 3 files changed, 296 insertions(+), 92 deletions(-) diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs index 6646540d86e..546fba04a26 100644 --- a/src/uu/timeout/src/timeout.rs +++ b/src/uu/timeout/src/timeout.rs @@ -12,12 +12,13 @@ use clap::{Arg, ArgAction, Command}; use std::io::{ErrorKind, Write}; use std::os::unix::process::ExitStatusExt; use std::process::{self, Child, Stdio}; -use std::sync::atomic::{self, AtomicBool}; use std::time::Duration; use uucore::display::Quotable; use uucore::error::{UResult, USimpleError, UUsageError}; use uucore::parser::parse_time; -use uucore::process::ChildExt; +#[cfg(unix)] +use uucore::process::unblock_signal; +use uucore::process::{ChildExt, TimeoutRet, timeout_signal_set}; use uucore::signals::install_signal_handler; use uucore::translate; @@ -184,41 +185,6 @@ fn install_sigchld() { let _ = install_signal_handler(Signal::as_raw(Signal::CHILD), chld); } -/// We should terminate child process when receiving termination signals. -static SIGNALED: AtomicBool = AtomicBool::new(false); -/// Track which signal was received (0 = none/timeout expired naturally). -static RECEIVED_SIGNAL: atomic::AtomicI32 = atomic::AtomicI32::new(0); - -/// Install signal handlers for termination signals. -fn install_signal_handlers(term_signal: usize) { - extern "C" fn handle_signal(sig: libc::c_int) { - SIGNALED.store(true, atomic::Ordering::Relaxed); - RECEIVED_SIGNAL.store(sig, atomic::Ordering::Relaxed); - } - - let sigpipe_ignored = uucore::signals::sigpipe_was_ignored(); - - for sig in [ - Signal::ALARM, - Signal::INT, - Signal::QUIT, - Signal::HUP, - Signal::TERM, - Signal::PIPE, - Signal::USR1, - Signal::USR2, - ] { - if sig == Signal::PIPE && sigpipe_ignored { - continue; // Skip SIGPIPE if it was ignored by parent - } - let _ = install_signal_handler(Signal::as_raw(sig), handle_signal); - } - - if let Some(sig) = signal_from_raw(term_signal as i32) { - let _ = install_signal_handler(Signal::as_raw(sig), handle_signal); - } -} - /// Report that a signal is being sent if the verbose flag is set. fn report_if_verbose(signal: usize, cmd: &str, verbose: bool) { if verbose { @@ -302,9 +268,8 @@ fn wait_or_kill_process( foreground: bool, verbose: bool, ) -> std::io::Result { - // ignore `SIGTERM` here - match process.wait_or_timeout(duration, None) { - Ok(Some(status)) => { + match process.wait_or_timeout(duration, true) { + Ok(TimeoutRet::Exited(status)) => { if preserve_status { let exit_code = status.code().unwrap_or_else(|| { status.signal().unwrap_or_else(|| { @@ -318,7 +283,8 @@ fn wait_or_kill_process( Ok(ExitStatus::CommandTimedOut.into()) } } - Ok(None) => { + // GNU timeout also kills on signals other than SIGTERM. + Ok(TimeoutRet::TimedOut) | Ok(TimeoutRet::Interrupted(_)) => { let signal = signal_by_name_or_value("KILL").unwrap(); report_if_verbose(signal, cmd, verbose); send_signal(process, signal, foreground); @@ -344,6 +310,7 @@ fn preserve_signal_info(signal: libc::c_int) -> libc::c_int { // ourselves with whatever signal our child exited with, which is // what the following is intended to accomplish. if let Some(sig) = signal_from_raw(signal) { + let _ = unblock_signal(sig); let _ = kill_process(getpid(), sig); } signal @@ -395,6 +362,7 @@ fn timeout( if stdin_was_closed { libc::close(libc::STDIN_FILENO); } + let _ = timeout_signal_set().thread_unblock(); #[cfg(target_os = "linux")] let _ = rustix::process::set_parent_process_death_signal(death_sig); Ok(()) @@ -403,7 +371,9 @@ fn timeout( } install_sigchld(); - install_signal_handlers(signal); + timeout_signal_set() + .thread_block() + .map_err(|err| USimpleError::new(ExitStatus::TimeoutFailed.into(), err.to_string()))?; let process = &mut cmd_builder.spawn().map_err(|err| { let status_code = match err.kind() { @@ -419,17 +389,20 @@ fn timeout( // Wait for the child process for the specified time period. // - // If the process exits within the specified time period (the - // `Ok(Some(_))` arm), then return the appropriate status code. + // If the process exits within the specified time period, + // then we return the appropriate status code. + // + // If the wait is interrupted by an external signal, + // then we forward it to the child. // - // If the process does not exit within that time (the `Ok(None)` - // arm) and `kill_after` is specified, then try sending `SIGKILL`. + // If the duration elapses naturally, we send `signal`; + // likewise for `kill_after`. // // TODO The structure of this block is extremely similar to the // structure of `wait_or_kill_process()`. They can probably be // refactored into some common function. - match process.wait_or_timeout(duration, Some(&SIGNALED)) { - Ok(Some(status)) => { + match process.wait_or_timeout(duration, false) { + Ok(TimeoutRet::Exited(status)) => { let exit_code = status.code().unwrap_or_else(|| { status .signal() @@ -437,17 +410,9 @@ fn timeout( }); Err(exit_code.into()) } - Ok(None) => { - let received_sig = RECEIVED_SIGNAL.load(atomic::Ordering::Relaxed); - let is_external_signal = received_sig > 0 && received_sig != libc::SIGALRM; - let signal_to_send = if is_external_signal { - received_sig as usize - } else { - signal - }; - - report_if_verbose(signal_to_send, &cmd[0], verbose); - send_signal(process, signal_to_send, foreground); + Ok(TimeoutRet::TimedOut) => { + report_if_verbose(signal, &cmd[0], verbose); + send_signal(process, signal, foreground); if let Some(kill_after) = kill_after { return match wait_or_kill_process( @@ -467,24 +432,46 @@ fn timeout( } let status = process.wait()?; - if is_external_signal { - Err(ExitStatus::SignalSent(received_sig as usize).into()) - } else if SIGNALED.load(atomic::Ordering::Relaxed) { - Err(ExitStatus::CommandTimedOut.into()) - } else if preserve_status { - Err(status + if preserve_status { + let exit_code = status .code() .or_else(|| { status .signal() .map(|s| ExitStatus::SignalSent(s as usize).into()) }) - .unwrap_or(ExitStatus::CommandTimedOut.into()) - .into()) + .unwrap_or_else(|| ExitStatus::CommandTimedOut.into()); + Err(exit_code.into()) } else { Err(ExitStatus::CommandTimedOut.into()) } } + Ok(TimeoutRet::Interrupted(sig)) => { + let signal_to_send = Signal::as_raw(sig) as usize; + + report_if_verbose(signal_to_send, &cmd[0], verbose); + send_signal(process, signal_to_send, foreground); + + if let Some(kill_after) = kill_after { + return match wait_or_kill_process( + process, + &cmd[0], + kill_after, + preserve_status, + foreground, + verbose, + ) { + Ok(status) => Err(status.into()), + Err(e) => Err(USimpleError::new( + ExitStatus::TimeoutFailed.into(), + e.to_string(), + )), + }; + } + + process.wait()?; + Err(ExitStatus::SignalSent(signal_to_send).into()) + } Err(_) => { // We're going to return ERR_EXIT_STATUS regardless of // whether `send_signal()` succeeds or fails diff --git a/src/uucore/src/lib/features/process.rs b/src/uucore/src/lib/features/process.rs index 06274eb59f9..68e4f334e83 100644 --- a/src/uucore/src/lib/features/process.rs +++ b/src/uucore/src/lib/features/process.rs @@ -6,19 +6,20 @@ // spell-checker:ignore (vars) cvar exitstatus cmdline kworker getsid getpid // spell-checker:ignore (sys/unix) WIFSIGNALED ESRCH // spell-checker:ignore pgrep pwait snice getpgrp +// spell-checker:ignore sigwait KTIME timeval itimerval setitimer itimer timerid +// spell-checker:ignore sigevent sigev sigval itimerspec signo clockid sevp use libc::{gid_t, pid_t, uid_t}; #[cfg(not(target_os = "redox"))] use nix::errno::Errno; -use nix::sys::signal::{self as nix_signal, SigHandler, Signal}; +use nix::sys::signal::{self as nix_signal, SigHandler, SigSet, Signal}; use nix::unistd::Pid; +use rustix::process::Signal as RixSignal; use std::io; use std::process::Child; use std::process::ExitStatus; -use std::sync::atomic; -use std::sync::atomic::AtomicBool; -use std::thread; use std::time::{Duration, Instant}; +use timer::Timer; /// `geteuid()` returns the effective user ID of the calling process. pub fn geteuid() -> uid_t { @@ -88,11 +89,13 @@ pub trait ChildExt { /// Wait for a process to finish or return after the specified duration. /// A `timeout` of zero disables the timeout. - fn wait_or_timeout( - &mut self, - timeout: Duration, - signaled: Option<&AtomicBool>, - ) -> io::Result>; + fn wait_or_timeout(&mut self, timeout: Duration, ignore_term: bool) -> io::Result; +} + +pub enum TimeoutRet { + Interrupted(RixSignal), + Exited(ExitStatus), + TimedOut, } impl ChildExt for Child { @@ -134,36 +137,246 @@ impl ChildExt for Child { result.map_err(|e| io::Error::from_raw_os_error(e as i32)) } - fn wait_or_timeout( - &mut self, - timeout: Duration, - signaled: Option<&AtomicBool>, - ) -> io::Result> { + fn wait_or_timeout(&mut self, timeout: Duration, ignore_term: bool) -> io::Result { if timeout == Duration::from_micros(0) { - return self.wait().map(Some); + return self.wait().map(TimeoutRet::Exited); } // .try_wait() doesn't drop stdin, so we do it manually drop(self.stdin.take()); + // Waits continuously whenever we receive an external SIGCHLD or + // we SIGTERM when we are ignoring them. let start = Instant::now(); + let mut remaining = timeout; + let mut timer = Timer::new()?; loop { - if let Some(status) = self.try_wait()? { - return Ok(Some(status)); + match timer.timed_sigwait(remaining) { + Ok(None) => break Ok(TimeoutRet::TimedOut), + Ok(Some(Signal::SIGCHLD)) => { + if let Some(status) = self.try_wait()? { + break Ok(TimeoutRet::Exited(status)); + } // otherwise waits again + } + Ok(Some(Signal::SIGTERM)) if ignore_term => {} // waits again + // SAFETY: nix's Signal is also a valid rustix Signal. + Ok(Some(signal)) => { + break Ok(TimeoutRet::Interrupted(unsafe { + RixSignal::from_raw_unchecked(signal as _) + })); + } + Err(e) => break Err(e), } + remaining = timeout.saturating_sub(start.elapsed()); + } + } +} + +/// These signals must be blocked before calling [`ChildExt::wait_or_timeout`]. +/// Consider unblocking them in the child's pre-exec hook. +pub fn timeout_signal_set() -> SigSet { + let mut set = SigSet::empty(); + set.add(Signal::SIGALRM); + set.add(Signal::SIGINT); + set.add(Signal::SIGQUIT); + set.add(Signal::SIGHUP); + set.add(Signal::SIGTERM); + set.add(Signal::SIGPIPE); + set.add(Signal::SIGUSR1); + set.add(Signal::SIGUSR2); + set.add(Signal::SIGCHLD); + set +} + +/// Unblocks a signal from the current thread. +pub fn unblock_signal(signal: RixSignal) -> io::Result<()> { + let mut set = SigSet::empty(); + // SAFETY: rustix's Signal is also a valid nix Signal. + set.add(unsafe { Signal::try_from(signal.as_raw()).unwrap_unchecked() }); + + set.thread_unblock().map_err(Into::into) +} + +/// Ensures there is no overflow on time_t operations. Some BSDs (notably XNU) +/// will return EINVAL otherwise; POSIX only defines it up to 10e8, so we cap +/// it on all targets we do not trust to support the full integer range. +const MAX_KTIME_T: Duration = if cfg!(target_os = "linux") { + Duration::from_secs(9_223_372_036) +} else { + Duration::from_secs(100_000_000) +}; + +/// Sets up a timer on SIGALRM for platforms that support POSIX.1-2008 realtime +/// clock extensions. Notably, both Android and Redox do not support the latter +/// fallback since it was removed in that same spec. +#[cfg(not(any(target_vendor = "apple", target_os = "openbsd", target_os = "windows")))] +mod timer { + use super::MAX_KTIME_T; + use std::io; + use std::ptr::null_mut; + use std::time::Duration; + #[cfg(any(target_os = "redox", target_os = "android"))] + use timer_sys as libc; // Complements their libc bindings. + + pub(super) struct Timer(libc::timer_t); - if start.elapsed() >= timeout - || signaled.is_some_and(|signaled| signaled.load(atomic::Ordering::Relaxed)) + impl Timer { + pub(super) fn new() -> io::Result { + use std::mem::MaybeUninit; + + // SAFETY: we must zero the reserved, private bits and other fields. + // We cannot use nix or rustix because they don't support it in Redox. + let mut sev: libc::sigevent = unsafe { MaybeUninit::zeroed().assume_init() }; + sev.sigev_notify = libc::SIGEV_SIGNAL; + sev.sigev_signo = libc::SIGALRM; + + // SAFETY: On cygwin, it's a u64; otherwise, a ptr with exposed provenance. + let mut timer_id = unsafe { MaybeUninit::zeroed().assume_init() }; + // SAFETY: All values are properly initialized. + if unsafe { libc::timer_create(libc::CLOCK_MONOTONIC, &raw mut sev, &raw mut timer_id) } + == -1 { - break; + return Err(io::Error::last_os_error()); + } + + Ok(Self(timer_id)) + } + + pub(super) fn arm(&mut self, timeout: Duration) -> Result<(), io::Error> { + let timeout = timeout.min(MAX_KTIME_T).max(Duration::from_micros(1)); + let time = libc::itimerspec { + it_interval: libc::timespec { + tv_sec: 0, + tv_nsec: 0, + }, + it_value: libc::timespec { + tv_sec: timeout.as_secs() as _, + tv_nsec: timeout.subsec_nanos() as _, + }, + }; + + // SAFETY: All values are properly initialized. + if unsafe { libc::timer_settime(self.0, 0, &raw const time, null_mut()) } == -1 { + return Err(io::Error::last_os_error()); } + Ok(()) + } + } - // XXX: this is kinda gross, but it's cleaner than starting a thread just to wait - // (which was the previous solution). We might want to use a different duration - // here as well - thread::sleep(Duration::from_millis(100)); + impl Drop for Timer { + fn drop(&mut self) { + unsafe { libc::timer_delete(self.0) }; } + } + + /// Complements the libc bindings of Redox and Android with missing items. + #[cfg(any(target_os = "redox", target_os = "android"))] + #[allow(non_camel_case_types)] + mod timer_sys { + pub(super) use libc::{CLOCK_MONOTONIC, SIGALRM, timespec}; + #[cfg(not(target_os = "redox"))] + pub(super) use libc::{SIGEV_SIGNAL, sigevent}; + + pub(super) type timer_t = *mut libc::c_void; - Ok(None) + unsafe extern "C" { + pub(super) fn timer_settime( + timerid: timer_t, + flags: libc::c_int, + new_value: *const itimerspec, + old_value: *mut itimerspec, + ) -> libc::c_int; + + pub(super) fn timer_create( + clockid: libc::clockid_t, + sevp: *mut sigevent, + timerid: *mut timer_t, + ) -> libc::c_int; + + pub(super) fn timer_delete(timerid: timer_t) -> libc::c_int; + } + + #[repr(C)] + #[derive(Clone, Copy, Debug)] + pub(super) struct itimerspec { + pub(super) it_interval: timespec, + pub(super) it_value: timespec, + } + + #[cfg(target_os = "redox")] + pub(super) const SIGEV_SIGNAL: libc::c_int = 0; + + #[repr(C)] + #[derive(Clone, Copy, Debug)] + #[cfg(target_os = "redox")] + pub(super) struct sigevent { + pub(super) sigev_value: libc::sigval, + pub(super) sigev_signo: libc::c_int, + pub(super) sigev_notify: libc::c_int, + pub(super) sigev_notify_thread_id: libc::c_int, + #[cfg(target_pointer_width = "64")] + __unused1: std::mem::MaybeUninit<[libc::c_int; 11]>, + #[cfg(target_pointer_width = "32")] + __unused1: std::mem::MaybeUninit<[libc::c_int; 12]>, + } + } +} + +/// Sets up a timer on SIGALRM for platforms that do not support POSIX.1-2008 +/// realtime clock extensions. Notably, Darwin, OpenBSD, and Windows. +#[cfg(any(target_vendor = "apple", target_os = "openbsd", target_os = "windows"))] +mod timer { + use super::MAX_KTIME_T; + use nix::errno::Errno; + use std::io; + use std::ptr::null_mut; + use std::time::Duration; + + pub(super) struct Timer; + + impl Timer { + #[allow(clippy::unnecessary_wraps)] + pub(super) fn new() -> io::Result { + Ok(Self) + } + + pub(super) fn arm(&mut self, timeout: Duration) -> io::Result<()> { + let timeout = timeout.min(MAX_KTIME_T).max(Duration::from_micros(1)); + let time = libc::itimerval { + it_interval: libc::timeval { + tv_sec: 0, + tv_usec: 0, + }, + it_value: libc::timeval { + tv_sec: timeout.as_secs() as _, + tv_usec: timeout.subsec_micros() as _, + }, + }; + + // SAFETY: All values are properly initialized. + Errno::result(unsafe { + libc::setitimer(libc::ITIMER_REAL, &raw const time, null_mut()) + })?; + Ok(()) + } + } +} + +impl Timer { + fn timed_sigwait(&mut self, timeout: Duration) -> io::Result> { + self.arm(timeout)?; + + let set = timeout_signal_set(); + let mut sig = 0; + // SAFETY: All values are properly initialized. + let res = unsafe { libc::sigwait(set.as_ref(), &raw mut sig) }; + + if res != 0 { + Err(io::Error::from_raw_os_error(res)) + } else if sig == libc::SIGALRM { + Ok(None) + } else { + Ok(Some(Signal::try_from(sig)?)) + } } } diff --git a/tests/by-util/test_timeout.rs b/tests/by-util/test_timeout.rs index 5e24fdb1095..3ed46bf5c4f 100644 --- a/tests/by-util/test_timeout.rs +++ b/tests/by-util/test_timeout.rs @@ -210,6 +210,10 @@ fn test_hex_timeout_ending_with_d() { } #[test] +#[cfg_attr( + all(target_os = "macos", target_arch = "x86_64"), + ignore = "wontfix: intermittent failure in obsolete macOS x86_64 (macOS bug)." +)] fn test_terminate_child_on_receiving_terminate() { let mut timeout_cmd = new_ucmd!() .args(&[