[patch] Fix for bz14333 -- race between atexit() and exit()

[patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

Greetings,

This patch is an update to previous patch by Anoop:
https://sourceware.org/ml/libc-alpha/2008-09/msg00013.html

See discussion in the above message.

Changes from the previous patch: I extended the lifetime of the lock
from __new_exitfn to its callers, since they *also* modify global
state.

Tested: "make check" on Linux/x86_64.
Also ran the test from
https://sourceware.org/bugzilla/show_bug.cgi?id=14333#c5 a few hundred
times.


2017-07-10  Paul Pluzhnikov  <ppluzhnikov@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.

-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz14333-20170709.txt --]
[-- Type: text/plain, Size: 5572 bytes --]

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)

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Joseph Myers]

On Mon, 10 Jul 2017, Paul Pluzhnikov wrote:

> Greetings,
> 
> This patch is an update to previous patch by Anoop:
> https://sourceware.org/ml/libc-alpha/2008-09/msg00013.html
> 
> See discussion in the above message.
> 
> Changes from the previous patch: I extended the lifetime of the lock
> from __new_exitfn to its callers, since they *also* modify global
> state.

Is there such a race for quick_exit / at_quick_exit (which postdate the 
previous patch), or not?  If there is, it should also be fixed (all 
atexit-like functions should be fixed).

Is it possible to add a test / tests for this issue to the glibc 
testsuite?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

Anoop removed from CC list since his e-mail no longer works.

On Tue, Jul 11, 2017 at 5:22 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>
> Is there such a race for quick_exit / at_quick_exit (which postdate the
> previous patch), or not?

Yes, the test from bz14333#c5 with s/atexit/at_quick_exit/ and
s/exit/quick_exit/ also crashes the same way, and the patch here fixes
that crash as well.

>  If there is, it should also be fixed (all
> atexit-like functions should be fixed).

They all share the implementation, so this is already done here :-)

> Is it possible to add a test / tests for this issue to the glibc
> testsuite?

Sure. I was going to add it after the "basic" atexit ordering test,
but since that patch requires revisions, I will send a patch to add
the multithreaded test for atexit/at_quick_exit/__cxa_at_quick_exit
separately (or should it be part of this patch?).

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Joseph Myers]

On Tue, 11 Jul 2017, Paul Pluzhnikov wrote:

> >  If there is, it should also be fixed (all
> > atexit-like functions should be fixed).
> 
> They all share the implementation, so this is already done here :-)
> 
> > Is it possible to add a test / tests for this issue to the glibc
> > testsuite?
> 
> Sure. I was going to add it after the "basic" atexit ordering test,
> but since that patch requires revisions, I will send a patch to add
> the multithreaded test for atexit/at_quick_exit/__cxa_at_quick_exit
> separately (or should it be part of this patch?).

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

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

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.

-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz14333-20170712.txt --]
[-- Type: text/plain, Size: 12468 bytes --]

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>

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

On Wed, Jul 12, 2017 at 9:09 AM, Andreas Schwab <schwab@suse.de> wrote:
>
> On Jul 12 2017, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
> > +extern int __exit_funcs_done attribute_hidden;
>
> That should be bool.


Thanks. Revised patch attached.

2017-07-13  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.



-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz14333-20170713.txt --]
[-- Type: text/plain, Size: 12478 bytes --]

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..a21d865844 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)
+    /* 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..2b14817811 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 false.  */
+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 +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 = 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;
+
+	  __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..de08b9c5ec 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 bool __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>

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Torvald Riegel]

On Thu, 2017-07-13 at 10:45 -0700, Paul Pluzhnikov wrote:
> On Wed, Jul 12, 2017 at 9:09 AM, Andreas Schwab <schwab@suse.de> wrote:
> >
> > On Jul 12 2017, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> >
> > > +extern int __exit_funcs_done attribute_hidden;
> >
> > That should be bool.
> 
> 
> Thanks. Revised patch attached.

It would be good if you could add comments describing the locking scheme
you are changing

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

On Wed, Jul 19, 2017 at 5:26 AM, Torvald Riegel <triegel@redhat.com> wrote:

> It would be good if you could add comments describing the locking scheme
> you are changing

The locking scheme is kind of trivial: hold the lock while reading or
writing any of the relevant globals.

I've added some comments; please let me know if/where more is desired.

Thanks,


2017-07-19  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.

[-- Attachment #2: glibc-bz14333-20170719.txt --]
[-- Type: text/plain, Size: 13211 bytes --]

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..a21d865844 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)
+    /* 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..a3205ae8c0 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 false.  */
+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 +47,31 @@ __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 = 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 into user-provided code.  */
+	  __libc_lock_unlock (__exit_funcs_lock);
 	  switch (f->flavor)
 	    {
 	      void (*atfct) (void);
@@ -83,6 +103,16 @@ __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.  */
+	      __libc_lock_unlock (__exit_funcs_lock);
+	      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)
diff --git a/stdlib/exit.h b/stdlib/exit.h
index 7f2e679246..21776f6dcb 100644
--- a/stdlib/exit.h
+++ b/stdlib/exit.h
@@ -20,6 +20,7 @@
 
 #include <stdbool.h>
 #include <stdint.h>
+#include <libc-lock.h>
 
 enum
 {
@@ -57,11 +58,24 @@ 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;
+/* flag to mark that all registered atexit/onexit handlers are called */
+extern bool __exit_funcs_done attribute_hidden;
+extern uint64_t __new_exitfn_called attribute_hidden;
+
+/* This lock protects above globals: __exit_funcs, __quick_exit_funcs,
+   __exit_funcs_done and __new_exitfn_called (must be held while reading
+   or modifying any of these 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.  */
+__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,
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>

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Torvald Riegel]

On Wed, 2017-07-19 at 17:37 -0700, Paul Pluzhnikov wrote:
> On Wed, Jul 19, 2017 at 5:26 AM, Torvald Riegel <triegel@redhat.com> wrote:
> 
> > It would be good if you could add comments describing the locking scheme
> > you are changing
> 
> The locking scheme is kind of trivial: hold the lock while reading or
> writing any of the relevant globals.

Even if the scheme itself is not complex in the sense that it's simply
one lock that protects a few pieces of data, figuring out what is
intended can be nontrivial.  It's certainly harder than just reading a
comment, so it makes sense to write this down.

> I've added some comments; please let me know if/where more is desired.

Kind of, I guess.  When you write that __exit_funcs_lock protects
__exit_funcs, do you mean that it also protects the full list that this
global points to?  If so, please say that.

Does that fully remove the need for what looks like an (incorrect)
attempt to build a concurrent list?  I see an atomic_write_barrier in
__on_exit that doesn't seem to have a matching acquire load elsewhere;
there's another atomic_write_barrier that may have been intended to
synchronize with the acquire CAS in __cxa_finalize (but other loads from
that field are plain loads).

