GNU bug report logs -
#25624
[PATCH] timeout: Fix signal race in SIGALRM handling
Previous Next
Full log
View this message in rfc822 format
[Message part 1 (text/plain, inline)]
Your bug report
#25624: [PATCH] timeout: Fix signal race in SIGALRM handling
which was filed against the coreutils package, has been closed.
The explanation is attached below, along with your original report.
If you require more details, please reply to 25624 <at> debbugs.gnu.org.
--
25624: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=25624
GNU Bug Tracking System
Contact help-debbugs <at> gnu.org with problems
[Message part 2 (message/rfc822, inline)]
[Message part 3 (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 part 5 (message/rfc822, inline)]
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
This bug report was last modified 8 years and 183 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.