[1/2] linux: Reserve a new signal for SIGTIMER
diff mbox series

Message ID 20191231193129.2590-1-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [1/2] linux: Reserve a new signal for SIGTIMER
Related show

Commit Message

Adhemerval Zanella Dec. 31, 2019, 7:31 p.m. UTC
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

Florian Weimer Dec. 31, 2019, 7:41 p.m. UTC | #1
* Adhemerval Zanella:

> The fix for BZ#10815 requires decouple SIGTIMER and SIGCANCEL.

How so?  I don't see why, sorry.
Adhemerval Zanella Jan. 3, 2020, 5:19 p.m. UTC | #2
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.
Florian Weimer Jan. 4, 2020, 10:55 a.m. UTC | #3
* 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).
Adhemerval Zanella Jan. 6, 2020, 5:13 p.m. UTC | #4
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.

Patch
diff mbox series

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);