So, getting back to the level of detail in comments, what I'm looking
for is text that is sufficient to express the intent behind how
concurreny is handled in this piece of code.  I think for a simple
locking scheme such as this one, having one block of comments around the
central lock is good enough; generally, I think it's nicer to add
comments to the respective data (eg, __exit_funcs) that at least say
something like "See __exit_funcs_lock for concurrency notes.", so that
developers get a heads-up when trying to access the data.  (In this
case, both lock and data are really next to each other, so that might
not add that much.)

Has anyone reviewed this patch in detail yet (including the concurrency
aspect)?  I'm not familiar enough with the exit handler code currently,
so I don't want to dive into this unless (1) nobody else has done it nor
plans to do so and (2) we could still get this into the current release.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Mon, Jul 24, 2017 at 1:07 PM, Torvald Riegel <triegel@redhat.com> wrote:
> On Wed, 2017-07-19 at 17:37 -0700, Paul Pluzhnikov wrote:
>> On Wed, Jul 19, 2017 at 5:26 AM, Torvald Riegel <triegel@redhat.com> wrote:
>>
>> > It would be good if you could add comments describing the locking scheme
>> > you are changing
>>
>> The locking scheme is kind of trivial: hold the lock while reading or
>> writing any of the relevant globals.
>
> Even if the scheme itself is not complex in the sense that it's simply
> one lock that protects a few pieces of data, figuring out what is
> intended can be nontrivial.  It's certainly harder than just reading a
> comment, so it makes sense to write this down.
>
>> I've added some comments; please let me know if/where more is desired.
>
> Kind of, I guess.  When you write that __exit_funcs_lock protects
> __exit_funcs, do you mean that it also protects the full list that this
> global points to?  If so, please say that.

Yes, Will do.

> Does that fully remove the need for what looks like an (incorrect)
> attempt to build a concurrent list?

Probably. Let me review these. I suspect they are no longer necessary.

> Has anyone reviewed this patch in detail yet?

I don't believe so.


Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Carlos O'Donell]

On 07/24/2017 04:07 PM, Torvald Riegel wrote:
> Has anyone reviewed this patch in detail yet (including the concurrency
> aspect)?  I'm not familiar enough with the exit handler code currently,
> so I don't want to dive into this unless (1) nobody else has done it nor
> plans to do so and (2) we could still get this into the current release.
 
I have not reviewed this in detail and I don't plan to review it before
2.26 goes out the door since there are other more important blockers to
the release (bug 21298 which I'm still reviewing).

I do not think bug 14333 (this bug) counts as a blocker. We've had this
bug for years, and we've known about it, and lived with it.

I think we can continue to work on this set of patches to meet our
expectations.

Paul, just for reference look at f8bf15febcaf137bbec5a61101e88cd5a9d56ca8
and the amount of comments for what started off as a "simple change" that
was less than a few lines of code originally posted by VMWare :-)

I don't want to scare you, and you need not be held responsible for
documenting the P&C semantics of the code you touch, that's a responsibility
that all of us collectively bear. So I'm happy to help write more of this up,
but I can't do that until August.

-- 
Cheers,
Carlos.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Mon, Jul 24, 2017 at 1:20 PM, Carlos O'Donell <carlos@redhat.com> wrote:

> I do not think bug 14333 (this bug) counts as a blocker. We've had this
> bug for years, and we've known about it, and lived with it.

For the record, there is no urgency to get this into 2.26 from my side.

We do hit this bug on and off, and figuring out that it's *that* bug
(again) takes time, and the fix appears to be within reach ;-) but we
can certainly wait a bit more.


-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

On Mon, Jul 24, 2017 at 1:14 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

>> Kind of, I guess.  When you write that __exit_funcs_lock protects
>> __exit_funcs, do you mean that it also protects the full list that this
>> global points to?  If so, please say that.
>
> Yes, Will do.

Done.

>> Does that fully remove the need for what looks like an (incorrect)
>> attempt to build a concurrent list?
>
> Probably. Let me review these. I suspect they are no longer necessary.

That is correct: the half-cooked atomic accesses are no longer
necessary, since all modifications (and reads) now happen under the
lock. I've removed them.


2017-07-31  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): 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.




-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz14333-20170731.txt --]
[-- Type: text/plain, Size: 15593 bytes --]

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 0314d5926b..c768b17cd4 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..10b74d2982 100644
--- a/stdlib/cxa_atexit.c
+++ b/stdlib/cxa_atexit.c
@@ -21,21 +21,29 @@
 
 #include <libc-lock.h>
 #include "exit.h"
-#include <atomic.h>
 #include <sysdep.h>
 
 #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);
@@ -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,10 +68,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 +80,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 finished processing all handlers
+       so fail this registration */
+    return NULL;
 
   for (l = *listp; l != NULL; p = l, l = l->next)
     {
@@ -127,7 +134,5 @@ __new_exitfn (struct exit_function_list **listp)
       ++__new_exitfn_called;
     }
 
-  __libc_lock_unlock (lock);
-
   return r;
 }
diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c
index aa0a70cb58..2216a3d87e 100644
--- a/stdlib/cxa_finalize.c
+++ b/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,35 @@ __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)
-	{
-	  void (*cxafn) (void *arg, int status);
-	  void *cxaarg;
+	if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
+	  {
+	    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.  */
-	      && (cxafn = f->func.cxa.fn,
-		  cxaarg = f->func.cxa.arg,
-		  ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
-							   ef_cxa)))
-	    {
-	      uint64_t check = __new_exitfn_called;
+	    /* We don't want to run this cleanup more than once.  */
+	    f->flavor = ef_free;
 
 #ifdef PTR_DEMANGLE
-	      PTR_DEMANGLE (cxafn);
+	    PTR_DEMANGLE (cxafn);
 #endif
-	      cxafn (cxaarg, 0);
+	    __libc_lock_unlock (__exit_funcs_lock);
+	    cxafn (cxaarg, 0);
+	    __libc_lock_lock (__exit_funcs_lock);
 
-	      /* It is possible that that last exit function registered
-		 more exit functions.  Start the loop over.  */
-	      if (__glibc_unlikely (check != __new_exitfn_called))
-		goto restart;
-	    }
-	}
+	    /* It is possible that that last exit function registered
+	       more exit functions.  Start the loop over.  */
+	    if (__glibc_unlikely (check != __new_exitfn_called))
+	      goto restart;
+	  }
     }
 
   /* Also remove the quick_exit handlers, but do not call them.  */
@@ -79,4 +77,5 @@ __cxa_finalize (void *d)
   if (d != NULL)
     UNREGISTER_ATFORK (d);
 #endif
+  __libc_lock_unlock (__exit_funcs_lock);
 }
