Re: [PATCH v2 04/19] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)

[Florian Weimer via Libc-alpha <libc-alpha@sourceware.org>]

* Adhemerval Zanella:

> The use after free described in BZ#19951 is due the use of two different
> PD fields, 'joinid' and 'cancelhandling', to describe the thread state
> to synchronize the calls of pthread_join(), pthread_detach(),
> pthread_exit(), and normal thread exit.
>
> And any state change potentially requires to check for both fields
> atomically to handle partial state (such as pthread_join() with a
> cancellation handler to issue a 'joinstate' field rollback).
>
> This patch uses a different PD member with 4 possible states (JOINABLE,
> DETACHED, EXITING, and EXITED) instead of pthread 'tid' field:
>
>   1. On pthread_create() the inital state is set either to JOINABLE or
>      DETACHED depending of the pthread attribute used.
>
>   2. On pthread_detach(), a CAS is issued on the state.  If the CAS
>      fails it means that thread is already detached (DETACHED) or is
>      being terminated (EXITING).  For former an EINVAL is returned,
>      while for latter pthread_detach() should be reponsible to join the
>      thread (and deallocate any internal resources).
>
>   3. On pthread_create() exit phase (reached either if the thread

Suggest: “In the exit phase of the wrapper function for the thread start
routine (reached …”

>      function has returned, pthread_exit() has being called, or
>      cancellation handled has been acted upon) we issue a CAS on state
>      to set to EXITING mode.  If the thread is previously on DETACHED
>      mode it will be the thread itself responsible to deallocate any

Suggest: “the thread itself is responsible for arranging the
deallocation of any resource”

(detached threads cannot immediately deallocate themselves)

>      resource, otherwise the threads needs to be joined.

“the thread[] needs to be joined”


>   4. The clear_tid_field on 'clone' call is changed to set the new
>      'state' field on thread exit (EXITED).  This state ins only
>      reached at thread termination.
>
>   4. The pthread_join() implementation is now simpler: the futex wait
>      is done directly on thread state and there is no need to reset it
>      in case of timeout (since the state is now set either by
>      pthread_detach() or by the kernel on process termination).

Duplicate 4.

> The race condition on pthread_detach is avoided with only one atomic
> operation on PD state: once the mode is set to THREAD_STATE_DETACHED
> it is up to thread itself to deallocate its memory (done on the exit
> phase at pthread_create()).

See above regarding thread self-deallocation.

The design as described above looks sound to me, those are just nits.

> This change trigger an invalid C11 thread tests: it crates a thread,
> which detaches itself, and after a timeout the creating thread checks
> if the join fails.  The issue is once thrd_join() is called the thread
> lifetime has already expired.  The test is changed so the sleep is done
> by the thread itself, so the creating thread will try to join a valid
> thread.

This could be a separate commit.

> diff --git a/nptl/nptl-stack.h b/nptl/nptl-stack.h
> index 8bcfde7ec5..c6b2b3078f 100644
> --- a/nptl/nptl-stack.h
> +++ b/nptl/nptl-stack.h
> @@ -32,7 +32,7 @@ extern size_t __nptl_stack_cache_maxsize attribute_hidden;
>  static inline bool
>  __nptl_stack_in_use (struct pthread *pd)
>  {
> -  return pd->tid <= 0;
> +  return atomic_load_relaxed (&pd->joinstate) == THREAD_STATE_EXITED;
>  }

Maybe this should be an acquire load, to synchronize with the thread
exit as communicated by the kernel?

> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 38f637c5cf..67e00ef007 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -63,7 +63,8 @@ __pthread_cancel (pthread_t th)
>    volatile struct pthread *pd = (volatile struct pthread *) th;
>  
>    /* Make sure the descriptor is valid.  */
> -  if (INVALID_TD_P (pd))
> +  int state = atomic_load_acquire (&pd->joinstate);
> +  if (state == THREAD_STATE_EXITED || state == THREAD_STATE_EXITING)
>      /* Not a valid thread handle.  */
>      return ESRCH;

This is fixed in patch 08/19, so it's okay.

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 08e5189ad6..763e32bc3e 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -286,7 +286,7 @@ static int create_thread (struct pthread *pd,
> const struct
> @@ -351,13 +351,16 @@ start_thread (void *arg)
>           and free any resource prior return to the pthread_create caller.  */
>        setup_failed = pd->setup_failed == 1;
>        if (setup_failed)
> -	pd->joinid = NULL;
> +	pd->joinstate = THREAD_STATE_JOINABLE;
>  
>        /* And give it up right away.  */
>        lll_unlock (pd->lock, LLL_PRIVATE);
>  
>        if (setup_failed)
> -	goto out;
> +	{
> +	  pd->tid = 0;
> +	  goto out;
> +	}
>      }

What's the advantage of setting pd->tid here and below in start_thread?

It destroys information that could be useful for debugging purposes,
answering the question what the TID was before the thread exited.

> @@ -481,9 +484,20 @@ start_thread (void *arg)
>       the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE.  */
>    atomic_bit_set (&pd->cancelhandling, EXITING_BIT);
>  
> -  if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads)))
> -    /* This was the last thread.  */
> -    exit (0);
> +
> +  /* CONCURRENCY NOTES:
> +
> +     Concurrent pthread_detach() will either set state to
> +     THREAD_STATE_DETACHED or wait the thread to terminate.  The exiting state

“wait [for] the thread to terminate”

> +     set here is set so a pthread_join() wait until all the required cleanup
> +     steps are done.
> +
> +     The 'joinstate' field will be used to determine who is responsible to
> +     call __nptl_free_tcb below.  */
> +
> +  unsigned int joinstate = THREAD_STATE_JOINABLE;
> +  atomic_compare_exchange_weak_acquire (&pd->joinstate, &joinstate,
> +					THREAD_STATE_EXITING);

