GNU bug report logs - #25624
[PATCH] timeout: Fix signal race in SIGALRM handling

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 25624 in the body.
You can then email your comments to 25624 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: bug-coreutils <at> gnu.org
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
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 | 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)
   else
     {
       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)
         {
-- 
2.11.1

Message #8 received at 25624 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Tobias Stoeckmann <tobias <at> stoeckmann.org>, 25624 <at> debbugs.gnu.org
Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
Date: Sun, 5 Feb 2017 09:56:54 -0800

On 05/02/17 04:23, Tobias Stoeckmann wrote:
> 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.

> diff --git a/src/timeout.c b/src/timeout.c
>  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);

Removing the call above causes this test to fail:
tests/misc/timeout-blocked.pl
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=tests/misc/timeout-blocked.pl;hb=HEAD

> -      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);

I think we'd need some conditional code to allow
sigsuspend() to compile on mingw, MSVC 9.

In general this is a largely theoretical race right?
I.E. pids would need to be recycled between the waitpid() and exit()?
Just trying to get an idea of the practicality of the issue.

thanks!
Pádraig

Message #11 received at 25624 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>,
 Tobias Stoeckmann <tobias <at> stoeckmann.org>, 25624 <at> debbugs.gnu.org
Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
Date: Sun, 5 Feb 2017 10:26:35 -0800

Pádraig Brady wrote:
> In general this is a largely theoretical race right?
> I.E. pids would need to be recycled between the waitpid() and exit()?

Not that theoretical, in the common case of systems with wraparaound PIDs and a 
small PID space. All you need is a long-running child on a busy system.

Message #14 received at 25624 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Pádraig Brady <P <at> draigBrady.com>, 25624 <at> debbugs.gnu.org
Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
Date: Sun, 5 Feb 2017 19:50:14 +0100

On Sun, Feb 05, 2017 at 10:26:35AM -0800, Paul Eggert wrote:
> Pádraig Brady wrote:
> > In general this is a largely theoretical race right?
> > I.E. pids would need to be recycled between the waitpid() and exit()?
> 
> Not that theoretical, in the common case of systems with wraparaound PIDs
> and a small PID space. All you need is a long-running child on a busy
> system.

Yes, normally it is small enough to overflow in less than a minute if an
attacker runs fork() kill() in a loop.

I have updated the patch so it passes the test. As I don't have enough
experience in portable #ifdef's for all supported systems, I hope you can
adjust the patch as needed.


Tobias

Message #17 received at 25624 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Pádraig Brady <P <at> draigBrady.com>, 25624 <at> debbugs.gnu.org
Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
Date: Sun, 5 Feb 2017 19:50:37 +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
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

Message #20 received at 25624 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Tobias Stoeckmann <tobias <at> stoeckmann.org>
Cc: 25624 <at> debbugs.gnu.org
Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
Date: Sun, 5 Feb 2017 18:13:47 -0800

On 05/02/17 10:50, Tobias Stoeckmann wrote:
>      {
>        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);

cool

>        settimeout (timeout, true);
>  
> -      while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0
> -             && errno == EINTR)
> -        continue;
> +      block_signal (SIGALRM, &old_set);

SIGALRM isn't the only signal causing this race?
If a SIGTERM etc. is sent to the timeout process during the race,
cleanup() will be called with the same consequences.
Don't we need to block all signals for the cleanup() handler?

> +      while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0)
> +        sigsuspend (&old_set);

> +      monitored_pid = 0;
> +      unblock_signal (SIGALRM);

There doesn't seem to be much point in re-enabling SIGALRM (or other signals) here,
as if it does fire it means we'll return 143 rather than the correct 124.
Since the child is reaped, we can just proceed and process the exit status
of the child and exit() I think.

cheers,
Pádraig.

Message #23 received at 25624 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 25624 <at> debbugs.gnu.org
Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
Date: Thu, 9 Feb 2017 21:09:42 +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
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.
---
Thanks for your great review and feedback. :)
---
 src/timeout.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/timeout.c b/src/timeout.c
index c38e3657a..b9f8ef5a2 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.  */
@@ -102,6 +102,22 @@ static struct option const long_options[] =
   {NULL, 0, NULL, 0}
 };
 
+/* Blocks all signals which were registered with cleanup
+   as signal handler.  */
+static void
+block_cleanup (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);
+  if (sigprocmask (SIG_BLOCK, &block_set, old_set) != 0)
+    error (0, errno, _("warning: sigprocmask"));
+}
+
 static void
 unblock_signal (int sig)
 {
@@ -121,10 +137,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 +177,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 +191,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 +455,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 +479,17 @@ 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_cleanup (&old_set);
+      while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0)
+        sigsuspend (&old_set);
 
       if (wait_result < 0)
         {
-- 
2.11.1

Message #28 received at 25624-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Tobias Stoeckmann <tobias <at> stoeckmann.org>
Cc: 25624-done <at> debbugs.gnu.org
Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
Date: Thu, 9 Feb 2017 21:39:29 -0800

[Message part 1 (text/plain, inline)]

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.

thanks!
Pádraig

[timeout-race.patch (text/x-patch, attachment)]

Message #31 received at 25624 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Tobias Stoeckmann <tobias <at> stoeckmann.org>
Cc: 25624 <at> debbugs.gnu.org
Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
Date: Thu, 9 Feb 2017 22:15:05 -0800

On 09/02/17 21:39, Pádraig Brady wrote:
> 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.

Actually to ensure we indicate when the monitored
command was terminated with a signal, we need to
unblock that signal as follows.  I'll merge that in too.

diff --git a/src/timeout.c b/src/timeout.c
index d5a90fd..6fe0542 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -516,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.  *

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.