bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling

[Tobias Stoeckmann]

It is possible to trigger a signal race in timeout if the alarm is
triggered AFTER the child process already finished and was waited for
by the parent.

If a child terminates, it becomes a zombie process until the parent
called waitpid() (or an equivalent system call) on it. This means that
the process id cannot ge given to a new process. But if waitpid() has
been called, the process id stored in the variable monitored_pid could
already be assigned to another one. If the alarm -- due to timeout --
is triggered afterwards, the tool can send a signal to that new process
instead.

To fix this, I have introduced a critical section around waitpid(),
which blocks SIGALRM and resets monitored_pid to 0 if waitpid() returned
anything except 0 (be it a success or a failure). This way, a later
alarm won't send any signal.

While at it, I changed int to pid_t, because that's the actual data
type for process ids. Granted, normally it can be assumed that pid_t
is an int.
---
 src/timeout.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/src/timeout.c b/src/timeout.c
index c38e3657a..99819840a 100644
--- 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.  */
@@ -103,6 +103,16 @@ static struct option const long_options[] =
 };
 
 static void
+block_signal (int sig, sigset_t *old_set)
+{
+  sigset_t block_set;
+  sigemptyset (&block_set);
+  sigaddset (&block_set, sig);
+  if (sigprocmask (SIG_BLOCK, &block_set, old_set) != 0)
+    error (0, errno, _("warning: sigprocmask"));
+}
+
+static void
 unblock_signal (int sig)
 {
   sigset_t unblock_set;
@@ -121,10 +131,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,7 +171,7 @@ 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
@@ -179,6 +185,13 @@ 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)
 {
@@ -436,7 +449,7 @@ main (int argc, char **argv)
   install_signal_handlers (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);      /* Use a signal handler for SIGCHLD.          */
 
   monitored_pid = fork ();
   if (monitored_pid == -1)
@@ -460,13 +473,19 @@ main (int argc, char **argv)
   else
     {
       pid_t wait_result;
+      sigset_t old_set;
       int status;
 
+      /* We configure timers below 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;
+      block_signal (SIGALRM, &old_set);
+      while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0)
+        sigsuspend (&old_set);
+      monitored_pid = 0;
+      unblock_signal (SIGALRM);
 
       if (wait_result < 0)
         {
-- 
2.11.1