I think you need a strong CAS here.  We don't have, so you'll have to
add a loop.

> diff --git a/nptl/pthread_detach.c b/nptl/pthread_detach.c
> index ac50db9b0e..97d292cab8 100644
> --- a/nptl/pthread_detach.c
> +++ b/nptl/pthread_detach.c
> @@ -26,32 +26,24 @@ ___pthread_detach (pthread_t th)

> +     For the case the thread is being terminated (THREAD_STATE_EXITING),
> +     pthread_detach() will responsible to clean up the stack.  */
> +
> +  int curstate = THREAD_STATE_JOINABLE;
> +  if (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &curstate,
> +					     THREAD_STATE_DETACHED))

This should be a strong CAS, so this needs a loop, I think.

> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index 7303069316..428ed35101 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -22,113 +22,73 @@
>  #include <time.h>
>  #include <futex-internal.h>
>  
> -static void
> -cleanup (void *arg)
> +/* Check for a possible deadlock situation where the threads are waiting for
> +   each other to finish.  Note that this is a "may" error.  To be 100% sure we
> +   catch this error we would have to lock the data structures but it is not
> +   necessary.  In the unlikely case that two threads are really caught in this
> +   situation they will deadlock.  It is the programmer's problem to figure
> +   this out.  */
> +static inline bool
> +check_for_deadlock (int state, struct pthread *pd)
>  {
> -  /* If we already changed the waiter ID, reset it.  The call cannot
> -     fail for any reason but the thread not having done that yet so
> -     there is no reason for a loop.  */
>    struct pthread *self = THREAD_SELF;
> -  atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
> +  return ((pd == self
> +	   || (atomic_load_acquire (&self->joinstate) == THREAD_STATE_DETACHED
> +	       && (pd->cancelhandling
> +		   & (CANCELED_BITMASK | EXITING_BITMASK
> +		      | TERMINATED_BITMASK)) == 0))
> +	   && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
> +		&& (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
> +					  | TERMINATED_BITMASK))
> +		== CANCELED_BITMASK));
>  }

The state argument to check_for_deadlock appears to be unused.

> +      /* pthread_join() is a cancellation entrypoint and we use the same
> +         rationale for pthread_timedjoin_np().
> +
> +	 The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
> +	 a memory zerouing and futex wake up when the process terminates.

“memory zero[]ing and futex wake[-]up”

>  #if __TIMESIZE == 64
> diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
> index fd938e8780..726f2abc68 100644
> --- a/nptl/pthread_tryjoin.c
> +++ b/nptl/pthread_tryjoin.c
> @@ -22,15 +22,17 @@
>  int
>  __pthread_tryjoin_np (pthread_t threadid, void **thread_return)
>  {
> -  /* Return right away if the thread hasn't terminated yet.  */
> -  struct pthread *pd = (struct pthread *) threadid;
> -  if (pd->tid != 0)
> -    return EBUSY;
> +  /* The joinable state (THREAD_STATE_JOINABLE) is straigthforward since the
> +     thread hasn't finished yet and trying to join might block.
> +     Both detached (THREAD_STATE_DETACHED) and exiting (THREAD_STATE_EXITING)
> +     might also result in a possible blocking call: a detached thread might
> +     change its state to exiting and a exiting thread my take some time to
> +     exit (and thus let the kernel set the state to THREAD_STATE_EXITED).  */

pthread_tryjoin_np on a thread which is THREAD_STATE_DETACHED is
invalid, so that case doesn't matter, I think.

> +  struct pthread *pd = (struct pthread *) threadid;
> +  return atomic_load_acquire (&pd->joinstate) != THREAD_STATE_EXITED
> +	 ? EBUSY
> +	 : __pthread_clockjoin_ex (threadid, thread_return, 0, NULL);
>  }

But the code is correct, I believe.

> diff --git a/sysdeps/pthread/tst-thrd-detach.c b/sysdeps/pthread/tst-thrd-detach.c
> index c844767748..e1906a0e10 100644
> --- a/sysdeps/pthread/tst-thrd-detach.c
> +++ b/sysdeps/pthread/tst-thrd-detach.c

> -  if (thrd_join (id, NULL) == thrd_success)
> -    FAIL_EXIT1 ("thrd_join succeed where it should fail");
> +  TEST_COMPARE (thrd_join (id, NULL), thrd_error);

This is still a user-after-free bug, right?

Thanks,
Florian