diff mbox series

nptl: Do not set signal mask on second setjmp return [BZ #28607]

Message ID 87sfvtcjua.fsf@oldenburg.str.redhat.com
State Committed
Commit e186fc5a31e46f2cbf5ea1a75223b4412907f3d8
Headers show
Series nptl: Do not set signal mask on second setjmp return [BZ #28607] | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Florian Weimer Nov. 18, 2021, 8:03 p.m. UTC
__libc_signal_restore_set was in the wrong place: It also ran
when setjmp returned the second time (after pthread_exit or
pthread_cancel).  This is observable with blocked pending
signals during thread exit.

Tested on i686-linux-gnu and x86_64-linux-gnu.

---
 nptl/pthread_create.c                     |  4 +--
 sysdeps/pthread/Makefile                  |  1 +
 sysdeps/pthread/tst-pthread-exit-signal.c | 45 +++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 2 deletions(-)

Comments

Adhemerval Zanella Nov. 22, 2021, 7:24 p.m. UTC | #1
On 18/11/2021 17:03, Florian Weimer via Libc-alpha wrote:
> __libc_signal_restore_set was in the wrong place: It also ran
> when setjmp returned the second time (after pthread_exit or
> pthread_cancel).  This is observable with blocked pending
> signals during thread exit.
> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.,

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
> ---
>  nptl/pthread_create.c                     |  4 +--
>  sysdeps/pthread/Makefile                  |  1 +
>  sysdeps/pthread/tst-pthread-exit-signal.c | 45 +++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index d6ea43a754..bad9eeb52f 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -406,8 +406,6 @@ start_thread (void *arg)
>    unwind_buf.priv.data.prev = NULL;
>    unwind_buf.priv.data.cleanup = NULL;
>  
> -  __libc_signal_restore_set (&pd->sigmask);
> -
>    /* Allow setxid from now onwards.  */
>    if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
>      futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
> @@ -417,6 +415,8 @@ start_thread (void *arg)
>        /* Store the new cleanup handler info.  */
>        THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
>  
> +      __libc_signal_restore_set (&pd->sigmask);
> +
>        LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
>  
>        /* Run the code the user provided.  */

Ok.

> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index df8943f486..c657101696 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -118,6 +118,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
>  	 tst-unload \
>  	 tst-unwind-thread \
>  	 tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \
> +	 tst-pthread-exit-signal \
>  	 tst-pthread-setuid-loop \
>  	 tst-pthread_cancel-exited \
>  	 tst-pthread_cancel-select-loop \

Ok.

> diff --git a/sysdeps/pthread/tst-pthread-exit-signal.c b/sysdeps/pthread/tst-pthread-exit-signal.c
> new file mode 100644
> index 0000000000..b4526fe663
> --- /dev/null
> +++ b/sysdeps/pthread/tst-pthread-exit-signal.c
> @@ -0,0 +1,45 @@
> +/* Test that pending signals are not delivered on thread exit (bug 28607).
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* Due to bug 28607, pthread_kill (or pthread_cancel) restored the
> +   signal mask during during thread exit, triggering the delivery of a
> +   blocked pending signal (SIGUSR1 in this test).  */
> +
> +#include <support/xthread.h>
> +#include <support/xsignal.h>
> +
> +static void *
> +threadfunc (void *closure)
> +{
> +  sigset_t sigmask;
> +  sigfillset (&sigmask);
> +  xpthread_sigmask (SIG_SETMASK, &sigmask, NULL);
> +  xpthread_kill (pthread_self (), SIGUSR1);
> +  pthread_exit (NULL);
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  pthread_t thr = xpthread_create (NULL, threadfunc, NULL);
> +  xpthread_join (thr);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 

Ok.
diff mbox series

Patch

diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index d6ea43a754..bad9eeb52f 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -406,8 +406,6 @@  start_thread (void *arg)
   unwind_buf.priv.data.prev = NULL;
   unwind_buf.priv.data.cleanup = NULL;
 
-  __libc_signal_restore_set (&pd->sigmask);
-
   /* Allow setxid from now onwards.  */
   if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
     futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
@@ -417,6 +415,8 @@  start_thread (void *arg)
       /* Store the new cleanup handler info.  */
       THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
 
+      __libc_signal_restore_set (&pd->sigmask);
+
       LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
 
       /* Run the code the user provided.  */
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index df8943f486..c657101696 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -118,6 +118,7 @@  tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	 tst-unload \
 	 tst-unwind-thread \
 	 tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \
+	 tst-pthread-exit-signal \
 	 tst-pthread-setuid-loop \
 	 tst-pthread_cancel-exited \
 	 tst-pthread_cancel-select-loop \
diff --git a/sysdeps/pthread/tst-pthread-exit-signal.c b/sysdeps/pthread/tst-pthread-exit-signal.c
new file mode 100644
index 0000000000..b4526fe663
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread-exit-signal.c
@@ -0,0 +1,45 @@ 
+/* Test that pending signals are not delivered on thread exit (bug 28607).
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* Due to bug 28607, pthread_kill (or pthread_cancel) restored the
+   signal mask during during thread exit, triggering the delivery of a
+   blocked pending signal (SIGUSR1 in this test).  */
+
+#include <support/xthread.h>
+#include <support/xsignal.h>
+
+static void *
+threadfunc (void *closure)
+{
+  sigset_t sigmask;
+  sigfillset (&sigmask);
+  xpthread_sigmask (SIG_SETMASK, &sigmask, NULL);
+  xpthread_kill (pthread_self (), SIGUSR1);
+  pthread_exit (NULL);
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t thr = xpthread_create (NULL, threadfunc, NULL);
+  xpthread_join (thr);
+  return 0;
+}
+
+#include <support/test-driver.c>