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 v2] nptl: Fix deadlock on atfork handler which calls dlclose (BZ#24595)


Ping.

On 08/08/2019 16:12, Adhemerval Zanella wrote:
> Changes from previous version:
> 
>   - Removed use of recursive lock in favor or release and re-acquire the
>     lock when calling the atfork callback.
> 
> ---
> 
> Some real-world cases rely upon that atfork handlers can call functions
> that might change the atfork handlers, such as dlclose.  Since 27761a10
> (Refactor atfork handlers), all internal atfork handlers access is
> protected with a simple lock, not allowing reentrancy.  This leads to
> deadlocks for the aforementioned scenario. Although this specific usage
> is far from portable (as comment #2 in the bug report), glibc did allow
> it in the past.
> 
> This patch fixes by using a double-linked lists along with a lock
> release while calling the atfork handlers.  This is similar to the
> strategy used on atexit.
> 
> The list should be kept concise as long as it is invalid to dlclose itself.
> It should also be safe also call pthread_atfork from a registered callback,
> although it is up to the caller to handle the resulting list state regarding
> callback call.  Since the new element is added on the end of the list, the
> prepare handler won't see the new element (since it ran in reverse order),
> however both the parent and child will.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> 
> 	* misc/sys/queue.h (TAILQ_FOREACH_SAFE): New macro.
> 	* nptl/Makefile [build-shared == yes] (tests): Add tst-atfork3.
> 	(modules-names): Add tst-atfork3mod.
> 	(tst-atfork3mod.so-no-z-defs, $(objpfx)tst-atfork3:,
> 	LDFLAGS-tst-atfork3, $(objpfx)tst-atfork3mod.so,
> 	$(objpfx)tst-atfork3.out): New rules.
> 	* nptl/register-atfork.c: (__register_atfork, __unregister_atfork,
> 	__run_fork_handlers, libc_freeres_fn): Remove usage of dynarray in
> 	 favor of a double linked list.
> 	(atfork_lock): Unlock before calling the callback.
> 	(fork_handlers_push_back, fork_handlers_remove,
> 	fork_handlers_remove_if, fork_handlers_remove_all): New functions.
> 	(fork_handler_list_find): Remove function.
> 	* nptl/tst-atfork3.c, nptl/tst-atfork3mod.c: New files.
> 	* sysdeps/nptl/fork.h (fork_handler): Remove definition.
> ---
>  misc/sys/queue.h       |   5 ++
>  nptl/Makefile          |  10 ++-
>  nptl/register-atfork.c | 159 +++++++++++++++++++++--------------------
>  nptl/tst-atfork3.c     | 120 +++++++++++++++++++++++++++++++
>  nptl/tst-atfork3mod.c  |  45 ++++++++++++
>  sysdeps/nptl/fork.h    |   9 ---
>  6 files changed, 257 insertions(+), 91 deletions(-)
>  create mode 100644 nptl/tst-atfork3.c
>  create mode 100644 nptl/tst-atfork3mod.c
> 
> diff --git a/misc/sys/queue.h b/misc/sys/queue.h
> index daf4553d33..0187f298ce 100644
> --- a/misc/sys/queue.h
> +++ b/misc/sys/queue.h
> @@ -437,6 +437,11 @@ struct {								\
>  		(var);							\
>  		(var) = ((var)->field.tqe_next))
>  
> +#define	TAILQ_FOREACH_SAFE(var, head, field, tvar)			\
> +	for ((var) = ((head)->tqh_first);				\
> +	     (var) && ((tvar) = ((var))->field.tqe_next, 1);		\
> +	     (var) = (tvar))
> +
>  #define	TAILQ_FOREACH_REVERSE(var, head, headname, field)		\
>  	for ((var) = (*(((struct headname *)((head)->tqh_last))->tqh_last));	\
>  		(var);							\
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 0567e77a79..3a61319789 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -396,7 +396,7 @@ tests += tst-cancelx2 tst-cancelx3 tst-cancelx4 tst-cancelx5 \
>  	 tst-oncex3 tst-oncex4
>  ifeq ($(build-shared),yes)
>  tests += tst-atfork2 tst-tls4 tst-_res1 tst-fini1 tst-compat-forwarder \
> -	 tst-audit-threads
> +	 tst-audit-threads tst-atfork3
>  tests-internal += tst-tls3 tst-tls3-malloc tst-tls5 tst-stackguard1
>  tests-nolibpthread += tst-fini1
>  ifeq ($(have-z-execstack),yes)
> @@ -409,13 +409,14 @@ modules-names = tst-atfork2mod tst-tls3mod tst-tls4moda tst-tls4modb \
>  		tst-tls5modd tst-tls5mode tst-tls5modf tst-stack4mod \
>  		tst-_res1mod1 tst-_res1mod2 tst-execstack-mod tst-fini1mod \
>  		tst-join7mod tst-compat-forwarder-mod tst-audit-threads-mod1 \
> -		tst-audit-threads-mod2
> +		tst-audit-threads-mod2 tst-atfork3mod
>  extra-test-objs += $(addsuffix .os,$(strip $(modules-names))) \
>  		   tst-cleanup4aux.o tst-cleanupx4aux.o
>  test-extras += tst-cleanup4aux tst-cleanupx4aux
>  test-modules = $(addprefix $(objpfx),$(addsuffix .so,$(modules-names)))
>  
>  tst-atfork2mod.so-no-z-defs = yes
> +tst-atfork3mod.so-no-z-defs = yes
>  tst-tls3mod.so-no-z-defs = yes
>  tst-tls5mod.so-no-z-defs = yes
>  tst-tls5moda.so-no-z-defs = yes
> @@ -552,9 +553,12 @@ tst-cancelx7-ARGS = $(tst-cancel7-ARGS)
>  tst-umask1-ARGS = $(objpfx)tst-umask1.temp
>  
>  $(objpfx)tst-atfork2: $(libdl) $(shared-thread-library)
> +$(objpfx)tst-atfork3: $(libdl) $(shared-thread-library)
>  LDFLAGS-tst-atfork2 = -rdynamic
> +LDFLAGS-tst-atfork3 = -rdynamic
>  tst-atfork2-ENV = MALLOC_TRACE=$(objpfx)tst-atfork2.mtrace
>  $(objpfx)tst-atfork2mod.so: $(shared-thread-library)
> +$(objpfx)tst-atfork3mod.so: $(shared-thread-library)
>  
>  tst-stack3-ENV = MALLOC_TRACE=$(objpfx)tst-stack3.mtrace
>  $(objpfx)tst-stack3-mem.out: $(objpfx)tst-stack3.out
> @@ -654,7 +658,7 @@ $(addprefix $(objpfx), $(tests-reverse)): \
>  $(objpfx)../libc.so: $(common-objpfx)libc.so ;
>  $(addprefix $(objpfx),$(tests-static) $(xtests-static)): $(objpfx)libpthread.a
>  
> -$(objpfx)tst-atfork2.out: $(objpfx)tst-atfork2mod.so
> +$(objpfx)tst-atfork3.out: $(objpfx)tst-atfork3mod.so
>  else
>  $(addprefix $(objpfx),$(tests) $(test-srcs)): $(objpfx)libpthread.a
>  endif
> diff --git a/nptl/register-atfork.c b/nptl/register-atfork.c
> index 80a1becb5f..3e7b40024e 100644
> --- a/nptl/register-atfork.c
> +++ b/nptl/register-atfork.c
> @@ -20,131 +20,132 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <fork.h>
> -#include <atomic.h>
>  
> -#define DYNARRAY_ELEMENT           struct fork_handler
> -#define DYNARRAY_STRUCT            fork_handler_list
> -#define DYNARRAY_PREFIX            fork_handler_list_
> -#define DYNARRAY_INITIAL_SIZE      48
> -#include <malloc/dynarray-skeleton.c>
> -
> -static struct fork_handler_list fork_handlers;
> -static bool fork_handler_init = false;
> +#include <sys/queue.h>
>  
> +struct fork_handler
> +{
> +  TAILQ_ENTRY (fork_handler) qe;
> +  void (*prepare_handler) (void);
> +  void (*parent_handler) (void);
> +  void (*child_handler) (void);
> +  void *dso_handle;
> +};
> +
> +typedef TAILQ_HEAD (fork_handler_list, fork_handler) fork_handler_list;
> +static fork_handler_list fork_handlers = TAILQ_HEAD_INITIALIZER (fork_handlers);
>  static int atfork_lock = LLL_LOCK_INITIALIZER;
>  
>  int
>  __register_atfork (void (*prepare) (void), void (*parent) (void),
>  		   void (*child) (void), void *dso_handle)
>  {
> -  lll_lock (atfork_lock, LLL_PRIVATE);
> +  struct fork_handler *entry = malloc (sizeof (struct fork_handler));
> +  if (entry == NULL)
> +    return ENOMEM;
>  
> -  if (!fork_handler_init)
> -    {
> -      fork_handler_list_init (&fork_handlers);
> -      fork_handler_init = true;
> -    }
> +  entry->prepare_handler = prepare;
>  
> -  struct fork_handler *newp = fork_handler_list_emplace (&fork_handlers);
> -  if (newp != NULL)
> -    {
> -      newp->prepare_handler = prepare;
> -      newp->parent_handler = parent;
> -      newp->child_handler = child;
> -      newp->dso_handle = dso_handle;
> -    }
> +  entry->parent_handler = parent;
> +  entry->child_handler = child;
> +  entry->dso_handle = dso_handle;
>  
> -  /* Release the lock.  */
> +  lll_lock (atfork_lock, LLL_PRIVATE);
> +  TAILQ_INSERT_TAIL (&fork_handlers, entry, qe);
>    lll_unlock (atfork_lock, LLL_PRIVATE);
>  
> -  return newp == NULL ? ENOMEM : 0;
> +  return 0;
>  }
>  libc_hidden_def (__register_atfork)
>  
> -static struct fork_handler *
> -fork_handler_list_find (struct fork_handler_list *fork_handlers,
> -			void *dso_handle)
> -{
> -  for (size_t i = 0; i < fork_handler_list_size (fork_handlers); i++)
> -    {
> -      struct fork_handler *elem = fork_handler_list_at (fork_handlers, i);
> -      if (elem->dso_handle == dso_handle)
> -	return elem;
> -    }
> -  return NULL;
> -}
> -
>  void
>  __unregister_atfork (void *dso_handle)
>  {
> -  lll_lock (atfork_lock, LLL_PRIVATE);
> +  fork_handler_list temp_list = TAILQ_HEAD_INITIALIZER (temp_list);
> +  struct fork_handler *it, *it1;
>  
> -  struct fork_handler *first = fork_handler_list_find (&fork_handlers,
> -						       dso_handle);
> -  /* Removing is done by shifting the elements in the way the elements
> -     that are not to be removed appear in the beginning in dynarray.
> -     This avoid the quadradic run-time if a naive strategy to remove and
> -     shift one element at time.  */
> -  if (first != NULL)
> +  lll_lock (atfork_lock, LLL_PRIVATE);
> +  TAILQ_FOREACH_SAFE (it, &fork_handlers, qe, it1)
>      {
> -      struct fork_handler *new_end = first;
> -      first++;
> -      for (; first != fork_handler_list_end (&fork_handlers); ++first)
> +      if (it->dso_handle == dso_handle)
>  	{
> -	  if (first->dso_handle != dso_handle)
> -	    {
> -	      *new_end = *first;
> -	      ++new_end;
> -	    }
> +	  TAILQ_REMOVE (&fork_handlers, it, qe);
> +	  TAILQ_INSERT_TAIL (&temp_list, it, qe);
>  	}
> +    }
> +  lll_unlock (atfork_lock, LLL_PRIVATE);
>  
> -      ptrdiff_t removed = first - new_end;
> -      for (size_t i = 0; i < removed; i++)
> -	fork_handler_list_remove_last (&fork_handlers);
> +  while ((it = TAILQ_FIRST (&temp_list)) != NULL)
> +    {
> +      TAILQ_REMOVE (&temp_list, it, qe);
> +      free (it);
>      }
> +}
>  
> -  lll_unlock (atfork_lock, LLL_PRIVATE);
> +static inline void
> +lock_atfork (_Bool do_locking)
> +{
> +  if (do_locking)
> +    lll_lock (atfork_lock, LLL_PRIVATE);
> +}
> +
> +static inline void
> +unlock_atfork (_Bool do_locking)
> +{
> +  if (do_locking)
> +    lll_unlock (atfork_lock, LLL_PRIVATE);
>  }
>  
>  void
>  __run_fork_handlers (enum __run_fork_handler_type who, _Bool do_locking)
>  {
> -  struct fork_handler *runp;
> +  struct fork_handler *it;
>  
>    if (who == atfork_run_prepare)
>      {
> -      if (do_locking)
> -	lll_lock (atfork_lock, LLL_PRIVATE);
> -      size_t sl = fork_handler_list_size (&fork_handlers);
> -      for (size_t i = sl; i > 0; i--)
> +      lock_atfork (do_locking);
> +      TAILQ_FOREACH_REVERSE (it, &fork_handlers, fork_handler_list, qe)
>  	{
> -	  runp = fork_handler_list_at (&fork_handlers, i - 1);
> -	  if (runp->prepare_handler != NULL)
> -	    runp->prepare_handler ();
> +	  if (it->prepare_handler != NULL)
> +	    {
> +	      /* Unlock the list while we call a foreign function.  */
> +	      unlock_atfork (do_locking);
> +	      it->prepare_handler ();
> +	      lock_atfork (do_locking);
> +	    }
>  	}
>      }
>    else
>      {
> -      size_t sl = fork_handler_list_size (&fork_handlers);
> -      for (size_t i = 0; i < sl; i++)
> +      TAILQ_FOREACH (it, &fork_handlers, qe)
>  	{
> -	  runp = fork_handler_list_at (&fork_handlers, i);
> -	  if (who == atfork_run_child && runp->child_handler)
> -	    runp->child_handler ();
> -	  else if (who == atfork_run_parent && runp->parent_handler)
> -	    runp->parent_handler ();
> +	  /* Unlock the list while we call a foreign function.  */
> +	  if (who == atfork_run_child && it->child_handler)
> +	    {
> +	      unlock_atfork (do_locking);
> +	      it->child_handler ();
> +	      lock_atfork (do_locking);
> +	    }
> +	  else if (who == atfork_run_parent && it->parent_handler)
> +	    {
> +	      unlock_atfork (do_locking);
> +	      it->parent_handler ();
> +	      lock_atfork (do_locking);
> +	    }
>  	}
> -      if (do_locking)
> -	lll_unlock (atfork_lock, LLL_PRIVATE);
> +      unlock_atfork (do_locking);
>      }
>  }
>  
> -
>  libc_freeres_fn (free_mem)
>  {
> -  lll_lock (atfork_lock, LLL_PRIVATE);
> -
> -  fork_handler_list_free (&fork_handlers);
> +  struct fork_handler *it, *it1;
>  
> +  lll_lock (atfork_lock, LLL_PRIVATE);
> +  TAILQ_FOREACH_SAFE (it, &fork_handlers, qe, it1)
> +    {
> +      TAILQ_REMOVE (&fork_handlers, it, qe);
> +      free (it);
> +    }
>    lll_unlock (atfork_lock, LLL_PRIVATE);
>  }
> diff --git a/nptl/tst-atfork3.c b/nptl/tst-atfork3.c
> new file mode 100644
> index 0000000000..466a5933c1
> --- /dev/null
> +++ b/nptl/tst-atfork3.c
> @@ -0,0 +1,120 @@
> +/* Check if pthread_atfork handler can call dlclose (BZ#24595)
> +   Copyright (C) 2019 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 <stdio.h>
> +#include <dlfcn.h>
> +#include <pthread.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +
> +#include <support/check.h>
> +#include <support/xthread.h>
> +#include <support/capture_subprocess.h>
> +
> +/* Check if pthread_atfork handlers do not deadlock when calling a function
> +   that might alter the internal fork handle list, such as dlclose.
> +
> +   The test registers a callback set with pthread_atfork(), dlopen() a shared
> +   library (nptl/tst-atfork3mod.c), calls an exported symbol from the library
> +   (which in turn also registers atfork handlers), and calls fork to trigger
> +   the callbacks.  */
> +
> +static void *handler;
> +static bool run_dlclose_prepare;
> +static bool run_dlclose_parent;
> +static bool run_dlclose_child;
> +
> +static void
> +prepare (void)
> +{
> +  if (run_dlclose_prepare)
> +    TEST_COMPARE (dlclose (handler), 0);
> +}
> +
> +static void
> +parent (void)
> +{
> +  if (run_dlclose_parent)
> +    TEST_COMPARE (dlclose (handler), 0);
> +}
> +
> +static void
> +child (void)
> +{
> +  if (run_dlclose_child)
> +    TEST_COMPARE (dlclose (handler), 0);
> +}
> +
> +static void
> +proc_func (void *closure)
> +{
> +}
> +
> +static void
> +do_test_generic (bool dlclose_prepare, bool dlclose_parent, bool dlclose_child)
> +{
> +  run_dlclose_prepare = dlclose_prepare;
> +  run_dlclose_parent = dlclose_parent;
> +  run_dlclose_child = dlclose_child;
> +
> +  handler = dlopen ("tst-atfork3mod.so", RTLD_NOW);
> +  TEST_VERIFY (handler != NULL);
> +
> +  int (*atfork3mod_func)(void);
> +  atfork3mod_func = dlsym (handler, "atfork3mod_func");
> +  TEST_VERIFY (atfork3mod_func != NULL);
> +
> +  atfork3mod_func ();
> +
> +  struct support_capture_subprocess proc
> +    = support_capture_subprocess (proc_func, NULL);
> +  support_capture_subprocess_check (&proc, "tst-atfork3", 0, sc_allow_none);
> +
> +  handler = atfork3mod_func = NULL;
> +
> +  support_capture_subprocess_free (&proc);
> +}
> +
> +static void *
> +thread_func (void *closure)
> +{
> +  return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  {
> +    /* Make the process acts as multithread.  */
> +    pthread_attr_t attr;
> +    xpthread_attr_init (&attr);
> +    xpthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED);
> +    xpthread_create (&attr, thread_func, NULL);
> +  }
> +
> +  TEST_COMPARE (pthread_atfork (prepare, parent, child), 0);
> +
> +  do_test_generic (true  /* prepare */, false /* parent */, false /* child */);
> +  do_test_generic (false /* prepare */, true  /* parent */, false /* child */);
> +  do_test_generic (false /* prepare */, false /* parent */, true  /* child */);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/nptl/tst-atfork3mod.c b/nptl/tst-atfork3mod.c
> new file mode 100644
> index 0000000000..7449962e57
> --- /dev/null
> +++ b/nptl/tst-atfork3mod.c
> @@ -0,0 +1,45 @@
> +/* Copyright (C) 2003-2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
> +
> +   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 <unistd.h>
> +#include <stdlib.h>
> +#include <pthread.h>
> +
> +#include <support/check.h>
> +
> +static void
> +mod_prepare (void)
> +{
> +}
> +
> +static void
> +mod_parent (void)
> +{
> +}
> +
> +static void
> +mod_child (void)
> +{
> +}
> +
> +int atfork3mod_func (void)
> +{
> +  TEST_COMPARE (pthread_atfork (mod_prepare, mod_parent, mod_child), 0);
> +
> +  return 0;
> +}
> diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h
> index 99ed76034b..cda22158d8 100644
> --- a/sysdeps/nptl/fork.h
> +++ b/sysdeps/nptl/fork.h
> @@ -24,15 +24,6 @@ extern unsigned long int __fork_generation attribute_hidden;
>  /* Pointer to the fork generation counter in the thread library.  */
>  extern unsigned long int *__fork_generation_pointer attribute_hidden;
>  
> -/* Elements of the fork handler lists.  */
> -struct fork_handler
> -{
> -  void (*prepare_handler) (void);
> -  void (*parent_handler) (void);
> -  void (*child_handler) (void);
> -  void *dso_handle;
> -};
> -
>  /* Function to call to unregister fork handlers.  */
>  extern void __unregister_atfork (void *dso_handle) attribute_hidden;
>  #define UNREGISTER_ATFORK(dso_handle) __unregister_atfork (dso_handle)
> 


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