From f406b56f4a76488346da9ff10265f8e7ea0a208e Mon Sep 17 00:00:00 2001
From: Miles Liu <miles@bung.cc>
Date: Wed, 1 Feb 2023 15:39:07 +0800
Subject: [PATCH] timeout: fix subprocess is never terminated

---
 src/uu/timeout/src/timeout.rs          | 33 +++++++++++++++++++-------
 src/uucore/src/lib/features/process.rs | 15 ++++++++++++
 tests/by-util/test_timeout.rs          | 16 +++++++++++++
 3 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs
index 26235e0df1..fe342727ba 100644
--- a/src/uu/timeout/src/timeout.rs
+++ b/src/uu/timeout/src/timeout.rs
@@ -196,6 +196,22 @@ fn report_if_verbose(signal: usize, cmd: &str, verbose: bool) {
     }
 }
 
+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.
+    _ = process.send_signal(signal);
+    if !foreground {
+        _ = 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(continued_signal);
+            _ = process.send_signal_group(continued_signal);
+        }
+    }
+}
+
 /// Wait for a child process and send a kill signal if it does not terminate.
 ///
 /// This function waits for the child `process` for the time period
@@ -217,10 +233,11 @@ fn report_if_verbose(signal: usize, cmd: &str, verbose: bool) {
 /// If there is a problem sending the `SIGKILL` signal or waiting for
 /// the process after that signal is sent.
 fn wait_or_kill_process(
-    mut process: Child,
+    process: &mut Child,
     cmd: &str,
     duration: Duration,
     preserve_status: bool,
+    foreground: bool,
     verbose: bool,
 ) -> std::io::Result<i32> {
     match process.wait_or_timeout(duration) {
@@ -234,7 +251,7 @@ fn wait_or_kill_process(
         Ok(None) => {
             let signal = signal_by_name_or_value("KILL").unwrap();
             report_if_verbose(signal, cmd, verbose);
-            process.send_signal(signal)?;
+            send_signal(process, signal, foreground);
             process.wait()?;
             Ok(ExitStatus::SignalSent(signal).into())
         }
@@ -300,7 +317,7 @@ fn timeout(
 
     enable_pipe_errors()?;
 
-    let mut process = process::Command::new(&cmd[0])
+    let process = &mut process::Command::new(&cmd[0])
         .args(&cmd[1..])
         .stdin(Stdio::inherit())
         .stdout(Stdio::inherit())
@@ -335,7 +352,7 @@ fn timeout(
             .into()),
         Ok(None) => {
             report_if_verbose(signal, &cmd[0], verbose);
-            process.send_signal(signal)?;
+            send_signal(process, signal, foreground);
             match kill_after {
                 None => {
                     if preserve_status {
@@ -350,6 +367,7 @@ fn timeout(
                         &cmd[0],
                         kill_after,
                         preserve_status,
+                        foreground,
                         verbose,
                     ) {
                         Ok(status) => Err(status.into()),
@@ -363,11 +381,8 @@ fn timeout(
         }
         Err(_) => {
             // We're going to return ERR_EXIT_STATUS regardless of
-            // whether `send_signal()` succeeds or fails, so just
-            // ignore the return value.
-            process
-                .send_signal(signal)
-                .map_err(|e| USimpleError::new(ExitStatus::TimeoutFailed.into(), format!("{e}")))?;
+            // whether `send_signal()` succeeds or fails
+            send_signal(process, signal, foreground);
             Err(ExitStatus::TimeoutFailed.into())
         }
     }
diff --git a/src/uucore/src/lib/features/process.rs b/src/uucore/src/lib/features/process.rs
index 933b1f21ba..b8b2f178f1 100644
--- a/src/uucore/src/lib/features/process.rs
+++ b/src/uucore/src/lib/features/process.rs
@@ -48,6 +48,9 @@ pub trait ChildExt {
     /// send the signal to an unrelated process that recycled the PID.
     fn send_signal(&mut self, signal: usize) -> io::Result<()>;
 
+    /// Send a signal to a process group.
+    fn send_signal_group(&mut self, signal: usize) -> io::Result<()>;
+
     /// 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) -> io::Result<Option<ExitStatus>>;
@@ -62,6 +65,18 @@ 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) } != 0 {
+            return Err(io::Error::last_os_error());
+        }
+        if unsafe { libc::kill(0, signal as i32) } != 0 {
+            Err(io::Error::last_os_error())
+        } else {
+            Ok(())
+        }
+    }
+
     fn wait_or_timeout(&mut self, timeout: Duration) -> io::Result<Option<ExitStatus>> {
         if timeout == Duration::from_micros(0) {
             return self.wait().map(Some);
diff --git a/tests/by-util/test_timeout.rs b/tests/by-util/test_timeout.rs
index deedea6c74..eb6c99211c 100644
--- a/tests/by-util/test_timeout.rs
+++ b/tests/by-util/test_timeout.rs
@@ -138,3 +138,19 @@ fn test_kill_after_long() {
         .no_stdout()
         .no_stderr();
 }
+
+#[test]
+fn test_kill_subprocess() {
+    new_ucmd!()
+        .args(&[
+            // Make sure the CI can spawn the subprocess.
+            "10",
+            "sh",
+            "-c",
+            "sh -c \"trap 'echo xyz' TERM; sleep 30\"",
+        ])
+        .fails()
+        .code_is(124)
+        .stdout_contains("xyz")
+        .stderr_contains("Terminated");
+}