This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Some notes:

- This patch has been signed off on by Paul Pluzhnikov and Brook Moses
at Google.

- I'm aware, as the description states, that async signal safety isn't
strictly required of pthread_getspecific (or setspecific, for that
matter), but they're extremely useful as such and as I mentioned MSAN
(a project the libc team has seemed commited to supporting in the
past) relies on this [see
http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/msan/msan_interceptors.cc?view=markup
SignalHandlerScope + definition of GetCurrentThread()], and this bug
ended up blowing up fairly spectacularly, if rarely, for us this month
due to that usage (and some similar stuff in our internal libraries.
getspecific from signal handlers is an extremely useful API that we'd
like to be able to support.

On Tue, Dec 16, 2014 at 10:57 AM, 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.
>
> (tst-key{6,7} are "proof of work" benchmarks; they don't add much value and I will revert them before submitting for real.)
> ---
>  ChangeLog                  |  4 +++
>  nptl/Makefile              |  4 ++-
>  nptl/pthread_getspecific.c | 12 ++------
>  nptl/tst-key5.c            | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>  nptl/tst-key6.c            | 17 ++++++++++
>  nptl/tst-key7.c            | 18 +++++++++++
>  6 files changed, 122 insertions(+), 10 deletions(-)
>  create mode 100644 nptl/tst-key5.c
>  create mode 100644 nptl/tst-key6.c
>  create mode 100644 nptl/tst-key7.c
>
> diff --git a/ChangeLog b/ChangeLog
> index fede1bb..d22bc3a 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +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
> +
>  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/nptl/Makefile b/nptl/Makefile
> index ac76596..b510ced 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-key6 tst-key7 \
>         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 \
> @@ -619,6 +619,8 @@ $(objpfx)tst-execstack: $(libdl)
>  $(objpfx)tst-execstack.out: $(objpfx)tst-execstack-mod.so
>  LDFLAGS-tst-execstack = -Wl,-z,noexecstack
>
> +LDFLAGS-tst-key5 = -lrt
> +
>  $(objpfx)tst-fini1mod.so: $(shared-thread-library)
>
>  tst-stackguard1-ARGS = --command "$(host-test-program-cmd) --child"
> diff --git a/nptl/pthread_getspecific.c b/nptl/pthread_getspecific.c
> index e0cc199..1f46571 100644
> --- a/nptl/pthread_getspecific.c
> +++ b/nptl/pthread_getspecific.c
> @@ -53,16 +53,10 @@ __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;
> -    }
> +  if (__glibc_unlikely (data->seq != __pthread_keys[key].seq))
> +    return NULL;
>
> -  return result;
> +  return data->data;
>  }
>  strong_alias (__pthread_getspecific, pthread_getspecific)
>  hidden_def (__pthread_getspecific)
> diff --git a/nptl/tst-key5.c b/nptl/tst-key5.c
> new file mode 100644
> index 0000000..b5cdbf9
> --- /dev/null
> +++ b/nptl/tst-key5.c
> @@ -0,0 +1,77 @@
> +/* 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>
> +
> +
> +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;
> +
> +  assert (0 == sigaction (SIGUSR1, &sa, NULL));
> +
> +  timer_t timer;
> +  struct sigevent sevp;
> +  sevp.sigev_notify = SIGEV_SIGNAL;
> +  sevp.sigev_signo = SIGUSR1;
> +  assert (0 == timer_create(CLOCK_MONOTONIC, &sevp, &timer));
> +  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)
> +    {
> +      value = (void *)((intptr_t)i + 1);
> +      assert (0 == pthread_key_create(&key, NULL));
> +      assert (0 == pthread_setspecific(key, value));
> +      if (value != pthread_getspecific(key))
> +        {
> +          printf ("Lost a value\n");
> +          return 1;
> +        }
> +      assert (0 == pthread_key_delete(key));
> +    }
> +  timer_delete(timer);
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/nptl/tst-key6.c b/nptl/tst-key6.c
> new file mode 100644
> index 0000000..8ea9d50
> --- /dev/null
> +++ b/nptl/tst-key6.c
> @@ -0,0 +1,17 @@
> +#include <assert.h>
> +#include <pthread.h>
> +
> +#define NITERS (1000 * 1000 * 1000)
> +int
> +do_test (void)
> +{
> +  pthread_key_t key;
> +  assert (0 == pthread_key_create(&key, NULL));
> +  for (int i = 0; i < NITERS; ++i)
> +    pthread_getspecific(key);
> +
> +  return 0;
> +}
> +#define TIMEOUT 100
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/nptl/tst-key7.c b/nptl/tst-key7.c
> new file mode 100644
> index 0000000..9bfed5a
> --- /dev/null
> +++ b/nptl/tst-key7.c
> @@ -0,0 +1,18 @@
> +#include <assert.h>
> +#include <pthread.h>
> +
> +#define NITERS (1000 * 1000 * 1000)
> +int
> +do_test (void)
> +{
> +  pthread_key_t key;
> +  assert (0 == pthread_key_create(&key, NULL));
> +  assert (0 == pthread_setspecific(key, &key));
> +  for (int i = 0; i < NITERS; ++i)
> +    pthread_getspecific(key);
> +
> +  return 0;
> +}
> +#define TIMEOUT 100
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> --
> 2.2.0.rc0.207.ga3a616c
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]