Re: [PATCH] nptl: Use SA_RESTART for SIGCANCEL handler

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella via Libc-alpha:

> The usage of signals to implementation pthread cancellation is an
> implementation detail and should not be visible through cancellation
> entrypoints.
>
> However now that pthread_cancel always send the SIGCANCEL, some
> entrypoint might be interruptable and return EINTR to the caller
> (for instance on sem_wait).
>
> Using SA_RESTART hides this, since the cancellation handler should
> either act uppon cancellation (if asynchronous cancellation is enable)
> or ignore the cancellation internal signal.

I think this still needs a NEWS entry because there have been kernel
bugs in this area (e.g. in CIFS).

> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  nptl/pthread_cancel.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 0698cd2046..cc25ff21f3 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -72,7 +72,11 @@ __pthread_cancel (pthread_t th)
>      {
>        struct sigaction sa;
>        sa.sa_sigaction = sigcancel_handler;
> -      sa.sa_flags = SA_SIGINFO;
> +      /* The signal handle should be non-interruptible to avoid the risk of
> +	 spurious EINTR caused by SIGCANCEL sent to process or if
> +	 pthread_cancel() is called while cancellation is disabled in the
> +	 target thread.  */
> +      sa.sa_flags = SA_SIGINFO | SA_RESTART;
>        __sigemptyset (&sa.sa_mask);
>        __libc_sigaction (SIGCANCEL, &sa, NULL);
>        atomic_store_relaxed (&init_sigcancel, 1);

I really don't feel comfortable reviewing this.  However I think it is
still consistent with the (buggy) SYSCALL_CANCEL implementation:

        int sc_cancel_oldtype = LIBC_CANCEL_ASYNC ();                        \
        sc_ret = INLINE_SYSCALL_CALL (__VA_ARGS__);                          \
        LIBC_CANCEL_RESET (sc_cancel_oldtype);                               \

We temporary enable async cancellation, in which case we unwind through
the signal handler if canceled.  We do not rely on a EINTR error return
from the system call and a cancellation check outside of the signal
handler.  So adding SA_RESTART should really be okay.

Thanks,
Florian