On 11/03/2024 15:17, Pádraig Brady wrote:
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.
Actually the second one to ensure processes are killed
is only moving the race around, since the child may
receive the signal in the same narrow window before assigning
monitored_pid, and ignore it.
The attached avoids the issue by blocking signal handling
until monitored_pid is in a deterministic state wrt fork().
I'll think a bit more about this before applying.
cheers,
Pádraig
From 8654d3564de24d3db1577da21f45014153b828b2 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] timeout: fix narrow race in failing to kill processes
* src/timeout.c (main): Block cleanup signals so that cleanup()
is not runnable until monitored_pid is in a deterministic state.
* NEWS: Mention the bug fix.
Reported at https://github.com/coreutils/coreutils/issues/82
---
NEWS | 4 ++++
src/timeout.c | 8 +++++++-
2 files changed, 11 insertions(+), 1 deletion(-)
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..1b1507bc7 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -248,7 +248,7 @@ cleanup (int sig)
{ /* 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. */
+ { /* parent hasn't forked yet, or child has not exec'd yet. */
_exit (128 + sig);
}
}
@@ -537,7 +537,13 @@ main (int argc, char **argv)
signal (SIGTTOU, SIG_IGN); /* Don't stop if background child needs tty. */
install_sigchld (); /* Interrupt sigsuspend() when child exits. */
+ /* Block signals so monitored_pid is deterministic in cleanup(). */
+ sigset_t fork_set;
+ block_cleanup_and_chld (term_signal, &fork_set);
monitored_pid = fork ();
+ if (sigprocmask (SIG_SETMASK, &fork_set, nullptr) != 0)
+ error (0, errno, _("warning: sigprocmask"));
+
if (monitored_pid == -1)
{
error (0, errno, _("fork system call failed"));
--
2.43.0
