This is the mail archive of the
libc-help@sourceware.org
mailing list for the glibc project.
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 */
>