This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Ping Re: [PATCH v2] Fix the race between atexit() and exit()
- From: Peng Haitao <penght at cn dot fujitsu dot com>
- To: libc-alpha at sourceware dot org
- Date: Thu, 13 Sep 2012 13:58:18 +0800
- Subject: Ping Re: [PATCH v2] Fix the race between atexit() and exit()
- References: <20120709160210.5BC5E2C0B2@topped-with-meat.com> <1341888669-28624-1-git-send-email-penght@cn.fujitsu.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