Re: [PATCH v4 02/21] nptl: Fix Race conditions in pthread cancellation [BZ#12683]

[Zack Weinberg <zackw@panix.com>]

On Fri, Apr 3, 2020 at 4:32 PM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> This patch is the initial fix for race conditions in NPTL cancellation
> code by redefining how cancellable syscalls are defined and handled.
> The current buggy approach is to enable asynchronous cancellation
> before making the syscall and restore the previous cancellation
> type once the syscall returns.

I want to see this bug fixed.  Unfortunately I don't know the guts of
NPTL well enough to review your patches completely, but here are a few
things I noticed:

> As a side note regarding SIGCANCEL and SIGTIMER being the the same,
> it should not impact timer_create functionality.  It arranges for
> SIGCANCEL/SIGTIMER to be sent to the internal helper thread, which
> in turn check if the si.si_code is SI_TIMER and call pthread_exit
> otherwise (sysdeps/unix/sysv/linux/timer_routines.c:129).

Can we be absolutely certain that SIGCANCEL/SIGTIMER will always be
sent to a specific thread and not to a process?

> +  /* Add SIGCANCEL on ignored sigmask to avoid the handler to be called
> +     again.  */
> +  ucontext_block_sigcancel (ctx);
> +
> +  /* Check if asynchronous cancellation mode is set or if interrupted
> +     instruction pointer falls within the cancellable syscall bridge.  For
> +     interruptable syscalls that might generate external side-effects (partial
> +     reads or writes, for instance), the kernel will set the IP to after
> +     '__syscall_cancel_arch_end', thus disabling the cancellation and allowing
> +     the process to handle such conditions.  */
> +  if (self->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS
> +      || cancellation_pc_check (ctx))
> +    __do_cancel (PTHREAD_CANCELED);

Shouldn't this check happen _before_ we block further SIGCANCELs?
If cancellation_pc_check fails, because the signal was delivered on
exit from a system call that has had side effects, don't we need to
be able to receive future SIGCANCELs in order for the next cancellation
point to trigger?

>    /* Install the cancellation signal handler.  If for some reason we
>       cannot install the handler we do not abort.  Maybe we should, but
>       it is only asynchronous cancellation which is affected.  */

1) I think the third sentence of this comment has always been wrong.
2) Perhaps, if we cannot install a handler for SIGCANCEL, we should set
   a global flag which causes all calls to pthread_cancel to fail?

> +    /* Install the handle to change the threads' uid/gid.  */

Typo: handle -> handler (it was wrong before, but you may as well fix it)

> +    struct sigaction sa;
> +    __sigemptyset (&sa.sa_mask);
> +    sa.sa_sigaction = sighandler_setxid;
> +    sa.sa_flags = SA_SIGINFO | SA_RESTART;
> +    __libc_sigaction (SIGSETXID, &sa, NULL);

Unrelated preexisting bug, but I think we probably _should_ crash the
whole process if the SIGSETXID handler cannot be installed,
particularly when __libc_enable_secure is true.

> +  /* Avoid signaling when thread attempts cancel itself (pthread_kill
> +     is expensive).  */
> +  if (pd == THREAD_SELF)
> +    {
> +      if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
> +         && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> +       __pthread_exit (PTHREAD_CANCELED);
> +      return 0;

This works because __pthread_exit is actually a tiny wrapper around
__do_cancel, but I think the logic would be easier to understand,
here, if it called __do_cancel.

zw