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

[Anoop <acv at linux dot vnet dot ibm dot com>]

POSIX requires that at normal program termination, all functions
registered by the atexit() function shall be called. A race exits among
__cxa_atexit() and exit() on __exit_funcs can cause a successfully registered 
handler not to be called. I have a test-case which reveals this bug, which can 
be posted if required. The following patch solves this by extending the locking
used by __new_exitfn() to exit() and failing atexit() registrations once the 
exit() code completed traversing the __exit_funcs list.

Feedbacks appreciated.

Cheers
- Anoop

Signed-off-by: Anoop Vijayan <anoop.vijayan@in.ibm.com>

diff -Naurp glibc-2.7.orig/stdlib/cxa_atexit.c glibc-2.7/stdlib/cxa_atexit.c
--- glibc-2.7.orig/stdlib/cxa_atexit.c	2006-07-26 12:54:45.000000000 +0530
+++ glibc-2.7/stdlib/cxa_atexit.c	2008-06-03 02:36:57.000000000 +0530
@@ -51,7 +51,7 @@ 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;
@@ -66,7 +66,14 @@ __new_exitfn (void)
   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 = __exit_funcs; l != NULL; p = l, l = l->next)
     {
@@ -117,7 +124,7 @@ __new_exitfn (void)
       ++__new_exitfn_called;
     }

-  __libc_lock_unlock (lock);
+  __libc_lock_unlock (__exit_funcs_lock);

   return r;
 }
diff -Naurp glibc-2.7.orig/stdlib/exit.c glibc-2.7/stdlib/exit.c
--- glibc-2.7.orig/stdlib/exit.c	2005-12-18 23:01:14.000000000 +0530
+++ glibc-2.7/stdlib/exit.c	2008-06-03 04:19:52.000000000 +0530
@@ -18,13 +18,17 @@

 #include <stdio.h>
 #include <stdlib.h>
+#include <atomic.h>
 #include <unistd.h>
 #include <sysdep.h>
+#include <bits/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
@@ -36,53 +40,87 @@ exit (int status)
      the functions registered with `atexit' and `on_exit'. We call
      everyone on the list and use the status value in the last
      exit (). */
-  while (__exit_funcs != NULL)
+    while(1)
     {
-      struct exit_function_list *old;
-
-      while (__exit_funcs->idx > 0)
-	{
-	  const struct exit_function *const f =
-	    &__exit_funcs->fns[--__exit_funcs->idx];
-	  switch (f->flavor)
-	    {
-	      void (*atfct) (void);
-	      void (*onfct) (int status, void *arg);
-	      void (*cxafct) (void *arg, int status);
-
-	    case ef_free:
-	    case ef_us:
-	      break;
-	    case ef_on:
-	      onfct = f->func.on.fn;
+        struct exit_function_list *funcs;
+        struct exit_function *f;
+restart:
+        __libc_lock_lock (__exit_funcs_lock);
+        funcs = __exit_funcs;
+        if(funcs==NULL)
+        {
+            /* mark the processing complete
+               we wont allow to register more handlers */
+            __exit_funcs_done = 1;
+            __libc_lock_unlock (__exit_funcs_lock);
+            break;
+        }
+        f = &funcs->fns[funcs->idx - 1];
+        uint64_t check1 = __new_exitfn_called;
+        __libc_lock_unlock (__exit_funcs_lock);
+        for(; f >= &funcs->fns[0]; --f)
+        {
+            uint64_t check2 = __new_exitfn_called;
+            switch (f->flavor)
+            {
+                void (*atfct) (void);
+                void (*onfct) (int status, void *arg);
+                void (*cxafct) (void *arg, int status);
+                void *arg;
+
+                case ef_free:
+                                break;
+                case ef_us:
+                                goto restart;
+                case ef_on:
+                                onfct = f->func.on.fn;
+                                arg = f->func.on.arg;
+                while(atomic_compare_and_exchange_bool_acq (&f->flavor, 
ef_free, ef_on));
 #ifdef PTR_DEMANGLE
-	      PTR_DEMANGLE (onfct);
+                PTR_DEMANGLE (onfct);
 #endif
-	      onfct (status, f->func.on.arg);
-	      break;
-	    case ef_at:
-	      atfct = f->func.at;
+                                onfct (status, arg);
+                                break;
+                case ef_at:
+                                atfct = f->func.at;
+                while(atomic_compare_and_exchange_bool_acq (&f->flavor, 
ef_free, ef_at));
 #ifdef PTR_DEMANGLE
-	      PTR_DEMANGLE (atfct);
+                PTR_DEMANGLE (atfct);
 #endif
-	      atfct ();
-	      break;
-	    case ef_cxa:
-	      cxafct = f->func.cxa.fn;
+                                atfct ();
+                                break;
+                case ef_cxa:
+                                cxafct = f->func.cxa.fn;
+                                arg = f->func.cxa.arg;
+                while(atomic_compare_and_exchange_bool_acq (&f->flavor, 
ef_free, ef_cxa));
 #ifdef PTR_DEMANGLE
-	      PTR_DEMANGLE (cxafct);
+                PTR_DEMANGLE (cxafct);
 #endif
-	      cxafct (f->func.cxa.arg, status);
-	      break;
-	    }
-	}
-
-      old = __exit_funcs;
-      __exit_funcs = __exit_funcs->next;
-      if (__exit_funcs != NULL)
-	/* Don't free the last element in the chain, this is the statically
-	   allocate element.  */
-	free (old);
+                                cxafct (arg, status);
+                                break;
+            }
+
+            /* It is possible that that last exit function registered
+               more exit functions.  Start the loop over.  */
+            if (__builtin_expect (check2 != __new_exitfn_called, 0))
+                goto restart;
+        }
+
+        __libc_lock_lock (__exit_funcs_lock);
+        if (__builtin_expect (check1 != __new_exitfn_called, 0))
+        {
+            __libc_lock_unlock (__exit_funcs_lock);
+            goto restart;
+        }
+        else
+        {
+            __exit_funcs = __exit_funcs->next;
+            /* Don't free the last element in the chain, this is the statically
+               allocate element.  */
+            if(__exit_funcs != NULL)
+                free(funcs);
+            __libc_lock_unlock (__exit_funcs_lock);
+        }
     }

   RUN_HOOK (__libc_atexit, ());
diff -Naurp glibc-2.7.orig/stdlib/exit.h glibc-2.7/stdlib/exit.h
--- glibc-2.7.orig/stdlib/exit.h	2006-07-26 12:53:43.000000000 +0530
+++ glibc-2.7/stdlib/exit.h	2008-06-03 02:36:57.000000000 +0530
@@ -21,7 +21,7 @@
 #define _EXIT_H 1

 #include <stdint.h>
-
+#include <bits/libc-lock.h>
 enum
 {
   ef_free,	/* `ef_free' MUST be zero!  */
@@ -62,5 +62,9 @@ extern struct exit_function_list *__exit

 extern struct exit_function *__new_exitfn (void);
 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;

 #endif	/* exit.h  */