This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] nptl: Fix deadlock on atfork handler which calls dlclose (BZ#24595)
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Mon, 7 Oct 2019 15:24:53 -0300
- Subject: Re: [PATCH v2] nptl: Fix deadlock on atfork handler which calls dlclose (BZ#24595)
- References: <20190808191240.7172-1-adhemerval.zanella@linaro.org>
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)
>