This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[RFC] stdlib: Make atexit to not act as __cxa_atexit
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Mon, 1 Jul 2019 17:59:04 -0300
- Subject: [RFC] stdlib: Make atexit to not act as __cxa_atexit
This is RFC patch to address the issue brought in recent discussion
regarding atexit and shared libraries [1] [2]. As indicated in the
libc-alpha discussion, the issue is since atexit uses __cxa_atexit
its interaction __cxa_finalize could lead to atexit handlers being
executed in a different order than the expected one. The github
project gives a small example that triggers it [3].
The changes I could come with changes slight the atexit semantic
as described in my last email [4]. So basically the changes are:
1. Add the __atexit symbol which is linked as __cxa_finalize in
static mode (so __dso_handle is correctly set). The __atexit
symbol adds an ef_at exit_function entry on __exit_funcs,
different than an ef_cxa one from __cxa_atexit.
Old binaries would still call __cxa_atexit, so we do not actually
need to add a compat symbol.
2. Make __cxa_finalize set ef_at handlers to ef_at_finalize. This
is to for dlclose to mark the atexit handlers as free, so a
subsequent __cxa_finalize won't run potentially unmmap code.
3. exit is the only way where atexit handlers are actually called.
Unloading a library through dlclose does not issue the atexit
anymore (this is the main semantic change).
I have not opened a bug report about it because I am not sure from
the previous discussion this is indeed a bug or an expected side-effect
implementing atexit based on __cxa_atexit. This is also only happens
when atexit is created from a shared library (which has its own
__dso_handle value) on some specific scenarios where the unloading
order is changed.
The RFC also has a *lot* of missing parts (tests, abifiles, comments,
CL).
[1] https://sourceware.org/ml/libc-alpha/2019-06/msg00229.html
[2] https://sourceware.org/ml/libc-help/2019-06/msg00025.html
[3] https://github.com/mulle-nat/ld-so-breakage
[4] https://sourceware.org/ml/libc-alpha/2019-06/msg00231.html
---
dlfcn/dlclose.c | 4 +-
dlfcn/modatexit.c | 11 +-
dlfcn/tstatexit.c | 30 +---
include/stdlib.h | 3 +
stdlib/Versions | 4 +
stdlib/atexit.c | 2 +-
stdlib/cxa_atexit.c | 28 +++-
stdlib/cxa_finalize.c | 133 +++++++++++-------
stdlib/exit.c | 3 +-
stdlib/exit.h | 7 +-
.../unix/sysv/linux/x86_64/64/libc.abilist | 1 +
11 files changed, 141 insertions(+), 85 deletions(-)
diff --git a/dlfcn/dlclose.c b/dlfcn/dlclose.c
index 98e1375c43..c9a1e643f3 100644
--- a/dlfcn/dlclose.c
+++ b/dlfcn/dlclose.c
@@ -43,7 +43,9 @@ __dlclose (void *handle)
return _dlfcn_hook->dlclose (handle);
# endif
- return _dlerror_run (dlclose_doit, handle) ? -1 : 0;
+ int r = _dlerror_run (dlclose_doit, handle) ? -1 : 0;
+ __atexit_cleanup ();
+ return r;
}
# ifdef SHARED
strong_alias (__dlclose, dlclose)
diff --git a/dlfcn/modatexit.c b/dlfcn/modatexit.c
index 118ccf5459..8b16e0ba8f 100644
--- a/dlfcn/modatexit.c
+++ b/dlfcn/modatexit.c
@@ -21,11 +21,14 @@
int global;
int *ip;
-extern void dummy (void);
-extern void foo (void *p);
+/* glibc function for registering DSO-specific unload functions. */
+extern int __cxa_atexit (void (*func) (void *), void *arg, void *d);
+
+/* Hidden compiler handle to this shared object. */
+extern void *__dso_handle __attribute__ ((__weak__));
void
-dummy (void)
+dummy (void *arg)
{
printf ("This is %s\n", __FUNCTION__);
*ip = global = 1;
@@ -36,6 +39,6 @@ void
foo (void *p)
{
printf ("This is %s\n", __FUNCTION__);
- atexit (dummy);
+ __cxa_atexit (dummy, NULL, __dso_handle);
ip = p;
}
diff --git a/dlfcn/tstatexit.c b/dlfcn/tstatexit.c
index c8cb272ce0..1f6037cb47 100644
--- a/dlfcn/tstatexit.c
+++ b/dlfcn/tstatexit.c
@@ -19,6 +19,8 @@
#include <stdio.h>
#include <stdlib.h>
+#include <support/xdlfcn.h>
+#include <support/check.h>
int
main (void)
@@ -28,35 +30,15 @@ main (void)
void (*fp) (void *);
int v = 0;
- h = dlopen (fname, RTLD_NOW);
- if (h == NULL)
- {
- printf ("cannot open \"%s\": %s\n", fname, dlerror ());
- exit (1);
- }
+ h = xdlopen (fname, RTLD_NOW);
- fp = dlsym (h, "foo");
- if (fp == NULL)
- {
- printf ("cannot find \"foo\": %s\n", dlerror ());
- exit (1);
- }
+ fp = xdlsym (h, "foo");
fp (&v);
- if (dlclose (h) != 0)
- {
- printf ("cannot close \"%s\": %s\n", fname, dlerror ());
- exit (1);
- }
+ xdlclose (h);
- if (v != 1)
- {
- puts ("module unload didn't change `v'");
- exit (1);
- }
-
- puts ("finishing now");
+ TEST_COMPARE (v, 1);
return 0;
}
diff --git a/include/stdlib.h b/include/stdlib.h
index 114e12d255..3bf47ea49d 100644
--- a/include/stdlib.h
+++ b/include/stdlib.h
@@ -103,6 +103,9 @@ extern int __on_exit (void (*__func) (int __status, void *__arg), void *__arg);
extern int __cxa_atexit (void (*func) (void *), void *arg, void *d);
libc_hidden_proto (__cxa_atexit);
+extern int __atexit (void (*func) (void), void *);
+extern void __atexit_cleanup (void);
+
extern int __cxa_thread_atexit_impl (void (*func) (void *), void *arg,
void *d);
extern void __call_tls_dtors (void)
diff --git a/stdlib/Versions b/stdlib/Versions
index 9e665d4c26..9a9d911621 100644
--- a/stdlib/Versions
+++ b/stdlib/Versions
@@ -136,6 +136,9 @@ libc {
strtof32; strtof64; strtof32x;
strtof32_l; strtof64_l; strtof32x_l;
}
+ GLIBC_2.29 {
+ __atexit;
+ }
GLIBC_PRIVATE {
# functions which have an additional interface since they are
# are cancelable.
@@ -146,5 +149,6 @@ libc {
__libc_secure_getenv;
__call_tls_dtors;
__strtof_nan; __strtod_nan; __strtold_nan;
+ __atexit_cleanup;
}
}
diff --git a/stdlib/atexit.c b/stdlib/atexit.c
index 5671a43926..de8570e3bf 100644
--- a/stdlib/atexit.c
+++ b/stdlib/atexit.c
@@ -43,5 +43,5 @@ attribute_hidden
#endif
atexit (void (*func) (void))
{
- return __cxa_atexit ((void (*) (void *)) func, NULL, __dso_handle);
+ return __atexit (func, __dso_handle);
}
diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
index 5a39778959..88ee811ac3 100644
--- a/stdlib/cxa_atexit.c
+++ b/stdlib/cxa_atexit.c
@@ -23,7 +23,6 @@
#include "exit.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)
@@ -71,6 +70,33 @@ __cxa_atexit (void (*func) (void *), void *arg, void *d)
}
libc_hidden_def (__cxa_atexit)
+int
+__atexit (void (*func) (void), void *dso_handle)
+{
+ struct exit_function *new;
+
+ /* As a QoI issue we detect NULL early with an assertion instead
+ of a SIGSEGV at program exit when the handler is run (bug 20544). */
+ assert (func != NULL);
+
+ __libc_lock_lock (__exit_funcs_lock);
+ new = __new_exitfn (&__exit_funcs);
+
+ if (new == NULL)
+ {
+ __libc_lock_unlock (__exit_funcs_lock);
+ return -1;
+ }
+
+#ifdef PTR_MANGLE
+ PTR_MANGLE (func);
+#endif
+ new->func.at.fn = func;
+ new->func.at.dso_handle = dso_handle;
+ new->flavor = ef_at;
+ __libc_lock_unlock (__exit_funcs_lock);
+ return 0;
+}
static struct exit_function_list initial;
struct exit_function_list *__exit_funcs = &initial;
diff --git a/stdlib/cxa_finalize.c b/stdlib/cxa_finalize.c
index e31b23467c..aeb364eb9b 100644
--- a/stdlib/cxa_finalize.c
+++ b/stdlib/cxa_finalize.c
@@ -22,6 +22,60 @@
#include <sysdep.h>
#include <stdint.h>
+static bool
+handle_cxa (struct exit_function *f)
+{
+ 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.
+
+ 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);
+#endif
+ /* 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))
+ return true;
+ return false;
+}
+
/* If D is non-NULL, call all functions registered with `__cxa_atexit'
with the same dso handle. Otherwise, if D is NULL, call all of the
registered handlers. */
@@ -35,59 +89,19 @@ __cxa_finalize (void *d)
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)
- 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.
-
- 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);
-#endif
- /* 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))
+ for (struct exit_function *f = &funcs->fns[funcs->idx - 1];
+ f >= &funcs->fns[0]; --f)
+ {
+ if ((d == NULL || d == f->func.cxa.dso_handle)
+ && f->flavor == ef_cxa)
+ if (handle_cxa (f))
goto restart;
- }
+
+ /* Mark ef_at_exit handlers are handled by __cxa_finalize. */
+ if ((d == NULL || d == f->func.at.dso_handle)
+ && f->flavor == ef_at)
+ f->flavor = ef_at_finalize;
+ }
}
/* Also remove the quick_exit handlers, but do not call them. */
@@ -108,3 +122,18 @@ __cxa_finalize (void *d)
#endif
__libc_lock_unlock (__exit_funcs_lock);
}
+
+void
+__atexit_cleanup (void)
+{
+ __libc_lock_lock (__exit_funcs_lock);
+ for (struct exit_function_list * funcs = __exit_funcs; funcs != NULL;
+ funcs = funcs->next)
+ {
+ for (struct exit_function *f = &funcs->fns[funcs->idx - 1];
+ f >= &funcs->fns[0]; --f)
+ if (f->flavor == ef_at_finalize)
+ f->flavor = ef_free;
+ }
+ __libc_lock_unlock (__exit_funcs_lock);
+}
diff --git a/stdlib/exit.c b/stdlib/exit.c
index 49d12d9952..8352760fae 100644
--- a/stdlib/exit.c
+++ b/stdlib/exit.c
@@ -91,7 +91,8 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
onfct (status, f->func.on.arg);
break;
case ef_at:
- atfct = f->func.at;
+ case ef_at_finalize:
+ atfct = f->func.at.fn;
#ifdef PTR_DEMANGLE
PTR_DEMANGLE (atfct);
#endif
diff --git a/stdlib/exit.h b/stdlib/exit.h
index 6fab49c94f..a1d4860206 100644
--- a/stdlib/exit.h
+++ b/stdlib/exit.h
@@ -28,6 +28,7 @@ enum
ef_us,
ef_on,
ef_at,
+ ef_at_finalize,
ef_cxa
};
@@ -38,7 +39,11 @@ struct exit_function
long int flavor;
union
{
- void (*at) (void);
+ struct
+ {
+ void (*fn) (void);
+ void *dso_handle;
+ } at;
struct
{
void (*fn) (int status, void *arg);
diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
index 59f85d9373..699fd105de 100644
--- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
@@ -1895,6 +1895,7 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __atexit F
GLIBC_2.29 getcpu F
GLIBC_2.29 posix_spawn_file_actions_addchdir_np F
GLIBC_2.29 posix_spawn_file_actions_addfchdir_np F
--
2.17.1