Re: [PATCH 6/7] Block all signals on timer_create thread (BZ#10815)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 12/12/2019 10:10, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> The behavior of the signal mask on threads created by timer_create
>> for SIGEV_THREAD timers are implementation defined and glibc explicit
>                               implementation-defined           explictlly

Ack.

> 
>> unblocks all signals before calling the user-defined function.
>>
>> This behavior, although not incorrect standard-wise, opens a race if a
>> program using a blocked rt-signal plus sigwaitinfo (and without an
>> installed signal handler for the rt-signal) receives the signal while
>> executing the timer threads created by SIGEV_THREAD.
> 
> while executing the user-defined function for SIGEV_THREAD?

Ack.

> 
>> A better alternative discussed in bug report is to rather block all
>> signals (besides the internal ones not available to application
>> usage).
>>
>> This patch fixes by only unblock the SIGSETXID (used on set*uid).
>              fixes this issiue by only unblicking SIGSETXID
> 
>> The SIGTIMER is the same as SIGCANCEL and it will be handled by
> 
> No “The”?

Ack.

> 
>> sigwaitinfo on the helper thread (thus it can be masked as well).
>>
>> Checked on x86_64-linux-gnu and i686-linux-gnu.
> 
> Please also try to build this on Hurd.

I usually do, both build and rt/tests builds fine on i686-gnu.

> 
>> diff --git a/rt/tst-timer-sigmask.c b/rt/tst-timer-sigmask.c
>> new file mode 100644
>> index 0000000000..1030e4c79f
>> --- /dev/null
>> +++ b/rt/tst-timer-sigmask.c
>> @@ -0,0 +1,96 @@
> 
>> +static void
>> +thread_handler (union sigval sv)
>> +{
>> +  bool failure = false;
>> +
>> +  sigset_t ss;
>> +  sigprocmask (SIG_BLOCK, NULL, &ss);
>> +  if (test_verbose > 0)
>> +    printf ("%s: blocked signal mask = { ", __func__);
>> +  for (int sig = 1; sig < NSIG; sig++)
>> +    {
>> +#ifdef __linux__
>> +      /* POSIX timers threads created to handle SIGEV_THREAD blocks
>> +	 all signals except SIGKILL, SIGSTOP, and SIGSETXID.  */
>> +      if (!sigismember (&ss, sig)
>> +	  && (sig != SIGSETXID && sig != SIGKILL && sig != SIGSTOP))
>> +	{
>> +	  failure = true;
>> +	}
>> +#endif
>> +      if (test_verbose && sigismember (&ss, sig))
>> +	printf ("%d, ", sig);
>> +    }
>> +  if (test_verbose > 0)
>> +    printf ("}\n");
>> +
>> +  pthread_mutex_lock (&lock);
>> +  thread_handler_check = failure
>> +			 ? THREAD_SIGNAL_CHECK_FAIL : THREAD_SIGNAL_CHECK_OK;
>> +  pthread_cond_signal (&cond);
>> +  pthread_mutex_unlock (&lock);
> 
> Should you use the x variants?  xpthread_sigmask, xpthread_mutex_lock
> etc.?
> 
> The synchronization might more easily expressed using a POSIX barrier.

Ack, I changed to use xpthread_barrier_*.

> 
>> diff --git a/sysdeps/unix/sysv/linux/kernel-posix-timers.h b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
>> index 1ded4df51a..d60cb95f80 100644
>> --- a/sysdeps/unix/sysv/linux/kernel-posix-timers.h
>> +++ b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
>> @@ -43,7 +43,6 @@ extern pthread_mutex_t __active_timer_sigev_thread_lock attribute_hidden;
>>  /* Type of timers in the kernel.  */
>>  typedef int kernel_timer_t;
>>  
>> -
>>  /* Internal representation of timer.  */
>>  struct timer
>>  {
> 
> Unrelated change?

Ack.

> 
>> +		  /* This is the signal we are waiting for.  */
>> +		  td->thrfunc = tk->thrfunc;
>> +		  td->sival = tk->sival;
>>  
>> +		  pthread_t th;
>> +		  pthread_create (&th, &tk->attr, timer_sigev_thread, td);
>> +		}
> 
> I think this does not unblock SIGCANCEL on the new thread with the
> current pthread_create implementation.  This will change with the
> block-signals-around-clone patch I submitted, though.

It does not indeed, although not sure how valid is the scenario to
pthread cancel a thread not created explicitly by a pthread_create
issued by the user (since POSIX also state it is not valid to call
pthread_join() timer thread).

The sigcancel is explicit blocked with __libc_signal_block_sigtimer
on __start_helper_thread, so sigwaitinfo would be the only point
where the timer is triggered.  I don't think it is correct to unblock
it before calling pthread_create, so the created thread inherit the
unblock SIGCANCEL, for the same reason the SIGCANCEL is blocked on the
helper thread that calls sigwaitinfo (possible lose wakeups).

Another possibility is decouple SIGTIMER from SIGCANCEL, so the
helper sigwaitinfo thread is created with SIGTIMER blocked, but
not SIGCANCEL.