diff --git a/stdlib/exit.c b/stdlib/exit.c
index c0b6d666c7..69acef5c23 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 false.  */
+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 +47,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 into user-provided code.  */
+	  __libc_lock_unlock (__exit_funcs_lock);
 	  switch (f->flavor)
 	    {
 	      void (*atfct) (void);
@@ -83,6 +104,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 +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..700163c8be 100644
--- a/stdlib/exit.h
+++ b/stdlib/exit.h
@@ -20,6 +20,7 @@
 
 #include <stdbool.h>
 #include <stdint.h>
+#include <libc-lock.h>
 
 enum
 {
@@ -57,11 +58,26 @@ 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 is
+   protected 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,
diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c
index 83845e76d8..f4ede2b1a7 100644
--- a/stdlib/on_exit.c
+++ b/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)
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>
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>

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
<ppluzhnikov@google.com> wrote:

> 2017-07-31  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): 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.

Ping?

(Re-sending as plain text. Sorry if you got duplicate.)



-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Mon, Aug 7, 2017 at 12:28 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
> <ppluzhnikov@google.com> wrote:
>
>> 2017-07-31  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): 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.
>
> Ping?

Ping x2?

-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Mon, Aug 28, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Mon, Aug 7, 2017 at 12:28 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
>> <ppluzhnikov@google.com> wrote:
>>
>>> 2017-07-31  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>>             Ricky Zhou <rickyz@google.com>
>>>             Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>>>
>>>         [BZ #14333]
>>
>> Ping?
>
> Ping x2?

Ping x3?


-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Wed, Sep 6, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Mon, Aug 28, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> On Mon, Aug 7, 2017 at 12:28 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>>> On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
>>> <ppluzhnikov@google.com> wrote:
>>>
>>>> 2017-07-31  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>>>             Ricky Zhou <rickyz@google.com>
>>>>             Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>>>>
>>>>         [BZ #14333]
>>>
>>> Ping?
>>
>> Ping x2?
>
> Ping x3?

Ping x4?


-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Carlos O'Donell]

On 09/13/2017 10:41 AM, Paul Pluzhnikov wrote:
> On Wed, Sep 6, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> On Mon, Aug 28, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>>> On Mon, Aug 7, 2017 at 12:28 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>>>> On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
>>>> <ppluzhnikov@google.com> wrote:
>>>>
>>>>> 2017-07-31  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>>>>             Ricky Zhou <rickyz@google.com>
>>>>>             Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>>>>>
>>>>>         [BZ #14333]
>>>>
>>>> Ping?
>>>
>>> Ping x2?
>>
>> Ping x3?
> 
> Ping x4?
 
I'm reviewing this and I'm about 75% done. Should post the review tomorrow.


-- 
Cheers,
Carlos.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Carlos O'Donell]

On 07/31/2017 01:05 PM, Paul Pluzhnikov wrote:
> On Mon, Jul 24, 2017 at 1:14 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> 
>>> Kind of, I guess.  When you write that __exit_funcs_lock protects
>>> __exit_funcs, do you mean that it also protects the full list that this
>>> global points to?  If so, please say that.
>> Yes, Will do.
> Done.
> 
>>> Does that fully remove the need for what looks like an (incorrect)
>>> attempt to build a concurrent list?
>> Probably. Let me review these. I suspect they are no longer necessary.
> That is correct: the half-cooked atomic accesses are no longer
> necessary, since all modifications (and reads) now happen under the
> lock. I've removed them.

This is looking awesome.

Thank you for the cleanup.

(1) Design:

I think the design is better now. We removed the half-cooked concurrent
list access and are using a single lock to order exit function access.
That's the best solution for right now and it looks good.

(2) Implementation:

The implementation looks good to me. You've removed the atomic accesses,
and atomic.h includes, and cleaned up the lock usage.

(3) Details:

We have some remaining documentation details which I'll help you work
through. I've provided notes below along with suggestions.

I think the next version will be ready to commit.

> 2017-07-31  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): 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.
> 
> 
> 
> 
> -- Paul Pluzhnikov
> 
> 
> glibc-bz14333-20170731.txt
> 
> 
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 0314d5926b..c768b17cd4 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..10b74d2982 100644
> --- a/stdlib/cxa_atexit.c
> +++ b/stdlib/cxa_atexit.c
> @@ -21,21 +21,29 @@
>  
>  #include <libc-lock.h>
>  #include "exit.h"
> -#include <atomic.h>

OK. Good, remove the atomic types because we are switching to locking.

>  #include <sysdep.h>
>  
>  #undef __cxa_atexit
>  
> +/* We change global data, so we need locking.  */

This is a low quality comment. It should reference the master
definition of the lock.

Suggest:

/* See concurrency notes in stdlib/exit.h where this lock is defined.  */

> +__libc_lock_define_initialized (, __exit_funcs_lock)
> +
>  

OK.

>  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);

OK. Take the lock because __new_exitfn() manipulates, or even
allocates a new list.

> +  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,10 +68,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)
> -

OK.

> -
>  static struct exit_function_list initial;
>  struct exit_function_list *__exit_funcs = &initial;
>  uint64_t __new_exitfn_called;
> @@ -76,7 +80,10 @@ __new_exitfn (struct exit_function_list **listp)

You need a comment for __new_exitfn that says it must be called with
__exit_funcs_lock held.

>    struct exit_function *r = NULL;
>    size_t i = 0;
>  
> -  __libc_lock_lock (lock);
> +  if (__exit_funcs_done)
> +    /* exit code finished processing all handlers
> +       so fail this registration */

Full sentence please.

Suggest:

/* 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 +134,5 @@ __new_exitfn (struct exit_function_list **listp)
>        ++__new_exitfn_called;
>      }
>  
> -  __libc_lock_unlock (lock);
> -

OK.

>    return r;
>  }
> diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c
> index aa0a70cb58..2216a3d87e 100644
> --- a/stdlib/cxa_finalize.c
> +++ b/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,35 @@ __cxa_finalize (void *d)
>  {
>    struct exit_function_list *funcs;
>  
> +  __libc_lock_lock (__exit_funcs_lock);
> +

OK. I like how you have moved the locking to the entry points to avoid
any problems with overlap of the functions.

>   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)
> -	{
> -	  void (*cxafn) (void *arg, int status);
> -	  void *cxaarg;
> +	if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
> +	  {
> +	    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.  */
> -	      && (cxafn = f->func.cxa.fn,
> -		  cxaarg = f->func.cxa.arg,
> -		  ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
> -							   ef_cxa)))
> -	    {
> -	      uint64_t check = __new_exitfn_called;
> +	    /* We don't want to run this cleanup more than once.  */
> +	    f->flavor = ef_free;

OK.

>  
>  #ifdef PTR_DEMANGLE
> -	      PTR_DEMANGLE (cxafn);
> +	    PTR_DEMANGLE (cxafn);
>  #endif
> -	      cxafn (cxaarg, 0);
> +	    __libc_lock_unlock (__exit_funcs_lock);
> +	    cxafn (cxaarg, 0);
> +	    __libc_lock_lock (__exit_funcs_lock);

OK. Unlock before calling foreign function.

>  
> -	      /* It is possible that that last exit function registered
> -		 more exit functions.  Start the loop over.  */
> -	      if (__glibc_unlikely (check != __new_exitfn_called))
> -		goto restart;
> -	    }
> -	}
> +	    /* It is possible that that last exit function registered
> +	       more exit functions.  Start the loop over.  */
> +	    if (__glibc_unlikely (check != __new_exitfn_called))
> +	      goto restart;
> +	  }

