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

[Pádraig Brady]

On 18/10/17 19:45, Pádraig Brady wrote:
> 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:

On second thoughts there probably best as separate patchces
as they're separate issues really.
1. consistent hang with blocked SIGCHLD in parent
2. intermittent hang in all cases.

cheers,
Pádraig