Fix BZ 14333

ChangeLog

@@ -1,3 +1,25 @@
+2017-09-20  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.
+	Remove atomics.
+	(__new_exitfn): Fail registration when we finished at_exit processing.
+	* stdlib/cxa_finalize.c (__cxa_finalize): Likewise.
+	* 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, test-on_exit-race): New tests.
+	* stdlib/test-atexit-race-common.c: New file.
+	* stdlib/test-atexit-race.c: New file.
+	* stdlib/test-at_quick_exit-race.c: New file.
+	* stdlib/test-cxa_atexit-race.c: New file.
+	* stdlib/test-on_exit-race.c: New file.
+
 2017-09-20  Szabolcs Nagy  <szabolcs.nagy@arm.com>
 
 	* benchtests/Makefile: Add exp2f and log2f benchmarks.

stdlib/Makefile

@@ -81,7 +81,9 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-quick_exit tst-thread-quick_exit tst-width	    \
 		   tst-width-stdint tst-strfrom tst-strfrom-locale	    \
 		   tst-getrandom tst-atexit tst-at_quick_exit 		    \
-		   tst-cxa_atexit tst-on_exit
+		   tst-cxa_atexit tst-on_exit test-atexit-race 		    \
+		   test-at_quick_exit-race test-cxa_atexit-race             \
+		   test-on_exit-race
 
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
@@ -91,6 +93,11 @@ 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)
+LDLIBS-test-on_exit-race = $(shared-thread-library)
+
 ifeq ($(have-cxx-thread_local),yes)
 CFLAGS-tst-quick_exit.o = -std=c++11
 LDLIBS-tst-quick_exit = -lstdc++

stdlib/cxa_atexit.c

@@ -21,21 +21,29 @@
 
 #include <libc-lock.h>
 #include "exit.h"
-#include <atomic.h>
 #include <sysdep.h>
 
 #undef __cxa_atexit
 
+/* See concurrency notes in stdlib/exit.h where this lock is declared.  */
+__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);
@@ -43,8 +51,8 @@ __internal_atexit (void (*func) (void *), void *arg, void *d,
   new->func.cxa.fn = (void (*) (void *, int)) func;
   new->func.cxa.arg = arg;
   new->func.cxa.dso_handle = d;
-  atomic_write_barrier ();
   new->flavor = ef_cxa;
+  __libc_lock_unlock (__exit_funcs_lock);
   return 0;
 }
 
@@ -60,14 +68,11 @@ __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;
 
+/* Must be called with __exit_funcs_lock held.  */
 struct exit_function *
 __new_exitfn (struct exit_function_list **listp)
 {
@@ -76,7 +81,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)
+    /* Exit code is finished processing all registered exit functions,
+       therefore we fail this registration.  */
+    return NULL;
 
   for (l = *listp; l != NULL; p = l, l = l->next)
     {
@@ -127,7 +135,5 @@ __new_exitfn (struct exit_function_list **listp)
       ++__new_exitfn_called;
     }
 
-  __libc_lock_unlock (lock);
-
   return r;
 }

stdlib/cxa_finalize.c

@@ -17,7 +17,6 @@
 
 #include <assert.h>
 #include <stdlib.h>
-#include <atomic.h>
 #include "exit.h"
 #include <fork.h>
 #include <sysdep.h>
@@ -31,36 +30,64 @@ __cxa_finalize (void *d)
 {
   struct exit_function_list *funcs;
 
+  __libc_lock_lock (__exit_funcs_lock);
+
  restart:
   for (funcs = __exit_funcs; funcs; funcs = funcs->next)
     {
       struct exit_function *f;
 
       for (f = &funcs->fns[funcs->idx - 1]; f >= &funcs->fns[0]; --f)
-	{
+	if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
-	  void (*cxafn) (void *arg, int status);
+	  {
-	  void *cxaarg;
+	    const uint64_t check = __new_exitfn_called;
+	    void (*cxafn) (void *arg, int status) = f->func.cxa.fn;
+	    void *cxaarg = f->func.cxa.arg;
 
-	  if ((d == NULL || d == f->func.cxa.dso_handle)
+	    /* We don't want to run this cleanup more than once.  The Itanium
-	      /* We don't want to run this cleanup more than once.  */
+	       C++ ABI requires that multiple calls to __cxa_finalize not
-	      && (cxafn = f->func.cxa.fn,
+	       result in calling termination functions more than once.  One
-		  cxaarg = f->func.cxa.arg,
+	       potential scenario where that could happen is with a concurrent
-		  ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
+	       dlclose and exit, where the running dlclose must at some point
-							   ef_cxa)))
+	       release the list lock, an exiting thread may acquire it, and
-	    {
+	       without setting flavor to ef_free, might re-run this destructor
-	      uint64_t check = __new_exitfn_called;
+	       which could result in undefined behaviour.  Therefore we must
+	       set flavor to ef_free to avoid calling this destructor again.
+	       Note that the concurrent exit must also take the dynamic loader
+	       lock (for library finalizer processing) and therefore will
+	       block while dlclose completes the processing of any in-progress
+	       exit functions. Lastly, once we release the list lock for the
+	       entry marked ef_free, we must not read from that entry again
+	       since it may have been reused by the time we take the list lock
+	       again.  Lastly the detection of new registered exit functions is
+	       based on a monotonically incrementing counter, and there is an
+	       ABA if between the unlock to run the exit function and the
+	       re-lock after completion the user registers 2^64 exit functions,
+	       the implementation will not detect this and continue without
+	       executing any more functions.
+
+	       One minor issue remains: A registered exit function that is in
+	       progress by a call to dlclose() may not completely finish before
+	       the next registered exit function is run. This may, according to
+	       some readings of POSIX violate the requirement that functions
+	       run in effective LIFO order.  This should probably be fixed in a
+	       future implementation to ensure the functions do not run in
+	       parallel.  */
+	    f->flavor = ef_free;
 
 #ifdef PTR_DEMANGLE
-	      PTR_DEMANGLE (cxafn);
+	    PTR_DEMANGLE (cxafn);
 #endif
-	      cxafn (cxaarg, 0);
+	    /* Unlock the list while we call a foreign function.  */
+	    __libc_lock_unlock (__exit_funcs_lock);
+	    cxafn (cxaarg, 0);
+	    __libc_lock_lock (__exit_funcs_lock);
 
-	      /* It is possible that that last exit function registered
+	    /* It is possible that that last exit function registered
-		 more exit functions.  Start the loop over.  */
+	       more exit functions.  Start the loop over.  */
-	      if (__glibc_unlikely (check != __new_exitfn_called))
+	    if (__glibc_unlikely (check != __new_exitfn_called))
-		goto restart;
+	      goto restart;
-	    }
+	  }
-	}
     }
 
   /* Also remove the quick_exit handlers, but do not call them.  */
