timeout: fix signal handling, SIGPIPE preservation, and GNU test compatibility

Author: ChrisDryden

.vscode/cspell.dictionaries/jargon.wordlist.txt

@@ -114,8 +114,10 @@ noxfer
 ofile
 oflag
 oflags
+pdeathsig
 peekable
 performant
+prctl
 precompiled
 precompute
 preload
@@ -140,8 +142,17 @@ SETFL
 setlocale
 shortcode
 shortcodes
+setpgid
 sigaction
+CHLD
+chld
+SIGCHLD
+sigchld
 siginfo
+SIGTTIN
+sigttin
+SIGTTOU
+sigttou
 sigusr
 strcasecmp
 subcommand

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<ExitStatus> for i32 {
@@ -46,7 +43,6 @@ impl From<ExitStatus> for i32 {
             ExitStatus::CannotInvoke => 126,
             ExitStatus::CommandNotFound => 127,
             ExitStatus::SignalSent(s) => 128 + s as Self,
-            ExitStatus::Terminated => 143,
         }
     }
 }

src/uu/timeout/src/timeout.rs

@@ -9,8 +9,8 @@ mod status;
 
 use crate::status::ExitStatus;
 use clap::{Arg, ArgAction, Command};
-use std::io::ErrorKind;
-use std::os::unix::process::{CommandExt, ExitStatusExt};
+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;
@@ -21,12 +21,14 @@ use uucore::process::ChildExt;
 use uucore::translate;
 
 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";
@@ -177,32 +179,46 @@ 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;
-
-    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);
+    let handler = SigHandler::Handler(handle_signal);
+    let sigpipe_ignored = uucore::signals::sigpipe_was_ignored();
+
+    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 {
+            continue; // Skip SIGPIPE if it was ignored by parent
         }
+        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.
@@ -213,26 +229,29 @@ fn report_if_verbose(signal: usize, cmd: &str, verbose: bool) {
         } else {
             signal_name_by_value(signal).unwrap().to_string()
         };
-        show_error!(
-            "{}",
+        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_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 {
+        let _ = process.send_signal(continued_signal);
+        let _ = process.send_signal_group(continued_signal);
     }
 }
 
@@ -330,24 +349,46 @@ fn timeout(
         let _ = setpgid(Pid::from_raw(0), Pid::from_raw(0));
     }
 
-    let mut command = process::Command::new(&cmd[0]);
-    command
+    let mut cmd_builder = process::Command::new(&cmd[0]);
+    cmd_builder
         .args(&cmd[1..])
         .stdin(Stdio::inherit())
         .stdout(Stdio::inherit())
         .stderr(Stdio::inherit());
 
-    // If stdin was closed before Rust reopened it as /dev/null, close it in child
-    if uucore::signals::stdin_was_closed() {
+    #[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();
+        let stdin_was_closed = uucore::signals::stdin_was_closed();
+
         unsafe {
-            command.pre_exec(|| {
-                libc::close(libc::STDIN_FILENO);
+            cmd_builder.pre_exec(move || {
+                // Reset terminal signals to default
+                let _ = nix::sys::signal::signal(Signal::SIGTTIN, SigHandler::SigDfl);
+                let _ = nix::sys::signal::signal(Signal::SIGTTOU, SigHandler::SigDfl);
+                // Preserve SIGPIPE ignore status if parent had it ignored
+                if sigpipe_was_ignored {
+                    let _ = nix::sys::signal::signal(Signal::SIGPIPE, SigHandler::SigIgn);
+                }
+                // If stdin was closed before Rust reopened it as /dev/null, close it in child
+                if stdin_was_closed {
+                    libc::close(libc::STDIN_FILENO);
+                }
+                #[cfg(target_os = "linux")]
+                if let Some(sig) = death_sig {
+                    let _ = nix::sys::prctl::set_pdeathsig(sig);
+                }
                 Ok(())
             });
         }
     }
 
-    let process = &mut command.spawn().map_err(|err| {
+    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(),
@@ -358,8 +399,7 @@ fn timeout(
             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
@@ -381,41 +421,51 @@ fn timeout(
             Err(exit_code.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(_) => {

src/uu/yes/src/yes.rs

@@ -27,7 +27,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
 
     match exec(&buffer) {
         Ok(()) => Ok(()),
-        Err(err) if err.kind() == io::ErrorKind::BrokenPipe => Ok(()),
         Err(err) => Err(USimpleError::new(
             1,
             translate!("yes-error-standard-output", "error" => err),

src/uucore/src/lib/features/process.rs

@@ -105,11 +105,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())