On 18/10/17 09:47, Thomas Jarosch wrote:
> We inherit the signal mask from our parent process,
> therefore ensure SIGCHLD is not blocked.
>
> If SIGCHLD is blocked, sigsuspend() won't be interrupted
> when the child process exits and we hang until the timeout (SIGALRM).
>
> The issue has been found by an "autotest" testsuite
> doing high level tests of a custom distribution.
> One complex test case got stuck 100% of the time.
>
> This fixes a regression from
>
> commit 2f69dba5df8caaf9eda658c1808b1379e9949f22
> Author: Tobias Stoeckmann <tob...@stoeckmann.org>
> Date: Sun Feb 5 13:23:22 2017 +0100
>
> timeout: fix race possibly terminating wrong process
>
>
> Please CC: comments, I'm not subscribed to the list.
>
> Signed-off-by: Thomas Jarosch <thomas.jaro...@intra2net.com>
> ---
> src/timeout.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/src/timeout.c b/src/timeout.c
> index a58b84f4e..aa2fff7d9 100644
> --- a/src/timeout.c
> +++ b/src/timeout.c
> @@ -323,6 +323,16 @@ parse_duration (const char* str)
> return duration;
> }
>
> +static void
> +unblock_signal (int sig)
> +{
> + sigset_t unblock_set;
> + sigemptyset (&unblock_set);
> + sigaddset (&unblock_set, sig);
> + if (sigprocmask (SIG_UNBLOCK, &unblock_set, NULL) != 0)
> + error (0, errno, _("warning: sigprocmask"));
> +}
> +
> static void
> install_sigchld (void)
> {
> @@ -333,6 +343,10 @@ install_sigchld (void)
> more likely to work cleanly. */
>
> sigaction (SIGCHLD, &sa, NULL);
> +
> + /* we inherit the signal mask from our parent process,
> + ensure SIGCHLD is not blocked. */
> + unblock_signal(SIGCHLD);
> }
>
> static void
> @@ -369,16 +383,6 @@ block_cleanup (int sigterm, sigset_t *old_set)
> error (0, errno, _("warning: sigprocmask"));
> }
>
> -static void
> -unblock_signal (int sig)
> -{
> - sigset_t unblock_set;
> - sigemptyset (&unblock_set);
> - sigaddset (&unblock_set, sig);
> - if (sigprocmask (SIG_UNBLOCK, &unblock_set, NULL) != 0)
> - error (0, errno, _("warning: sigprocmask"));
> -}
> -
> /* Try to disable core dumps for this process.
> Return TRUE if successful, FALSE otherwise. */
> static bool
>
Drats, right thanks.
Isn't there also a small race if SIGCHLD is received
after wait() returns but before the sigsuspend() runs?
I.E. would this also be an appropriate addition to your patch:
diff --git a/src/timeout.c b/src/timeout.c
index a58b84f..1a9fe50 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -352,10 +352,12 @@ install_cleanup (int sigterm)
sigaction (sigterm, &sa, NULL); /* user specified termination signal. */
}
-/* Blocks all signals which were registered with cleanup
- as signal handler. Return original mask in OLD_SET. */
+/* Block all signals which were registered with cleanup
+ as signal handler. Also block SIGCHLD to ensure
+ sigsuspend() doesn't miss it.
+ Return original mask in OLD_SET. */
static void
-block_cleanup (int sigterm, sigset_t *old_set)
+block_cleanup_and_chld (int sigterm, sigset_t *old_set)
{
sigset_t block_set;
sigemptyset (&block_set);
@@ -364,6 +366,7 @@ block_cleanup (int sigterm, sigset_t *old_set)
sigaddset (&block_set, SIGQUIT);
sigaddset (&block_set, SIGHUP);
sigaddset (&block_set, SIGTERM);
+ sigaddset (&block_set, SIGCHLD);
sigaddset (&block_set, sigterm);
if (sigprocmask (SIG_BLOCK, &block_set, old_set) != 0)
error (0, errno, _("warning: sigprocmask"));
@@ -504,7 +507,7 @@ main (int argc, char **argv)
/* Ensure we don't cleanup() after waitpid() reaps the child,
to avoid sending signals to a possibly different process. */
sigset_t cleanup_set;
- block_cleanup (term_signal, &cleanup_set);
+ block_cleanup_and_chld (term_signal, &cleanup_set);
while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0)
sigsuspend (&cleanup_set); /* Wait with cleanup signals unblocked. */
