This is the mail archive of the libc-help@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Carlos,

Do you want me to post this to libc-alpha again?

Cheers,
- Anoop.

Anoop wrote:
> Carlos O'Donell wrote:
>> On Tue, Jun 10, 2008 at 8:25 AM, Anoop <acv@linux.vnet.ibm.com> wrote:
>>> Please see https://www.opengroup.org/austin/mailarchives/ag/ for the
>>> discussions that happened
>>> on the opengroup list.
>>
>> Interesting discussion.
>>
>>> The result of this discussion as I would infer is that exit() and
>>> atexit()
>>> need to be safe with each other in the matter of keeping the list
>>> consistent.
>>> But it CAN'T be guaranteed that, if atexit() is called, 'during or
>>> after'
>>> exit() is called,
>>> from a different thread, the handler that atexit() tries to register
>>> will be
>>> invoked by exit().
>>
>> I agree.
>>
>>> The proposed patch does 2 things -
>>> 1. Keeps the list in a consistent state by protecting it with lock
>>> 2. Fails the atexit() registrations after exit() finishes processing the
>>> list.
>>
>> That sounds reasonable.
>>
>>> Accommodating as many handlers as possible is the important and right
>>> thing
>>> to do. But you cant
>>> avoid the "visibility problem" where a new list head appears only after
>>> exit() processing is over.
>>> This problem should be taken care of, more by an application
>>> programmer than
>>> by glibc.
>>> Making the registration fail may not be of any worth as the thread
>>> might not
>>> get a chance to take
>>> any action afterwards, but from glibc perspective this is just for
>>> the sake
>>> of keeping with the standard.
>>
>> Where's the patch?
>>
>> Cheers,
>> Carlos.
> 
> I had it posted in libc-alpha -
> http://sources.redhat.com/ml/libc-alpha/2008-06/msg00002.html
> Posting it here as well. (Same approach as cxa_finilize)
> 
> 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  */
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]