exit(): add a lock.
POSIX hasn't accepted https://austingroupbugs.net/view.php?id=1845 yet,
but all the libc maintainers who've commented on
https://www.openwall.com/lists/libc-coord/2024/07/24/4 agree that this
is a good idea, with the only disagreement being whether or not the
mutex should be recursive.
I preferred the recursive mutex because suddenly disallowing exit() from
an atexit handler is an unnecessary behavior change that seemed likely
to trip someone up, and glibc found that they have existing examples of
exit() called from ELF destructors or atexit() handlers in test cases if
nothing else (https://sourceware.org/bugzilla/show_bug.cgi?id=31997).
So go with the recursive mutex option. The new test fails most of the
time without this fix, and runs fine with this fix.
Change-Id: I33c2229512e70113739e0dd9ffd2a745292ba8c3
diff --git a/libc/bionic/exit.cpp b/libc/bionic/exit.cpp
index a5aed78..8eda5b2 100644
--- a/libc/bionic/exit.cpp
+++ b/libc/bionic/exit.cpp
@@ -26,6 +26,7 @@
* SUCH DAMAGE.
*/
+#include <pthread.h>
#include <stdlib.h>
#include <unistd.h>
@@ -34,8 +35,13 @@
extern "C" void __cxa_finalize(void* dso_handle);
extern "C" void __cxa_thread_finalize();
+static pthread_mutex_t g_exit_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+
__BIONIC_WEAK_FOR_NATIVE_BRIDGE
void exit(int status) {
+ // https://austingroupbugs.net/view.php?id=1845
+ pthread_mutex_lock(&g_exit_mutex);
+
__cxa_thread_finalize();
__cxa_finalize(nullptr);
_exit(status);
diff --git a/tests/stdlib_test.cpp b/tests/stdlib_test.cpp
index 4793150..f5e57a5 100644
--- a/tests/stdlib_test.cpp
+++ b/tests/stdlib_test.cpp
@@ -29,6 +29,7 @@
#include <limits>
#include <string>
+#include <thread>
#include <android-base/file.h>
#include <android-base/macros.h>
@@ -650,6 +651,58 @@
AssertChildExited(pid, 99);
}
+static void exit_from_atexit_func4() {
+ std::thread([] { exit(4); }).detach();
+ usleep(1000);
+ fprintf(stderr, "4");
+}
+
+static void exit_from_atexit_func3() {
+ std::thread([] { exit(3); }).detach();
+ fprintf(stderr, "3");
+ usleep(1000);
+ // This should cause us to exit with status 99,
+ // but not before printing "4",
+ // and without re-running the previous atexit handlers.
+ exit(99);
+}
+
+static void exit_from_atexit_func2() {
+ std::thread([] { exit(2); }).detach();
+ fprintf(stderr, "2");
+ usleep(1000);
+ // Register another atexit handler from within an atexit handler.
+ atexit(exit_from_atexit_func3);
+}
+
+static void exit_from_atexit_func1() {
+ // These atexit handlers all spawn another thread that tries to exit,
+ // and sleep to try to lose the race.
+ // The lock in exit() should ensure that only the first thread to call
+ // exit() can ever win (but see exit_from_atexit_func3() for a subtelty).
+ std::thread([] { exit(1); }).detach();
+ usleep(1000);
+ fprintf(stderr, "1");
+}
+
+static void exit_torturer() {
+ atexit(exit_from_atexit_func4);
+ // We deliberately don't register exit_from_atexit_func3() here;
+ // see exit_from_atexit_func2().
+ atexit(exit_from_atexit_func2);
+ atexit(exit_from_atexit_func1);
+ exit(0);
+}
+
+TEST(stdlib, exit_torture) {
+ // Test that the atexit() handlers are run in the defined order (reverse
+ // order of registration), even though one of them is registered by another
+ // when it runs, and that we get the exit code from the last call to exit()
+ // on the first thread to call exit() (rather than one of the other threads
+ // or a deadlock from the second call on the same thread).
+ ASSERT_EXIT(exit_torturer(), testing::ExitedWithCode(99), "1234");
+}
+
TEST(unistd, _Exit) {
pid_t pid = fork();
ASSERT_NE(-1, pid) << strerror(errno);