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

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

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.