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

Message ID CALoOobNCZqgvcLJhuJt5eSWseuTfHJ9oVsG9TpPvytyfzF56mg@mail.gmail.com
State New, archived
Headers

Commit Message

Paul Pluzhnikov July 11, 2017, 12:53 a.m. UTC
  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.
  

Comments

Joseph Myers July 11, 2017, 12:22 p.m. UTC | #1
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?
  
Paul Pluzhnikov July 11, 2017, 3:03 p.m. UTC | #2
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,
  
Joseph Myers July 11, 2017, 7:01 p.m. UTC | #3
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.
  

Patch

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)