Re: [PATCH] Make pthread_getspecific async-signal-safe

[Andrew Hunter <ahh at google dot com>]

This is version 3 of the patch.  Changes:

- formatting and documentation as requested by Roland
- dropped tst-key{6,7} (as I always planned to)
- use of atomics

the last requires a bit of explanation w/r/t my earlier mail:
essentially, the C{,++}11 standard is hugely in error when it comes to
races with signal handler--my complaints above mostly were in
reference to that language.  The C++14 standard largely fixes this,
using the language of races used elsewhere in atomics.  My new
versions of the code use atomic (relaxed) synchronization on
data->seq, plus signal_fence, to make this (I think) a race-free
program [1] (nit: so long as proper acq/rel barriers communicate the
value of the key used to call getspecific into the signal handler, but
we needed that anyway.)  It is fairly straightforward, and does not
require doing all accesses via sigatomic_t or similar :).

PTAL.

[1] for keys in the static block.  Technically speaking it's still
possible to race, though probably not worth fixing for now, the read
of THREAD_GETMEM_NC (THREAD_SELF, specific, idx1st).  The easy way to
fix this is to specify THREAD_GETMEM_NC as doing a relaxed atomic
load, which we should do anyway (and it certainly does, on x86 at
least.)

On Mon, Dec 29, 2014 at 12:07 PM, Andrew Hunter <ahh@google.com> wrote:
> While not specified as such as POSIX, projects (notably MSAN) rely on
> pthread_getspecific from signal handlers as being async-signal-safe,
> and it very nearly is.  However, currently, getspecific clears out
> "out of date" data values (with old seqnums) instead of just returning
> NULL and not touching the data. This is unnecessary (nothing will ever
> look at the stale data), and in the signal handler case, is dangerous:
> if a signal interrupts pthread_setspecific, and the compiler (legally)
> decides to write data before seq, we can stomp on a perfectly valid
> piece of per-thread data.
>
> Fix this by making pthread_getspecific look but don't touch.  This
> very modestly _improves_ its performance, in both the case where data
> is present and absent (but it's fast enough that we really don't
> care--I'm mostly just pointing out that this isn't a performance
> *loss*.)  More importantly, it's now safe to call getspecific from a
> signal handler, even in the presence of a racing setspecific. This is
> verified by tst-key5.
> ---
>  ChangeLog                  |   5 +++
>  include/atomic.h           |  17 ++++++++
>  manual/threads.texi        |   3 +-
>  nptl/Makefile              |   3 +-
>  nptl/pthread_getspecific.c |  19 ++++----
>  nptl/pthread_setspecific.c |   6 ++-
>  nptl/tst-key5.c            | 106 +++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 146 insertions(+), 13 deletions(-)
>  create mode 100644 nptl/tst-key5.c
>
> diff --git a/ChangeLog b/ChangeLog
> index fede1bb..b511884 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-12-16  Andrew Hunter <ahh@google.com>
> +       * nptl/pthread_getspecific.c : fix async signal safety
> +       * nptl/tst-key5.c : test for async signal safety
> +       * include/atomic.h : add atomic_signal_fence
> +
>  2014-12-05  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
>
>         * libio/tst-ftell-active-handler.c (do_ftell_test): Fix buffer overrun
> diff --git a/include/atomic.h b/include/atomic.h
> index c4df8f1..dc500b8 100644
> --- a/include/atomic.h
> +++ b/include/atomic.h
> @@ -575,6 +575,13 @@ void __atomic_link_error (void);
>  # define atomic_thread_fence_seq_cst() \
>    __atomic_thread_fence (__ATOMIC_SEQ_CST)
>
> +# define atomic_signal_fence_acquire() \
> +  __atomic_signal_fence (__ATOMIC_ACQUIRE)
> +# define atomic_signal_fence_release() \
> +  __atomic_signal_fence (__ATOMIC_RELEASE)
> +# define atomic_signal_fence_seq_cst() \
> +  __atomic_signal_fence (__ATOMIC_SEQ_CST)
> +
>  # define atomic_load_relaxed(mem) \
>    ({ __atomic_check_size((mem)); __atomic_load_n ((mem), __ATOMIC_RELAXED); })
>  # define atomic_load_acquire(mem) \
> @@ -651,6 +658,16 @@ void __atomic_link_error (void);
>  #  define atomic_thread_fence_seq_cst() atomic_full_barrier ()
>  # endif
>
> +# ifndef atomic_signal_fence_acquire
> +#  define atomic_signal_fence_acquire() __asm ("" ::: "memory")
> +# endif
> +# ifndef atomic_signal_fence_release
> +#  define atomic_signal_fence_release() __asm ("" ::: "memory")
> +# endif
> +# ifndef atomic_signal_fence_seq_cst
> +#  define atomic_signal_fence_seq_cst() __asm ("" ::: "memory")
> +# endif
> +
>  # ifndef atomic_load_relaxed
>  #  define atomic_load_relaxed(mem) \
>     ({ __typeof (*(mem)) __atg100_val;                                        \
> diff --git a/manual/threads.texi b/manual/threads.texi
> index 4d080d4..18ae530 100644
> --- a/manual/threads.texi
> +++ b/manual/threads.texi
> @@ -55,7 +55,8 @@ is it called during thread exit.
>  @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
>  @c pthread_getspecific ok
>  Return the thread-specific data associated with @var{key} in the calling
> -thread.
> +thread.  This function is async-signal-safe, by special glibc
> +guarantee over and above the POSIX specification.
>  @end deftypefun
>
>  @comment pthread.h
> diff --git a/nptl/Makefile b/nptl/Makefile
> index ac76596..dcfccbb 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -224,7 +224,7 @@ tests = tst-typesizes \
>         tst-rwlock5 tst-rwlock6 tst-rwlock7 tst-rwlock8 tst-rwlock9 \
>         tst-rwlock10 tst-rwlock11 tst-rwlock12 tst-rwlock13 tst-rwlock14 \
>         tst-once1 tst-once2 tst-once3 tst-once4 \
> -       tst-key1 tst-key2 tst-key3 tst-key4 \
> +       tst-key1 tst-key2 tst-key3 tst-key4 tst-key5 \
>         tst-sem1 tst-sem2 tst-sem3 tst-sem4 tst-sem5 tst-sem6 tst-sem7 \
>         tst-sem8 tst-sem9 tst-sem10 tst-sem11 tst-sem12 tst-sem13 tst-sem14 \
>         tst-barrier1 tst-barrier2 tst-barrier3 tst-barrier4 \
> @@ -529,6 +529,7 @@ endif
>
>  $(objpfx)tst-cancel17: $(librt)
>  $(objpfx)tst-cancelx17: $(librt)
> +$(objpfx)tst-key5: $(librt)
>  $(objpfx)tst-_res1mod2.so: $(objpfx)tst-_res1mod1.so
>  LDFLAGS-tst-_res1mod1.so = -Wl,-soname,tst-_res1mod1.so
>  LDFLAGS-tst-_res1mod2.so = -Wl,-soname,tst-_res1mod2.so
> diff --git a/nptl/pthread_getspecific.c b/nptl/pthread_getspecific.c
> index e0cc199..355f147 100644
> --- a/nptl/pthread_getspecific.c
> +++ b/nptl/pthread_getspecific.c
> @@ -53,16 +53,15 @@ __pthread_getspecific (key)
>        data = &level2[idx2nd];
>      }
>
> -  void *result = data->data;
> -  if (result != NULL)
> -    {
> -      uintptr_t seq = data->seq;
> -
> -      if (__glibc_unlikely (seq != __pthread_keys[key].seq))
> -       result = data->data = NULL;
> -    }
> -
> -  return result;
> +  /* Since we might be inside a signal handler, use atomic ops to
> +     synchronize with ourselves, and prevent speculative (and racy)
> +     reading of data--we don't know it's valid until after we see a
> +     valid seqnum.  */
> +  if (__glibc_unlikely (
> +          atomic_load_relaxed(&data->seq) != __pthread_keys[key].seq))
> +    return NULL;
> +  atomic_signal_fence_acquire();
> +  return data->data;
>  }
>  strong_alias (__pthread_getspecific, pthread_getspecific)
>  hidden_def (__pthread_getspecific)
> diff --git a/nptl/pthread_setspecific.c b/nptl/pthread_setspecific.c
> index 0ace861..52ce948 100644
> --- a/nptl/pthread_setspecific.c
> +++ b/nptl/pthread_setspecific.c
> @@ -86,8 +86,12 @@ __pthread_setspecific (key, value)
>
>    /* Store the data and the sequence number so that we can recognize
>       stale data.  */
> -  level2->seq = seq;
>    level2->data = (void *) value;
> +  /* pthread_getspecific might be called from a signal handler--order
> +     writes so that if it can see a valid seqnum, it can
> +     see our data.  */
> +  atomic_signal_fence_release();
> +  atomic_store_relaxed(&level2->seq, seq);
>
>    return 0;
>  }
> diff --git a/nptl/tst-key5.c b/nptl/tst-key5.c
> new file mode 100644
> index 0000000..56bd4f0
> --- /dev/null
> +++ b/nptl/tst-key5.c
> @@ -0,0 +1,106 @@
> +/* Copyright (C) 2014 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +#include <unistd.h>
> +#include <signal.h>
> +#include <time.h>
> +#include <assert.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +/* Test that pthread_getspecific called from a signal handler doesn't
> +   cause problems, even when racing with potential key_create or
> +   setspecific calls.  */
> +
> +pthread_key_t key;
> +void *value;
> +size_t r;
> +static void
> +handler (int signo)
> +{
> +  void *ret = pthread_getspecific (key);
> +  /* We race with the setspecific--either result is fine, just not junk.  */
> +  assert (ret == value || ret == NULL);
> +  r++;
> +}
> +
> +
> +int
> +do_test (void)
> +{
> +  struct sigaction sa;
> +  memset (&sa, 0, sizeof (sa));
> +  sa.sa_handler = handler;
> +
> +  if (0 != == sigaction (SIGUSR1, &sa, NULL))
> +    {
> +      puts ("sigaction failed");
> +      exit (1);
> +    }
> +
> +  timer_t timer;
> +  struct sigevent sevp;
> +  sevp.sigev_notify = SIGEV_SIGNAL;
> +  sevp.sigev_signo = SIGUSR1;
> +  if (0 != timer_create (CLOCK_MONOTONIC, &sevp, &timer))
> +    {
> +      puts ("timer_create failed");
> +      exit (1);
> +    }
> +  struct itimerspec spec;
> +  spec.it_value.tv_sec = 0;
> +  spec.it_value.tv_nsec = 500;
> +  spec.it_interval = spec.it_value;
> +  timer_settime (timer, 0, &spec, NULL);
> +#define NITERS (1000 * 1000)
> +  for (int i = 0; i < NITERS; ++i)
> +    {
> +      /* This is an arbitrary value, unique between iterations, so
> +         we're not fooled by seeing an old value.  */
> +      value = (void *) ((intptr_t)i + 1);
> +      if (0 != pthread_key_create (&key, NULL))
> +        {
> +          puts ("pthread_key_create failed");
> +          exit (1);
> +        }
> +      if (0 != pthread_setspecific (key, value))
> +        {
> +          puts ("pthread_setspecific failed");
> +          exit (1);
> +        }
> +      /* This is the key test: if we raced with a getspecific from
> +         signal handler, it's important we get back the key we asked
> +         for--it should not have been removed or changed by the other
> +         call.  */
> +      if (value != pthread_getspecific (key))
> +        {
> +          printf ("Lost a value\n");
> +          return 1;
> +        }
> +      if (0 != pthread_key_delete (key))
> +        {
> +          puts ("pthread_key_delete failed");
> +          exit (1);
> +        }
> +    }
> +  timer_delete (timer);
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> --
> 2.2.0.rc0.207.ga3a616c
>