From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x32d.google.com (mail-ot1-x32d.google.com [IPv6:2607:f8b0:4864:20::32d]) by sourceware.org (Postfix) with ESMTPS id C05893856DD2 for ; Thu, 21 Apr 2022 13:45:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C05893856DD2 Received: by mail-ot1-x32d.google.com with SMTP id c11-20020a9d684b000000b00603307cef05so3315262oto.3 for ; Thu, 21 Apr 2022 06:45:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=1xCAeZPEchVHrD/jgwf58UPuqDPFQWs06Pnxtr9ePz4=; b=A2u3Ke6jhnpa4Fl8oW5tbTGma7zCv9wJ7lrgJTYD28LfGfISaH9Pl9W+oRsuUPbHgj 5Fmz+lZ/LQTnKLRUMhF/UQz5kkLyWNYbj/ZAKIK956hdUeIaohmRktZCelnr5cNENqcw 6f5TK4ix1Or8UuJESDoZGVrNNb6wmSCZHEEkdPxc+AmQX3JtLOrO4gW/Qw+4g4I9AEQ1 SY08D9Q24wqPLNh6WUZeaLUE3++R+GpWwiZICE33xLT/YV6vlzaWNnb5XQwwKuBlxptC 55pkjyFMUq2ZIJsmyzie6NCAyl+k5j5b/XfiRXGP/plhoJP85cUGGQhXL4AOk48YFXsD OdwQ== X-Gm-Message-State: AOAM531A7Bt552+E1lntpwKU9UtlUllmPv1eT3mnCL1rFqtAai8KGVcM K7TKpWMXjOmkTQk8iPfeeSrkpOCqByaaOA== X-Google-Smtp-Source: ABdhPJz08El4UFdRx824LxQbJe0pH3SZ7hXKYZxaRBcatBMceoZQeLHIRkTcSNvZURGmtg0c1GOL2g== X-Received: by 2002:a9d:6a18:0:b0:605:447e:cd2f with SMTP id g24-20020a9d6a18000000b00605447ecd2fmr8762495otn.314.1650548713851; Thu, 21 Apr 2022 06:45:13 -0700 (PDT) Received: from ?IPV6:2804:431:c7ca:c9d0:4761:3591:6c37:ad26? ([2804:431:c7ca:c9d0:4761:3591:6c37:ad26]) by smtp.gmail.com with ESMTPSA id mk8-20020a0568700d0800b000e67b008f75sm383199oab.32.2022.04.21.06.45.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 21 Apr 2022 06:45:13 -0700 (PDT) Message-ID: <1bdc5a3c-4257-a69d-174d-c457f362ff03@linaro.org> Date: Thu, 21 Apr 2022 10:45:10 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCH] Fix deadlock when pthread_atfork handler calls pthread_atfork or dlclose Content-Language: en-US To: Arjun Shankar , libc-alpha@sourceware.org, Florian Weimer References: <20220421123144.3796982-1-arjun@redhat.com> From: Adhemerval Zanella In-Reply-To: <20220421123144.3796982-1-arjun@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Apr 2022 13:45:20 -0000 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 > > 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 > + . */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +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 **) (®_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 > 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 > + . */ > + > +#include > +#include > + > +/* 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); > +}