Fix for bz14333 -- race between atexit() and exit()
Commit Message
Greetings,
This patch is an update to previous patch by Anoop:
https://sourceware.org/ml/libc-alpha/2008-09/msg00013.html
See discussion in the above message.
Changes from the previous patch: I extended the lifetime of the lock
from __new_exitfn to its callers, since they *also* modify global
state.
Tested: "make check" on Linux/x86_64.
Also ran the test from
https://sourceware.org/bugzilla/show_bug.cgi?id=14333#c5 a few hundred
times.
2017-07-10 Paul Pluzhnikov <ppluzhnikov@google.com>
Anoop V Chakkalakkal <anoop.vijayan@in.ibm.com>
[BZ #14333]
* stdlib/cxa_atexit.c (__internal_atexit): Use __exit_funcs_lock.
(__new_exitfn): Fail registration when we finished at_exit processing.
* 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.
Comments
On Mon, 10 Jul 2017, Paul Pluzhnikov wrote:
> Greetings,
>
> This patch is an update to previous patch by Anoop:
> https://sourceware.org/ml/libc-alpha/2008-09/msg00013.html
>
> See discussion in the above message.
>
> Changes from the previous patch: I extended the lifetime of the lock
> from __new_exitfn to its callers, since they *also* modify global
> state.
Is there such a race for quick_exit / at_quick_exit (which postdate the
previous patch), or not? If there is, it should also be fixed (all
atexit-like functions should be fixed).
Is it possible to add a test / tests for this issue to the glibc
testsuite?
Anoop removed from CC list since his e-mail no longer works.
On Tue, Jul 11, 2017 at 5:22 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>
> Is there such a race for quick_exit / at_quick_exit (which postdate the
> previous patch), or not?
Yes, the test from bz14333#c5 with s/atexit/at_quick_exit/ and
s/exit/quick_exit/ also crashes the same way, and the patch here fixes
that crash as well.
> If there is, it should also be fixed (all
> atexit-like functions should be fixed).
They all share the implementation, so this is already done here :-)
> Is it possible to add a test / tests for this issue to the glibc
> testsuite?
Sure. I was going to add it after the "basic" atexit ordering test,
but since that patch requires revisions, I will send a patch to add
the multithreaded test for atexit/at_quick_exit/__cxa_at_quick_exit
separately (or should it be part of this patch?).
Thanks,
On Tue, 11 Jul 2017, Paul Pluzhnikov wrote:
> > If there is, it should also be fixed (all
> > atexit-like functions should be fixed).
>
> They all share the implementation, so this is already done here :-)
>
> > Is it possible to add a test / tests for this issue to the glibc
> > testsuite?
>
> Sure. I was going to add it after the "basic" atexit ordering test,
> but since that patch requires revisions, I will send a patch to add
> the multithreaded test for atexit/at_quick_exit/__cxa_at_quick_exit
> separately (or should it be part of this patch?).
A patch fixing a bug should also include the tests for that bug.
@@ -26,16 +26,25 @@
#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);
@@ -45,6 +54,7 @@ __internal_atexit (void (*func) (void *), void *arg, void *d,
new->func.cxa.dso_handle = d;
atomic_write_barrier ();
new->flavor = ef_cxa;
+ __libc_lock_unlock (__exit_funcs_lock);
return 0;
}
@@ -60,10 +70,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 +82,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 == 1)
+ /* exit code finished processing all handlers
+ so fail this registration */
+ return NULL;
for (l = *listp; l != NULL; p = l, l = l->next)
{
@@ -127,7 +136,5 @@ __new_exitfn (struct exit_function_list **listp)
++__new_exitfn_called;
}
- __libc_lock_unlock (lock);
-
return r;
}
@@ -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 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
@@ -44,14 +47,30 @@ __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;
+
+ restart:
+ __libc_lock_lock (__exit_funcs_lock);
+ cur = *listp;
+
+ if (cur == NULL)
+ {
+ /* Exit processing complete. We will not allow any more
+ atexit/on_exit registrations. */
+ __exit_funcs_done = 1;
+ __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;
+
+ __libc_lock_unlock (__exit_funcs_lock);
switch (f->flavor)
{
void (*atfct) (void);
@@ -83,6 +102,15 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
cxafct (f->func.cxa.arg, status);
break;
}
+ __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. */
+ __libc_lock_unlock (__exit_funcs_lock);
+ 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
{
@@ -62,6 +63,10 @@ extern struct exit_function_list *__quick_exit_funcs attribute_hidden;
extern struct exit_function *__new_exitfn (struct exit_function_list **listp);
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;
extern void __run_exit_handlers (int status,
struct exit_function_list **listp,
@@ -24,10 +24,16 @@
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);
@@ -36,6 +42,7 @@ __on_exit (void (*func) (int status, void *arg), void *arg)
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)