[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
From: |
Tobias Stoeckmann |
Subject: |
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
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling, Tobias Stoeckmann, 2017/02/05
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling, Pádraig Brady, 2017/02/05
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling, Paul Eggert, 2017/02/05
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling, Tobias Stoeckmann, 2017/02/05
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling, Tobias Stoeckmann, 2017/02/05
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling, Pádraig Brady, 2017/02/05
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling,
Tobias Stoeckmann <=
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling, Pádraig Brady, 2017/02/10
- bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling, Pádraig Brady, 2017/02/10