| Message ID | CALoOobMptbtaA-vfcyQuepxZrMzsPKPqTWMZbO=vwK9s0FoGhQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix dlclose / exit running in parallel resulting in dtor being called twice | expand |
On 09/20/2017 12:30 PM, Paul Pluzhnikov wrote: > Greetings, > > This a followup to and fix for the problem discovered in > https://sourceware.org/ml/libc-alpha/2017-09/msg00762.html. > > As the test program in above message shows, dlclose running in > parallel with exit currently results in "second" being called twice. > > The fix for this is trivial (2 line change in stdlib/exit.c). The > majority of this patch is the test case, which is modified from > original to avoid sleep. This looks *awesome*! Almost done. Move the open_library/close_library to support/ so other future tests can use them. I've given instructions on how to do that. With that cleanup and the commit message I wrote below I think we're done. Repost and I'll ack it. > Thanks, > > P.S. Not sure what commit message should be for this patch. Suggest: ~~~ POSIX requires that dlclose() and exit() be thread safe, therefore you can have one thread in the middle of dlclose() and another thread executing exit() without causing any undefined behaviour on the part of the implementation. The existing implementation had a flaw that exit() exit handler processing did not consider a concurrent dlclose() and would not mark already run exit handlers using the ef_free flavour. The consequence of this is that a concurrent exit() with dlclose() will run all the exit handlers that dlclose() had not yet run, but then will block on the loader lock. The concurrent dlclose() will continue to run all the exit handlers again (twice) in violation of the Itanium C++ ABI requirements for __cxa_atexit(). This commit fixes this by having exit() mark all handlers with ef_free to ensure that concurrent dlclose() won't re-run registered exit handlers that have already run. ~~~ > -- Paul Pluzhnikov > > > glibc-dlclose-exit-race-20170920.txt > > > 2017-09-20 Paul Pluzhnikov <ppluzhnikov@google.com> > Carlos O'Donell <carlos@redhat.com> > > * stdlib/Makefile (tests): Add test-dlclose-exit-race. > * stdlib/test-dlclose-exit-race.c: New file. > * stdlib/test-dlclose-exit-race-helper.c: New file. > * stdlib/exit.c (__run_exit_handlers): Mark slot as free. > > > diff --git a/stdlib/Makefile b/stdlib/Makefile > index 2fb08342e0..443230bbb7 100644 > --- a/stdlib/Makefile > +++ b/stdlib/Makefile > @@ -83,7 +83,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \ > tst-getrandom tst-atexit tst-at_quick_exit \ > tst-cxa_atexit tst-on_exit test-atexit-race \ > test-at_quick_exit-race test-cxa_atexit-race \ > - test-on_exit-race > + test-on_exit-race test-dlclose-exit-race OK. > > tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \ > tst-tls-atexit tst-tls-atexit-nodelete > @@ -98,6 +98,10 @@ LDLIBS-test-at_quick_exit-race = $(shared-thread-library) > LDLIBS-test-cxa_atexit-race = $(shared-thread-library) > LDLIBS-test-on_exit-race = $(shared-thread-library) > > +LDLIBS-test-dlclose-exit-race = $(libsupport) $(shared-thread-library) $(libdl) > +LDFLAGS-test-dlclose-exit-race = $(LDFLAGS-rdynamic) > +LDLIBS-test-dlclose-exit-race-helper.so = $(libsupport) $(shared-thread-library) > + OK. > ifeq ($(have-cxx-thread_local),yes) > CFLAGS-tst-quick_exit.o = -std=c++11 > LDLIBS-tst-quick_exit = -lstdc++ > @@ -108,7 +112,7 @@ else > tests-unsupported += tst-quick_exit tst-thread-quick_exit > endif > > -modules-names = tst-tls-atexit-lib > +modules-names = tst-tls-atexit-lib test-dlclose-exit-race-helper OK. > extra-test-objs += $(addsuffix .os, $(modules-names)) > > ifeq ($(build-shared),yes) > @@ -177,6 +181,7 @@ $(objpfx)tst-strtod-nan-locale.out: $(gen-locales) > $(objpfx)tst-strfmon_l.out: $(gen-locales) > $(objpfx)tst-strfrom.out: $(gen-locales) > $(objpfx)tst-strfrom-locale.out: $(gen-locales) > +$(objpfx)test-dlclose-exit-race.out: $(objpfx)test-dlclose-exit-race-helper.so OK. > endif > > # Testdir has to be named stdlib and needs to be writable > @@ -215,6 +220,7 @@ $(objpfx)tst-strtod6: $(libm) > $(objpfx)tst-strtod-nan-locale: $(libm) > > tst-tls-atexit-lib.so-no-z-defs = yes > +test-dlclose-exit-race-helper.so-no-z-defs = yes OK. > > $(objpfx)tst-tls-atexit: $(shared-thread-library) $(libdl) > $(objpfx)tst-tls-atexit.out: $(objpfx)tst-tls-atexit-lib.so > diff --git a/stdlib/exit.c b/stdlib/exit.c > index b74f1825f0..6a354fd0af 100644 > --- a/stdlib/exit.c > +++ b/stdlib/exit.c > @@ -69,8 +69,7 @@ __run_exit_handlers (int status, struct exit_function_list **listp, > > while (cur->idx > 0) > { > - const struct exit_function *const f = > - &cur->fns[--cur->idx]; > + struct exit_function *const f = &cur->fns[--cur->idx]; OK. > const uint64_t new_exitfn_called = __new_exitfn_called; > > /* Unlock the list while we call a foreign function. */ > @@ -99,6 +98,9 @@ __run_exit_handlers (int status, struct exit_function_list **listp, > atfct (); > break; > case ef_cxa: > + /* To avoid dlopen/exit race calling cxafct twice, we must > + mark this function as ef_free. */ > + f->flavor = ef_free; > cxafct = f->func.cxa.fn; OK. > #ifdef PTR_DEMANGLE > PTR_DEMANGLE (cxafct); > diff --git a/stdlib/test-dlclose-exit-race-helper.c b/stdlib/test-dlclose-exit-race-helper.c > new file mode 100644 > index 0000000000..1b443e0b52 > --- /dev/null > +++ b/stdlib/test-dlclose-exit-race-helper.c > @@ -0,0 +1,80 @@ > +/* Helper for exit/dlclose race test. > + Copyright (C) 2017 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 <stdbool.h> > +#include <stdlib.h> > +#include <semaphore.h> > +#include <unistd.h> > +#include <support/xthread.h> > + > +/* Semaphore defined in executable to ensure we have a happens-before > + between the first function starting and exit being called. */ > +extern sem_t order1; > + > +/* Semaphore defined in executable to ensure we have a happens-before > + between the second function starting and the first function returning. */ > +extern sem_t order2; > + > +/* glibc function for registering DSO-specific exit functions. */ > +extern int __cxa_atexit (void (*func) (void *), void *arg, void *dso_handle); > + > +/* Hidden compiler handle to this shared object. */ > +extern void *__dso_handle __attribute__ ((__weak__)); > + > +static void > +first (void *start) > +{ > + /* Let the exiting thread run. */ > + sem_post (&order1); > + > + /* Wait for exiting thread to finish. */ > + sem_wait (&order2); > + > + printf ("first\n"); > +} > + > +static void > +second (void *start) > +{ > + /* We may be called from different threads. > + This lock protects called. */ > + static pthread_mutex_t mtx = PTHREAD_MUTEX_INITIALIZER; > + static bool called = false; > + > + xpthread_mutex_lock (&mtx); > + if (called) > + { > + fprintf (stderr, "second called twice!\n"); > + abort (); > + } > + called = true; > + xpthread_mutex_unlock (&mtx); > + > + printf ("second\n"); > +} > + > + > +__attribute__ ((constructor)) static void > +constructor (void) > +{ > + sem_init (&order1, 0, 0); > + sem_init (&order2, 0, 0); > + __cxa_atexit (second, NULL, __dso_handle); > + __cxa_atexit (first, NULL, __dso_handle); > +} > diff --git a/stdlib/test-dlclose-exit-race.c b/stdlib/test-dlclose-exit-race.c > new file mode 100644 > index 0000000000..bbb97c7c64 > --- /dev/null > +++ b/stdlib/test-dlclose-exit-race.c > @@ -0,0 +1,116 @@ > +/* Test for exit/dlclose race. > + Copyright (C) 2017 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/>. */ > + > +/* This file must be run from within a directory called "stdlib". */ > + > +/* This test verifies that when dlopen in one thread races against exit > + in another thread, we don't call registered destructor twice. > + > + Expected result: > + second > + first > + ... clean termination > +*/ > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <dlfcn.h> > +#include <semaphore.h> > +#include <support/xthread.h> > + > +/* Semaphore to ensure we have a happens-before between the first function > + starting and exit being called. */ > +sem_t order1; > + > +/* Semaphore to ensure we have a happens-before between the second function > + starting and the first function returning. */ > +sem_t order2; > + > +void * > +open_library (const char * pathname) > +{ > + void *dso; > + char *err; > + > + /* Open the DSO. */ > + dso = dlopen (pathname, RTLD_NOW|RTLD_GLOBAL); > + if (dso == NULL) > + { > + err = dlerror (); > + fprintf (stderr, "%s\n", err); > + exit (1); > + } > + /* Clear any errors. */ > + dlerror (); > + return dso; > +} This should go into support/xdlopen.c as a new wrapper so we don't have to repeat this boiler plate in all new code. e.g. void * xdlopen (const char *filename, int flags) { void *dso; char *err; *dso = dlopen (filename, flags); if (dso == NULL) { *err = dlerror (); FAIL_EXIT1 ("error: dlopen: %s\n", err); } dlerror (); return dso; } Add it to support/Makefile (libsupport-routines), and add a xdlfcn.h similar to xthread.h, but just with xdlopen and xdlclose defined. We always want xdlopen to succeed or otherwise abort the process with a relevant error message. > + > +int > +close_library (void *dso) > +{ > + int ret; > + char *err; > + > + /* Close the library and look for errors too. */ > + ret = dlclose (dso); > + if (ret != 0) > + { > + err = dlerror (); > + fprintf (stderr, "%s\n", err); > + exit (1); > + } > + return ret; > +} Similarly to xdlopen we need an xdlclose. > + > +void * > +exit_thread (void *arg) > +{ > + /* Wait for the dlclose to start... */ > + sem_wait (&order1); > + /* Then try to run the exit sequence which should call all > + __cxa_atexit registered functions and in parallel with > + the executing dlclose(). */ > + exit (0); > +} > + > + > +void > +last (void) > +{ > + /* Let dlclose thread proceed. */ > + sem_post (&order2); > +} > + > +int > +main (void) > +{ > + void *dso; > + pthread_t thread; > + > + atexit (last); > + > + dso = open_library ("$ORIGIN/test-dlclose-exit-race-helper.so"); This becomes xdlopen. > + thread = xpthread_create (NULL, exit_thread, NULL); > + > + close_library (dso); This becomes xdlclose. > + xpthread_join (thread); > + OK. > + /* Unreached. */ > + fprintf (stderr, "Reached unreachable code."); > + abort (); This should be: FAIL_EXIT1 ("Did not terminate via exit(0) in exit_thread() as expected."); > +}
On Wed, Sep 20, 2017 at 2:50 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> Repost and I'll ack it.
Updated patch attached. Thanks!
On 09/21/2017 09:04 AM, Paul Pluzhnikov wrote: > On Wed, Sep 20, 2017 at 2:50 PM, Carlos O'Donell <carlos@redhat.com> wrote: > >> Repost and I'll ack it. > Updated patch attached. Thanks! > > -- Paul Pluzhnikov > > > glibc-dlclose-exit-race-20170921.txt > > > 2017-09-21 Paul Pluzhnikov <ppluzhnikov@google.com> > Carlos O'Donell <carlos@redhat.com> > > * stdlib/Makefile (tests): Add test-dlclose-exit-race. > * stdlib/test-dlclose-exit-race.c: New file. > * stdlib/test-dlclose-exit-race-helper.c: New file. > * stdlib/exit.c (__run_exit_handlers): Mark slot as free. Reviewed-by: Carlos O'Donell <carlos@redhat.com> OK to commit if you fix 3 nits: * needs a bug number, this is a publicily visible change in destructor call ordering and number of calls. An application might have expected second to run twice at this point and worked around it. * dlopen comment * use of FAIL_EXIT1 > diff --git a/stdlib/Makefile b/stdlib/Makefile > index 2fb08342e0..0a51b7bc90 100644 > --- a/stdlib/Makefile > +++ b/stdlib/Makefile > @@ -83,7 +83,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \ > tst-getrandom tst-atexit tst-at_quick_exit \ > tst-cxa_atexit tst-on_exit test-atexit-race \ > test-at_quick_exit-race test-cxa_atexit-race \ > - test-on_exit-race > + test-on_exit-race test-dlclose-exit-race OK. > > tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \ > tst-tls-atexit tst-tls-atexit-nodelete > @@ -98,6 +98,10 @@ LDLIBS-test-at_quick_exit-race = $(shared-thread-library) > LDLIBS-test-cxa_atexit-race = $(shared-thread-library) > LDLIBS-test-on_exit-race = $(shared-thread-library) > > +LDLIBS-test-dlclose-exit-race = $(shared-thread-library) $(libdl) > +LDFLAGS-test-dlclose-exit-race = $(LDFLAGS-rdynamic) > +LDLIBS-test-dlclose-exit-race-helper.so = $(libsupport) $(shared-thread-library) > + OK. > ifeq ($(have-cxx-thread_local),yes) > CFLAGS-tst-quick_exit.o = -std=c++11 > LDLIBS-tst-quick_exit = -lstdc++ > @@ -108,7 +112,7 @@ else > tests-unsupported += tst-quick_exit tst-thread-quick_exit > endif > > -modules-names = tst-tls-atexit-lib > +modules-names = tst-tls-atexit-lib test-dlclose-exit-race-helper OK. > extra-test-objs += $(addsuffix .os, $(modules-names)) > > ifeq ($(build-shared),yes) > @@ -177,6 +181,7 @@ $(objpfx)tst-strtod-nan-locale.out: $(gen-locales) > $(objpfx)tst-strfmon_l.out: $(gen-locales) > $(objpfx)tst-strfrom.out: $(gen-locales) > $(objpfx)tst-strfrom-locale.out: $(gen-locales) > +$(objpfx)test-dlclose-exit-race.out: $(objpfx)test-dlclose-exit-race-helper.so > endif > > # Testdir has to be named stdlib and needs to be writable > @@ -215,6 +220,7 @@ $(objpfx)tst-strtod6: $(libm) > $(objpfx)tst-strtod-nan-locale: $(libm) > > tst-tls-atexit-lib.so-no-z-defs = yes > +test-dlclose-exit-race-helper.so-no-z-defs = yes OK. > > $(objpfx)tst-tls-atexit: $(shared-thread-library) $(libdl) > $(objpfx)tst-tls-atexit.out: $(objpfx)tst-tls-atexit-lib.so > diff --git a/stdlib/exit.c b/stdlib/exit.c > index b74f1825f0..6a354fd0af 100644 > --- a/stdlib/exit.c > +++ b/stdlib/exit.c > @@ -69,8 +69,7 @@ __run_exit_handlers (int status, struct exit_function_list **listp, > > while (cur->idx > 0) > { > - const struct exit_function *const f = > - &cur->fns[--cur->idx]; > + struct exit_function *const f = &cur->fns[--cur->idx]; OK. > const uint64_t new_exitfn_called = __new_exitfn_called; > > /* Unlock the list while we call a foreign function. */ > @@ -99,6 +98,9 @@ __run_exit_handlers (int status, struct exit_function_list **listp, > atfct (); > break; > case ef_cxa: > + /* To avoid dlopen/exit race calling cxafct twice, we must s/dlopen/dlclose/g > + mark this function as ef_free. */ > + f->flavor = ef_free; > cxafct = f->func.cxa.fn; > #ifdef PTR_DEMANGLE > PTR_DEMANGLE (cxafct); > diff --git a/stdlib/test-dlclose-exit-race-helper.c b/stdlib/test-dlclose-exit-race-helper.c > new file mode 100644 > index 0000000000..1b443e0b52 > --- /dev/null > +++ b/stdlib/test-dlclose-exit-race-helper.c > @@ -0,0 +1,80 @@ > +/* Helper for exit/dlclose race test. > + Copyright (C) 2017 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 <stdbool.h> > +#include <stdlib.h> > +#include <semaphore.h> > +#include <unistd.h> > +#include <support/xthread.h> > + > +/* Semaphore defined in executable to ensure we have a happens-before > + between the first function starting and exit being called. */ > +extern sem_t order1; > + > +/* Semaphore defined in executable to ensure we have a happens-before > + between the second function starting and the first function returning. */ > +extern sem_t order2; > + > +/* glibc function for registering DSO-specific exit functions. */ > +extern int __cxa_atexit (void (*func) (void *), void *arg, void *dso_handle); > + > +/* Hidden compiler handle to this shared object. */ > +extern void *__dso_handle __attribute__ ((__weak__)); > + > +static void > +first (void *start) > +{ > + /* Let the exiting thread run. */ > + sem_post (&order1); > + > + /* Wait for exiting thread to finish. */ > + sem_wait (&order2); > + > + printf ("first\n"); > +} > + > +static void > +second (void *start) > +{ > + /* We may be called from different threads. > + This lock protects called. */ > + static pthread_mutex_t mtx = PTHREAD_MUTEX_INITIALIZER; > + static bool called = false; > + > + xpthread_mutex_lock (&mtx); > + if (called) > + { > + fprintf (stderr, "second called twice!\n"); > + abort (); Use 'FAIL_EXIT1 ("second called twice!\n");' instead of fprtinf/abort. > + } > + called = true; > + xpthread_mutex_unlock (&mtx); > + > + printf ("second\n"); > +} > + > + > +__attribute__ ((constructor)) static void > +constructor (void) > +{ > + sem_init (&order1, 0, 0); > + sem_init (&order2, 0, 0); > + __cxa_atexit (second, NULL, __dso_handle); > + __cxa_atexit (first, NULL, __dso_handle); > +} > diff --git a/stdlib/test-dlclose-exit-race.c b/stdlib/test-dlclose-exit-race.c > new file mode 100644 > index 0000000000..d822c8bae9 > --- /dev/null > +++ b/stdlib/test-dlclose-exit-race.c > @@ -0,0 +1,80 @@ > +/* Test for exit/dlclose race. > + Copyright (C) 2017 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/>. */ > + > +/* This file must be run from within a directory called "stdlib". */ > + > +/* This test verifies that when dlopen in one thread races against exit > + in another thread, we don't call registered destructor twice. > + > + Expected result: > + second > + first > + ... clean termination > +*/ > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <semaphore.h> > +#include <support/check.h> > +#include <support/xdlfcn.h> > +#include <support/xthread.h> > + > +/* Semaphore to ensure we have a happens-before between the first function > + starting and exit being called. */ > +sem_t order1; > + > +/* Semaphore to ensure we have a happens-before between the second function > + starting and the first function returning. */ > +sem_t order2; > + > +void * > +exit_thread (void *arg) > +{ > + /* Wait for the dlclose to start... */ > + sem_wait (&order1); > + /* Then try to run the exit sequence which should call all > + __cxa_atexit registered functions and in parallel with > + the executing dlclose(). */ > + exit (0); > +} > + > + > +void > +last (void) > +{ > + /* Let dlclose thread proceed. */ > + sem_post (&order2); > +} OK. Clever use of atexit here to sequence first/second. > + > +int > +main (void) > +{ > + void *dso; > + pthread_t thread; > + > + atexit (last); > + > + dso = xdlopen ("$ORIGIN/test-dlclose-exit-race-helper.so", > + RTLD_NOW|RTLD_GLOBAL); > + thread = xpthread_create (NULL, exit_thread, NULL); > + > + xdlclose (dso); > + xpthread_join (thread); > + > + FAIL_EXIT1 ("Did not terminate via exit(0) in exit_thread() as expected."); > +}
* Paul Pluzhnikov: > +void * > +open_library (const char * pathname) > +{ > + void *dso; > + char *err; > + > + /* Open the DSO. */ > + dso = dlopen (pathname, RTLD_NOW|RTLD_GLOBAL); > + if (dso == NULL) > + { > + err = dlerror (); > + fprintf (stderr, "%s\n", err); > + exit (1); > + } > + /* Clear any errors. */ > + dlerror (); > + return dso; > +} Why did you add the dlerror call in the success case? Thanks, Florian
On Wed, Feb 6, 2019 at 1:37 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Paul Pluzhnikov: > > > + /* Clear any errors. */ > > + dlerror (); > > + return dso; > > +} > > Why did you add the dlerror call in the success case? I don't remember / can't recall. But it seems like it should always be a no-op. Do you have a case where dlopen() succeeds and dlerror() is not a no-op? Thanks,
* Paul Pluzhnikov: > On Wed, Feb 6, 2019 at 1:37 AM Florian Weimer <fweimer@redhat.com> wrote: >> >> * Paul Pluzhnikov: >> >> > + /* Clear any errors. */ >> > + dlerror (); >> > + return dso; >> > +} >> >> Why did you add the dlerror call in the success case? > > I don't remember / can't recall. > > But it seems like it should always be a no-op. Do you have a case > where dlopen() succeeds and dlerror() is not a no-op? I think it may matter for dlsym, where you need to look dlerror to tell if the symbol was NULL, or there was an actual error. But you would have to clear dlerror *before* dlsym anyway, in case something else had called dlsym without clearing the error. Thanks, Florian
* Florian Weimer: > * Paul Pluzhnikov: > >> On Wed, Feb 6, 2019 at 1:37 AM Florian Weimer <fweimer@redhat.com> wrote: >>> >>> * Paul Pluzhnikov: >>> >>> > + /* Clear any errors. */ >>> > + dlerror (); >>> > + return dso; >>> > +} >>> >>> Why did you add the dlerror call in the success case? >> >> I don't remember / can't recall. >> >> But it seems like it should always be a no-op. Do you have a case >> where dlopen() succeeds and dlerror() is not a no-op? > > I think it may matter for dlsym, where you need to look dlerror to tell > if the symbol was NULL, or there was an actual error. But you would > have to clear dlerror *before* dlsym anyway, in case something else had > called dlsym without clearing the error. In other words, something like this. Thanks, Florian support: Use dlerror to detect NULL symbols in xdlsym 2019-02-06 Florian Weimer <fweimer@redhat.com> * support/xdlfcn.c (xdlopen, xdlclose): Do not call dlerror. (xdlsym): Use dlerror to detect a NULL symbol. diff --git a/support/xdlfcn.c b/support/xdlfcn.c index 2f2ac76003..b2e5c21134 100644 --- a/support/xdlfcn.c +++ b/support/xdlfcn.c @@ -28,22 +28,25 @@ xdlopen (const char *filename, int flags) if (dso == NULL) FAIL_EXIT1 ("error: dlopen: %s\n", dlerror ()); - /* Clear any errors. */ - dlerror (); - return dso; } void * xdlsym (void *handle, const char *symbol) { + /* Clear any pending errors. */ + dlerror (); + void *sym = dlsym (handle, symbol); if (sym == NULL) - FAIL_EXIT1 ("error: dlsym: %s\n", dlerror ()); - - /* Clear any errors. */ - dlerror (); + { + const char *error = dlerror (); + if (error != NULL) + FAIL_EXIT1 ("error: dlsym: %s\n", error); + /* If there was no error, we found a NULL symbol. Return the + NULL value in this case. */ + } return sym; } @@ -53,7 +56,4 @@ xdlclose (void *handle) { if (dlclose (handle) != 0) FAIL_EXIT1 ("error: dlclose: %s\n", dlerror ()); - - /* Clear any errors. */ - dlerror (); }
On Wed, Feb 6, 2019 at 7:28 AM Florian Weimer <fweimer@redhat.com> wrote: > > I think it may matter for dlsym, where you need to look dlerror to tell > > if the symbol was NULL, or there was an actual error. But you would > > have to clear dlerror *before* dlsym anyway, in case something else had > > called dlsym without clearing the error. > > In other words, something like this. The patch looks good to me. Thanks!
diff --git a/stdlib/Makefile b/stdlib/Makefile index 2fb08342e0..443230bbb7 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -83,7 +83,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \ tst-getrandom tst-atexit tst-at_quick_exit \ tst-cxa_atexit tst-on_exit test-atexit-race \ test-at_quick_exit-race test-cxa_atexit-race \ - test-on_exit-race + test-on_exit-race test-dlclose-exit-race tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \ tst-tls-atexit tst-tls-atexit-nodelete @@ -98,6 +98,10 @@ LDLIBS-test-at_quick_exit-race = $(shared-thread-library) LDLIBS-test-cxa_atexit-race = $(shared-thread-library) LDLIBS-test-on_exit-race = $(shared-thread-library) +LDLIBS-test-dlclose-exit-race = $(libsupport) $(shared-thread-library) $(libdl) +LDFLAGS-test-dlclose-exit-race = $(LDFLAGS-rdynamic) +LDLIBS-test-dlclose-exit-race-helper.so = $(libsupport) $(shared-thread-library) + ifeq ($(have-cxx-thread_local),yes) CFLAGS-tst-quick_exit.o = -std=c++11 LDLIBS-tst-quick_exit = -lstdc++ @@ -108,7 +112,7 @@ else tests-unsupported += tst-quick_exit tst-thread-quick_exit endif -modules-names = tst-tls-atexit-lib +modules-names = tst-tls-atexit-lib test-dlclose-exit-race-helper extra-test-objs += $(addsuffix .os, $(modules-names)) ifeq ($(build-shared),yes) @@ -177,6 +181,7 @@ $(objpfx)tst-strtod-nan-locale.out: $(gen-locales) $(objpfx)tst-strfmon_l.out: $(gen-locales) $(objpfx)tst-strfrom.out: $(gen-locales) $(objpfx)tst-strfrom-locale.out: $(gen-locales) +$(objpfx)test-dlclose-exit-race.out: $(objpfx)test-dlclose-exit-race-helper.so endif # Testdir has to be named stdlib and needs to be writable @@ -215,6 +220,7 @@ $(objpfx)tst-strtod6: $(libm) $(objpfx)tst-strtod-nan-locale: $(libm) tst-tls-atexit-lib.so-no-z-defs = yes +test-dlclose-exit-race-helper.so-no-z-defs = yes $(objpfx)tst-tls-atexit: $(shared-thread-library) $(libdl) $(objpfx)tst-tls-atexit.out: $(objpfx)tst-tls-atexit-lib.so diff --git a/stdlib/exit.c b/stdlib/exit.c index b74f1825f0..6a354fd0af 100644 --- a/stdlib/exit.c +++ b/stdlib/exit.c @@ -69,8 +69,7 @@ __run_exit_handlers (int status, struct exit_function_list **listp, while (cur->idx > 0) { - const struct exit_function *const f = - &cur->fns[--cur->idx]; + struct exit_function *const f = &cur->fns[--cur->idx]; const uint64_t new_exitfn_called = __new_exitfn_called; /* Unlock the list while we call a foreign function. */ @@ -99,6 +98,9 @@ __run_exit_handlers (int status, struct exit_function_list **listp, atfct (); break; case ef_cxa: + /* To avoid dlopen/exit race calling cxafct twice, we must + mark this function as ef_free. */ + f->flavor = ef_free; cxafct = f->func.cxa.fn; #ifdef PTR_DEMANGLE PTR_DEMANGLE (cxafct); diff --git a/stdlib/test-dlclose-exit-race-helper.c b/stdlib/test-dlclose-exit-race-helper.c new file mode 100644 index 0000000000..1b443e0b52 --- /dev/null +++ b/stdlib/test-dlclose-exit-race-helper.c @@ -0,0 +1,80 @@ +/* Helper for exit/dlclose race test. + Copyright (C) 2017 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 <stdbool.h> +#include <stdlib.h> +#include <semaphore.h> +#include <unistd.h> +#include <support/xthread.h> + +/* Semaphore defined in executable to ensure we have a happens-before + between the first function starting and exit being called. */ +extern sem_t order1; + +/* Semaphore defined in executable to ensure we have a happens-before + between the second function starting and the first function returning. */ +extern sem_t order2; + +/* glibc function for registering DSO-specific exit functions. */ +extern int __cxa_atexit (void (*func) (void *), void *arg, void *dso_handle); + +/* Hidden compiler handle to this shared object. */ +extern void *__dso_handle __attribute__ ((__weak__)); + +static void +first (void *start) +{ + /* Let the exiting thread run. */ + sem_post (&order1); + + /* Wait for exiting thread to finish. */ + sem_wait (&order2); + + printf ("first\n"); +} + +static void +second (void *start) +{ + /* We may be called from different threads. + This lock protects called. */ + static pthread_mutex_t mtx = PTHREAD_MUTEX_INITIALIZER; + static bool called = false; + + xpthread_mutex_lock (&mtx); + if (called) + { + fprintf (stderr, "second called twice!\n"); + abort (); + } + called = true; + xpthread_mutex_unlock (&mtx); + + printf ("second\n"); +} + + +__attribute__ ((constructor)) static void +constructor (void) +{ + sem_init (&order1, 0, 0); + sem_init (&order2, 0, 0); + __cxa_atexit (second, NULL, __dso_handle); + __cxa_atexit (first, NULL, __dso_handle); +} diff --git a/stdlib/test-dlclose-exit-race.c b/stdlib/test-dlclose-exit-race.c new file mode 100644 index 0000000000..bbb97c7c64 --- /dev/null +++ b/stdlib/test-dlclose-exit-race.c @@ -0,0 +1,116 @@ +/* Test for exit/dlclose race. + Copyright (C) 2017 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/>. */ + +/* This file must be run from within a directory called "stdlib". */ + +/* This test verifies that when dlopen in one thread races against exit + in another thread, we don't call registered destructor twice. + + Expected result: + second + first + ... clean termination +*/ + +#include <stdio.h> +#include <stdlib.h> +#include <dlfcn.h> +#include <semaphore.h> +#include <support/xthread.h> + +/* Semaphore to ensure we have a happens-before between the first function + starting and exit being called. */ +sem_t order1; + +/* Semaphore to ensure we have a happens-before between the second function + starting and the first function returning. */ +sem_t order2; + +void * +open_library (const char * pathname) +{ + void *dso; + char *err; + + /* Open the DSO. */ + dso = dlopen (pathname, RTLD_NOW|RTLD_GLOBAL); + if (dso == NULL) + { + err = dlerror (); + fprintf (stderr, "%s\n", err); + exit (1); + } + /* Clear any errors. */ + dlerror (); + return dso; +} + +int +close_library (void *dso) +{ + int ret; + char *err; + + /* Close the library and look for errors too. */ + ret = dlclose (dso); + if (ret != 0) + { + err = dlerror (); + fprintf (stderr, "%s\n", err); + exit (1); + } + return ret; +} + +void * +exit_thread (void *arg) +{ + /* Wait for the dlclose to start... */ + sem_wait (&order1); + /* Then try to run the exit sequence which should call all + __cxa_atexit registered functions and in parallel with + the executing dlclose(). */ + exit (0); +} + + +void +last (void) +{ + /* Let dlclose thread proceed. */ + sem_post (&order2); +} + +int +main (void) +{ + void *dso; + pthread_t thread; + + atexit (last); + + dso = open_library ("$ORIGIN/test-dlclose-exit-race-helper.so"); + thread = xpthread_create (NULL, exit_thread, NULL); + + close_library (dso); + xpthread_join (thread); + + /* Unreached. */ + fprintf (stderr, "Reached unreachable code."); + abort (); +}