Fix for bz14333 -- race between atexit() and exit()
Commit Message
On Mon, Jul 24, 2017 at 1:14 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> Kind of, I guess. When you write that __exit_funcs_lock protects
>> __exit_funcs, do you mean that it also protects the full list that this
>> global points to? If so, please say that.
>
> Yes, Will do.
Done.
>> Does that fully remove the need for what looks like an (incorrect)
>> attempt to build a concurrent list?
>
> Probably. Let me review these. I suspect they are no longer necessary.
That is correct: the half-cooked atomic accesses are no longer
necessary, since all modifications (and reads) now happen under the
lock. I've removed them.
2017-07-31 Paul Pluzhnikov <ppluzhnikov@google.com>
Ricky Zhou <rickyz@google.com>
Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
[BZ #14333]
* stdlib/cxa_atexit.c (__internal_atexit): Use __exit_funcs_lock.
Remove atomics.
(__new_exitfn): Fail registration when we finished at_exit processing.
* stdlib/cxa_finalize.c (__cxa_finalize): Likewise.
* stdlib/on_exit.c (__on_exit): Likewise.
* stdlib/exit.c (__exit_funcs_done): New variable.
(__run_exit_handlers): Use __exit_funcs_lock.
* stdlib/exit.h (__exit_funcs_lock, __exit_funcs_done): New
declarations.
* stdlib/Makefile (test-atexit-race, test-quick_at_exit-race)
(test-cxa_atexit-race): New tests.
* stdlib/test-atexit-race-common.c: New.
* stdlib/test-atexit-race.c: New.
* stdlib/test-at_quick_exit-race.c: New.
* stdlib/test-cxa_atexit-race.c: New.
Comments
On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
<ppluzhnikov@google.com> wrote:
> 2017-07-31 Paul Pluzhnikov <ppluzhnikov@google.com>
> Ricky Zhou <rickyz@google.com>
> Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>
> [BZ #14333]
> * stdlib/cxa_atexit.c (__internal_atexit): Use __exit_funcs_lock.
> Remove atomics.
> (__new_exitfn): Fail registration when we finished at_exit processing.
> * stdlib/cxa_finalize.c (__cxa_finalize): Likewise.
> * stdlib/on_exit.c (__on_exit): Likewise.
> * stdlib/exit.c (__exit_funcs_done): New variable.
> (__run_exit_handlers): Use __exit_funcs_lock.
> * stdlib/exit.h (__exit_funcs_lock, __exit_funcs_done): New
> declarations.
> * stdlib/Makefile (test-atexit-race, test-quick_at_exit-race)
> (test-cxa_atexit-race): New tests.
> * stdlib/test-atexit-race-common.c: New.
> * stdlib/test-atexit-race.c: New.
> * stdlib/test-at_quick_exit-race.c: New.
> * stdlib/test-cxa_atexit-race.c: New.
Ping?
(Re-sending as plain text. Sorry if you got duplicate.)
On Mon, Aug 7, 2017 at 12:28 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
> <ppluzhnikov@google.com> wrote:
>
>> 2017-07-31 Paul Pluzhnikov <ppluzhnikov@google.com>
>> Ricky Zhou <rickyz@google.com>
>> Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>>
>> [BZ #14333]
>> * stdlib/cxa_atexit.c (__internal_atexit): Use __exit_funcs_lock.
>> Remove atomics.
>> (__new_exitfn): Fail registration when we finished at_exit processing.
>> * stdlib/cxa_finalize.c (__cxa_finalize): Likewise.
>> * stdlib/on_exit.c (__on_exit): Likewise.
>> * stdlib/exit.c (__exit_funcs_done): New variable.
>> (__run_exit_handlers): Use __exit_funcs_lock.
>> * stdlib/exit.h (__exit_funcs_lock, __exit_funcs_done): New
>> declarations.
>> * stdlib/Makefile (test-atexit-race, test-quick_at_exit-race)
>> (test-cxa_atexit-race): New tests.
>> * stdlib/test-atexit-race-common.c: New.
>> * stdlib/test-atexit-race.c: New.
>> * stdlib/test-at_quick_exit-race.c: New.
>> * stdlib/test-cxa_atexit-race.c: New.
>
> Ping?
Ping x2?
On Mon, Aug 28, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Mon, Aug 7, 2017 at 12:28 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
>> <ppluzhnikov@google.com> wrote:
>>
>>> 2017-07-31 Paul Pluzhnikov <ppluzhnikov@google.com>
>>> Ricky Zhou <rickyz@google.com>
>>> Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>>>
>>> [BZ #14333]
>>
>> Ping?
>
> Ping x2?
Ping x3?
On Wed, Sep 6, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Mon, Aug 28, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> On Mon, Aug 7, 2017 at 12:28 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>>> On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
>>> <ppluzhnikov@google.com> wrote:
>>>
>>>> 2017-07-31 Paul Pluzhnikov <ppluzhnikov@google.com>
>>>> Ricky Zhou <rickyz@google.com>
>>>> Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>>>>
>>>> [BZ #14333]
>>>
>>> Ping?
>>
>> Ping x2?
>
> Ping x3?
Ping x4?
On 09/13/2017 10:41 AM, Paul Pluzhnikov wrote:
> On Wed, Sep 6, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> On Mon, Aug 28, 2017 at 7:59 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>>> On Mon, Aug 7, 2017 at 12:28 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>>>> On Mon, Jul 31, 2017 at 11:05 AM, Paul Pluzhnikov
>>>> <ppluzhnikov@google.com> wrote:
>>>>
>>>>> 2017-07-31 Paul Pluzhnikov <ppluzhnikov@google.com>
>>>>> Ricky Zhou <rickyz@google.com>
>>>>> Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>>>>>
>>>>> [BZ #14333]
>>>>
>>>> Ping?
>>>
>>> Ping x2?
>>
>> Ping x3?
>
> Ping x4?
I'm reviewing this and I'm about 75% done. Should post the review tomorrow.
On 07/31/2017 01:05 PM, Paul Pluzhnikov wrote:
> On Mon, Jul 24, 2017 at 1:14 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
>>> Kind of, I guess. When you write that __exit_funcs_lock protects
>>> __exit_funcs, do you mean that it also protects the full list that this
>>> global points to? If so, please say that.
>> Yes, Will do.
> Done.
>
>>> Does that fully remove the need for what looks like an (incorrect)
>>> attempt to build a concurrent list?
>> Probably. Let me review these. I suspect they are no longer necessary.
> That is correct: the half-cooked atomic accesses are no longer
> necessary, since all modifications (and reads) now happen under the
> lock. I've removed them.
This is looking awesome.
Thank you for the cleanup.
(1) Design:
I think the design is better now. We removed the half-cooked concurrent
list access and are using a single lock to order exit function access.
That's the best solution for right now and it looks good.
(2) Implementation:
The implementation looks good to me. You've removed the atomic accesses,
and atomic.h includes, and cleaned up the lock usage.
(3) Details:
We have some remaining documentation details which I'll help you work
through. I've provided notes below along with suggestions.
I think the next version will be ready to commit.
> 2017-07-31 Paul Pluzhnikov <ppluzhnikov@google.com>
> Ricky Zhou <rickyz@google.com>
> Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
>
> [BZ #14333]
> * stdlib/cxa_atexit.c (__internal_atexit): Use __exit_funcs_lock.
> Remove atomics.
> (__new_exitfn): Fail registration when we finished at_exit processing.
> * stdlib/cxa_finalize.c (__cxa_finalize): Likewise.
> * stdlib/on_exit.c (__on_exit): Likewise.
> * stdlib/exit.c (__exit_funcs_done): New variable.
> (__run_exit_handlers): Use __exit_funcs_lock.
> * stdlib/exit.h (__exit_funcs_lock, __exit_funcs_done): New
> declarations.
> * stdlib/Makefile (test-atexit-race, test-quick_at_exit-race)
> (test-cxa_atexit-race): New tests.
> * stdlib/test-atexit-race-common.c: New.
> * stdlib/test-atexit-race.c: New.
> * stdlib/test-at_quick_exit-race.c: New.
> * stdlib/test-cxa_atexit-race.c: New.
>
>
>
>
> -- Paul Pluzhnikov
>
>
> glibc-bz14333-20170731.txt
>
>
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 0314d5926b..c768b17cd4 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -80,7 +80,8 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \
> tst-strtol-locale tst-strtod-nan-locale tst-strfmon_l \
> tst-quick_exit tst-thread-quick_exit tst-width \
> tst-width-stdint tst-strfrom tst-strfrom-locale \
> - tst-getrandom
> + tst-getrandom test-atexit-race test-at_quick_exit-race \
> + test-cxa_atexit-race
> tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
> tst-tls-atexit tst-tls-atexit-nodelete
> tests-static := tst-secure-getenv
> @@ -89,6 +90,10 @@ ifeq ($(build-hardcoded-path-in-tests),yes)
> tests += tst-empty-env
> endif
>
> +LDLIBS-test-atexit-race = $(shared-thread-library)
> +LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
> +LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
> +
> ifeq ($(have-cxx-thread_local),yes)
> CFLAGS-tst-quick_exit.o = -std=c++11
> LDLIBS-tst-quick_exit = -lstdc++
> diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
> index ce5d9f22b4..10b74d2982 100644
> --- a/stdlib/cxa_atexit.c
> +++ b/stdlib/cxa_atexit.c
> @@ -21,21 +21,29 @@
>
> #include <libc-lock.h>
> #include "exit.h"
> -#include <atomic.h>
OK. Good, remove the atomic types because we are switching to locking.
> #include <sysdep.h>
>
> #undef __cxa_atexit
>
> +/* We change global data, so we need locking. */
This is a low quality comment. It should reference the master
definition of the lock.
Suggest:
/* See concurrency notes in stdlib/exit.h where this lock is defined. */
> +__libc_lock_define_initialized (, __exit_funcs_lock)
> +
>
OK.
> int
> attribute_hidden
> __internal_atexit (void (*func) (void *), void *arg, void *d,
> struct exit_function_list **listp)
> {
> - struct exit_function *new = __new_exitfn (listp);
> + struct exit_function *new;
> +
> + __libc_lock_lock (__exit_funcs_lock);
OK. Take the lock because __new_exitfn() manipulates, or even
allocates a new list.
> + new = __new_exitfn (listp);
>
> if (new == NULL)
> - return -1;
> + {
> + __libc_lock_unlock (__exit_funcs_lock);
> + return -1;
> + }
>
> #ifdef PTR_MANGLE
> PTR_MANGLE (func);
> @@ -43,8 +51,8 @@ __internal_atexit (void (*func) (void *), void *arg, void *d,
> new->func.cxa.fn = (void (*) (void *, int)) func;
> new->func.cxa.arg = arg;
> new->func.cxa.dso_handle = d;
> - atomic_write_barrier ();
> new->flavor = ef_cxa;
> + __libc_lock_unlock (__exit_funcs_lock);
> return 0;
> }
>
> @@ -60,10 +68,6 @@ __cxa_atexit (void (*func) (void *), void *arg, void *d)
> libc_hidden_def (__cxa_atexit)
>
>
> -/* We change global data, so we need locking. */
> -__libc_lock_define_initialized (static, lock)
> -
OK.
> -
> static struct exit_function_list initial;
> struct exit_function_list *__exit_funcs = &initial;
> uint64_t __new_exitfn_called;
> @@ -76,7 +80,10 @@ __new_exitfn (struct exit_function_list **listp)
You need a comment for __new_exitfn that says it must be called with
__exit_funcs_lock held.
> struct exit_function *r = NULL;
> size_t i = 0;
>
> - __libc_lock_lock (lock);
> + if (__exit_funcs_done)
> + /* exit code finished processing all handlers
> + so fail this registration */
Full sentence please.
Suggest:
/* Exit code is finished processing all registered exit functions,
therefore we fail this registration. */
> + return NULL;
>
> for (l = *listp; l != NULL; p = l, l = l->next)
> {
> @@ -127,7 +134,5 @@ __new_exitfn (struct exit_function_list **listp)
> ++__new_exitfn_called;
> }
>
> - __libc_lock_unlock (lock);
> -
OK.
> return r;
> }
> diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c
> index aa0a70cb58..2216a3d87e 100644
> --- a/stdlib/cxa_finalize.c
> +++ b/stdlib/cxa_finalize.c
> @@ -17,7 +17,6 @@
>
> #include <assert.h>
> #include <stdlib.h>
> -#include <atomic.h>
> #include "exit.h"
> #include <fork.h>
> #include <sysdep.h>
> @@ -31,36 +30,35 @@ __cxa_finalize (void *d)
> {
> struct exit_function_list *funcs;
>
> + __libc_lock_lock (__exit_funcs_lock);
> +
OK. I like how you have moved the locking to the entry points to avoid
any problems with overlap of the functions.
> restart:
> for (funcs = __exit_funcs; funcs; funcs = funcs->next)
> {
> struct exit_function *f;
>
> for (f = &funcs->fns[funcs->idx - 1]; f >= &funcs->fns[0]; --f)
> - {
> - void (*cxafn) (void *arg, int status);
> - void *cxaarg;
> + if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
> + {
> + const uint64_t check = __new_exitfn_called;
> + void (*cxafn) (void *arg, int status) = f->func.cxa.fn;
> + void *cxaarg = f->func.cxa.arg;
>
> - if ((d == NULL || d == f->func.cxa.dso_handle)
> - /* We don't want to run this cleanup more than once. */
> - && (cxafn = f->func.cxa.fn,
> - cxaarg = f->func.cxa.arg,
> - ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
> - ef_cxa)))
> - {
> - uint64_t check = __new_exitfn_called;
> + /* We don't want to run this cleanup more than once. */
> + f->flavor = ef_free;
OK.
>
> #ifdef PTR_DEMANGLE
> - PTR_DEMANGLE (cxafn);
> + PTR_DEMANGLE (cxafn);
> #endif
> - cxafn (cxaarg, 0);
> + __libc_lock_unlock (__exit_funcs_lock);
> + cxafn (cxaarg, 0);
> + __libc_lock_lock (__exit_funcs_lock);
OK. Unlock before calling foreign function.
>
> - /* It is possible that that last exit function registered
> - more exit functions. Start the loop over. */
> - if (__glibc_unlikely (check != __new_exitfn_called))
> - goto restart;
> - }
> - }
> + /* It is possible that that last exit function registered
> + more exit functions. Start the loop over. */
> + if (__glibc_unlikely (check != __new_exitfn_called))
> + goto restart;
> + }
OK.
> }
>
> /* Also remove the quick_exit handlers, but do not call them. */
> @@ -79,4 +77,5 @@ __cxa_finalize (void *d)
> if (d != NULL)
> UNREGISTER_ATFORK (d);
> #endif
> + __libc_lock_unlock (__exit_funcs_lock);
OK.
> }
> diff --git a/stdlib/exit.c b/stdlib/exit.c
> index c0b6d666c7..69acef5c23 100644
> --- a/stdlib/exit.c
> +++ b/stdlib/exit.c
> @@ -19,11 +19,14 @@
> #include <stdlib.h>
> #include <unistd.h>
> #include <sysdep.h>
> +#include <libc-lock.h>
OK.
> #include "exit.h"
>
> #include "set-hooks.h"
> DEFINE_HOOK (__libc_atexit, (void))
>
> +/* Initialise the processing complete flag to false. */
> +bool __exit_funcs_done = false;
Suggest:
/* Initialize the flag that indicates exit function processing
is complete. See concurrency notes in stdlib/exit.h where
__exit_funcs_lock is defined. */
>
> /* Call all functions registered with `atexit' and `on_exit',
> in the reverse of the order in which they were registered
> @@ -44,14 +47,32 @@ __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 (true)
OK.
> {
> - struct exit_function_list *cur = *listp;
> + struct exit_function_list *cur;
> +
> + __libc_lock_lock (__exit_funcs_lock);
OK.
> +
> + restart:
> + cur = *listp;
> +
> + if (cur == NULL)
> + {
> + /* Exit processing complete. We will not allow any more
> + atexit/on_exit registrations. */
> + __exit_funcs_done = true;
> + __libc_lock_unlock (__exit_funcs_lock);
OK.
> + break;
> + }
>
> while (cur->idx > 0)
> {
> const struct exit_function *const f =
> &cur->fns[--cur->idx];
> + const uint64_t new_exitfn_called = __new_exitfn_called;
> +
> + /* Unlock the list while we call into user-provided code. */
The common phrase is "Don't call foreign functions with locks held."
Suggest:
/* Unlock the list while we call foreign functions. */
> + __libc_lock_unlock (__exit_funcs_lock);
> switch (f->flavor)
> {
> void (*atfct) (void);
> @@ -83,6 +104,13 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
> cxafct (f->func.cxa.arg, status);
> break;
> }
> + /* Re-lock again before looking at global state. */
> + __libc_lock_lock (__exit_funcs_lock);
> +
> + if (__glibc_unlikely (new_exitfn_called != __new_exitfn_called))
> + /* The last exit function, or another thread, has registered
> + more exit functions. Start the loop over. */
> + goto restart;
OK.
> }
>
> *listp = cur->next;
> @@ -90,6 +118,8 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
> /* Don't free the last element in the chain, this is the statically
> allocate element. */
> free (cur);
> +
> + __libc_lock_unlock (__exit_funcs_lock);
OK.
> }
>
> if (run_list_atexit)
> diff --git a/stdlib/exit.h b/stdlib/exit.h
> index 7f2e679246..700163c8be 100644
> --- a/stdlib/exit.h
> +++ b/stdlib/exit.h
> @@ -20,6 +20,7 @@
>
> #include <stdbool.h>
> #include <stdint.h>
> +#include <libc-lock.h>
OK.
>
> enum
> {
> @@ -57,11 +58,26 @@ struct exit_function_list
> size_t idx;
> struct exit_function fns[32];
> };
> +
> extern struct exit_function_list *__exit_funcs attribute_hidden;
> extern struct exit_function_list *__quick_exit_funcs attribute_hidden;
> +extern uint64_t __new_exitfn_called attribute_hidden;
OK.
> +
> +/* True once all registered atexit/at_quick_exit/onexit handlers have been
> + called */
> +extern bool __exit_funcs_done attribute_hidden;
> +
> +/* This lock protects __exit_funcs, __quick_exit_funcs, __exit_funcs_done
> + and __new_exitfn_called globals against simultaneous access from
> + atexit/on_exit/at_quick_exit in multiple threads, and also from
> + simultaneous access while another thread is in the middle of calling
> + exit handlers. See BZ#14333. Note: for lists, the entire list is
> + protected by this lock. */
> +__libc_lock_define (extern, __exit_funcs_lock);
Suggest a slight adjustment:
... Note: for lists, the entire list, and each associated entry in the list,
is protected from any access by this lock.
> +
>
> extern struct exit_function *__new_exitfn (struct exit_function_list **listp);
> -extern uint64_t __new_exitfn_called attribute_hidden;
> +
>
> extern void __run_exit_handlers (int status,
> struct exit_function_list **listp,
> diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c
> index 83845e76d8..f4ede2b1a7 100644
> --- a/stdlib/on_exit.c
> +++ b/stdlib/on_exit.c
> @@ -17,25 +17,30 @@
>
> #include <stdlib.h>
> #include "exit.h"
> -#include <atomic.h>
OK.
> #include <sysdep.h>
>
> /* Register a function to be called by exit. */
> int
> __on_exit (void (*func) (int status, void *arg), void *arg)
> {
> - struct exit_function *new = __new_exitfn (&__exit_funcs);
> + struct exit_function *new;
> +
> + __libc_lock_lock (__exit_funcs_lock);
> + new = __new_exitfn (&__exit_funcs);
OK.
>
> if (new == NULL)
> - return -1;
> + {
> + __libc_lock_unlock (__exit_funcs_lock);
> + return -1;
OK.
> + }
>
> #ifdef PTR_MANGLE
> PTR_MANGLE (func);
> #endif
> new->func.on.fn = func;
> new->func.on.arg = arg;
> - atomic_write_barrier ();
> new->flavor = ef_on;
> + __libc_lock_unlock (__exit_funcs_lock);
OK.
> return 0;
> }
> weak_alias (__on_exit, on_exit)
> diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
> new file mode 100644
> index 0000000000..2521a6b77c
> --- /dev/null
> +++ b/stdlib/test-at_quick_exit-race.c
> @@ -0,0 +1,30 @@
> +/* A test for at_quick_exit/quick_exit race from bz14333.
Suggest:
Bug 14333: A test for at_quick_exit/quick_exit race.
> +
> + Copyright (C) 2017 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
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +/* This file must be run from within a directory called "stdlib". */
> +
Needs a comment explaining what this specific test is looking for and
what are the expected results.
> +#define CALL_ATEXIT at_quick_exit (&no_op)
> +#define CALL_EXIT quick_exit (0)
> +
> +static void
> +no_op (void)
> +{
> +}
> +
> +#include <stdlib/test-atexit-race-common.c>
OK.
> diff --git a/stdlib/test-atexit-race-common.c b/stdlib/test-atexit-race-common.c
> new file mode 100644
> index 0000000000..c4cbd9e592
> --- /dev/null
> +++ b/stdlib/test-atexit-race-common.c
> @@ -0,0 +1,62 @@
> +/* Support file for atexit/exit, at_quick_exit/quick_exit, etc. race tests
> + from bz14333.
Suggest:
Bug 14333: Support file for atexit/exit, at_quick_exit/quick_exit, etc. race tests.
> +
> + Copyright (C) 2017 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
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +/* This file must be run from within a directory called "stdlib". */
> +
> +#if !defined(CALL_EXIT) || !defined(CALL_ATEXIT)
> +#error Must define CALL_EXIT and CALL_ATEXIT before using this file.
> +#endif
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <pthread.h>
> +
> +const size_t kNumThreads = 1024;
> +const size_t kNumHandlers = 1024;
> +
> +static void *
> +threadfunc (void *unused)
> +{
> + size_t i;
> + for (i = 0; i < kNumHandlers; ++i) {
> + CALL_ATEXIT;
> + }
> + return NULL;
> +}
> +
Needs a comment explaining what this is doing and why.
> +static int
> +do_test (void)
> +{
> + size_t i;
> + pthread_t thr;
> + pthread_attr_t attr;
> +
> + pthread_attr_init (&attr);
> + pthread_attr_setdetachstate (&attr, 1);
Use xpthread_* variants.
> +
> + for (i = 0; i < kNumThreads; ++i) {
> + pthread_create (&thr, &attr, threadfunc, NULL);
Likewise.
> + }
> +
> + CALL_EXIT;
> +}
> +
> +#define TEST_FUNCTION do_test
> +#include <support/test-driver.c>
> diff --git a/stdlib/test-atexit-race.c b/stdlib/test-atexit-race.c
> new file mode 100644
> index 0000000000..b183ecfd7e
> --- /dev/null
> +++ b/stdlib/test-atexit-race.c
> @@ -0,0 +1,30 @@
> +/* A test for atexit/exit race from bz14333.
Suggest:
Bug 14333: A test for atexit/exit race.
> +
> + Copyright (C) 2017 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
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +/* This file must be run from within a directory called "stdlib". */
> +
Needs a comment explaining what this test is doing and what are the expected
results.
> +#define CALL_ATEXIT atexit (&no_op)
> +#define CALL_EXIT exit (0)
> +
> +static void
> +no_op (void)
> +{
> +}
> +
> +#include <stdlib/test-atexit-race-common.c>
> diff --git a/stdlib/test-cxa_atexit-race.c b/stdlib/test-cxa_atexit-race.c
> new file mode 100644
> index 0000000000..b86f6ce212
> --- /dev/null
> +++ b/stdlib/test-cxa_atexit-race.c
> @@ -0,0 +1,34 @@
> +/* A test for __cxa_atexit/exit race from bz14333.
Suggest:
Bug 14333: A test for __cxa_atexit/exit race.
> +
> + Copyright (C) 2017 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
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +/* This file must be run from within a directory called "stdlib". */
> +
> +#include <stdio.h>
> +
Needs a comment explaining what this test is doing and why.
> +#define CALL_ATEXIT __cxa_atexit (&no_op, NULL, NULL)
> +#define CALL_EXIT exit (0)
> +
> +int __cxa_atexit (void (*func) (void *), void *arg, void *d);
> +
> +static void
> +no_op (void *ignored)
> +{
> +}
> +
> +#include <stdlib/test-atexit-race-common.c>
@@ -80,7 +80,8 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \
tst-strtol-locale tst-strtod-nan-locale tst-strfmon_l \
tst-quick_exit tst-thread-quick_exit tst-width \
tst-width-stdint tst-strfrom tst-strfrom-locale \
- tst-getrandom
+ tst-getrandom test-atexit-race test-at_quick_exit-race \
+ test-cxa_atexit-race
tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
tst-tls-atexit tst-tls-atexit-nodelete
tests-static := tst-secure-getenv
@@ -89,6 +90,10 @@ ifeq ($(build-hardcoded-path-in-tests),yes)
tests += tst-empty-env
endif
+LDLIBS-test-atexit-race = $(shared-thread-library)
+LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
+LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
+
ifeq ($(have-cxx-thread_local),yes)
CFLAGS-tst-quick_exit.o = -std=c++11
LDLIBS-tst-quick_exit = -lstdc++
@@ -21,21 +21,29 @@
#include <libc-lock.h>
#include "exit.h"
-#include <atomic.h>
#include <sysdep.h>
#undef __cxa_atexit
+/* We change global data, so we need locking. */
+__libc_lock_define_initialized (, __exit_funcs_lock)
+
int
attribute_hidden
__internal_atexit (void (*func) (void *), void *arg, void *d,
struct exit_function_list **listp)
{
- struct exit_function *new = __new_exitfn (listp);
+ struct exit_function *new;
+
+ __libc_lock_lock (__exit_funcs_lock);
+ new = __new_exitfn (listp);
if (new == NULL)
- return -1;
+ {
+ __libc_lock_unlock (__exit_funcs_lock);
+ return -1;
+ }
#ifdef PTR_MANGLE
PTR_MANGLE (func);
@@ -43,8 +51,8 @@ __internal_atexit (void (*func) (void *), void *arg, void *d,
new->func.cxa.fn = (void (*) (void *, int)) func;
new->func.cxa.arg = arg;
new->func.cxa.dso_handle = d;
- atomic_write_barrier ();
new->flavor = ef_cxa;
+ __libc_lock_unlock (__exit_funcs_lock);
return 0;
}
@@ -60,10 +68,6 @@ __cxa_atexit (void (*func) (void *), void *arg, void *d)
libc_hidden_def (__cxa_atexit)
-/* We change global data, so we need locking. */
-__libc_lock_define_initialized (static, lock)
-
-
static struct exit_function_list initial;
struct exit_function_list *__exit_funcs = &initial;
uint64_t __new_exitfn_called;
@@ -76,7 +80,10 @@ __new_exitfn (struct exit_function_list **listp)
struct exit_function *r = NULL;
size_t i = 0;
- __libc_lock_lock (lock);
+ if (__exit_funcs_done)
+ /* exit code finished processing all handlers
+ so fail this registration */
+ return NULL;
for (l = *listp; l != NULL; p = l, l = l->next)
{
@@ -127,7 +134,5 @@ __new_exitfn (struct exit_function_list **listp)
++__new_exitfn_called;
}
- __libc_lock_unlock (lock);
-
return r;
}
@@ -17,7 +17,6 @@
#include <assert.h>
#include <stdlib.h>
-#include <atomic.h>
#include "exit.h"
#include <fork.h>
#include <sysdep.h>
@@ -31,36 +30,35 @@ __cxa_finalize (void *d)
{
struct exit_function_list *funcs;
+ __libc_lock_lock (__exit_funcs_lock);
+
restart:
for (funcs = __exit_funcs; funcs; funcs = funcs->next)
{
struct exit_function *f;
for (f = &funcs->fns[funcs->idx - 1]; f >= &funcs->fns[0]; --f)
- {
- void (*cxafn) (void *arg, int status);
- void *cxaarg;
+ if ((d == NULL || d == f->func.cxa.dso_handle) && f->flavor == ef_cxa)
+ {
+ const uint64_t check = __new_exitfn_called;
+ void (*cxafn) (void *arg, int status) = f->func.cxa.fn;
+ void *cxaarg = f->func.cxa.arg;
- if ((d == NULL || d == f->func.cxa.dso_handle)
- /* We don't want to run this cleanup more than once. */
- && (cxafn = f->func.cxa.fn,
- cxaarg = f->func.cxa.arg,
- ! catomic_compare_and_exchange_bool_acq (&f->flavor, ef_free,
- ef_cxa)))
- {
- uint64_t check = __new_exitfn_called;
+ /* We don't want to run this cleanup more than once. */
+ f->flavor = ef_free;
#ifdef PTR_DEMANGLE
- PTR_DEMANGLE (cxafn);
+ PTR_DEMANGLE (cxafn);
#endif
- cxafn (cxaarg, 0);
+ __libc_lock_unlock (__exit_funcs_lock);
+ cxafn (cxaarg, 0);
+ __libc_lock_lock (__exit_funcs_lock);
- /* It is possible that that last exit function registered
- more exit functions. Start the loop over. */
- if (__glibc_unlikely (check != __new_exitfn_called))
- goto restart;
- }
- }
+ /* It is possible that that last exit function registered
+ more exit functions. Start the loop over. */
+ if (__glibc_unlikely (check != __new_exitfn_called))
+ goto restart;
+ }
}
/* Also remove the quick_exit handlers, but do not call them. */
@@ -79,4 +77,5 @@ __cxa_finalize (void *d)
if (d != NULL)
UNREGISTER_ATFORK (d);
#endif
+ __libc_lock_unlock (__exit_funcs_lock);
}
@@ -19,11 +19,14 @@
#include <stdlib.h>
#include <unistd.h>
#include <sysdep.h>
+#include <libc-lock.h>
#include "exit.h"
#include "set-hooks.h"
DEFINE_HOOK (__libc_atexit, (void))
+/* Initialise 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
@@ -44,14 +47,32 @@ __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 (true)
{
- struct exit_function_list *cur = *listp;
+ struct exit_function_list *cur;
+
+ __libc_lock_lock (__exit_funcs_lock);
+
+ restart:
+ cur = *listp;
+
+ if (cur == NULL)
+ {
+ /* Exit processing complete. We will not allow any more
+ atexit/on_exit registrations. */
+ __exit_funcs_done = true;
+ __libc_lock_unlock (__exit_funcs_lock);
+ break;
+ }
while (cur->idx > 0)
{
const struct exit_function *const f =
&cur->fns[--cur->idx];
+ const uint64_t new_exitfn_called = __new_exitfn_called;
+
+ /* Unlock the list while we call into user-provided code. */
+ __libc_lock_unlock (__exit_funcs_lock);
switch (f->flavor)
{
void (*atfct) (void);
@@ -83,6 +104,13 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
cxafct (f->func.cxa.arg, status);
break;
}
+ /* Re-lock again before looking at global state. */
+ __libc_lock_lock (__exit_funcs_lock);
+
+ if (__glibc_unlikely (new_exitfn_called != __new_exitfn_called))
+ /* The last exit function, or another thread, has registered
+ more exit functions. Start the loop over. */
+ goto restart;
}
*listp = cur->next;
@@ -90,6 +118,8 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
/* 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)
@@ -20,6 +20,7 @@
#include <stdbool.h>
#include <stdint.h>
+#include <libc-lock.h>
enum
{
@@ -57,11 +58,26 @@ struct exit_function_list
size_t idx;
struct exit_function fns[32];
};
+
extern struct exit_function_list *__exit_funcs attribute_hidden;
extern struct exit_function_list *__quick_exit_funcs attribute_hidden;
+extern uint64_t __new_exitfn_called attribute_hidden;
+
+/* True once all registered atexit/at_quick_exit/onexit handlers have been
+ called */
+extern bool __exit_funcs_done attribute_hidden;
+
+/* This lock protects __exit_funcs, __quick_exit_funcs, __exit_funcs_done
+ and __new_exitfn_called globals against simultaneous access from
+ atexit/on_exit/at_quick_exit in multiple threads, and also from
+ simultaneous access while another thread is in the middle of calling
+ exit handlers. See BZ#14333. Note: for lists, the entire list is
+ protected by this lock. */
+__libc_lock_define (extern, __exit_funcs_lock);
+
extern struct exit_function *__new_exitfn (struct exit_function_list **listp);
-extern uint64_t __new_exitfn_called attribute_hidden;
+
extern void __run_exit_handlers (int status,
struct exit_function_list **listp,
@@ -17,25 +17,30 @@
#include <stdlib.h>
#include "exit.h"
-#include <atomic.h>
#include <sysdep.h>
/* Register a function to be called by exit. */
int
__on_exit (void (*func) (int status, void *arg), void *arg)
{
- struct exit_function *new = __new_exitfn (&__exit_funcs);
+ struct exit_function *new;
+
+ __libc_lock_lock (__exit_funcs_lock);
+ new = __new_exitfn (&__exit_funcs);
if (new == NULL)
- return -1;
+ {
+ __libc_lock_unlock (__exit_funcs_lock);
+ return -1;
+ }
#ifdef PTR_MANGLE
PTR_MANGLE (func);
#endif
new->func.on.fn = func;
new->func.on.arg = arg;
- atomic_write_barrier ();
new->flavor = ef_on;
+ __libc_lock_unlock (__exit_funcs_lock);
return 0;
}
weak_alias (__on_exit, on_exit)
new file mode 100644
@@ -0,0 +1,30 @@
+/* A test for at_quick_exit/quick_exit race from bz14333.
+
+ Copyright (C) 2017 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
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+/* This file must be run from within a directory called "stdlib". */
+
+#define CALL_ATEXIT at_quick_exit (&no_op)
+#define CALL_EXIT quick_exit (0)
+
+static void
+no_op (void)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>
new file mode 100644
@@ -0,0 +1,62 @@
+/* Support file for atexit/exit, at_quick_exit/quick_exit, etc. race tests
+ from bz14333.
+
+ Copyright (C) 2017 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
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+/* This file must be run from within a directory called "stdlib". */
+
+#if !defined(CALL_EXIT) || !defined(CALL_ATEXIT)
+#error Must define CALL_EXIT and CALL_ATEXIT before using this file.
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+const size_t kNumThreads = 1024;
+const size_t kNumHandlers = 1024;
+
+static void *
+threadfunc (void *unused)
+{
+ size_t i;
+ for (i = 0; i < kNumHandlers; ++i) {
+ CALL_ATEXIT;
+ }
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ size_t i;
+ pthread_t thr;
+ pthread_attr_t attr;
+
+ pthread_attr_init (&attr);
+ pthread_attr_setdetachstate (&attr, 1);
+
+ for (i = 0; i < kNumThreads; ++i) {
+ pthread_create (&thr, &attr, threadfunc, NULL);
+ }
+
+ CALL_EXIT;
+}
+
+#define TEST_FUNCTION do_test
+#include <support/test-driver.c>
new file mode 100644
@@ -0,0 +1,30 @@
+/* A test for atexit/exit race from bz14333.
+
+ Copyright (C) 2017 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
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+/* This file must be run from within a directory called "stdlib". */
+
+#define CALL_ATEXIT atexit (&no_op)
+#define CALL_EXIT exit (0)
+
+static void
+no_op (void)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>
new file mode 100644
@@ -0,0 +1,34 @@
+/* A test for __cxa_atexit/exit race from bz14333.
+
+ Copyright (C) 2017 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
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+/* This file must be run from within a directory called "stdlib". */
+
+#include <stdio.h>
+
+#define CALL_ATEXIT __cxa_atexit (&no_op, NULL, NULL)
+#define CALL_EXIT exit (0)
+
+int __cxa_atexit (void (*func) (void *), void *arg, void *d);
+
+static void
+no_op (void *ignored)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>