diff --git a/.vscode/cspell.dictionaries/jargon.wordlist.txt b/.vscode/cspell.dictionaries/jargon.wordlist.txt index 33f4959487c..e9593ac1b32 100644 --- a/.vscode/cspell.dictionaries/jargon.wordlist.txt +++ b/.vscode/cspell.dictionaries/jargon.wordlist.txt @@ -139,8 +139,17 @@ setfattr setlocale shortcode shortcodes +setpgid sigaction +CHLD +chld +SIGCHLD +sigchld siginfo +SIGTTIN +sigttin +SIGTTOU +sigttou sigusr strcasecmp subcommand @@ -219,3 +228,5 @@ VMULL vmull SETFL tmpfs +pdeathsig +prctl diff --git a/src/uu/timeout/src/status.rs b/src/uu/timeout/src/status.rs index 1134fb88d8c..70fa2c09726 100644 --- a/src/uu/timeout/src/status.rs +++ b/src/uu/timeout/src/status.rs @@ -33,9 +33,6 @@ pub(crate) enum ExitStatus { /// When a signal is sent to the child process or `timeout` itself. SignalSent(usize), - - /// When `SIGTERM` signal received. - Terminated, } impl From for i32 { @@ -46,7 +43,6 @@ impl From for i32 { ExitStatus::CannotInvoke => 126, ExitStatus::CommandNotFound => 127, ExitStatus::SignalSent(s) => 128 + s as Self, - ExitStatus::Terminated => 143, } } } diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs index de20bec83c3..a0cd0a5abb3 100644 --- a/src/uu/timeout/src/timeout.rs +++ b/src/uu/timeout/src/timeout.rs @@ -8,7 +8,7 @@ mod status; use crate::status::ExitStatus; use clap::{Arg, ArgAction, Command}; -use std::io::ErrorKind; +use std::io::{ErrorKind, Write}; use std::os::unix::process::ExitStatusExt; use std::process::{self, Child, Stdio}; use std::sync::atomic::{self, AtomicBool}; @@ -19,16 +19,15 @@ use uucore::parser::parse_time; use uucore::process::ChildExt; use uucore::translate; -#[cfg(unix)] -use uucore::signals::enable_pipe_errors; - use uucore::{ - format_usage, show_error, + format_usage, signals::{signal_by_name_or_value, signal_name_by_value}, }; -use nix::sys::signal::{Signal, kill}; +use nix::sys::signal::{SigHandler, Signal, kill}; use nix::unistd::{Pid, getpid, setpgid}; +#[cfg(unix)] +use std::os::unix::process::CommandExt; pub mod options { pub static FOREGROUND: &str = "foreground"; @@ -105,8 +104,16 @@ impl Config { } } +#[cfg(unix)] +uucore::init_sigpipe_capture!(); + #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { + #[cfg(unix)] + if !uucore::signals::sigpipe_was_ignored() { + uucore::signals::enable_pipe_errors()?; + } + let matches = uucore::clap_localization::handle_clap_result_with_exit_code(uu_app(), args, 125)?; @@ -179,58 +186,78 @@ pub fn uu_app() -> Command { .after_help(translate!("timeout-after-help")) } -/// Remove pre-existing SIGCHLD handlers that would make waiting for the child's exit code fail. -fn unblock_sigchld() { - unsafe { - nix::sys::signal::signal( - nix::sys::signal::Signal::SIGCHLD, - nix::sys::signal::SigHandler::SigDfl, - ) - .unwrap(); - } +/// Install SIGCHLD handler to ensure waiting for child works even if parent ignored SIGCHLD. +fn install_sigchld() { + extern "C" fn chld(_: libc::c_int) {} + let _ = unsafe { nix::sys::signal::signal(Signal::SIGCHLD, SigHandler::Handler(chld)) }; } -/// We should terminate child process when receiving TERM signal. +/// 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: std::sync::atomic::AtomicI32 = std::sync::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); + } -fn catch_sigterm() { - use nix::sys::signal; + let handler = SigHandler::Handler(handle_signal); + let sigpipe_ignored = uucore::signals::sigpipe_was_ignored(); - extern "C" fn handle_sigterm(signal: libc::c_int) { - let signal = signal::Signal::try_from(signal).unwrap(); - if signal == signal::Signal::SIGTERM { - SIGNALED.store(true, atomic::Ordering::Relaxed); + for sig in [ + Signal::SIGALRM, + Signal::SIGINT, + Signal::SIGQUIT, + Signal::SIGHUP, + Signal::SIGTERM, + Signal::SIGPIPE, + Signal::SIGUSR1, + Signal::SIGUSR2, + ] { + if sig != Signal::SIGPIPE || !sigpipe_ignored { + let _ = unsafe { nix::sys::signal::signal(sig, handler) }; } } - let handler = signal::SigHandler::Handler(handle_sigterm); - unsafe { signal::signal(signal::Signal::SIGTERM, handler) }.unwrap(); + if let Ok(sig) = Signal::try_from(term_signal as i32) { + let _ = unsafe { nix::sys::signal::signal(sig, handler) }; + } } /// 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 { - let s = signal_name_by_value(signal).unwrap(); - show_error!( - "{}", + let s = if signal == 0 { + "0".to_string() + } else { + signal_name_by_value(signal).unwrap().to_string() + }; + let mut stderr = std::io::stderr(); + let _ = writeln!( + stderr, + "timeout: {}", translate!("timeout-verbose-sending-signal", "signal" => s, "command" => cmd.quote()) ); + let _ = stderr.flush(); } } fn send_signal(process: &mut Child, signal: usize, foreground: bool) { // NOTE: GNU timeout doesn't check for errors of signal. // The subprocess might have exited just after the timeout. - // Sending a signal now would return "No such process", but we should still try to kill the children. - if foreground { - let _ = process.send_signal(signal); - } else { - let _ = process.send_signal_group(signal); - let kill_signal = signal_by_name_or_value("KILL").unwrap(); - let continued_signal = signal_by_name_or_value("CONT").unwrap(); - if signal != kill_signal && signal != continued_signal { - _ = process.send_signal_group(continued_signal); - } + let _ = process.send_signal(signal); + if signal == 0 || foreground { + return; + } + let _ = process.send_signal_group(signal); + let kill_sig = signal_by_name_or_value("KILL").unwrap(); + let cont_sig = signal_by_name_or_value("CONT").unwrap(); + if signal != kill_sig && signal != cont_sig { + let _ = process.send_signal(cont_sig); + let _ = process.send_signal_group(cont_sig); } } @@ -320,85 +347,105 @@ fn timeout( if !foreground { let _ = setpgid(Pid::from_raw(0), Pid::from_raw(0)); } - #[cfg(unix)] - enable_pipe_errors()?; - let process = &mut process::Command::new(&cmd[0]) + let mut cmd_builder = process::Command::new(&cmd[0]); + cmd_builder .args(&cmd[1..]) .stdin(Stdio::inherit()) .stdout(Stdio::inherit()) - .stderr(Stdio::inherit()) - .spawn() - .map_err(|err| { - let status_code = match err.kind() { - ErrorKind::NotFound => ExitStatus::CommandNotFound.into(), - ErrorKind::PermissionDenied => ExitStatus::CannotInvoke.into(), - _ => ExitStatus::CannotInvoke.into(), - }; - USimpleError::new( - status_code, - translate!("timeout-error-failed-to-execute-process", "error" => err), - ) - })?; - unblock_sigchld(); - catch_sigterm(); - // 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 does not exit within that time (the `Ok(None)` - // arm) and `kill_after` is specified, then try sending `SIGKILL`. - // - // 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. + .stderr(Stdio::inherit()); + + #[cfg(unix)] + { + #[cfg(target_os = "linux")] + let death_sig = Signal::try_from(signal as i32).ok(); + let sigpipe_was_ignored = uucore::signals::sigpipe_was_ignored(); + + unsafe { + cmd_builder.pre_exec(move || { + let _ = nix::sys::signal::signal(Signal::SIGTTIN, SigHandler::SigDfl); + let _ = nix::sys::signal::signal(Signal::SIGTTOU, SigHandler::SigDfl); + if sigpipe_was_ignored { + let _ = nix::sys::signal::signal(Signal::SIGPIPE, SigHandler::SigIgn); + } + #[cfg(target_os = "linux")] + if let Some(sig) = death_sig { + let _ = nix::sys::prctl::set_pdeathsig(sig); + } + Ok(()) + }); + } + } + + install_sigchld(); + install_signal_handlers(signal); + + let process = &mut cmd_builder.spawn().map_err(|err| { + let status_code = match err.kind() { + ErrorKind::NotFound => ExitStatus::CommandNotFound.into(), + ErrorKind::PermissionDenied => ExitStatus::CannotInvoke.into(), + _ => ExitStatus::CannotInvoke.into(), + }; + USimpleError::new( + status_code, + translate!("timeout-error-failed-to-execute-process", "error" => err), + ) + })?; + match process.wait_or_timeout(duration, Some(&SIGNALED)) { Ok(Some(status)) => Err(status .code() .unwrap_or_else(|| preserve_signal_info(status.signal().unwrap())) .into()), Ok(None) => { - report_if_verbose(signal, &cmd[0], verbose); - send_signal(process, signal, foreground); - match kill_after { - None => { - let status = process.wait()?; - if SIGNALED.load(atomic::Ordering::Relaxed) { - Err(ExitStatus::Terminated.into()) - } else if preserve_status { - if let Some(ec) = status.code() { - Err(ec.into()) - } else if let Some(sc) = status.signal() { - Err(ExitStatus::SignalSent(sc.try_into().unwrap()).into()) - } else { - Err(ExitStatus::CommandTimedOut.into()) - } - } else { - Err(ExitStatus::CommandTimedOut.into()) - } - } - Some(kill_after) => { - 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(), - )), - } - } + 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); + + 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(), + )), + }; + } + + 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 + .code() + .or_else(|| { + status + .signal() + .map(|s| ExitStatus::SignalSent(s as usize).into()) + }) + .unwrap_or(ExitStatus::CommandTimedOut.into()) + .into()) + } else { + Err(ExitStatus::CommandTimedOut.into()) } } Err(_) => { - // We're going to return ERR_EXIT_STATUS regardless of - // whether `send_signal()` succeeds or fails send_signal(process, signal, foreground); Err(ExitStatus::TimeoutFailed.into()) } diff --git a/src/uu/yes/src/yes.rs b/src/uu/yes/src/yes.rs index a5aaa18a867..6cb5f1e5d7b 100644 --- a/src/uu/yes/src/yes.rs +++ b/src/uu/yes/src/yes.rs @@ -11,16 +11,22 @@ use std::ffi::OsString; use std::io::{self, Write}; use uucore::error::{UResult, USimpleError}; use uucore::format_usage; -#[cfg(unix)] -use uucore::signals::enable_pipe_errors; use uucore::translate; // it's possible that using a smaller or larger buffer might provide better performance on some // systems, but honestly this is good enough const BUF_SIZE: usize = 16 * 1024; +#[cfg(unix)] +uucore::init_sigpipe_capture!(); + #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { + #[cfg(unix)] + if !uucore::signals::sigpipe_was_ignored() { + let _ = uucore::signals::enable_pipe_errors(); + } + let matches = uucore::clap_localization::handle_clap_result(uu_app(), args)?; let mut buffer = Vec::with_capacity(BUF_SIZE); @@ -29,7 +35,16 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { match exec(&buffer) { Ok(()) => Ok(()), - Err(err) if err.kind() == io::ErrorKind::BrokenPipe => Ok(()), + Err(err) if err.kind() == io::ErrorKind::BrokenPipe => { + #[cfg(unix)] + if uucore::signals::sigpipe_was_ignored() { + uucore::show_error!( + "{}", + translate!("yes-error-standard-output", "error" => err) + ); + } + Ok(()) + } Err(err) => Err(USimpleError::new( 1, translate!("yes-error-standard-output", "error" => err), @@ -113,8 +128,6 @@ fn prepare_buffer(buf: &mut Vec) { pub fn exec(bytes: &[u8]) -> io::Result<()> { let stdout = io::stdout(); let mut stdout = stdout.lock(); - #[cfg(unix)] - enable_pipe_errors()?; loop { stdout.write_all(bytes)?; diff --git a/src/uucore/src/lib/features/process.rs b/src/uucore/src/lib/features/process.rs index 55e8c36482b..1f7b8de17d3 100644 --- a/src/uucore/src/lib/features/process.rs +++ b/src/uucore/src/lib/features/process.rs @@ -107,11 +107,29 @@ impl ChildExt for Child { } fn send_signal_group(&mut self, signal: usize) -> io::Result<()> { - // Ignore the signal, so we don't go into a signal loop. - if unsafe { libc::signal(signal as i32, libc::SIG_IGN) } == usize::MAX { - return Err(io::Error::last_os_error()); + // Send signal to our process group (group 0 = caller's group). + // This matches GNU coreutils behavior: if the child has remained in our + // process group, it will receive this signal along with all other processes + // in the group. If the child has created its own process group (via setpgid), + // it won't receive this group signal, but will have received the direct signal. + + // Signal 0 is special - it just checks if process exists, doesn't send anything. + // No need to manipulate signal handlers for it. + if signal == 0 { + let result = unsafe { libc::kill(0, 0) }; + return if result == 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + }; } - if unsafe { libc::kill(0, signal as i32) } == 0 { + + // Ignore the signal temporarily so we don't receive it ourselves. + let old_handler = unsafe { libc::signal(signal as i32, libc::SIG_IGN) }; + let result = unsafe { libc::kill(0, signal as i32) }; + // Restore the old handler + unsafe { libc::signal(signal as i32, old_handler) }; + if result == 0 { Ok(()) } else { Err(io::Error::last_os_error()) diff --git a/tests/by-util/test_timeout.rs b/tests/by-util/test_timeout.rs index 9c5c6c1a465..a9b9b29dbc0 100644 --- a/tests/by-util/test_timeout.rs +++ b/tests/by-util/test_timeout.rs @@ -8,7 +8,8 @@ use std::time::Duration; use rstest::rstest; use uucore::display::Quotable; -use uutests::new_ucmd; +use uutests::util::TestScenario; +use uutests::{new_ucmd, util_name}; #[test] fn test_invalid_arg() { @@ -58,7 +59,7 @@ fn test_verbose() { new_ucmd!() .args(&[verbose_flag, "-s0", "-k.1", ".1", "sleep", "1"]) .fails() - .stderr_only("timeout: sending signal EXIT to command 'sleep'\ntimeout: sending signal KILL to command 'sleep'\n"); + .stderr_only("timeout: sending signal 0 to command 'sleep'\ntimeout: sending signal KILL to command 'sleep'\n"); } } @@ -235,3 +236,53 @@ fn test_command_cannot_invoke() { // Try to execute a directory (should give permission denied or similar) new_ucmd!().args(&["1", "/"]).fails_with_code(126); } + +#[test] +#[cfg(unix)] +fn test_sigchld_ignored_by_parent() { + let ts = TestScenario::new(util_name!()); + let bin_path = ts.bin_path.to_string_lossy(); + ts.ucmd() + .args(&[ + "10", + "sh", + "-c", + &format!("trap '' CHLD; exec {bin_path} timeout 1 true"), + ]) + .succeeds(); +} + +#[test] +#[cfg(unix)] +fn test_with_background_child() { + new_ucmd!() + .args(&[".5", "sh", "-c", "sleep .1 & sleep 2"]) + .fails_with_code(124) + .no_stdout(); +} + +#[test] +#[cfg(unix)] +fn test_forward_sigint_to_child() { + let mut cmd = new_ucmd!() + .args(&[ + "10", + "sh", + "-c", + "trap 'echo got_int; exit 42' INT; sleep 5", + ]) + .run_no_wait(); + cmd.delay(100); + cmd.kill_with_custom_signal(nix::sys::signal::Signal::SIGINT); + cmd.make_assertion() + .is_not_alive() + .with_current_output() + .stdout_contains("got_int"); +} + +#[test] +fn test_foreground_signal0_kill_after() { + new_ucmd!() + .args(&["--foreground", "-s0", "-k.1", ".1", "sleep", "10"]) + .fails_with_code(137); +}