OK.

>      }
>  
>    /* Also remove the quick_exit handlers, but do not call them.  */
> @@ -79,4 +77,5 @@ __cxa_finalize (void *d)
>    if (d != NULL)
>      UNREGISTER_ATFORK (d);
>  #endif
> +  __libc_lock_unlock (__exit_funcs_lock);

OK.

>  }
> diff --git a/stdlib/exit.c b/stdlib/exit.c
> index c0b6d666c7..69acef5c23 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>

OK.

>  #include "exit.h"
>  
>  #include "set-hooks.h"
>  DEFINE_HOOK (__libc_atexit, (void))
>  
> +/* Initialise the processing complete flag to false.  */
> +bool __exit_funcs_done = false;

Suggest:

/* Initialize the flag that indicates exit function processing
   is complete. See concurrency notes in stdlib/exit.h where 
   __exit_funcs_lock is defined.  */

>  
>  /* Call all functions registered with `atexit' and `on_exit',
>     in the reverse of the order in which they were registered
> @@ -44,14 +47,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)

OK.

>      {
> -      struct exit_function_list *cur = *listp;
> +      struct exit_function_list *cur;
> +
> +      __libc_lock_lock (__exit_funcs_lock);

OK.

> +
> +    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);

OK.

> +	  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 into user-provided code.  */

The common phrase is "Don't call foreign functions with locks held."

Suggest:

/* Unlock the list while we call foreign functions.  */

> +	  __libc_lock_unlock (__exit_funcs_lock);
>  	  switch (f->flavor)
>  	    {
>  	      void (*atfct) (void);
> @@ -83,6 +104,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;

OK.

>  	}
>  
>        *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);

OK.

>      }
>  
>    if (run_list_atexit)
> diff --git a/stdlib/exit.h b/stdlib/exit.h
> index 7f2e679246..700163c8be 100644
> --- a/stdlib/exit.h
> +++ b/stdlib/exit.h
> @@ -20,6 +20,7 @@
>  
>  #include <stdbool.h>
>  #include <stdint.h>
> +#include <libc-lock.h>

OK.

>  
>  enum
>  {
> @@ -57,11 +58,26 @@ 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;

OK.

> +
> +/* 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 is
> +   protected by this lock.  */
> +__libc_lock_define (extern, __exit_funcs_lock);

Suggest a slight adjustment:

... Note: for lists, the entire list, and each associated entry in the list,
is protected from any access by this 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,
> diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c
> index 83845e76d8..f4ede2b1a7 100644
> --- a/stdlib/on_exit.c
> +++ b/stdlib/on_exit.c
> @@ -17,25 +17,30 @@
>  
>  #include <stdlib.h>
>  #include "exit.h"
> -#include <atomic.h>

OK.

>  #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);

OK.

>  
>    if (new == NULL)
> -    return -1;
> +    {
> +      __libc_lock_unlock (__exit_funcs_lock);
> +      return -1;

OK.

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

OK.

>    return 0;
>  }
>  weak_alias (__on_exit, on_exit)
> 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.

Suggest:

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".  */
> +

Needs a comment explaining what this specific test is looking for and
what are the expected results.

> +#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>

OK.

> 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.

Suggest:

Bug 14333: Support file for atexit/exit, at_quick_exit/quick_exit, etc. race tests.

> +
> +   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;
> +}
> +

Needs a comment explaining what this is doing and why.

> +static int
> +do_test (void)
> +{
> +  size_t i;
> +  pthread_t thr;
> +  pthread_attr_t attr;
> +
> +  pthread_attr_init (&attr);
> +  pthread_attr_setdetachstate (&attr, 1);

Use xpthread_* variants.

> +
> +  for (i = 0; i < kNumThreads; ++i) {
> +    pthread_create (&thr, &attr, threadfunc, NULL);

Likewise.

> +  }
> +
> +  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.

Suggest:

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".  */
> +

Needs a comment explaining what this test is doing and what are the expected
results.

> +#define CALL_ATEXIT atexit (&no_op)
> +#define CALL_EXIT exit (0)
> +
> +static void
> +no_op (void)
> +{
> +}
> +
> +#include <stdlib/test-atexit-race-common.c>
> 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.

Suggest:

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".  */
> +
> +#include <stdio.h>
> +

Needs a comment explaining what this test is doing and why.

> +#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>


-- 
Cheers,
Carlos.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

Carlos,

Thanks for the review.

On Thu, Sep 14, 2017 at 8:07 AM, Carlos O'Donell <carlos@redhat.com> wrote:

> Suggest:
>
> /* See concurrency notes in stdlib/exit.h where this lock is defined.  */

Nit: the lock is defined here, but is declared in exit.h

>> diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
>
> Needs a comment explaining what this specific test is looking for and
> what are the expected results.

Rather than repeating such comment in every test-*exit*-race.c, I
added a note to look in test-atexit-race-common.c.

I also added a test for on_exit/exit -- I missed on_exit in previous iteration.

Thanks,

2017-09-18  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.



-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz14333-20170918.txt --]
[-- Type: text/plain, Size: 18984 bytes --]

2017-09-18  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.



diff --git a/stdlib/Makefile b/stdlib/Makefile
index 2da39e067c..2fb08342e0 100644
--- a/stdlib/Makefile
+++ b/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++
diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
index ce5d9f22b4..beb31691d5 100644
--- a/stdlib/cxa_atexit.c
+++ b/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;
 }
diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c
index aa0a70cb58..213acafe12 100644
--- a/stdlib/cxa_finalize.c
+++ b/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,36 @@ __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)
-	{
-	  void (*cxafn) (void *arg, int status);
-	  void *cxaarg;
+	if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
+	  {
+	    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.  */
-	      && (cxafn = f->func.cxa.fn,
-		  cxaarg = f->func.cxa.arg,
-		  ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
-							   ef_cxa)))
-	    {
-	      uint64_t check = __new_exitfn_called;
+	    /* We don't want to run this cleanup more than once.  */
+	    f->flavor = ef_free;
 
 #ifdef PTR_DEMANGLE
-	      PTR_DEMANGLE (cxafn);
+	    PTR_DEMANGLE (cxafn);
 #endif
-	      cxafn (cxaarg, 0);
+	    /* Unlock the list while we call 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
-		 more exit functions.  Start the loop over.  */
-	      if (__glibc_unlikely (check != __new_exitfn_called))
-		goto restart;
-	    }
-	}
+	    /* It is possible that that last exit function registered
+	       more exit functions.  Start the loop over.  */
+	    if (__glibc_unlikely (check != __new_exitfn_called))
+	      goto restart;
+	  }
     }
 
   /* Also remove the quick_exit handlers, but do not call them.  */
