Skip to content
This repository was archived by the owner on Nov 8, 2023. It is now read-only.

Commit 089f4d1

Browse files
committed
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
1 parent 1e158af commit 089f4d1

File tree

2 files changed

+59
-0
lines changed

2 files changed

+59
-0
lines changed

libc/bionic/exit.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
* SUCH DAMAGE.
2727
*/
2828

29+
#include <pthread.h>
2930
#include <stdlib.h>
3031
#include <unistd.h>
3132

@@ -34,8 +35,13 @@
3435
extern "C" void __cxa_finalize(void* dso_handle);
3536
extern "C" void __cxa_thread_finalize();
3637

38+
static pthread_mutex_t g_exit_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
39+
3740
__BIONIC_WEAK_FOR_NATIVE_BRIDGE
3841
void exit(int status) {
42+
// https://austingroupbugs.net/view.php?id=1845
43+
pthread_mutex_lock(&g_exit_mutex);
44+
3945
__cxa_thread_finalize();
4046
__cxa_finalize(nullptr);
4147
_exit(status);

tests/stdlib_test.cpp

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929

3030
#include <limits>
3131
#include <string>
32+
#include <thread>
3233

3334
#include <android-base/file.h>
3435
#include <android-base/macros.h>
@@ -650,6 +651,58 @@ TEST(stdlib, at_quick_exit) {
650651
AssertChildExited(pid, 99);
651652
}
652653

654+
static void exit_from_atexit_func4() {
655+
std::thread([] { exit(4); }).detach();
656+
usleep(1000);
657+
fprintf(stderr, "4");
658+
}
659+
660+
static void exit_from_atexit_func3() {
661+
std::thread([] { exit(3); }).detach();
662+
fprintf(stderr, "3");
663+
usleep(1000);
664+
// This should cause us to exit with status 99,
665+
// but not before printing "4",
666+
// and without re-running the previous atexit handlers.
667+
exit(99);
668+
}
669+
670+
static void exit_from_atexit_func2() {
671+
std::thread([] { exit(2); }).detach();
672+
fprintf(stderr, "2");
673+
usleep(1000);
674+
// Register another atexit handler from within an atexit handler.
675+
atexit(exit_from_atexit_func3);
676+
}
677+
678+
static void exit_from_atexit_func1() {
679+
// These atexit handlers all spawn another thread that tries to exit,
680+
// and sleep to try to lose the race.
681+
// The lock in exit() should ensure that only the first thread to call
682+
// exit() can ever win (but see exit_from_atexit_func3() for a subtelty).
683+
std::thread([] { exit(1); }).detach();
684+
usleep(1000);
685+
fprintf(stderr, "1");
686+
}
687+
688+
static void exit_torturer() {
689+
atexit(exit_from_atexit_func4);
690+
// We deliberately don't register exit_from_atexit_func3() here;
691+
// see exit_from_atexit_func2().
692+
atexit(exit_from_atexit_func2);
693+
atexit(exit_from_atexit_func1);
694+
exit(0);
695+
}
696+
697+
TEST(stdlib, exit_torture) {
698+
// Test that the atexit() handlers are run in the defined order (reverse
699+
// order of registration), even though one of them is registered by another
700+
// when it runs, and that we get the exit code from the last call to exit()
701+
// on the first thread to call exit() (rather than one of the other threads
702+
// or a deadlock from the second call on the same thread).
703+
ASSERT_EXIT(exit_torturer(), testing::ExitedWithCode(99), "1234");
704+
}
705+
653706
TEST(unistd, _Exit) {
654707
pid_t pid = fork();
655708
ASSERT_NE(-1, pid) << strerror(errno);

0 commit comments

Comments
 (0)