Fix for bz14333 -- race between atexit() and exit()

Message ID CALoOobOgwZx2K-FWFi5zmqjk4vfxB4B2+Bv6koou7Ks9WCT4Yw@mail.gmail.com
State New, archived
Headers

Commit Message

Paul Pluzhnikov July 12, 2017, 3:56 p.m. UTC
  On Tue, Jul 11, 2017 at 12:01 PM, Joseph Myers <joseph@codesourcery.com> wrote:

> A patch fixing a bug should also include the tests for that bug.

Revised patch attached. Thanks.

2017-07-12  Paul Pluzhnikov  <ppluzhnikov@google.com>
            Ricky Zhou <rickyz@google.com>
            Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>

        [BZ #14333]
        * stdlib/cxa_atexit.c (__internal_atexit): Use __exit_funcs_lock.
        (__new_exitfn): Fail registration when we finished at_exit processing.
        * stdlib/on_exit.c (__on_exit): Likewise.
        * stdlib/exit.c (__exit_funcs_done): New variable.
        (__run_exit_handlers): Use __exit_funcs_lock.
        * stdlib/exit.h (__exit_funcs_lock, __exit_funcs_done): New
        declarations.
        * stdlib/Makefile (test-atexit-race, test-quick_at_exit-race)
        (test-__cxa_atexit-race): New tests.
        * stdlib/test-atexit-race-common.c: New.
        * stdlib/test-atexit-race.c: New.
        * stdlib/test-at_quick_exit-race.c: New.
        * stdlib/test-__cxa_atexit-race.c: New.
  

Comments

Andreas Schwab July 12, 2017, 4:09 p.m. UTC | #1
On Jul 12 2017, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> diff --git a/stdlib/exit.h b/stdlib/exit.h
> index 7f2e679246..023e81b83f 100644
> --- a/stdlib/exit.h
> +++ b/stdlib/exit.h
> @@ -20,6 +20,7 @@
>  
>  #include <stdbool.h>
>  #include <stdint.h>
> +#include <libc-lock.h>
>  
>  enum
>  {
> @@ -62,6 +63,10 @@ extern struct exit_function_list *__quick_exit_funcs attribute_hidden;
>  
>  extern struct exit_function *__new_exitfn (struct exit_function_list **listp);
>  extern uint64_t __new_exitfn_called attribute_hidden;
> +__libc_lock_define (extern, __exit_funcs_lock);
> +
> +/* flag to mark that all registered atexit/onexit handlers are called */
> +extern int __exit_funcs_done attribute_hidden;

That should be bool.

Andreas.
  

Patch

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 0314d5926b..7731b53cc8 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -80,7 +80,8 @@  tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-strtol-locale tst-strtod-nan-locale tst-strfmon_l    \
 		   tst-quick_exit tst-thread-quick_exit tst-width	    \
 		   tst-width-stdint tst-strfrom tst-strfrom-locale	    \
-		   tst-getrandom
+		   tst-getrandom test-atexit-race test-at_quick_exit-race   \
+		   test-__cxa_atexit-race
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
 tests-static	:= tst-secure-getenv
@@ -89,6 +90,10 @@  ifeq ($(build-hardcoded-path-in-tests),yes)
 tests += tst-empty-env
 endif
 
+LDLIBS-test-atexit-race = $(shared-thread-library)
+LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
+LDLIBS-test-__cxa_atexit-race = $(shared-thread-library)
+
 ifeq ($(have-cxx-thread_local),yes)
 CFLAGS-tst-quick_exit.o = -std=c++11
 LDLIBS-tst-quick_exit = -lstdc++
diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
index ce5d9f22b4..5db0b33060 100644
--- a/stdlib/cxa_atexit.c
+++ b/stdlib/cxa_atexit.c
@@ -26,16 +26,25 @@ 
 
 #undef __cxa_atexit
 
+/* We change global data, so we need locking.  */
+__libc_lock_define_initialized (, __exit_funcs_lock)
+
 
 int
 attribute_hidden
 __internal_atexit (void (*func) (void *), void *arg, void *d,
 		   struct exit_function_list **listp)
 {
-  struct exit_function *new = __new_exitfn (listp);
+  struct exit_function *new;
+
+  __libc_lock_lock (__exit_funcs_lock);
+  new = __new_exitfn (listp);
 
   if (new == NULL)
-    return -1;
+    {
+      __libc_lock_unlock (__exit_funcs_lock);
+      return -1;
+    }
 
 #ifdef PTR_MANGLE
   PTR_MANGLE (func);
@@ -45,6 +54,7 @@  __internal_atexit (void (*func) (void *), void *arg, void *d,
   new->func.cxa.dso_handle = d;
   atomic_write_barrier ();
   new->flavor = ef_cxa;
+  __libc_lock_unlock (__exit_funcs_lock);
   return 0;
 }
 
@@ -60,10 +70,6 @@  __cxa_atexit (void (*func) (void *), void *arg, void *d)
 libc_hidden_def (__cxa_atexit)
 
 
-/* We change global data, so we need locking.  */
-__libc_lock_define_initialized (static, lock)
-
-
 static struct exit_function_list initial;
 struct exit_function_list *__exit_funcs = &initial;
 uint64_t __new_exitfn_called;
@@ -76,7 +82,10 @@  __new_exitfn (struct exit_function_list **listp)
   struct exit_function *r = NULL;
   size_t i = 0;
 
-  __libc_lock_lock (lock);
+  if (__exit_funcs_done == 1)
+    /* exit code finished processing all handlers
+       so fail this registration */
+    return NULL;
 
   for (l = *listp; l != NULL; p = l, l = l->next)
     {
@@ -127,7 +136,5 @@  __new_exitfn (struct exit_function_list **listp)
       ++__new_exitfn_called;
     }
 
-  __libc_lock_unlock (lock);
-
   return r;
 }
diff --git a/stdlib/exit.c b/stdlib/exit.c
index c0b6d666c7..dfc4474891 100644
--- a/stdlib/exit.c
+++ b/stdlib/exit.c
@@ -19,11 +19,14 @@ 
 #include <stdlib.h>
 #include <unistd.h>
 #include <sysdep.h>
+#include <libc-lock.h>
 #include "exit.h"
 
 #include "set-hooks.h"
 DEFINE_HOOK (__libc_atexit, (void))
 
+/* initialise the processing complete flag to 0 */
+int __exit_funcs_done = 0;
 
 /* Call all functions registered with `atexit' and `on_exit',
    in the reverse of the order in which they were registered
@@ -44,14 +47,30 @@  __run_exit_handlers (int status, struct exit_function_list **listp,
      the functions registered with `atexit' and `on_exit'. We call
      everyone on the list and use the status value in the last
      exit (). */
-  while (*listp != NULL)
+  while (true)
     {
-      struct exit_function_list *cur = *listp;
+      struct exit_function_list *cur;
+
+    restart:
+      __libc_lock_lock (__exit_funcs_lock);
+      cur = *listp;
+
+      if (cur == NULL)
+	{
+	  /* Exit processing complete.  We will not allow any more
+	     atexit/on_exit registrations.  */
+	  __exit_funcs_done = 1;
+	  __libc_lock_unlock (__exit_funcs_lock);
+	  break;
+	}
 
       while (cur->idx > 0)
 	{
 	  const struct exit_function *const f =
 	    &cur->fns[--cur->idx];
+	  const uint64_t new_exitfn_called = __new_exitfn_called;
+
+	  __libc_lock_unlock (__exit_funcs_lock);
 	  switch (f->flavor)
 	    {
 	      void (*atfct) (void);
@@ -83,6 +102,15 @@  __run_exit_handlers (int status, struct exit_function_list **listp,
 	      cxafct (f->func.cxa.arg, status);
 	      break;
 	    }
+	  __libc_lock_lock (__exit_funcs_lock);
+
+	  if (__glibc_unlikely (new_exitfn_called != __new_exitfn_called))
+	    {
+	      /* The last exit function, or another thread, has registered
+		 more exit functions.  Start the loop over.  */
+	      __libc_lock_unlock (__exit_funcs_lock);
+	      goto restart;
+	    }
 	}
 
       *listp = cur->next;
@@ -90,6 +118,8 @@  __run_exit_handlers (int status, struct exit_function_list **listp,
 	/* Don't free the last element in the chain, this is the statically
 	   allocate element.  */
 	free (cur);
+
+      __libc_lock_unlock (__exit_funcs_lock);
     }
 
   if (run_list_atexit)
diff --git a/stdlib/exit.h b/stdlib/exit.h
index 7f2e679246..023e81b83f 100644
--- a/stdlib/exit.h
+++ b/stdlib/exit.h
@@ -20,6 +20,7 @@ 
 
 #include <stdbool.h>
 #include <stdint.h>
+#include <libc-lock.h>
 
 enum
 {
@@ -62,6 +63,10 @@  extern struct exit_function_list *__quick_exit_funcs attribute_hidden;
 
 extern struct exit_function *__new_exitfn (struct exit_function_list **listp);
 extern uint64_t __new_exitfn_called attribute_hidden;
+__libc_lock_define (extern, __exit_funcs_lock);
+
+/* flag to mark that all registered atexit/onexit handlers are called */
+extern int __exit_funcs_done attribute_hidden;
 
 extern void __run_exit_handlers (int status,
 				 struct exit_function_list **listp,
diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c
index 83845e76d8..2db3063db9 100644
--- a/stdlib/on_exit.c
+++ b/stdlib/on_exit.c
@@ -24,10 +24,16 @@ 
 int
 __on_exit (void (*func) (int status, void *arg), void *arg)
 {
-  struct exit_function *new = __new_exitfn (&__exit_funcs);
+  struct exit_function *new;
+
+   __libc_lock_lock (__exit_funcs_lock);
+  new = __new_exitfn (&__exit_funcs);
 
   if (new == NULL)
-    return -1;
+    {
+      __libc_lock_unlock (__exit_funcs_lock);
+      return -1;
+    }
 
 #ifdef PTR_MANGLE
   PTR_MANGLE (func);
@@ -36,6 +42,7 @@  __on_exit (void (*func) (int status, void *arg), void *arg)
   new->func.on.arg = arg;
   atomic_write_barrier ();
   new->flavor = ef_on;
+  __libc_lock_unlock (__exit_funcs_lock);
   return 0;
 }
 weak_alias (__on_exit, on_exit)
diff --git a/stdlib/test-__cxa_atexit-race.c b/stdlib/test-__cxa_atexit-race.c
new file mode 100644
index 0000000000..b86f6ce212
--- /dev/null
+++ b/stdlib/test-__cxa_atexit-race.c
@@ -0,0 +1,34 @@ 
+/* A test for __cxa_atexit/exit race from bz14333.
+
+   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".  */
+
+#include <stdio.h>
+
+#define CALL_ATEXIT __cxa_atexit (&no_op, NULL, NULL)
+#define CALL_EXIT exit (0)
+
+int __cxa_atexit (void (*func) (void *), void *arg, void *d);
+
+static void
+no_op (void *ignored)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>
diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
new file mode 100644
index 0000000000..2521a6b77c
--- /dev/null
+++ b/stdlib/test-at_quick_exit-race.c
@@ -0,0 +1,30 @@ 
+/* A test for at_quick_exit/quick_exit race from bz14333.
+
+   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".  */
+
+#define CALL_ATEXIT at_quick_exit (&no_op)
+#define CALL_EXIT quick_exit (0)
+
+static void
+no_op (void)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>
diff --git a/stdlib/test-atexit-race-common.c b/stdlib/test-atexit-race-common.c
new file mode 100644
index 0000000000..c4cbd9e592
--- /dev/null
+++ b/stdlib/test-atexit-race-common.c
@@ -0,0 +1,62 @@ 
+/* Support file for atexit/exit, at_quick_exit/quick_exit, etc. race tests
+   from bz14333.
+
+   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".  */
+
+#if !defined(CALL_EXIT) || !defined(CALL_ATEXIT)
+#error Must define CALL_EXIT and CALL_ATEXIT before using this file.
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+const size_t kNumThreads = 1024;
+const size_t kNumHandlers = 1024;
+
+static void *
+threadfunc (void *unused)
+{
+  size_t i;
+  for (i = 0; i < kNumHandlers; ++i) {
+    CALL_ATEXIT;
+  }
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  size_t i;
+  pthread_t thr;
+  pthread_attr_t attr;
+
+  pthread_attr_init (&attr);
+  pthread_attr_setdetachstate (&attr, 1);
+
+  for (i = 0; i < kNumThreads; ++i) {
+    pthread_create (&thr, &attr, threadfunc, NULL);
+  }
+
+  CALL_EXIT;
+}
+
+#define TEST_FUNCTION do_test
+#include <support/test-driver.c>
diff --git a/stdlib/test-atexit-race.c b/stdlib/test-atexit-race.c
new file mode 100644
index 0000000000..b183ecfd7e
--- /dev/null
+++ b/stdlib/test-atexit-race.c
@@ -0,0 +1,30 @@ 
+/* A test for atexit/exit race from bz14333.
+
+   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".  */
+
+#define CALL_ATEXIT atexit (&no_op)
+#define CALL_EXIT exit (0)
+
+static void
+no_op (void)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>