@@ -79,4 +78,5 @@ __cxa_finalize (void *d)
   if (d != NULL)
     UNREGISTER_ATFORK (d);
 #endif
+  __libc_lock_unlock (__exit_funcs_lock);
 }
diff --git a/stdlib/exit.c b/stdlib/exit.c
index c0b6d666c7..b18e252235 100644
--- a/stdlib/exit.c
+++ b/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 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)
diff --git a/stdlib/exit.h b/stdlib/exit.h
index 7f2e679246..dbf9f2d01f 100644
--- a/stdlib/exit.h
+++ b/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,
diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c
index 83845e76d8..f4ede2b1a7 100644
--- a/stdlib/on_exit.c
+++ b/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)
diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
new file mode 100644
index 0000000000..f93fb852a2
--- /dev/null
+++ b/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>
diff --git a/stdlib/test-atexit-race-common.c b/stdlib/test-atexit-race-common.c
new file mode 100644
index 0000000000..365c9d9c5a
--- /dev/null
+++ b/stdlib/test-atexit-race-common.c
@@ -0,0 +1,70 @@
+/* Bug 14333: Support file for atexit/exit, etc. race tests.
+
+   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".  */
+
+/* The atexit/exit, at_quick_exit/quick_exit, __cxa_atexit/exit, etc.
+   exhibited data race while accessing destructor function list (Bug 14333).
+
+   This test spawns large number of threads, which all race to register
+   large number of destructors.
+
+   Before the fix, running this test resulted in a SIGSEGV.
+   After the fix, we expect clean process termination.  */
+
+#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 <support/xthread.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_attr_t attr;
+
+  xpthread_attr_init (&attr);
+  xpthread_attr_setdetachstate (&attr, 1);
+
+  for (i = 0; i < kNumThreads; ++i) {
+    xpthread_create (&attr, threadfunc, NULL);
+  }
+  xpthread_attr_destroy (&attr);
+
+  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..a4df532ce5
--- /dev/null
+++ b/stdlib/test-atexit-race.c
@@ -0,0 +1,32 @@
+/* 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>
diff --git a/stdlib/test-cxa_atexit-race.c b/stdlib/test-cxa_atexit-race.c
new file mode 100644
index 0000000000..670f67538e
--- /dev/null
+++ b/stdlib/test-cxa_atexit-race.c
@@ -0,0 +1,36 @@
+/* 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>
diff --git a/stdlib/test-on_exit-race.c b/stdlib/test-on_exit-race.c
new file mode 100644
index 0000000000..fce0fa7492
--- /dev/null
+++ b/stdlib/test-on_exit-race.c
@@ -0,0 +1,32 @@
+/* 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>

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Carlos O'Donell]

On 09/18/2017 10:45 AM, Paul Pluzhnikov wrote:
> Carlos,
> 
> Thanks for the review.
> 
> On Thu, Sep 14, 2017 at 8:07 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> Suggest:
>>
>> /* See concurrency notes in stdlib/exit.h where this lock is defined.  */
> Nit: the lock is defined here, but is declared in exit.h

OK.

>>> diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
>> Needs a comment explaining what this specific test is looking for and
>> what are the expected results.
> Rather than repeating such comment in every test-*exit*-race.c, I
> added a note to look in test-atexit-race-common.c.
> 
> I also added a test for on_exit/exit -- I missed on_exit in previous iteration.

I noticed something I didn't see on my other review.

Please see below.

