Fix dlclose / exit running in parallel resulting in dtor being called twice
Commit Message
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!
Comments
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.");
> +}
@@ -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 = $(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
@@ -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);
new file mode 100644
@@ -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);
+}
new file mode 100644
@@ -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);
+}
+
+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.");
+}