Re: [patch] Fix dlclose / exit running in parallel resulting in dtor being called twice

[Carlos O'Donell <carlos at redhat dot com>]

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


-- 
Cheers,
Carlos.