[PATCH] Fix the race between atexit() and exit()

[Peng Haitao <penght at cn dot fujitsu dot com>]

exit() uses global variable __exit_funcs indirectly, which are not protected.
It is not safe in multithread circumstance.

When call exit() and atexit() simultaneously in multithread circumstance,
the following case will cause unsafe.
The case has main process A and thread B.

a. thread B call atexit()
b. process A call exit() to traverse the __exit_funcs list
c. thread B call calloc() to create a new entry p, and next to listp:
   p->next = *listp;
d. process A modify listp to cur's next, then free cur:
   *listp = cur->next;
e. thread B modify listp to p:
   *listp = p;
f. when get f, the f is undefined:
   const struct exit_function *const f =
     &cur->fns[--cur->idx];
g. programme may be Segmentation fault

Signed-off-by: Peng Haitao <penght@cn.fujitsu.com>
---
 ChangeLog           |    6 ++++
 stdlib/cxa_atexit.c |   15 ++++++++--
 stdlib/exit.c       |   76 +++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 79 insertions(+), 18 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5993992..c0549b8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2012-07-06  Peng Haitao  <penght@cn.fujitsu.com>
+
+	* stdlib/exit.c (__run_exit_handlers):
+	* stdlib/cxa_atexit.c (__new_exitfn):
+	Fix the race between atexit() and exit().
+
 2012-07-05  Joseph Myers  <joseph@codesourcery.com>
 
 	[BZ #14157]
diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
index 9704398..8722513 100644
--- a/stdlib/cxa_atexit.c
+++ b/stdlib/cxa_atexit.c
@@ -60,12 +60,14 @@ INTDEF(__cxa_atexit)
 
 
 /* We change global data, so we need locking.  */
-__libc_lock_define_initialized (static, lock)
+__libc_lock_define_initialized (, __exit_funcs_lock)
 
 
 static struct exit_function_list initial;
 struct exit_function_list *__exit_funcs = &initial;
 uint64_t __new_exitfn_called;
+/* flag to mark that all registered atexit/on_exit handlers are called */
+extern int __exit_funcs_done;
 
 struct exit_function *
 __new_exitfn (struct exit_function_list **listp)
@@ -75,7 +77,14 @@ __new_exitfn (struct exit_function_list **listp)
   struct exit_function *r = NULL;
   size_t i = 0;
 
-  __libc_lock_lock (lock);
+  __libc_lock_lock (__exit_funcs_lock);
+
+  if (__exit_funcs_done == 1)
+  {
+    /* exit code finished processing all handlers, so fail this registration */
+    __libc_lock_unlock (__exit_funcs_lock);
+    return NULL;
+  }
 
   for (l = *listp; l != NULL; p = l, l = l->next)
     {
@@ -126,7 +135,7 @@ __new_exitfn (struct exit_function_list **listp)
       ++__new_exitfn_called;
     }
 
-  __libc_lock_unlock (lock);
+  __libc_lock_unlock (__exit_funcs_lock);
 
   return r;
 }
diff --git a/stdlib/exit.c b/stdlib/exit.c
index 1ad548f..ea62639 100644
--- a/stdlib/exit.c
+++ b/stdlib/exit.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1991,95,96,97,99,2001,2002,2005,2009
+/* Copyright (C) 1991,95,96,97,99,2001,2002,2005,2009,2012
    Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -20,11 +20,17 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <sysdep.h>
+#include <atomic.h>
+#include <bits/libc-lock.h>
 #include "exit.h"
 
 #include "set-hooks.h"
 DEFINE_HOOK (__libc_atexit, (void))
 
+__libc_lock_define (extern, __exit_funcs_lock);
+
+/* 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
@@ -38,25 +44,44 @@ __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 (1)
     {
-      struct exit_function_list *cur = *listp;
+      struct exit_function_list *cur;
+      struct exit_function *f;
+
+restart:
+      __libc_lock_lock (__exit_funcs_lock);
+
+      cur = *listp;
+      if (cur == NULL)
+        {
+          /* mark the processing complete,
+             we wont allow to register more handlers. */
+          __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];
-	  switch (f->flavor)
+      f = &cur->fns[cur->idx];
+      uint64_t check = __new_exitfn_called;
+
+      __libc_lock_unlock (__exit_funcs_lock);
+
+      while (f > &cur->fns[0])
+        {
+	  switch ((--f)->flavor)
 	    {
 	      void (*atfct) (void);
 	      void (*onfct) (int status, void *arg);
 	      void (*cxafct) (void *arg, int status);
 
 	    case ef_free:
+              break;
 	    case ef_us:
-	      break;
+	      goto restart;
 	    case ef_on:
 	      onfct = f->func.on.fn;
+              atomic_compare_and_exchange_bool_acq (&f->flavor, ef_free, ef_on);
 #ifdef PTR_DEMANGLE
 	      PTR_DEMANGLE (onfct);
 #endif
@@ -64,6 +89,7 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
 	      break;
 	    case ef_at:
 	      atfct = f->func.at;
+              atomic_compare_and_exchange_bool_acq (&f->flavor, ef_free, ef_at);
 #ifdef PTR_DEMANGLE
 	      PTR_DEMANGLE (atfct);
 #endif
@@ -71,20 +97,40 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
 	      break;
 	    case ef_cxa:
 	      cxafct = f->func.cxa.fn;
+              atomic_compare_and_exchange_bool_acq (&f->flavor, ef_free, ef_cxa);
 #ifdef PTR_DEMANGLE
 	      PTR_DEMANGLE (cxafct);
 #endif
 	      cxafct (f->func.cxa.arg, status);
 	      break;
 	    }
+          /* It is possible that last exit function registered
+             more exit functions. Start the loop over. */
+          __libc_lock_lock (__exit_funcs_lock);
+          if (__builtin_expect (check != __new_exitfn_called, 0))
+            {
+              __libc_lock_unlock (__exit_funcs_lock);
+              goto restart;
+            }
+	  __libc_lock_unlock (__exit_funcs_lock);
 	}
 
-      *listp = cur->next;
-      if (*listp != NULL)
-	/* Don't free the last element in the chain, this is the statically
-	   allocate element.  */
-	free (cur);
-    }
+      __libc_lock_lock (__exit_funcs_lock);
+      if (__builtin_expect (check != __new_exitfn_called, 0))
+        {
+          __libc_lock_unlock (__exit_funcs_lock);
+          goto restart;
+        }
+      else
+        {
+          *listp = cur->next;
+          if (*listp != NULL)
+	  /* 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)
     RUN_HOOK (__libc_atexit, ());
-- 
1.7.10.4