[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[debbugs-tracker] bug#25624: closed ([PATCH] timeout: Fix signal race in

From: GNU bug Tracking System
Subject: [debbugs-tracker] bug#25624: closed ([PATCH] timeout: Fix signal race in SIGALRM handling)
Date: Fri, 10 Feb 2017 05:40:02 +0000

Your message dated Thu, 9 Feb 2017 21:39:29 -0800
with message-id <address@hidden>
and subject line Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM 
has caused the debbugs.gnu.org bug report #25624,
regarding [PATCH] timeout: Fix signal race in SIGALRM handling
to be marked as done.

(If you believe you have received this mail in error, please contact

25624: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=25624
GNU Bug Tracking System
Contact address@hidden with problems
--- Begin Message --- Subject: [PATCH] timeout: Fix signal race in SIGALRM handling Date: Sun, 5 Feb 2017 13:23:22 +0100
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

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 | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/src/timeout.c b/src/timeout.c
index c38e3657a..633ecef3d 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,16 @@ main (int argc, char **argv)
       pid_t wait_result;
+      sigset_t old_set;
       int status;
       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)

--- End Message ---
--- Begin Message --- Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling Date: Thu, 9 Feb 2017 21:39:29 -0800 User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0
That looks good.

I've changed the naming a bit,
ensured we also block the term_signal,
made the build of timeout optional on sigsuspend availability,
adjusted the commit message,
and added a NEWS entry.

Marking this as done now.

I'll push in your name later.


Attachment: timeout-race.patch
Description: Text Data

--- End Message ---

reply via email to

[Prev in Thread] Current Thread [Next in Thread]