Message ID | 20191231193129.2590-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | [1/2] linux: Reserve a new signal for SIGTIMER | expand |
* Adhemerval Zanella:
> The fix for BZ#10815 requires decouple SIGTIMER and SIGCANCEL.
How so? I don't see why, sorry.
On 31/12/2019 16:41, Florian Weimer wrote: > * Adhemerval Zanella: > >> The fix for BZ#10815 requires decouple SIGTIMER and SIGCANCEL. > > How so? I don't see why, sorry. > Indeed 'requires' is not the right word, since I added this patch to solve a different problem. What about: -- To simplify the fix for BZ#10815 SIGTIMER and SIGCANCEL are decoupled. It allow simplify the SIGCANCEL handler, since there is no need to check if the signal is really a cancellation one; and the thread created SIGEV_THREAD does not need to unblock SIGCANCEL at start. This patch also a new internal symbol, __contain_internal_signal, which checks if the sigset_t contains any internal signal. It is used to refactor and move the logic to check and remove the internal signal to internal-signals.h. Checked on x86_64-linux-gnu and i686-linux-gnu.
* Adhemerval Zanella: > On 31/12/2019 16:41, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> The fix for BZ#10815 requires decouple SIGTIMER and SIGCANCEL. >> >> How so? I don't see why, sorry. >> > > Indeed 'requires' is not the right word, since I added this patch to > solve a different problem. What about: I think the premise is wrong. Reserving another signal has backwards compatibility implications, and we should do not so lightly. I just don't see the need to do this to address this bug. There are better ways (like the patch I posted that disables signals around the clone call and pthread_create and automatically enables SIGCANCEL and thus SIGTIMER on the new thread as a side effect).
On 04/01/2020 07:55, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 31/12/2019 16:41, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> The fix for BZ#10815 requires decouple SIGTIMER and SIGCANCEL. >>> >>> How so? I don't see why, sorry. >>> >> >> Indeed 'requires' is not the right word, since I added this patch to >> solve a different problem. What about: > > I think the premise is wrong. Reserving another signal has backwards > compatibility implications, and we should do not so lightly. I just > don't see the need to do this to address this bug. There are better > ways (like the patch I posted that disables signals around the clone > call and pthread_create and automatically enables SIGCANCEL and thus > SIGTIMER on the new thread as a side effect). > The backwards compatibility might arise when application mixes rt-signals and different libc version or if it hard-code signal numbers in some ways or does not runtime check rt-signal allocation against SIGRTMAX (such as tst-signal6.c). So I am not sure it is something we should prevent do so (although I agree that it might not be a strong premise). But I don't have a strong opinion about this, it seemed the straightforward change to fix BZ#10815 while still allowing thread cancellation. I will send an updated version that explicit re-enable SIGCANCEL on the SIGEV_THREAD (without decoupling SIGCANCEL and SIGTIMER). Once your patch is upstream, we can remove SIGCANCEL handling on timer_routines.c.
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c index acc0f3672b..0fa799aaac 100644 --- a/nptl/nptl-init.c +++ b/nptl/nptl-init.c @@ -141,15 +141,6 @@ __nptl_set_robust (struct pthread *self) static void sigcancel_handler (int sig, siginfo_t *si, void *ctx) { - /* Safety check. It would be possible to call this function for - other signals and send a signal from another process. This is not - correct and might even be a security problem. Try to catch as - many incorrect invocations as possible. */ - if (sig != SIGCANCEL - || si->si_pid != __getpid() - || si->si_code != SI_TKILL) - return; - struct pthread *self = THREAD_SELF; int oldval = THREAD_GETMEM (self, cancelhandling); diff --git a/nptl/pthread_sigmask.c b/nptl/pthread_sigmask.c index 4aa774d01d..31f6defcba 100644 --- a/nptl/pthread_sigmask.c +++ b/nptl/pthread_sigmask.c @@ -29,13 +29,10 @@ pthread_sigmask (int how, const sigset_t *newmask, sigset_t *oldmask) /* The only thing we have to make sure here is that SIGCANCEL and SIGSETXID is not blocked. */ - if (newmask != NULL - && (__builtin_expect (__sigismember (newmask, SIGCANCEL), 0) - || __builtin_expect (__sigismember (newmask, SIGSETXID), 0))) + if (newmask != NULL && __contain_internal_signal (newmask)) { local_newmask = *newmask; - __sigdelset (&local_newmask, SIGCANCEL); - __sigdelset (&local_newmask, SIGSETXID); + __clear_internal_signals (&local_newmask); newmask = &local_newmask; } diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h index 41c24dc4b3..d78d919f62 100644 --- a/sysdeps/generic/internal-signals.h +++ b/sysdeps/generic/internal-signals.h @@ -29,6 +29,12 @@ __is_internal_signal (int sig) return false; } +static inline bool +__contain_internal_signal (const sigset_t *set) +{ + return false; +} + static inline void __clear_internal_signals (sigset_t *set) { diff --git a/sysdeps/nptl/allocrtsig.c b/sysdeps/nptl/allocrtsig.c index 3f62bf40e7..ffa2a48e65 100644 --- a/sysdeps/nptl/allocrtsig.c +++ b/sysdeps/nptl/allocrtsig.c @@ -19,13 +19,9 @@ #include <signal.h> #include <nptl/pthreadP.h> -#if SIGTIMER != SIGCANCEL -# error "SIGTIMER and SIGCANCEL must be the same" -#endif - /* This tells the generic code (included below) how many signal numbers need to be reserved for libpthread's private uses - (SIGCANCEL and SIGSETXID). */ -#define RESERVED_SIGRT 2 + (SIGCANCEL, SIGSETXID, and SIGTIMER). */ +#define RESERVED_SIGRT 3 #include <signal/allocrtsig.c> diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h index 2932e21fd0..06d1d38b7f 100644 --- a/sysdeps/unix/sysv/linux/internal-signals.h +++ b/sysdeps/unix/sysv/linux/internal-signals.h @@ -29,21 +29,27 @@ #define SIGCANCEL __SIGRTMIN -/* Signal needed for the kernel-supported POSIX timer implementation. - We can reuse the cancellation signal since we can distinguish - cancellation from timer expirations. */ -#define SIGTIMER SIGCANCEL - - /* Signal used to implement the setuid et.al. functions. */ #define SIGSETXID (__SIGRTMIN + 1) +/* Signal needed for the kernel-supported POSIX timer implementation. */ +#define SIGTIMER (__SIGRTMIN + 2) + + /* Return is sig is used internally. */ static inline bool __is_internal_signal (int sig) { - return (sig == SIGCANCEL) || (sig == SIGSETXID); + return (sig == SIGCANCEL) || (sig == SIGSETXID) || (sig == SIGTIMER); +} + +/* Return true is SIG is withing the signal set SET, or false otherwise. */ +static inline bool +__contain_internal_signal (const sigset_t *set) +{ + return __sigismember (set, SIGCANCEL) || __sigismember (set, SIGSETXID) + || __sigismember (set, SIGTIMER); } /* Remove internal glibc signal from the mask. */ @@ -52,6 +58,7 @@ __clear_internal_signals (sigset_t *set) { __sigdelset (set, SIGCANCEL); __sigdelset (set, SIGSETXID); + __sigdelset (set, SIGTIMER); } static const sigset_t sigall_set = { diff --git a/sysdeps/unix/sysv/linux/pthread_kill.c b/sysdeps/unix/sysv/linux/pthread_kill.c index 71305b90c6..6285ce5f75 100644 --- a/sysdeps/unix/sysv/linux/pthread_kill.c +++ b/sysdeps/unix/sysv/linux/pthread_kill.c @@ -44,7 +44,7 @@ __pthread_kill (pthread_t threadid, int signo) /* Disallow sending the signal we use for cancellation, timers, for the setxid implementation. */ - if (signo == SIGCANCEL || signo == SIGTIMER || signo == SIGSETXID) + if (__is_internal_signal (signo)) return EINVAL; /* We have a special syscall to do the work. */ diff --git a/sysdeps/unix/sysv/linux/pthread_sigqueue.c b/sysdeps/unix/sysv/linux/pthread_sigqueue.c index 62a7a24531..c4d8f4cd52 100644 --- a/sysdeps/unix/sysv/linux/pthread_sigqueue.c +++ b/sysdeps/unix/sysv/linux/pthread_sigqueue.c @@ -46,7 +46,7 @@ pthread_sigqueue (pthread_t threadid, int signo, const union sigval value) /* Disallow sending the signal we use for cancellation, timers, for the setxid implementation. */ - if (signo == SIGCANCEL || signo == SIGTIMER || signo == SIGSETXID) + if (__is_internal_signal (signo)) return EINVAL; pid_t pid = getpid (); diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c index 73b0d0c19a..62820e577e 100644 --- a/sysdeps/unix/sysv/linux/sigprocmask.c +++ b/sysdeps/unix/sysv/linux/sigprocmask.c @@ -26,13 +26,10 @@ __sigprocmask (int how, const sigset_t *set, sigset_t *oset) /* The only thing we have to make sure here is that SIGCANCEL and SIGSETXID are not blocked. */ - if (set != NULL - && __glibc_unlikely (__sigismember (set, SIGCANCEL) - || __glibc_unlikely (__sigismember (set, SIGSETXID)))) + if (set != NULL && __contain_internal_signal (set)) { local_newmask = *set; - __sigdelset (&local_newmask, SIGCANCEL); - __sigdelset (&local_newmask, SIGSETXID); + __clear_internal_signals (&local_newmask); set = &local_newmask; } diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c index d96d963177..84213dd4f7 100644 --- a/sysdeps/unix/sysv/linux/timer_routines.c +++ b/sysdeps/unix/sysv/linux/timer_routines.c @@ -167,7 +167,7 @@ __start_helper_thread (void) sigset_t ss; sigset_t oss; sigfillset (&ss); - __sigaddset (&ss, SIGCANCEL); + __sigaddset (&ss, SIGTIMER); INTERNAL_SYSCALL_DECL (err); INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, &ss, &oss, _NSIG / 8);