From: Pádraig Brady <P@draigBrady.com>
Date: Mon, 11 Mar 2024 13:46:24 +0000 (+0000)
Subject: timeout: fix narrow race in failing to kill processes
X-Git-Tag: v9.5~37
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ab4ffc85039f7398dde2ec4b307dfb2aa0fcf4f8;p=thirdparty%2Fcoreutils.git

timeout: fix narrow race in failing to kill processes

* src/timeout.c (main): Block cleanup signals earlier so that cleanup()
is not runnable until monitored_pid is in a deterministic state.
This ensures we always send a termination signal to the child
once it's forked.
* NEWS: Mention the bug fix.
Reported at https://github.com/coreutils/coreutils/issues/82
---

diff --git a/NEWS b/NEWS
index 864d7fe8b6..20cadf183f 100644
--- a/NEWS
+++ b/NEWS
@@ -37,6 +37,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   processes after a failed process fork.
   [bug introduced with timeout in coreutils-7.0]
 
+  timeout avoids a narrow race condition, where it might fail to
+  kill monitored processes immediately after forking them.
+  [bug introduced with timeout in coreutils-7.0]
+
   wc no longer fails to count unprintable characters as parts of words.
   [bug introduced in textutils-2.1]
 
diff --git a/src/timeout.c b/src/timeout.c
index 9aa46a4f5a..68d872b125 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -248,7 +248,7 @@ cleanup (int sig)
     { /* were in the parent, so let it continue to exit below.  */
     }
   else /* monitored_pid == 0  */
-    { /* we're the child or the child is not exec'd yet.  */
+    { /* parent hasn't forked yet, or child has not exec'd yet.  */
       _exit (128 + sig);
     }
 }
@@ -537,14 +537,29 @@ main (int argc, char **argv)
   signal (SIGTTOU, SIG_IGN);   /* Don't stop if background child needs tty.  */
   install_sigchld ();          /* Interrupt sigsuspend() when child exits.   */
 
+  /* We configure timers so that SIGALRM is sent on expiry.
+     Therefore ensure we don't inherit a mask blocking SIGALRM.  */
+  unblock_signal (SIGALRM);
+
+  /* Block signals now, so monitored_pid is deterministic in cleanup().  */
+  sigset_t orig_set;
+  block_cleanup_and_chld (term_signal, &orig_set);
+
   monitored_pid = fork ();
   if (monitored_pid == -1)
     {
       error (0, errno, _("fork system call failed"));
       return EXIT_CANCELED;
     }
-  else if (monitored_pid == 0)
-    {                           /* child */
+  else if (monitored_pid == 0)  /* child */
+    {
+      /* Restore signal mask for child.  */
+      if (sigprocmask (SIG_SETMASK, &orig_set, nullptr) != 0)
+        {
+          error (0, errno, _("child failed to reset signal mask"));
+          return EXIT_CANCELED;
+        }
+
       /* exec doesn't reset SIG_IGN -> SIG_DFL.  */
       signal (SIGTTIN, SIG_DFL);
       signal (SIGTTOU, SIG_DFL);
@@ -561,19 +576,14 @@ main (int argc, char **argv)
       pid_t wait_result;
       int status;
 
-      /* We configure timers so that SIGALRM is sent on expiry.
-         Therefore ensure we don't inherit a mask blocking SIGALRM.  */
-      unblock_signal (SIGALRM);
-
       settimeout (timeout, true);
 
-      /* Ensure we don't cleanup() after waitpid() reaps the child,
+      /* Note signals remain blocked in parent here, to ensure
+         we don't cleanup() after waitpid() reaps the child,
          to avoid sending signals to a possibly different process.  */
-      sigset_t cleanup_set;
-      block_cleanup_and_chld (term_signal, &cleanup_set);
 
       while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0)
-        sigsuspend (&cleanup_set);  /* Wait with cleanup signals unblocked.  */
+        sigsuspend (&orig_set);  /* Wait with cleanup signals unblocked.  */
 
       if (wait_result < 0)
         {