@@ -79,4 +106,5 @@ __cxa_finalize (void *d)
   if (d != NULL)
     UNREGISTER_ATFORK (d);
 #endif
+  __libc_lock_unlock (__exit_funcs_lock);
 }

stdlib/exit.c

@@ -19,11 +19,16 @@
 #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))
 
+/* Initialize the flag that indicates exit function processing
+   is complete. See concurrency notes in stdlib/exit.h where
+   __exit_funcs_lock is declared.  */
+bool __exit_funcs_done = false;
 
 /* Call all functions registered with `atexit' and `on_exit',
    in the reverse of the order in which they were registered
@@ -44,14 +49,32 @@ __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;
+
+      __libc_lock_lock (__exit_funcs_lock);
+
+    restart:
+      cur = *listp;
+
+      if (cur == NULL)
+	{
+	  /* Exit processing complete.  We will not allow any more
+	     atexit/on_exit registrations.  */
+	  __exit_funcs_done = true;
+	  __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;
+
+	  /* Unlock the list while we call a foreign function.  */
+	  __libc_lock_unlock (__exit_funcs_lock);
 	  switch (f->flavor)
 	    {
 	      void (*atfct) (void);
@@ -83,6 +106,13 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
 	      cxafct (f->func.cxa.arg, status);
 	      break;
 	    }
+	  /* Re-lock again before looking at global state.  */
+	  __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.  */
+	    goto restart;
 	}
 
       *listp = cur->next;
@@ -90,6 +120,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)

stdlib/exit.h

@@ -20,6 +20,7 @@
 
 #include <stdbool.h>
 #include <stdint.h>
+#include <libc-lock.h>
 
 enum
 {
@@ -57,11 +58,27 @@ struct exit_function_list
     size_t idx;
     struct exit_function fns[32];
   };
+
 extern struct exit_function_list *__exit_funcs attribute_hidden;
 extern struct exit_function_list *__quick_exit_funcs attribute_hidden;
+extern uint64_t __new_exitfn_called attribute_hidden;
+
+/* True once all registered atexit/at_quick_exit/onexit handlers have been
+   called */
+extern bool __exit_funcs_done attribute_hidden;
+
+/* This lock protects __exit_funcs, __quick_exit_funcs, __exit_funcs_done
+   and __new_exitfn_called globals against simultaneous access from
+   atexit/on_exit/at_quick_exit in multiple threads, and also from
+   simultaneous access while another thread is in the middle of calling
+   exit handlers.  See BZ#14333.  Note: for lists, the entire list, and
+   each associated entry in the list, is protected for all access by this
+   lock.  */
+__libc_lock_define (extern, __exit_funcs_lock);
+
 
 extern struct exit_function *__new_exitfn (struct exit_function_list **listp);
-extern uint64_t __new_exitfn_called attribute_hidden;
+
 
 extern void __run_exit_handlers (int status,
 				 struct exit_function_list **listp,

stdlib/on_exit.c

@@ -17,25 +17,30 @@
 
 #include <stdlib.h>
 #include "exit.h"
-#include <atomic.h>
 #include <sysdep.h>
 
 /* Register a function to be called by exit.  */
 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);
 #endif
   new->func.on.fn = func;
   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)

stdlib/test-at_quick_exit-race.c

@@ -0,0 +1,32 @@
+/* Bug 14333: a test for at_quick_exit/quick_exit 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".  */
+
+/* See stdlib/test-atexit-race-common.c for details on this test.  */
+
+#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>

stdlib/test-atexit-race.c

@@ -0,0 +1,31 @@
+/* Bug 14333: a test for atexit/exit 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".  */
+
+/* See stdlib/test-atexit-race-common.c for details on this test.  */
+
+#define CALL_ATEXIT atexit (&no_op)
+#define CALL_EXIT exit (0)
+
+static void
+no_op (void)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>

stdlib/test-cxa_atexit-race.c

@@ -0,0 +1,35 @@
+/* Bug 14333: a test for __cxa_atexit/exit 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".  */
+
+/* See stdlib/test-atexit-race-common.c for details on this test.  */
+
+#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>

stdlib/test-on_exit-race.c

@@ -0,0 +1,31 @@
+/* Bug 14333: a test for on_exit/exit 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".  */
+
+/* See stdlib/test-atexit-race-common.c for details on this test.  */
+
+#define CALL_ATEXIT on_exit (&no_op, (void *) 0)
+#define CALL_EXIT exit (0)
+
+static void
+no_op (int exit_code, void *ignored)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>