timeout: fix race possibly terminating wrong process

The race is unlikely, as timeout(1) needs to receive a signal
in the few operations between waitpid() returning and exit().
Also the system needs to have reallocated the just released pid
in this time window.

Previously we never disabled the signal handler that sent
the termination signal to the "child" pid.  However once waitpid()
has reaped the child, the system is free to allocate that pid,
so we must ensure we don't process any further signals.

* build-aux/gen-lists-of-programs.sh: Build timeout(1) optionally...
* configure.ac: ...predicated on sigsuspend() being available.
* src/timeout.c (block_cleanup): A new function to ensure the
cleanup() handler is disabled after waitpid has returned.
(main): Use sigsuspend() to wait with cleanup() enabled but
disabled once it returns, and thus disabled for the waitpid() call.
(monitored_pid): Change to the more accurate pid_t.
* NEWS: Mention the fix.

Fixes http://bugs.gnu.org/25624

diff --git a/NEWS b/NEWS
index 9528f89e07cef8867d7a7d69e741755c4ddedee3..deaab7bcfe1513ce39746e66883961baed0c0378 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,12 @@ GNU coreutils NEWS                                    -*- outline -*-
   which could be triggered especially when tail was suspended and resumed.
   [bug introduced with inotify support added in coreutils-7.5]
 
+  timeout no longer has a race that may terminate the wrong process.
+  The race is unlikely, as timeout(1) needs to receive a signal right
+  after the command being monitored finishes.  Also the system needs
+  to have reallocated that command's pid in that short time window.
+  [bug introduced when timeout was added in coreutils-7.0]
+
   wc --bytes --files0-from now correctly reports byte counts.
   Previously it may have returned values that were too large,
   depending on the size of the first file processed.

diff --git a/build-aux/gen-lists-of-programs.sh b/build-aux/gen-lists-of-programs.sh
index 2666492e82ddc9bf5bf3df38a694e38d8d057942..cdbcd0a9e4749bedca74328c3eb149fbda23eae8 100755 (executable)
--- a/build-aux/gen-lists-of-programs.sh
+++ b/build-aux/gen-lists-of-programs.sh
@@ -32,6 +32,7 @@ build_if_possible_progs='
     pinky
     stdbuf
     stty
+    timeout
     uptime
     users
     who
@@ -120,7 +121,6 @@ normal_progs='
     tail
     tee
     test
-    timeout
     touch
     tr
     true

diff --git a/configure.ac b/configure.ac
index a13295aa1626fca6d34443af845a0dcdf7624a98..7c7c8c23829220d0459f44b12a43de61639a7a5e 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -252,6 +252,8 @@ AC_CHECK_FUNCS([chroot],
         gl_ADD_PROG([optional_bin_progs], [chroot]))
 AC_CHECK_FUNCS([gethostid],
         gl_ADD_PROG([optional_bin_progs], [hostid]))
+AC_CHECK_FUNCS([sigsuspend],
+        gl_ADD_PROG([optional_bin_progs], [timeout]))
 
 gl_WINSIZE_IN_PTEM
 

diff --git a/src/timeout.c b/src/timeout.c
index c38e3657a069217970e5d64a796e54e8059f13c8..6fe0542b7001518ea5549f06c21774e0d60d4c8a 100644 (file)
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -79,7 +79,7 @@
 
 static int timed_out;
 static int term_signal = SIGTERM;  /* same default as kill command.  */
-static int monitored_pid;
+static pid_t monitored_pid;
 static double kill_after;
 static bool foreground;      /* whether to use another program group.  */
 static bool preserve_status; /* whether to use a timeout status or not.  */
@@ -102,16 +102,6 @@ static struct option const long_options[] =
   {NULL, 0, NULL, 0}
 };
 
-static void
-unblock_signal (int sig)
-{
-  sigset_t unblock_set;
-  sigemptyset (&unblock_set);
-  sigaddset (&unblock_set, sig);
-  if (sigprocmask (SIG_UNBLOCK, &unblock_set, NULL) != 0)
-    error (0, errno, _("warning: sigprocmask"));
-}
-
 /* Start the timeout after which we'll receive a SIGALRM.
    Round DURATION up to the next representable value.
    Treat out-of-range values as if they were maximal,
@@ -121,10 +111,6 @@ static void
 settimeout (double duration, bool warn)
 {
 
-  /* We configure timers below so that SIGALRM is sent on expiry.
-     Therefore ensure we don't inherit a mask blocking SIGALRM.  */
-  unblock_signal (SIGALRM);
-
 /* timer_settime() provides potentially nanosecond resolution.
    setitimer() is more portable (to Darwin for example),
    but only provides microsecond resolution and thus is
@@ -165,11 +151,11 @@ settimeout (double duration, bool warn)
 /* send SIG avoiding the current process.  */
 
 static int
