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 <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:
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
