| Message ID | 20190808191240.7172-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] nptl: Fix deadlock on atfork handler which calls dlclose (BZ#24595) | expand |
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) >
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)
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 -- 2.17.1