-send_sig (int where, int sig)
+send_sig (pid_t where, int sig)
 {
   /* If sending to the group, then ignore the signal,
      so we don't go into a signal loop.  Note that this will ignore any of the
-     signals registered in install_signal_handlers(), that are sent after we
+     signals registered in install_cleanup(), that are sent after we
      propagate the first one, which hopefully won't be an issue.  Note this
      process can be implicitly multithreaded due to some timer_settime()
      implementations, therefore a signal sent to the group, can be sent
@@ -179,6 +165,14 @@ send_sig (int where, int sig)
   return kill (where, sig);
 }
 
+/* Signal handler which is required for sigsuspend() to be interrupted
+   whenever SIGCHLD is received.  */
+static void
+chld (int sig)
+{
+}
+
+
 static void
 cleanup (int sig)
 {
@@ -330,7 +324,7 @@ parse_duration (const char* str)
 }
 
 static void
-install_signal_handlers (int sigterm)
+install_cleanup (int sigterm)
 {
   struct sigaction sa;
   sigemptyset (&sa.sa_mask);  /* Allow concurrent calls to handler */
@@ -346,6 +340,33 @@ install_signal_handlers (int sigterm)
   sigaction (sigterm, &sa, NULL); /* user specified termination signal.  */
 }
 
+/* Blocks all signals which were registered with cleanup
+   as signal handler.  Return original mask in OLD_SET.  */
+static void
+block_cleanup (int sigterm, sigset_t *old_set)
+{
+  sigset_t block_set;
+  sigemptyset (&block_set);
+  sigaddset (&block_set, SIGALRM);
+  sigaddset (&block_set, SIGINT);
+  sigaddset (&block_set, SIGQUIT);
+  sigaddset (&block_set, SIGHUP);
+  sigaddset (&block_set, SIGTERM);
+  sigaddset (&block_set, sigterm);
+  if (sigprocmask (SIG_BLOCK, &block_set, old_set) != 0)
+    error (0, errno, _("warning: sigprocmask"));
+}
+
+static void
+unblock_signal (int sig)
+{
+  sigset_t unblock_set;
+  sigemptyset (&unblock_set);
+  sigaddset (&unblock_set, sig);
+  if (sigprocmask (SIG_UNBLOCK, &unblock_set, NULL) != 0)
+    error (0, errno, _("warning: sigprocmask"));
+}
+
 /* Try to disable core dumps for this process.
    Return TRUE if successful, FALSE otherwise.  */
 static bool
@@ -433,10 +454,10 @@ main (int argc, char **argv)
 
   /* Setup handlers before fork() so that we
      handle any signals caused by child, without races.  */
-  install_signal_handlers (term_signal);
+  install_cleanup (term_signal);
   signal (SIGTTIN, SIG_IGN);   /* Don't stop if background child needs tty.  */
   signal (SIGTTOU, SIG_IGN);   /* Don't stop if background child needs tty.  */
-  signal (SIGCHLD, SIG_DFL);   /* Don't inherit CHLD handling from parent.   */
+  signal (SIGCHLD, chld);      /* Interrupt sigsuspend() when child exits.   */
 
   monitored_pid = fork ();
   if (monitored_pid == -1)
@@ -462,11 +483,19 @@ 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);
 
-      while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0
-             && errno == EINTR)
-        continue;
+      /* 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 (term_signal, &cleanup_set);
+
+      while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0)
+        sigsuspend (&cleanup_set);  /* Wait with cleanup signals unblocked.  */
 
       if (wait_result < 0)
         {
@@ -487,6 +516,7 @@ main (int argc, char **argv)
                 {
                   /* exit with the signal flag set.  */
                   signal (sig, SIG_DFL);
+                  unblock_signal (sig);
                   raise (sig);
                 }
               status = sig + 128; /* what sh returns for signaled processes.  */