> Thanks,
> 
> 2017-09-18  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.
> 
> 
> 
> -- Paul Pluzhnikov
> 
> 
> glibc-bz14333-20170918.txt
> 
> 
> 2017-09-18  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.
> 
> 
> 
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 2da39e067c..2fb08342e0 100644
> --- a/stdlib/Makefile
> +++ b/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++
> diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
> index ce5d9f22b4..beb31691d5 100644
> --- a/stdlib/cxa_atexit.c
> +++ b/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;
>  }
> diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c
> index aa0a70cb58..213acafe12 100644
> --- a/stdlib/cxa_finalize.c
> +++ b/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,36 @@ __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)
> -	{
> -	  void (*cxafn) (void *arg, int status);
> -	  void *cxaarg;
> +	if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
> +	  {
> +	    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.  */
> -	      && (cxafn = f->func.cxa.fn,
> -		  cxaarg = f->func.cxa.arg,
> -		  ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
> -							   ef_cxa)))
> -	    {
> -	      uint64_t check = __new_exitfn_called;
> +	    /* We don't want to run this cleanup more than once.  */

We have just changed the way locking works, and the above comment
worries me, particularly for test coverage.

Under what conditions can this function be called more than once?

The C++ runtime is responsible for calling __cxa_finalize to call
the destructors for C++ functions.

The Itanium C++ ABI specifically says things like:
~~~
Multiple calls to __cxa_finalize shall not result in calling 
termination function entries multiple times; the implementation 
may either remove entries or mark them finished.
~~~

In theory perhaps if one thread is calling dlclose() while another
calls exit() we might have a case where the dlclose() has released
the list lock to call a function, then another thread calls exit()
and we might run the same function twice.

Could we amend the comment here to be more descriptive then?

/* We don't want to run this cleanup more than once. The Itanium
   C++ ABI requires that multiple calls to __cxa_finalize not
   result in calling termination functions more than once. One
   potential scenario where that could happen is with a concurrent
   dlclose and exit, where the running dlclose must at some point
   release the list lock, an exiting thread may acquire it, and
   without setting flavor to ef_free, might re-run this destructor
   which could result in undefined behaviour.  Therefore we must
   set flavor to ef_free to avoid calling this destructor again.
   Technically there is a race condition in this example, the thread
   calling dlclose may not have enough time to complete the execution
   of the recently called function before the other thread completes
   the exit processing and terminates the process.  */

Is it a bug that the thread calling dlclose may be the only thread
running this particular function while the other thread is running
to exit?

T1-> dlclose
T1-> library destructors call __cxa_finalize
T1-> picks function foo off the list, marks flavor ef_free
T1-> unlocks list, starts executing foo.
T2-> exit
T2-> starts executing all destructors, skips foo marked ef_free
T2-> proceeds to terminate the process

Is T1's call to foo incomplete?

... I remember having a discussion about exit() having to be
delayed indefinitely waiting for something, but I can't find our
libc-alpha conversation about this. It would seem like this would be
another case where exit() would have to wait for other in-process
termination functions?

> +	    f->flavor = ef_free;
>  
>  #ifdef PTR_DEMANGLE
> -	      PTR_DEMANGLE (cxafn);
> +	    PTR_DEMANGLE (cxafn);
>  #endif
> -	      cxafn (cxaarg, 0);
> +	    /* Unlock the list while we call foreign function.  */

Unlock the list while we call foreign functions.

or

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
> -		 more exit functions.  Start the loop over.  */
> -	      if (__glibc_unlikely (check != __new_exitfn_called))
> -		goto restart;
> -	    }
> -	}
> +	    /* It is possible that that last exit function registered
> +	       more exit functions.  Start the loop over.  */
> +	    if (__glibc_unlikely (check != __new_exitfn_called))
> +	      goto restart;
> +	  }
>      }
>  
>    /* Also remove the quick_exit handlers, but do not call them.  */
> @@ -79,4 +78,5 @@ __cxa_finalize (void *d)
>    if (d != NULL)
>      UNREGISTER_ATFORK (d);
>  #endif
> +  __libc_lock_unlock (__exit_funcs_lock);
>  }
> diff --git a/stdlib/exit.c b/stdlib/exit.c
> index c0b6d666c7..b18e252235 100644
> --- a/stdlib/exit.c
> +++ b/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 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)
> diff --git a/stdlib/exit.h b/stdlib/exit.h
> index 7f2e679246..dbf9f2d01f 100644
> --- a/stdlib/exit.h
> +++ b/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,
> diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c
> index 83845e76d8..f4ede2b1a7 100644
> --- a/stdlib/on_exit.c
> +++ b/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)
> diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
> new file mode 100644
> index 0000000000..f93fb852a2
> --- /dev/null
> +++ b/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>
> diff --git a/stdlib/test-atexit-race-common.c b/stdlib/test-atexit-race-common.c
> new file mode 100644
> index 0000000000..365c9d9c5a
> --- /dev/null
> +++ b/stdlib/test-atexit-race-common.c
> @@ -0,0 +1,70 @@
> +/* Bug 14333: Support file for atexit/exit, etc. race tests.
> +
> +   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".  */
> +
> +/* The atexit/exit, at_quick_exit/quick_exit, __cxa_atexit/exit, etc.
> +   exhibited data race while accessing destructor function list (Bug 14333).
> +
> +   This test spawns large number of threads, which all race to register
> +   large number of destructors.
> +
> +   Before the fix, running this test resulted in a SIGSEGV.
> +   After the fix, we expect clean process termination.  */
> +
> +#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 <support/xthread.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_attr_t attr;
> +
> +  xpthread_attr_init (&attr);
> +  xpthread_attr_setdetachstate (&attr, 1);
> +
> +  for (i = 0; i < kNumThreads; ++i) {
> +    xpthread_create (&attr, threadfunc, NULL);
> +  }
> +  xpthread_attr_destroy (&attr);
> +
> +  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..a4df532ce5
> --- /dev/null
> +++ b/stdlib/test-atexit-race.c
> @@ -0,0 +1,32 @@
> +/* 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>
> diff --git a/stdlib/test-cxa_atexit-race.c b/stdlib/test-cxa_atexit-race.c
> new file mode 100644
> index 0000000000..670f67538e
> --- /dev/null
> +++ b/stdlib/test-cxa_atexit-race.c
> @@ -0,0 +1,36 @@
> +/* 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>
> diff --git a/stdlib/test-on_exit-race.c b/stdlib/test-on_exit-race.c
> new file mode 100644
> index 0000000000..fce0fa7492
> --- /dev/null
> +++ b/stdlib/test-on_exit-race.c
> @@ -0,0 +1,32 @@
> +/* 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>


-- 
Cheers,
Carlos.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Mon, Sep 18, 2017 at 2:15 PM, Carlos O'Donell <carlos@redhat.com> wrote:

>> -           uint64_t check = __new_exitfn_called;
>> +         /* We don't want to run this cleanup more than once.  */
>
> We have just changed the way locking works, and the above comment
> worries me, particularly for test coverage.
>
> Under what conditions can this function be called more than once?

Presumably the applicaton itself may call __cxa_finalize(NULL) from
multiple threads.

> Could we amend the comment here to be more descriptive then?

Sure.

> Is it a bug that the thread calling dlclose may be the only thread
> running this particular function while the other thread is running
> to exit?
>
> T1-> dlclose
> T1-> library destructors call __cxa_finalize
> T1-> picks function foo off the list, marks flavor ef_free
> T1-> unlocks list, starts executing foo.
> T2-> exit
> T2-> starts executing all destructors, skips foo marked ef_free
> T2-> proceeds to terminate the process
>
> Is T1's call to foo incomplete?

Yes. But an application that calls exit in parallel with running
threads always has the risk that any of its functions  will
"evaporate" mid-sentence.

Also, AFAICT this patch does not change the behavior here: the exact
same incomplete call to foo can happen with current code.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Carlos O'Donell]

On 09/18/2017 05:03 PM, Paul Pluzhnikov wrote:
> On Mon, Sep 18, 2017 at 2:15 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>>> -           uint64_t check = __new_exitfn_called;
>>> +         /* We don't want to run this cleanup more than once.  */
>>
>> We have just changed the way locking works, and the above comment
>> worries me, particularly for test coverage.
>>
>> Under what conditions can this function be called more than once?
> 
> Presumably the applicaton itself may call __cxa_finalize(NULL) from
> multiple threads.
> 
>> Could we amend the comment here to be more descriptive then?
> 
> Sure.

I'll make my concrete suggestion below.

>> Is it a bug that the thread calling dlclose may be the only thread
>> running this particular function while the other thread is running
>> to exit?
>>
>> T1-> dlclose
>> T1-> library destructors call __cxa_finalize
>> T1-> picks function foo off the list, marks flavor ef_free
>> T1-> unlocks list, starts executing foo.
>> T2-> exit
>> T2-> starts executing all destructors, skips foo marked ef_free
>> T2-> proceeds to terminate the process
>>
>> Is T1's call to foo incomplete?
> 
> Yes. But an application that calls exit in parallel with running
> threads always has the risk that any of its functions  will
> "evaporate" mid-sentence.

Yes, and no.

Users expect exit() to run *all* of their registered exit functions.

When dlclose() and exit() interleave, you have the potential for
one function which is currently being run by dlclose() to be unable
to finish, and that's not expected. In fact that function runs
partly in parallel with the next registered function, and that could
be seen as a violation of POSIX requirements that functions run in
LIFO order.

I just had to test this out, so I wrote the following program:

#include <stdio.h>
#include <stdlib.h>
#include <dlfcn.h>
#include <semaphore.h>
#include <pthread.h>

sem_t order;

void *
open_library (char * pathname)
{
  void *dso;
  char *err;
  /* Open the DSO.  */
  dso = dlopen (pathname, RTLD_NOW|RTLD_GLOBAL);
  if (dso == NULL)
    {
      err = dlerror ();
      fprintf (stderr, "%s\n", err);
      exit (1);
    }
  /* Clear any errors.  */
  dlerror ();
  return dso;
}

