Re: [PATCH] Fix deadlock when pthread_atfork handler calls pthread_atfork or dlclose

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 21/04/2022 09:31, Arjun Shankar via Libc-alpha wrote:
> In multi-threaded programs, registering via pthread_atfork,
> de-registering implicitly via dlclose, or running pthread_atfork
> handlers during fork was protected by an internal lock.  This meant
> that a pthread_atfork handler attempting to register another handler or
> dlclose a dynamically loaded library would lead to a deadlock.
> 
> This commit fixes the deadlock in the following way:
> 
> During the execution of handlers at fork time, the atfork lock is
> released prior to the execution of each handler and taken again upon its
> return.  Any handler registrations or de-registrations that occurred
> during the execution of the handler are accounted for before proceeding
> with further handler execution.
> 
> If a handler that hasn't been executed yet gets de-registered by another
> handler during fork, it will not be executed.   If a handler gets
> registered by another handler during fork, it will not be executed
> during that particular fork.
> 
> The possibility that handlers may now be registered or deregistered
> during handler execution means that identifying the next handler to be
> run after a given hander may register/de-register others requires some
> bookkeeping.  The fork_handler struct has an additional field, 'id',
> which is assigned sequentially during registration.  After the execution
> of a handler corresponding to ID, the next handler to be executed should
> have an id either < ID during 'prepare', or > ID during parent/child
> execution after the fork.
> 
> [BZ #24595, BZ #27054]

I sent a proposed fix sometime ago [1] which similar strategy, the 
difference it uses a linked list (there is no need to booking the last
index used since newer elements are added on the start of the list).
Not sure which would be best strategy.

[1] https://sourceware.org/pipermail/libc-alpha/2019-August/105590.html

> ---
>  include/register-atfork.h        |  26 +++---
>  posix/fork.c                     |   7 +-
>  posix/register-atfork.c          | 145 ++++++++++++++++++++++++-------
>  sysdeps/pthread/Makefile         |  10 ++-
>  sysdeps/pthread/tst-atfork3.c    | 130 +++++++++++++++++++++++++++
>  sysdeps/pthread/tst-atfork3mod.c |  48 ++++++++++
>  6 files changed, 318 insertions(+), 48 deletions(-)
>  create mode 100644 sysdeps/pthread/tst-atfork3.c
>  create mode 100644 sysdeps/pthread/tst-atfork3mod.c
> 
> diff --git a/include/register-atfork.h b/include/register-atfork.h
> index be631137b6..1852239b30 100644
> --- a/include/register-atfork.h
> +++ b/include/register-atfork.h
> @@ -26,6 +26,7 @@ struct fork_handler
>    void (*parent_handler) (void);
>    void (*child_handler) (void);
>    void *dso_handle;
> +  uint64_t id;
>  };
>  
>  /* Function to call to unregister fork handlers.  */
> @@ -39,19 +40,18 @@ enum __run_fork_handler_type
>    atfork_run_parent
>  };
>  
> -/* Run the atfork handlers and lock/unlock the internal lock depending
> -   of the WHO argument:
> -
> -   - atfork_run_prepare: run all the PREPARE_HANDLER in reverse order of
> -			 insertion and locks the internal lock.
> -   - atfork_run_child: run all the CHILD_HANDLER and unlocks the internal
> -		       lock.
> -   - atfork_run_parent: run all the PARENT_HANDLER and unlocks the internal
> -			lock.
> -
> -   Perform locking only if DO_LOCKING.  */
> -extern void __run_fork_handlers (enum __run_fork_handler_type who,
> -				 _Bool do_locking) attribute_hidden;
> +/* Run the atfork prepare handlers in the reverse order of registration and
> +return the ID of the last registered handler.  If DO_LOCKING is true, the
> +internal lock is held locked upon return.  */
> +extern uint64_t __run_prefork_handlers (_Bool do_locking) attribute_hidden;
> +
> +/* Given a handler type (parent or child), run all the atfork handlers in
> +the order of registration up to and including the handler with id equal to
> +LASTRUN.  If DO_LOCKING is true, the internal lock is unlocked prior to
> +return.  */
> +extern void __run_postfork_handlers (enum __run_fork_handler_type who,
> +				     _Bool do_locking,
> +				     uint64_t lastrun) attribute_hidden;
>  
>  /* C library side function to register new fork handlers.  */
>  extern int __register_atfork (void (*__prepare) (void),
> diff --git a/posix/fork.c b/posix/fork.c
> index 6b50c091f9..e1be3422ea 100644
> --- a/posix/fork.c
> +++ b/posix/fork.c
> @@ -46,8 +46,9 @@ __libc_fork (void)
>       best effort to make is async-signal-safe at least for single-thread
>       case.  */
>    bool multiple_threads = __libc_single_threaded == 0;
> +  uint64_t lastrun;
>  
> -  __run_fork_handlers (atfork_run_prepare, multiple_threads);
> +  lastrun = __run_prefork_handlers (multiple_threads);
>  
>    struct nss_database_data nss_database_data;
>  
> @@ -105,7 +106,7 @@ __libc_fork (void)
>        reclaim_stacks ();
>  
>        /* Run the handlers registered for the child.  */
> -      __run_fork_handlers (atfork_run_child, multiple_threads);
> +      __run_postfork_handlers (atfork_run_child, multiple_threads, lastrun);
>      }
>    else
>      {
> @@ -123,7 +124,7 @@ __libc_fork (void)
>  	}
>  
>        /* Run the handlers registered for the parent.  */
> -      __run_fork_handlers (atfork_run_parent, multiple_threads);
> +      __run_postfork_handlers (atfork_run_parent, multiple_threads, lastrun);
>  
>        if (pid < 0)
>  	__set_errno (save_errno);
> diff --git a/posix/register-atfork.c b/posix/register-atfork.c
> index 74b1b58404..4c2a4abc8b 100644
> --- a/posix/register-atfork.c
> +++ b/posix/register-atfork.c
> @@ -26,7 +26,7 @@
>  #include <malloc/dynarray-skeleton.c>
>  
>  static struct fork_handler_list fork_handlers;
> -static bool fork_handler_init = false;
> +static uint64_t fork_handler_counter = 0;
>  
>  static int atfork_lock = LLL_LOCK_INITIALIZER;
>  
> @@ -36,11 +36,8 @@ __register_atfork (void (*prepare) (void), void (*parent) (void),
>  {
>    lll_lock (atfork_lock, LLL_PRIVATE);
>  
> -  if (!fork_handler_init)
> -    {
> +  if (fork_handler_counter == 0)
>        fork_handler_list_init (&fork_handlers);
> -      fork_handler_init = true;
> -    }
>  
>    struct fork_handler *newp = fork_handler_list_emplace (&fork_handlers);
>    if (newp != NULL)
> @@ -49,6 +46,11 @@ __register_atfork (void (*prepare) (void), void (*parent) (void),
>        newp->parent_handler = parent;
>        newp->child_handler = child;
>        newp->dso_handle = dso_handle;
> +
> +      /* IDs assigned to handlers start at 1 and increment with handler
> +      registration.  Un-registering a handlers discards the corresponding
> +      ID. It is not reused in future registrations.  */
> +      newp->id = ++fork_handler_counter;
>      }
>  
>    /* Release the lock.  */
> @@ -103,37 +105,120 @@ __unregister_atfork (void *dso_handle)
>    lll_unlock (atfork_lock, LLL_PRIVATE);
>  }
>  
> -void
> -__run_fork_handlers (enum __run_fork_handler_type who, _Bool do_locking)
> +uint64_t
> +__run_prefork_handlers (_Bool do_locking)
>  {
> -  struct fork_handler *runp;
> +  uint64_t lastrun;
>  
> -  if (who == atfork_run_prepare)
> +  if (do_locking)
> +    lll_lock (atfork_lock, LLL_PRIVATE);
> +
> +  /* We run `prepare' handlers from last to first.  After fork, only
> +  handlers up to the last handler found here (pre-fork) will be run.
> +  Handlers registered between the pre-fork and post-fork calls to
> +  __run_fork_handlers will be positioned after this last handler, and
> +  since their prepare handlers won't be run now, their parent/child
> +  handlers should also be ignored.  */
> +  lastrun = fork_handler_counter;
> +
> +  size_t sl = fork_handler_list_size (&fork_handlers);
> +  for (size_t i = sl; i > 0;)
>      {
> -      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--)
> -	{
> -	  runp = fork_handler_list_at (&fork_handlers, i - 1);
> -	  if (runp->prepare_handler != NULL)
> -	    runp->prepare_handler ();
> -	}
> +      struct fork_handler *runp
> +        = fork_handler_list_at (&fork_handlers, i - 1);
> +
> +      uint64_t id = runp->id;
> +
> +      if (runp->prepare_handler != NULL)
> +        {
> +          if (do_locking)
> +            lll_unlock (atfork_lock, LLL_PRIVATE);
> +
> +          runp->prepare_handler ();
> +
> +          if (do_locking)
> +            lll_lock (atfork_lock, LLL_PRIVATE);
> +        }
> +
> +      /* We unlocked, ran the handler, and locked again.  In the
> +      meanwhile, one or more deregistrations could have occurred leading
> +      to the current (just run) handler being moved up the list or even
> +      removed from the list itself.  Since handler IDs are guaranteed to
> +      to be in increasing order, the next handler has to have:  */
> +
> +      /* A. An earlier position than the current one has.  */
> +      i--;
> +
> +      /* B. A lower ID than the current one does.  */
> +      while (i > 0
> +             && fork_handler_list_at (&fork_handlers, i - 1)->id >= id)
> +        i--;
>      }
> -  else
> +
> +  return lastrun;
> +}
> +
> +void
> +__run_postfork_handlers (enum __run_fork_handler_type who, _Bool do_locking,
> +                         uint64_t lastrun)
> +{
> +  size_t sl = fork_handler_list_size (&fork_handlers);
> +  for (size_t i = 0; i < sl;)
>      {
> -      size_t sl = fork_handler_list_size (&fork_handlers);
> -      for (size_t i = 0; i < sl; i++)
> -	{
> -	  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 ();
> -	}
> -      if (do_locking)
> -	lll_unlock (atfork_lock, LLL_PRIVATE);
> +      struct fork_handler *runp = fork_handler_list_at (&fork_handlers, i);
> +      uint64_t id = runp->id;
> +
> +      /* prepare handlers were not run for handlers with ID > LASTRUN.
> +      Thus, parent/child handlers will also not be run.  */
> +      if (id > lastrun)
> +        break;
> +
> +      if (who == atfork_run_child && runp->child_handler)
> +        {
> +          if (do_locking)
> +            lll_unlock (atfork_lock, LLL_PRIVATE);
> +
> +          runp->child_handler ();
> +
> +          if (do_locking)
> +            lll_lock (atfork_lock, LLL_PRIVATE);
> +        }
> +      else if (who == atfork_run_parent && runp->parent_handler)
> +        {
> +          if (do_locking)
> +            lll_unlock (atfork_lock, LLL_PRIVATE);
> +
> +          runp->parent_handler ();
> +
> +          if (do_locking)
> +            lll_lock (atfork_lock, LLL_PRIVATE);
> +        }
> +
> +      /* We unlocked, ran the handler, and locked again.  In the
> +      meanwhile, one or more deregistrations could have occurred leading
> +      to the current (just run) handler being moved up the list or even
> +      removed from the list itself.  Since handler IDs are guaranteed to
> +      to be in increasing order, the next handler has to have:  */
> +
> +      if (fork_handler_list_size (&fork_handlers) > i
> +          && fork_handler_list_at (&fork_handlers, i)->id == id)
> +        /* No deregistrations occurred.  */
> +        i++;
> +      else
> +        {
> +          /* Some deregistrations have occurred, and the next handler to
> +          be run is the first handler in the list to have an ID higher
> +          than the current one.  */
> +          sl = fork_handler_list_size (&fork_handlers);
> +          for (i = 0; i < sl; i++)
> +            {
> +              if (fork_handler_list_at (&fork_handlers, i)->id > id)
> +                break;
> +            }
> +        }
>      }
> +      if (do_locking)
> +    lll_unlock (atfork_lock, LLL_PRIVATE);
>  }
>  
>  
> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index e901c51df0..5a53fb30d6 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -154,16 +154,17 @@ tests += tst-cancelx2 tst-cancelx3 tst-cancelx6 tst-cancelx8 tst-cancelx9 \
>  	 tst-cleanupx0 tst-cleanupx1 tst-cleanupx2 tst-cleanupx3
>  
>  ifeq ($(build-shared),yes)
> -tests += tst-atfork2 tst-pt-tls4 tst-_res1 tst-fini1 tst-create1
> +tests += tst-atfork2 tst-pt-tls4 tst-_res1 tst-fini1 tst-create1 tst-atfork3
>  tests-nolibpthread += tst-fini1
>  endif
>  
>  modules-names += tst-atfork2mod tst-tls4moda tst-tls4modb \
>  		 tst-_res1mod1 tst-_res1mod2 tst-fini1mod \
> -		 tst-create1mod
> +		 tst-create1mod tst-atfork3mod
>  test-modules = $(addprefix $(objpfx),$(addsuffix .so,$(modules-names)))
>  
>  tst-atfork2mod.so-no-z-defs = yes
> +tst-atfork3mod.so-no-z-defs = yes
>  tst-create1mod.so-no-z-defs = yes
>  
>  ifeq ($(build-shared),yes)
> @@ -226,8 +227,13 @@ tst-atfork2-ENV = MALLOC_TRACE=$(objpfx)tst-atfork2.mtrace \
>  		  LD_PRELOAD=$(common-objpfx)/malloc/libc_malloc_debug.so
>  $(objpfx)tst-atfork2mod.so: $(shared-thread-library)
>  
> +$(objpfx)tst-atfork3: $(shared-thread-library)
> +LDFLAGS-tst-atfork3 = -rdynamic
> +$(objpfx)tst-atfork3mod.so: $(shared-thread-library)
> +
>  ifeq ($(build-shared),yes)
>  $(objpfx)tst-atfork2.out: $(objpfx)tst-atfork2mod.so
> +$(objpfx)tst-atfork3.out: $(objpfx)tst-atfork3mod.so
>  endif
>  
>  ifeq ($(build-shared),yes)
> diff --git a/sysdeps/pthread/tst-atfork3.c b/sysdeps/pthread/tst-atfork3.c
> new file mode 100644
> index 0000000000..826539c1e9
> --- /dev/null
> +++ b/sysdeps/pthread/tst-atfork3.c
> @@ -0,0 +1,130 @@
> +/* pthread_atfork supports handlers that call pthread_atfork or dlclose.
> +   Copyright (C) 2022 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <dlfcn.h>
> +#include <stdio.h>
> +#include <pthread.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +#include <support/check.h>
> +#include <stdlib.h>
> +
> +static void *
> +thread_func (void *x)
> +{
> +  return NULL;
> +}
> +
> +static unsigned int second_atfork_handler_runcount = 0;
> +
> +static void
> +second_atfork_handler (void)
> +{
> +  second_atfork_handler_runcount++;
> +}
> +
> +void *h;
> +
> +static unsigned int atfork_handler_runcount = 0;
> +
> +static void
> +prepare (void)
> +{
> +  /* These atfork handlers are registered while atfork handlers are being
> +  executed and thus will not be executed during the corresponding fork.  */
> +  pthread_atfork (second_atfork_handler,
> +                  second_atfork_handler,
> +                  second_atfork_handler);
> +
> +  /* This will de-register the atfork handlers registered by the dlopen'd
> +  library and so they will not be executed.  */
> +  dlclose (h);
> +
> +  atfork_handler_runcount++;
> +}
> +
> +static void
> +after (void)
> +{
> +  atfork_handler_runcount++;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  /* Make sure __libc_single_threaded is 0.  */
> +  pthread_t t;
> +  pthread_attr_t attr;
> +  TEST_VERIFY_EXIT (pthread_attr_init (&attr) == 0);
> +  TEST_VERIFY_EXIT (pthread_attr_setdetachstate (&attr,
> +                                                 PTHREAD_CREATE_DETACHED)
> +                    == 0);
> +  TEST_VERIFY_EXIT (pthread_create (&t, &attr, thread_func, NULL) == 0);
> +
> +  void (*reg_atfork_handlers) (void);
> +
> +  h = dlopen ("tst-atfork3mod.so", RTLD_LAZY);
> +  TEST_VERIFY_EXIT (h != NULL);
> +
> +  * (void **) (&reg_atfork_handlers) = dlsym (h, "reg_atfork_handlers");
> +  TEST_VERIFY_EXIT (reg_atfork_handlers != NULL);
> +
> +  reg_atfork_handlers ();
> +
> +  /* We register our atfork handlers *after* loading the module so that our
> +  prepare handler is called first at fork, where we then dlclose the module
> +  before its prepare handler has a chance to be called.  */
> +  pthread_atfork (prepare, after, after);
> +
> +  pid_t pid = fork ();
> +  TEST_VERIFY_EXIT (pid >= 0);
> +
> +  /* Both the parent and the child processes should observe this.  */
> +  TEST_VERIFY_EXIT (atfork_handler_runcount == 2);
> +  TEST_VERIFY_EXIT (second_atfork_handler_runcount == 0);
> +
> +  if (pid > 0)
> +    {
> +      int childstat;
> +
> +      wait (&childstat);
> +      TEST_VERIFY_EXIT (WIFEXITED (childstat)
> +                        && WEXITSTATUS (childstat) == 0);
> +
> +      /* This time, the second set of atfork handlers should also be called
> +      since the handlers are already in place before fork is called.  */
> +
> +      pid = fork ();
> +      TEST_VERIFY_EXIT (pid >= 0);
> +
> +      TEST_VERIFY_EXIT (atfork_handler_runcount == 4);
> +      TEST_VERIFY_EXIT (second_atfork_handler_runcount == 2);
> +
> +      if (pid > 0)
> +        {
> +          wait (&childstat);
> +          TEST_VERIFY_EXIT (WIFEXITED (childstat)
> +                            && WEXITSTATUS (childstat) == 0);
> +        }
> +    }
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/pthread/tst-atfork3mod.c b/sysdeps/pthread/tst-atfork3mod.c
> new file mode 100644
> index 0000000000..78e2a06f11
> --- /dev/null
> +++ b/sysdeps/pthread/tst-atfork3mod.c
> @@ -0,0 +1,48 @@
> +/* pthread_atfork supports handlers that call pthread_atfork or dlclose.
> +   Copyright (C) 2022 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +#include <stdlib.h>
> +
> +/* This dynamically loaded library simply registers its atfork handlers when
> +asked to.  The atfork handlers should never be executed because the library
> +is unloaded before fork is called by the test program.  */
> +
> +static void
> +prepare (void)
> +{
> +  abort ();
> +}
> +
> +static void
> +parent (void)
> +{
> +  abort ();
> +}
> +
> +static void
> +child (void)
> +{
> +  abort ();
> +}
> +
> +void
> +reg_atfork_handlers (void)
> +{
> +  pthread_atfork (prepare, parent, child);
> +}