[1/2] linux: Reserve a new signal for SIGTIMER
Commit Message
The fix for BZ#10815 requires decouple SIGTIMER and SIGCANCEL. The
POSIX timer threads created to handle SIGEV_THREAD will have the
SIGTIMER blocked but it should be able to cancel the created created
(which in turn requires SIGCANCEL unblocked).
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.
---
nptl/nptl-init.c | 9 ---------
nptl/pthread_sigmask.c | 7 ++-----
sysdeps/generic/internal-signals.h | 6 ++++++
sysdeps/nptl/allocrtsig.c | 8 ++------
sysdeps/unix/sysv/linux/internal-signals.h | 21 ++++++++++++++-------
sysdeps/unix/sysv/linux/pthread_kill.c | 2 +-
sysdeps/unix/sysv/linux/pthread_sigqueue.c | 2 +-
sysdeps/unix/sysv/linux/sigprocmask.c | 7 ++-----
sysdeps/unix/sysv/linux/timer_routines.c | 2 +-
9 files changed, 29 insertions(+), 35 deletions(-)
Comments
* 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.
@@ -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);
@@ -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;
}
@@ -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)
{
@@ -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>
@@ -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 = {
@@ -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. */
@@ -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 ();
@@ -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;
}
@@ -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);