Re: [PATCH 04/13] nptl: Fix Race conditions in pthread cancellation (BZ#12683)

[Szabolcs Nagy <szabolcs dot nagy at arm dot com>]

On 07/10/15 17:23, Adhemerval Zanella wrote:
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 79bcaab..3c5e3b7 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -38,6 +38,7 @@
>   #include <kernel-features.h>
>   #include <libc-internal.h>
>   #include <pthread-pids.h>
> +#include <sysdep-cancel.h>
>
>   #ifndef TLS_MULTIPLE_THREADS_IN_TCB
>   /* Pointer to the corresponding variable in libc.  */
> @@ -200,36 +201,41 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
>       return;
>
>     struct pthread *self = THREAD_SELF;
> +  volatile struct pthread *pd = (volatile struct pthread *) self;
> +  ucontext_t *uc = ctx;
> +  const char *tip = (const char *)__pthread_get_ip (ctx);

i assume *pd is volatile here (and elsewhere) because
pd->cancelhandling is modified in a signal handler (here).

i think marking the cancelhandling field in the pthread
struct volatile would be cleaner (to avoid volatile casts).

> -  int oldval = THREAD_GETMEM (self, cancelhandling);
> -  while (1)
> +  extern const char __syscall_cancel_arch_start[1];
> +  extern const char __syscall_cancel_arch_end[1];
> +
> +  if (((pd->cancelhandling & (CANCELSTATE_BITMASK)) != 0)
> +      || ((pd->cancelhandling & CANCELED_BITMASK) == 0))
> +    return;
> +
> +  __sigaddset (&uc->uc_sigmask, SIGCANCEL);
> +
> +  /* Check if asynchronous cancellation mode is set and if interrupted
> +     instruction pointer falls within the cancellable syscall code.  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 (pd->cancelhandling & CANCELTYPE_BITMASK ||
> +      (tip >= __syscall_cancel_arch_start && tip < __syscall_cancel_arch_end))
>       {
> -      /* We are canceled now.  When canceled by another thread this flag
> -        is already set but if the signal is directly send (internally or
> -        from another process) is has to be done here.  */
> -      int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> -
> -      if (oldval == newval || (oldval & EXITING_BITMASK) != 0)
> -       /* Already canceled or exiting.  */
> -       break;
> -
> -      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> -                                             oldval);
> -      if (curval == oldval)
> -       {
> -         /* Set the return value.  */
> -         THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> -
> -         /* Make sure asynchronous cancellation is still enabled.  */
> -         if ((newval & CANCELTYPE_BITMASK) != 0)
> -           /* Run the registered destructors and terminate the thread.  */
> -           __do_cancel ();
> -
> -         break;
> -       }
> -
> -      oldval = curval;
> +      THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
> +      THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> +

the atomic bit set here uses self->cancelhandling,
i assume atomic is ok in an async-signal-handler
and implies voltile, but is atomicity required here?

> +      /* __pthread_sigmask removes SIGCANCEL from the set.  */
> +      INTERNAL_SYSCALL_DECL (err);
> +      INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIGCANCEL, &uc->uc_sigmask, 0,
> +                        _NSIG / 8);
> +
> +      __do_cancel ();
>       }
> +
> +  INLINE_SYSCALL (tgkill, 3, THREAD_GETMEM (THREAD_SELF, pid), pd->tid,
> +                 SIGCANCEL);

does pd->tid have to be a volatile access here?

>   }
>   #endif
>
> @@ -400,7 +406,10 @@ __pthread_initialize_minimal_internal (void)
>        cannot install the handler we do not abort.  Maybe we should, but
>        it is only asynchronous cancellation which is affected.  */
>     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;
>     (void) __libc_sigaction (SIGCANCEL, &sa, NULL);
>   # endif