There was a narrow race condition reported at:
https://github.com/coreutils/coreutils/issues/82
Then I noticed another related race condition
in the case of failing fork().
Both should be addressed by the attached.
cheers,
Pádraig
From c1cf5148a1c6302d27661ff0af772de1e7dbb2b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 11 Mar 2024 13:18:37 +0000
Subject: [PATCH 1/2] timeout: fix race where we might kill arbitrary processes
* src/timeout.c (cleanup): Handle the case where monitored_pid
might be -1, which could happen if a signal was received
immediately after a failed fork() call. In that case it would
send the termination signal to all processes that the timeout
process has permission to send signals too.
* NEWS: Mention the bug fix.
---
NEWS | 4 ++++
src/timeout.c | 11 ++++++++---
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/NEWS b/NEWS
index 70c4e2f44..864d7fe8b 100644
--- a/NEWS
+++ b/NEWS
@@ -33,6 +33,10 @@ GNU coreutils NEWS -*- outline -*-
on systems with a page size larger than the stdio BUFSIZ.
[This bug was present in "the beginning".]
+ timeout avoids a narrow race condition, where it might kill arbitrary
+ processes after a failed process fork.
+ [bug introduced with timeout in coreutils-7.0]
+
wc no longer fails to count unprintable characters as parts of words.
[bug introduced in textutils-2.1]
diff --git a/src/timeout.c b/src/timeout.c
index 85d97c0b5..9aa46a4f5 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -207,7 +207,7 @@ cleanup (int sig)
timed_out = 1;
sig = term_signal;
}
- if (monitored_pid)
+ if (0 < monitored_pid)
{
if (kill_after)
{
@@ -244,8 +244,13 @@ cleanup (int sig)
}
}
}
- else /* we're the child or the child is not exec'd yet. */
- _exit (128 + sig);
+ else if (monitored_pid == -1)
+ { /* were in the parent, so let it continue to exit below. */
+ }
+ else /* monitored_pid == 0 */
+ { /* we're the child or the child is not exec'd yet. */
+ _exit (128 + sig);
+ }
}
void
--
2.43.0
From 347cd7cf68501bb00db965af48cf1bc47446e648 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 11 Mar 2024 13:46:24 +0000
Subject: [PATCH 2/2] timeout: fix narrow race in failing to kill processes
* src/timeout.c (cleanup): Detect when were in the possible
problematic window where monitored_pid is not initialized in the parent
while the child may be running. Instead record the signal for
handling later in the parent in ...
(main): Resend the cleanup signal now that we know monitored_pid is set.
* NEWS: Mention the bug fix.
Reported at https://github.com/coreutils/coreutils/issues/82
---
NEWS | 4 ++++
src/timeout.c | 12 ++++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/NEWS b/NEWS
index 864d7fe8b..20cadf183 100644
--- a/NEWS
+++ b/NEWS
@@ -37,6 +37,10 @@ GNU coreutils NEWS -*- outline -*-
processes after a failed process fork.
[bug introduced with timeout in coreutils-7.0]
+ timeout avoids a narrow race condition, where it might fail to
+ kill monitored processes immediately after forking them.
+ [bug introduced with timeout in coreutils-7.0]
+
wc no longer fails to count unprintable characters as parts of words.
[bug introduced in textutils-2.1]
diff --git a/src/timeout.c b/src/timeout.c
index 9aa46a4f5..a4ea86712 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -78,7 +78,8 @@
static int timed_out;
static int term_signal = SIGTERM; /* same default as kill command. */
-static pid_t monitored_pid;
+static pid_t monitored_pid = -2;
+static int pending_cleanup_signal;
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. */
@@ -247,8 +248,12 @@ cleanup (int sig)
else if (monitored_pid == -1)
{ /* were in the parent, so let it continue to exit below. */
}
+ else if (monitored_pid == -2)
+ { /* parent or child, where fork return value not assigned yet. */
+ pending_cleanup_signal = sig; /* flag for parent to handle later. */
+ }
else /* monitored_pid == 0 */
- { /* we're the child or the child is not exec'd yet. */
+ { /* we're the child and have not exec'd yet. */
_exit (128 + sig);
}
}
@@ -561,6 +566,9 @@ main (int argc, char **argv)
pid_t wait_result;
int status;
+ if (pending_cleanup_signal)
+ raise (pending_cleanup_signal); /* Handle with monitored_pid set. */
+
/* We configure timers so that SIGALRM is sent on expiry.
Therefore ensure we don't inherit a mask blocking SIGALRM. */
unblock_signal (SIGALRM);
--
2.43.0
