Re: [PATCH] timeout: Ensure SIGCHLD signal is not blocked

[Pádraig Brady]

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 <address@hidden>
> 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 <address@hidden>
> ---
>  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.  */