Ping Re: [PATCH v2] Fix the race between atexit() and exit()

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

On 07/10/2012 10:51 AM, Peng Haitao wrote:
> exit() uses global variable __exit_funcs indirectly, which are not protected.
> It is not safe in multithread circumstance.
> 

Ping.

-- 
Best Regards,
Peng

> 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           |   16 +++++++++++
>  stdlib/cxa_atexit.c |   16 +++++++----
>  stdlib/exit.c       |   74 ++++++++++++++++++++++++++++++++++++++++-----------
>  stdlib/exit.h       |   11 +++++---
>  4 files changed, 93 insertions(+), 24 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index c7070c5..613ce75 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,19 @@
> +2012-07-10  Peng Haitao  <penght@cn.fujitsu.com>
> +
> +	[BZ #14333]
> +	* stdlib/cxa_atexit.c: Do not include bits/libc-lock.h.
> +	(__libc_lock_define_initialized): Parameter 'lock' changed to global
> +	lock '__exit_funcs_lock'.
> +	(__new_exitfn): Fail new registration if exit code finished processing
> +	all handlers.
> +	* stdlib/exit.c: Include atomic.h.
> +	(__exit_funcs_done): New flag to indicate status of list traversal.
> +	(exit): Changes the handler processing style to that of
> +	__cxa_finalize, with locking.
> +	* stdlib/exit.h: Include bits/libc-lock.h.
> +	(__exit_funcs_lock): Declaration for external access.
> +	(__exit_funcs_done): Declaration for external access.
> +
>  2012-07-09  Roland McGrath  <roland@hack.frob.com>
>  
>  	[BZ #14336]
> diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
> index 9704398..24f110e 100644
> --- a/stdlib/cxa_atexit.c
> +++ b/stdlib/cxa_atexit.c
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 1999,2001,2002,2005,2006,2009 Free Software Foundation, Inc.
> +/* Copyright (C) 1999-2012 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
> @@ -18,7 +18,6 @@
>  #include <assert.h>
>  #include <stdlib.h>
>  
> -#include <bits/libc-lock.h>
>  #include "exit.h"
>  #include <atomic.h>
>  #include <sysdep.h>
> @@ -60,7 +59,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;
> @@ -75,7 +74,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)
> +  {
> +    /* exit code finished processing all handlers, so fail registration.  */
> +    __libc_lock_unlock (__exit_funcs_lock);
> +    return NULL;
> +  }
>  
>    for (l = *listp; l != NULL; p = l, l = l->next)
>      {
> @@ -126,7 +132,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..d686de5 100644
> --- a/stdlib/exit.c
> +++ b/stdlib/exit.c
> @@ -1,5 +1,4 @@
> -/* Copyright (C) 1991,95,96,97,99,2001,2002,2005,2009
> -   Free Software Foundation, Inc.
> +/* Copyright (C) 1991-2012 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
> @@ -20,11 +19,14 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <sysdep.h>
> +#include <atomic.h>
>  #include "exit.h"
>  
>  #include "set-hooks.h"
>  DEFINE_HOOK (__libc_atexit, (void))
>  
> +/* initialize 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
> @@ -38,25 +40,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;
>  
> -      while (cur->idx > 0)
> -	{
> -	  const struct exit_function *const f =
> -	    &cur->fns[--cur->idx];
> -	  switch (f->flavor)
> +restart:
> +      __libc_lock_lock (__exit_funcs_lock);
> +
> +      cur = *listp;
> +      if (cur == NULL)
> +        {
> +          /* mark the processing complete,
> +             we won't allow to register more handlers.  */
> +          __exit_funcs_done = true;
> +          __libc_lock_unlock (__exit_funcs_lock);
> +          break;
> +        }
> +
> +      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 +85,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 +93,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, ());
> diff --git a/stdlib/exit.h b/stdlib/exit.h
> index 818c7ac..25a398e 100644
> --- a/stdlib/exit.h
> +++ b/stdlib/exit.h
> @@ -21,6 +21,7 @@
>  
>  #include <stdbool.h>
>  #include <stdint.h>
> +#include <bits/libc-lock.h>
>  
>  enum
>  {
> @@ -66,12 +67,16 @@ extern uint64_t __new_exitfn_called attribute_hidden;
>  
>  extern void __run_exit_handlers (int status, struct exit_function_list **listp,
>  				 bool run_list_atexit)
> -  attribute_hidden __attribute__ ((__noreturn__));
> +	  attribute_hidden __attribute__ ((__noreturn__));
>  
>  extern int __internal_atexit (void (*func) (void *), void *arg, void *d,
> -			      struct exit_function_list **listp)
> -  attribute_hidden;
> +      struct exit_function_list **listp)
> +	  attribute_hidden;
>  extern int __cxa_at_quick_exit (void (*func) (void *), void *d);
>  
> +__libc_lock_define (extern, __exit_funcs_lock);
> +
> +/* flag to mark that all registered atexit/on_exit handlers are called.  */
> +extern bool __exit_funcs_done;
>  
>  #endif	/* exit.h  */
> 

-- 
Best Regards,
Peng