int
close_library (void *dso)
{
  int ret;
  char *err;
  /* Close the library and look for errors too.  */
  ret = dlclose (dso);
  if (ret != 0)
    {
      err = dlerror ();
      fprintf (stderr, "%s\n", err);
      exit (1);
    }
  return ret;
}

void *
exit_thread (void *arg)
{
  /* Wait for the dlclose to start...  */
  sem_wait (&order);
  /* Then try to run the exit sequence which should call all
     __cxa_atexit registered functions and in parallel with
     the executing dlclose().  */
  exit (0);
}

int
main (void)
{
  void *dso;
  pthread_t thread;
  dso = open_library ("./libhas-dtors.so");
  pthread_create (&thread, NULL, exit_thread, NULL); 
  close_library (dso);
  pthread_join (thread, NULL);
  return 1;
}

#include <stdio.h>
#include <stdlib.h>
#include <semaphore.h>
#include <unistd.h>

/* Semaphore defined in executable to ensure we have
   a happens-before between the first function starting
   and exit being called.  */
extern sem_t order;

/* 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)
{
  sem_post (&order);
  sleep (10);
  printf ("first\n");
}

static void
second (void *start)
{
  printf ("second\n");
}


__attribute__ ((constructor)) static void
constructor (void)
{
  sem_init (&order, 0, 0);
  __cxa_atexit (second, NULL, __dso_handle);
  __cxa_atexit (first, NULL, __dso_handle);
}

gcc -O0 -g3 -shared -fPIC -o libhas-dtors.so has-dtors.c -lpthread
gcc -O0 -g3 -export-dynamic -o tst-dlclose-exit tst-dlclose-exit.c -lpthread -ldl

$ ./tst-dlclose-exit 
second
first
second

Which is just wrong, so it shows the existing implementation is broken.

The thread runs second very quickly, then first runs, then second runs
again.

It doesn't exit right away... and the trick is this:

* exit() must call _dl_fini, which needs the loader lock.
* dlclose() already holds the loader lock.
* therefore exit() blocks on the completion of dlclose().

So we are actually safe to finish running our existing handlers, but as
you see it runs second twice.

Can you run the above test case with your patch?

> Also, AFAICT this patch does not change the behavior here: the exact
> same incomplete call to foo can happen with current code.

OK, concrete suggestion per my previous email:

/* We don't want to run this cleanup more than once. The Itanium
   C++ ABI requires that multiple calls to __cxa_finalize not
   result in calling termination functions more than once. One
   potential scenario where that could happen is with a concurrent
   dlclose and exit, where the running dlclose must at some point
   release the list lock, an exiting thread may acquire it, and
   without setting flavor to ef_free, might re-run this destructor
   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.  */

Before I sign off on this I'd like to see the results of your patch
running the example I provided above.

I expect it to print:
second
first

Thanks for wading through these issues.

-- 
Cheers,
Carlos.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Tue, Sep 19, 2017 at 2:32 PM, Carlos O'Donell <carlos@redhat.com> wrote:

> Can you run the above test case with your patch?
...
> I expect it to print:
> second
> first

The bad news: it still prints:

second
first
second

The good news: there is a trivial fix: the loop in stdlib/exit.c must
*also* mark f->flavor as ef_free, and with that fix we get expected
output.

I'll add your test to the set of tests and send updated patch.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Carlos O'Donell]

On 09/19/2017 04:33 PM, Paul Pluzhnikov wrote:
> On Tue, Sep 19, 2017 at 2:32 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> Can you run the above test case with your patch?
> ...
>> I expect it to print:
>> second
>> first
> 
> The bad news: it still prints:
> 
> second
> first
> second
> 
> The good news: there is a trivial fix: the loop in stdlib/exit.c must
> *also* mark f->flavor as ef_free, and with that fix we get expected
> output.
> 
> I'll add your test to the set of tests and send updated patch.

My test has an implicit timing dependency. We wait 10 seconds to allow
the other thread to make progress against the exit() call, we should
change that to use another semaphore so it proceeds in a tick-tock
fashion without any timing requirement.

-- 
Cheers,
Carlos.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

On Tue, Sep 19, 2017 at 6:42 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 09/19/2017 04:33 PM, Paul Pluzhnikov wrote:

>> I'll add your test to the set of tests and send updated patch.
>
> My test has an implicit timing dependency. We wait 10 seconds to allow
> the other thread to make progress against the exit() call, we should
> change that to use another semaphore so it proceeds in a tick-tock
> fashion without any timing requirement.

I decided to keep the fix for this newly-discovered problem and the
new test to a separate patch, which I'll mail after this one is
committed.

Attached is the current patch with comments updated per above discussion.

Thanks,
-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz14333-20170920.txt --]
[-- Type: text/plain, Size: 20849 bytes --]

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.


diff --git a/stdlib/Makefile b/stdlib/Makefile
index 2da39e067c..2fb08342e0 100644
--- a/stdlib/Makefile
+++ b/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++
diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
index ce5d9f22b4..beb31691d5 100644
--- a/stdlib/cxa_atexit.c
+++ b/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;
 }
diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c
index aa0a70cb58..23fc6dcafb 100644
--- a/stdlib/cxa_finalize.c
+++ b/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)
-	{
-	  void (*cxafn) (void *arg, int status);
-	  void *cxaarg;
+	if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
+	  {
+	    const uint64_t check = __new_exitfn_called;
+	    void (*cxafn) (void *arg, int status) = f->func.cxa.fn;
+	    void *cxaarg = f->func.cxa.arg;
+
+	    /* We don't want to run this cleanup more than once.  The Itanium
+	       C++ ABI requires that multiple calls to __cxa_finalize not
+	       result in calling termination functions more than once.  One
+	       potential scenario where that could happen is with a concurrent
+	       dlclose and exit, where the running dlclose must at some point
+	       release the list lock, an exiting thread may acquire it, and
+	       without setting flavor to ef_free, might re-run this destructor
+	       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.
 
-	  if ((d == NULL || d == f->func.cxa.dso_handle)
-	      /* We don't want to run this cleanup more than once.  */
-	      && (cxafn = f->func.cxa.fn,
-		  cxaarg = f->func.cxa.arg,
-		  ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
-							   ef_cxa)))
-	    {
-	      uint64_t check = __new_exitfn_called;
+	       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
-		 more exit functions.  Start the loop over.  */
-	      if (__glibc_unlikely (check != __new_exitfn_called))
-		goto restart;
-	    }
-	}
+	    /* It is possible that that last exit function registered
+	       more exit functions.  Start the loop over.  */
+	    if (__glibc_unlikely (check != __new_exitfn_called))
+	      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);
 }
