Bug 10815 - [timer_create / SIGEV_THREAD] signalmask of timer_sigev_thread dangerous
Summary: [timer_create / SIGEV_THREAD] signalmask of timer_sigev_thread dangerous
Status: REOPENED
Alias: None
Product: glibc
Classification: Unclassified
Component: librt (show other bugs)
Version: 2.9
: P2 normal
Target Milestone: ---
Assignee: Ulrich Drepper
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-20 10:11 UTC by Roland Lezuo
Modified: 2015-08-27 22:11 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Lezuo 2009-10-20 10:11:43 UTC
When a POSIX timer is created with timer_create and sigev_notify = SIGEV_THREAD
glibc uses internal helper threads (timer_sigev_thread) to invoke the callback
function. This helper threads unblocks all signals.

<snip>
/* Helper thread to call the user-provided function.  */
static void *
timer_sigev_thread (void *arg)
{
  /* The parent thread has all signals blocked.  This is a bit
     surprising for user code, although valid.  We unblock all
     signals.  */
  sigset_t ss;
  sigemptyset (&ss);
  INTERNAL_SYSCALL_DECL (err);
  INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, &ss, NULL, _NSIG / 8);

  struct thread_start_data *td = (struct thread_start_data *) arg;
<snap>

According to the POSIX standard the signalmask of the thread executing the timer
callback is implementation defined. The above comments indicates that the signal
mask is cleared to not suprise user code.

I have an application which uses multiple rt-signals. One of them is blocked by
all threads and received using sigwaitinfo. The behaviour of timer_sigev_thread
opens a race condition where the rt-signal is delivered and because there is no
signal handler the application crashes. It is unfeasible to add a signal handler
and propagate the event back to the signal handling code.

I did not actually try on FreeBSD, but it seems they do not clear the
signalmask. I think the correct way is to assume worst and have the signalmask
blocking all signals. User code must be prepared to be executed with any
signalmask, and by doing so glibc does not crash user applications depending on
blocked signals and sigwaitinfo.
Comment 1 Ulrich Drepper 2009-10-20 11:31:35 UTC
The behavior is correct.  You're relying on undefined behavior.  Just don't do it.
Comment 2 Trent Piepho 2012-11-13 23:44:35 UTC
(In reply to comment #1)
> The behavior is correct.  You're relying on undefined behavior.  Just don't do
> it.

This means it's impossible to use sigwait()/sigwaitinfo()/sigtimedwait() in the same process with a POSIX timer using SIGEV_THREAD.

It would be much more useful if the helper thread had the same signal mask as the thread that originally called timer_create.  That signal mask is already retrieved when the timer thread is created.  If it were simply stored somewhere, then the helper thread could set its mask to that instead of an empty set.
Comment 3 Rich Felker 2012-11-14 17:37:42 UTC
Drepper's comment is technically incorrect; the behavior is implementation-defined, not undefined. And in this case, glibc is actively providing detrimental behavior. I think this should be fixed.

As Trent suggested, it may be convenient to have the signal mask match the mask at the time of the call to timer_create, but storing this and restoring it would be extra work. I think the most-correct behavior is for the timer thread always to start with all signals blocked. If the applications wants some signals unblocked, it can always unblock them as part of the timer handling code. But if any of them start out unblocked, it's impossible to block them without race conditions.

Basically, this just comes down to common-sense about signal masking and race conditions. A library function (whether in the standard library or a third-party library) should NEVER unblock a signal unless that signal is only used for its own internal purposes (like the timer or cancellation signals). The only operations libraries ever perform on the signal mask should be blocking signals and restoring to a previously-saved mask.
Comment 4 Trent Piepho 2012-11-15 20:35:03 UTC
The signal mask is already stored by code in the timer_create() call, so using that mask in the timer thread doesn't actually take any more work that is done now.  It's just as matter of changing two existing rt_sigprocmask calls to use a global rather than a local variable.

But thinking about it more, I agree with Rich that blocking all signals is the only sensible way.  The sigprocmask call in the timer thread can be removed.  The user code can always add it back and unblock whatever it wants unblocked if it cares.  If it doesn't, then the call is avoided.

Restoring the mask used when calling timer_create() wouldn't work if the process has since blocked a signal.  For instance, if it needs to modify a data structure also used by a signal handler, it's necessary to block the signal while doing so to avoid racing with the handler.  If a completely unrelated timer goes off and creates a new thread with unblocked signals....
Comment 5 Rich Felker 2012-11-15 23:48:34 UTC
Using a global is definitely not viable; the sigset_t would have to be added to the structure allocated for the particular timer. There's no reason this couldn't be done, but I think we're both in agreement that starting SIGEV_TIMER threads with all signals blocked is the most desirable (and most flexible) behavior anyway.