Patchwork Fix dlclose / exit running in parallel resulting in dtor being called twice

login
register
mail settings
Submitter Paul Pluzhnikov
Date Sept. 21, 2017, 3:04 p.m.
Message ID <CALoOobM=CPSy7GWL7u2N=A_GEYyhsjqHzKh8FqWZhOiN+bn_Yw@mail.gmail.com>
Download mbox | patch
Permalink /patch/23059/
State New
Headers show

Comments

Paul Pluzhnikov - Sept. 21, 2017, 3:04 p.m.
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!
Carlos O'Donell - Sept. 21, 2017, 6:04 p.m.
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.");
> +}

Patch

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
 
 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
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..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);
+}
+
+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.");
+}