diff --git a/stdlib/exit.c b/stdlib/exit.c
index c0b6d666c7..b74f1825f0 100644
--- a/stdlib/exit.c
+++ b/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)
diff --git a/stdlib/exit.h b/stdlib/exit.h
index 7f2e679246..dbf9f2d01f 100644
--- a/stdlib/exit.h
+++ b/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,
diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c
index 83845e76d8..f4ede2b1a7 100644
--- a/stdlib/on_exit.c
+++ b/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)
diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
new file mode 100644
index 0000000000..f93fb852a2
--- /dev/null
+++ b/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>
diff --git a/stdlib/test-atexit-race-common.c b/stdlib/test-atexit-race-common.c
new file mode 100644
index 0000000000..365c9d9c5a
--- /dev/null
+++ b/stdlib/test-atexit-race-common.c
@@ -0,0 +1,70 @@
+/* Bug 14333: Support file for atexit/exit, etc. race tests.
+
+   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".  */
+
+/* The atexit/exit, at_quick_exit/quick_exit, __cxa_atexit/exit, etc.
+   exhibited data race while accessing destructor function list (Bug 14333).
+
+   This test spawns large number of threads, which all race to register
+   large number of destructors.
+
+   Before the fix, running this test resulted in a SIGSEGV.
+   After the fix, we expect clean process termination.  */
+
+#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 <support/xthread.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_attr_t attr;
+
+  xpthread_attr_init (&attr);
+  xpthread_attr_setdetachstate (&attr, 1);
+
+  for (i = 0; i < kNumThreads; ++i) {
+    xpthread_create (&attr, threadfunc, NULL);
+  }
+  xpthread_attr_destroy (&attr);
+
+  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..a4df532ce5
--- /dev/null
+++ b/stdlib/test-atexit-race.c
@@ -0,0 +1,32 @@
+/* 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>
diff --git a/stdlib/test-cxa_atexit-race.c b/stdlib/test-cxa_atexit-race.c
new file mode 100644
index 0000000000..670f67538e
--- /dev/null
+++ b/stdlib/test-cxa_atexit-race.c
@@ -0,0 +1,36 @@
+/* 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>
diff --git a/stdlib/test-on_exit-race.c b/stdlib/test-on_exit-race.c
new file mode 100644
index 0000000000..fce0fa7492
--- /dev/null
+++ b/stdlib/test-on_exit-race.c
@@ -0,0 +1,32 @@
+/* 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>

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Carlos O'Donell]

On 09/20/2017 09:52 AM, Paul Pluzhnikov wrote:
> On Tue, Sep 19, 2017 at 6:42 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 09/19/2017 04:33 PM, Paul Pluzhnikov wrote:
> 
>>> I'll add your test to the set of tests and send updated patch.
>>
>> My test has an implicit timing dependency. We wait 10 seconds to allow
>> the other thread to make progress against the exit() call, we should
>> change that to use another semaphore so it proceeds in a tick-tock
>> fashion without any timing requirement.
> 
> I decided to keep the fix for this newly-discovered problem and the
> new test to a separate patch, which I'll mail after this one is
> committed.
> 
> Attached is the current patch with comments updated per above discussion.

Looks great.

Please commit.

I look forward to reviewing the follow-on patch too :-)

-- 
Cheers,
Carlos.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Joseph Myers]

I note the commit message just said "Fix BZ 14333".  Can people please 
include the more detailed descriptions of changes made, as in their 
mailing list postings, in the commit message?  There are certainly some 
simple changes for which just a summary line is sufficient explanation, 
but I don't think this was such a change.  Descriptive commit messages are 
especially important if we stop using ChangeLog entries in future.

(This does mean it's good practice, when making a series of revisions of a 
patch, for each successive revision to have both the full self-contained 
explanation, updated for that patch revision, such as would go in the 
commit message, and a description of what changed from the previous patch 
version, which won't go in the commit message.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Carlos O'Donell]

On 09/20/2017 10:50 AM, Joseph Myers wrote:
> I note the commit message just said "Fix BZ 14333".  Can people please 
> include the more detailed descriptions of changes made, as in their 
> mailing list postings, in the commit message?  There are certainly some 
> simple changes for which just a summary line is sufficient explanation, 
> but I don't think this was such a change.  Descriptive commit messages are 
> especially important if we stop using ChangeLog entries in future.
> 
> (This does mean it's good practice, when making a series of revisions of a 
> patch, for each successive revision to have both the full self-contained 
> explanation, updated for that patch revision, such as would go in the 
> commit message, and a description of what changed from the previous patch 
> version, which won't go in the commit message.)
 
I agree.

I expect committers to follow what we ask for here:

https://sourceware.org/glibc/wiki/Contribution%20checklist#Detailed_Explanation_of_the_Patch
~~~
5. Detailed Explanation of the Patch

The detailed explanation will become the body of the commit message 
for your patch. Please keep this in mind and format accordingly or 
indicate to the reviewer which part of the email should be the 
body of the commit message. 
~~~

I will be more diligent in asking the submitter to clarify exactly
what will be in the commit message and to repost with a clear message
for subsequent versions.

-- 
Cheers,
Carlos.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Wed, Sep 20, 2017 at 9:57 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 09/20/2017 10:50 AM, Joseph Myers wrote:
>> I note the commit message just said "Fix BZ 14333".  Can people please
>> include the more detailed descriptions of changes made, as in their
>> mailing list postings, in the commit message?

Sorry about that.

(I've been using the ChangeLog entry as the commit message, but I
noticed that few others do.)

-- 
Paul Pluzhnikov

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[H.J. Lu]

On 9/20/17, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Wed, Sep 20, 2017 at 9:57 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 09/20/2017 10:50 AM, Joseph Myers wrote:
>>> I note the commit message just said "Fix BZ 14333".  Can people please
>>> include the more detailed descriptions of changes made, as in their
>>> mailing list postings, in the commit message?
>
> Sorry about that.
>
> (I've been using the ChangeLog entry as the commit message, but I
> noticed that few others do.)
>

New tests fail at random on i686:

https://sourceware.org/bugzilla/show_bug.cgi?id=22207

-- 
H.J.

Re: [patch] Fix for bz14333 -- race between atexit() and exit()

[Paul Pluzhnikov]

On Mon, Sep 25, 2017 at 3:53 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On 9/20/17, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> > On Wed, Sep 20, 2017 at 9:57 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> >> On 09/20/2017 10:50 AM, Joseph Myers wrote:
> >>> I note the commit message just said "Fix BZ 14333".  Can people please
> >>> include the more detailed descriptions of changes made, as in their
> >>> mailing list postings, in the commit message?
> >
> > Sorry about that.
> >
> > (I've been using the ChangeLog entry as the commit message, but I
> > noticed that few others do.)
> >
>
> New tests fail at random on i686:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=22207


With default "ulimit -s" of 8192, the test can try to create 1024
threads with a total of 8GiB of RAM usage, which is a bit too much for
a 32-bit system.

I'll send a patch.

Thanks,
-- 
Paul Pluzhnikov