aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPaul Pluzhnikov <ppluzhnikov@google.com>2017-09-20 09:31:48 -0700
committerPaul Pluzhnikov <ppluzhnikov@google.com>2017-09-20 09:31:48 -0700
commit26e70aec7028feeb196744eb97cd2dff3670b7aa (patch)
tree0eb04d182ca05b4161b1cc0f6c59265da6ccb621
parent0525ce4850f2c22a235dcd3422bc92f40815f377 (diff)
downloadglibc-26e70aec7028feeb196744eb97cd2dff3670b7aa.zip
glibc-26e70aec7028feeb196744eb97cd2dff3670b7aa.tar.gz
glibc-26e70aec7028feeb196744eb97cd2dff3670b7aa.tar.bz2
Fix BZ 14333
-rw-r--r--ChangeLog22
-rw-r--r--stdlib/Makefile9
-rw-r--r--stdlib/cxa_atexit.c28
-rw-r--r--stdlib/cxa_finalize.c68
-rw-r--r--stdlib/exit.c36
-rw-r--r--stdlib/exit.h19
-rw-r--r--stdlib/on_exit.c13
-rw-r--r--stdlib/test-at_quick_exit-race.c32
-rw-r--r--stdlib/test-atexit-race.c31
-rw-r--r--stdlib/test-cxa_atexit-race.c35
-rw-r--r--stdlib/test-on_exit-race.c31
11 files changed, 285 insertions, 39 deletions
diff --git a/ChangeLog b/ChangeLog
index 9352475..a07c903 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,25 @@
+2017-09-20 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, test-on_exit-race): New tests.
+ * stdlib/test-atexit-race-common.c: New file.
+ * stdlib/test-atexit-race.c: New file.
+ * stdlib/test-at_quick_exit-race.c: New file.
+ * stdlib/test-cxa_atexit-race.c: New file.
+ * stdlib/test-on_exit-race.c: New file.
+
2017-09-20 Szabolcs Nagy <szabolcs.nagy@arm.com>
* benchtests/Makefile: Add exp2f and log2f benchmarks.
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 2da39e0..2fb0834 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -81,7 +81,9 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \
tst-quick_exit tst-thread-quick_exit tst-width \
tst-width-stdint tst-strfrom tst-strfrom-locale \
tst-getrandom tst-atexit tst-at_quick_exit \
- tst-cxa_atexit tst-on_exit
+ tst-cxa_atexit tst-on_exit test-atexit-race \
+ test-at_quick_exit-race test-cxa_atexit-race \
+ test-on_exit-race
tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
tst-tls-atexit tst-tls-atexit-nodelete
@@ -91,6 +93,11 @@ 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)
+LDLIBS-test-on_exit-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 ce5d9f2..beb3169 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>
#include <sysdep.h>
#undef __cxa_atexit
+/* See concurrency notes in stdlib/exit.h where this lock is declared. */
+__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,14 +68,11 @@ __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;
+/* Must be called with __exit_funcs_lock held. */
struct exit_function *
__new_exitfn (struct exit_function_list **listp)
{
@@ -76,7 +81,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 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 +135,5 @@ __new_exitfn (struct exit_function_list **listp)
++__new_exitfn_called;
}
- __libc_lock_unlock (lock);
-
return r;
}
diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c
index aa0a70c..23fc6dc 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,64 @@ __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;
+
+ /* We don't want to run this cleanup more than once. The Itanium
+ C++ ABI requires that multiple calls to __cxa_finalize not
+ result in calling termination functions more than once. One
+ potential scenario where that could happen is with a concurrent
+ dlclose and exit, where the running dlclose must at some point
+ release the list lock, an exiting thread may acquire it, and
+ without setting flavor to ef_free, might re-run this destructor
+ which could result in undefined behaviour. Therefore we must
+ set flavor to ef_free to avoid calling this destructor again.
+ Note that the concurrent exit must also take the dynamic loader
+ lock (for library finalizer processing) and therefore will
+ block while dlclose completes the processing of any in-progress
+ exit functions. Lastly, once we release the list lock for the
+ entry marked ef_free, we must not read from that entry again
+ since it may have been reused by the time we take the list lock
+ again. Lastly the detection of new registered exit functions is
+ based on a monotonically incrementing counter, and there is an
+ ABA if between the unlock to run the exit function and the
+ re-lock after completion the user registers 2^64 exit functions,
+ the implementation will not detect this and continue without
+ executing any more functions.
- 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;
+ One minor issue remains: A registered exit function that is in
+ progress by a call to dlclose() may not completely finish before
+ the next registered exit function is run. This may, according to
+ some readings of POSIX violate the requirement that functions
+ run in effective LIFO order. This should probably be fixed in a
+ future implementation to ensure the functions do not run in
+ parallel. */
+ f->flavor = ef_free;
#ifdef PTR_DEMANGLE
- PTR_DEMANGLE (cxafn);
+ PTR_DEMANGLE (cxafn);
#endif
- cxafn (cxaarg, 0);
+ /* Unlock the list while we call a foreign function. */
+ __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 +106,5 @@ __cxa_finalize (void *d)
if (d != NULL)
UNREGISTER_ATFORK (d);
#endif
+ __libc_lock_unlock (__exit_funcs_lock);
}
diff --git a/stdlib/exit.c b/stdlib/exit.c
index c0b6d66..b74f182 100644
--- a/stdlib/exit.c
+++ b/stdlib/exit.c
@@ -19,11 +19,16 @@
#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))
+/* Initialize the flag that indicates exit function processing
+ is complete. See concurrency notes in stdlib/exit.h where
+ __exit_funcs_lock is declared. */
+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 +49,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 a foreign function. */
+ __libc_lock_unlock (__exit_funcs_lock);
switch (f->flavor)
{
void (*atfct) (void);
@@ -83,6 +106,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 +120,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)
diff --git a/stdlib/exit.h b/stdlib/exit.h
index 7f2e679..dbf9f2d 100644
--- a/stdlib/exit.h
+++ b/stdlib/exit.h
@@ -20,6 +20,7 @@
#include <stdbool.h>
#include <stdint.h>
+#include <libc-lock.h>
enum
{
@@ -57,11 +58,27 @@ 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, and
+ each associated entry in the list, is protected for all access 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,
diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c
index 83845e7..f4ede2b 100644
--- a/stdlib/on_exit.c
+++ b/stdlib/on_exit.c
@@ -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)
diff --git a/stdlib/test-at_quick_exit-race.c b/stdlib/test-at_quick_exit-race.c
new file mode 100644
index 0000000..f93fb85
--- /dev/null
+++ b/stdlib/test-at_quick_exit-race.c
@@ -0,0 +1,32 @@
+/* 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". */
+
+/* See stdlib/test-atexit-race-common.c for details on this test. */
+
+#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>
diff --git a/stdlib/test-atexit-race.c b/stdlib/test-atexit-race.c
new file mode 100644
index 0000000..11e0244
--- /dev/null
+++ b/stdlib/test-atexit-race.c
@@ -0,0 +1,31 @@
+/* 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". */
+
+/* See stdlib/test-atexit-race-common.c for details on this test. */
+
+#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 0000000..c695e71
--- /dev/null
+++ b/stdlib/test-cxa_atexit-race.c
@@ -0,0 +1,35 @@
+/* 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". */
+
+/* See stdlib/test-atexit-race-common.c for details on this test. */
+
+#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>
diff --git a/stdlib/test-on_exit-race.c b/stdlib/test-on_exit-race.c
new file mode 100644
index 0000000..3e67d3b
--- /dev/null
+++ b/stdlib/test-on_exit-race.c
@@ -0,0 +1,31 @@
+/* Bug 14333: a test for on_exit/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". */
+
+/* See stdlib/test-atexit-race-common.c for details on this test. */
+
+#define CALL_ATEXIT on_exit (&no_op, (void *) 0)
+#define CALL_EXIT exit (0)
+
+static void
+no_op (int exit_code, void *ignored)
+{
+}
+
+#include <stdlib/test-atexit-race-common.c>