diff --git a/libc/bionic/pthread_attr.cpp b/libc/bionic/pthread_attr.cpp index be1c25228..7ad34311e 100644 --- a/libc/bionic/pthread_attr.cpp +++ b/libc/bionic/pthread_attr.cpp @@ -170,6 +170,11 @@ int pthread_attr_getguardsize(const pthread_attr_t* attr, size_t* guard_size) { int pthread_getattr_np(pthread_t t, pthread_attr_t* attr) { pthread_internal_t* thread = reinterpret_cast(t); *attr = thread->attr; + // We prefer reading join_state here to setting thread->attr.flags in pthread_detach. + // Because data race exists in the latter case. + if (atomic_load(&thread->join_state) == THREAD_DETACHED) { + attr->flags |= PTHREAD_ATTR_FLAG_DETACHED; + } // The main thread's stack information is not stored in thread->attr, and we need to // collect that at runtime. if (thread->tid == getpid()) { diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp index 2bca43f83..a4bd054c7 100644 --- a/libc/bionic/pthread_create.cpp +++ b/libc/bionic/pthread_create.cpp @@ -86,6 +86,12 @@ void __init_alternate_signal_stack(pthread_internal_t* thread) { int __init_thread(pthread_internal_t* thread, bool add_to_thread_list) { int error = 0; + if (__predict_true((thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) == 0)) { + atomic_init(&thread->join_state, THREAD_NOT_JOINED); + } else { + atomic_init(&thread->join_state, THREAD_DETACHED); + } + // Set the scheduling policy/priority of the thread. if (thread->attr.sched_policy != SCHED_NORMAL) { sched_param param; @@ -263,7 +269,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr, if (init_errno != 0) { // Mark the thread detached and replace its start_routine with a no-op. // Letting the thread run is the easiest way to clean up its resources. - thread->attr.flags |= PTHREAD_ATTR_FLAG_DETACHED; + atomic_store(&thread->join_state, THREAD_DETACHED); thread->start_routine = __do_nothing; pthread_mutex_unlock(&thread->startup_handshake_mutex); return init_errno; diff --git a/libc/bionic/pthread_detach.cpp b/libc/bionic/pthread_detach.cpp index c80066008..0712d0d67 100644 --- a/libc/bionic/pthread_detach.cpp +++ b/libc/bionic/pthread_detach.cpp @@ -38,21 +38,18 @@ int pthread_detach(pthread_t t) { return ESRCH; } - if (thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) { - return EINVAL; // Already detached. + ThreadJoinState old_state = THREAD_NOT_JOINED; + while (old_state == THREAD_NOT_JOINED && + !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_DETACHED)) { } - - if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) { - return 0; // Already being joined; silently do nothing, like glibc. - } - - // If the thread has not exited, we can detach it safely. - if ((thread->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) == 0) { - thread->attr.flags |= PTHREAD_ATTR_FLAG_DETACHED; - return 0; + switch (old_state) { + case THREAD_NOT_JOINED: return 0; + case THREAD_JOINED: return 0; // Already being joined; silently do nothing, like glibc. + case THREAD_DETACHED: return THREAD_DETACHED; + case THREAD_EXITED_NOT_JOINED: // Call pthread_join out of scope of pthread_accessor. } } - // The thread is in zombie state, use pthread_join to clean it up. + // The thread is in THREAD_EXITED_NOT_JOINED, use pthread_join to clean it up. return pthread_join(t, NULL); } diff --git a/libc/bionic/pthread_exit.cpp b/libc/bionic/pthread_exit.cpp index d0d64b0ec..81cc67be9 100644 --- a/libc/bionic/pthread_exit.cpp +++ b/libc/bionic/pthread_exit.cpp @@ -87,9 +87,12 @@ void pthread_exit(void* return_value) { thread->alternate_signal_stack = NULL; } - bool free_mapped_space = false; - pthread_mutex_lock(&g_thread_list_lock); - if ((thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) != 0) { + ThreadJoinState old_state = THREAD_NOT_JOINED; + while (old_state == THREAD_NOT_JOINED && + !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_EXITED_NOT_JOINED)) { + } + + if (old_state == THREAD_DETACHED) { // The thread is detached, no one will use pthread_internal_t after pthread_exit. // So we can free mapped space, which includes pthread_internal_t and thread stack. // First make sure that the kernel does not try to clear the tid field @@ -97,28 +100,25 @@ void pthread_exit(void* return_value) { __set_tid_address(NULL); // pthread_internal_t is freed below with stack, not here. + pthread_mutex_lock(&g_thread_list_lock); _pthread_internal_remove_locked(thread, false); - free_mapped_space = true; - } else { - // Mark the thread as exiting without freeing pthread_internal_t. - thread->attr.flags |= PTHREAD_ATTR_FLAG_ZOMBIE; + pthread_mutex_unlock(&g_thread_list_lock); + + if (thread->mmap_size != 0) { + // We need to free mapped space for detached threads when they exit. + // That's not something we can do in C. + + // We don't want to take a signal after we've unmapped the stack. + // That's one last thing we can handle in C. + sigset_t mask; + sigfillset(&mask); + sigprocmask(SIG_SETMASK, &mask, NULL); + + _exit_with_stack_teardown(thread->attr.stack_base, thread->mmap_size); + } } - pthread_mutex_unlock(&g_thread_list_lock); - if (free_mapped_space && thread->mmap_size != 0) { - // We need to free mapped space for detached threads when they exit. - // That's not something we can do in C. - - // We don't want to take a signal after we've unmapped the stack. - // That's one last thing we can handle in C. - sigset_t mask; - sigfillset(&mask); - sigprocmask(SIG_SETMASK, &mask, NULL); - - _exit_with_stack_teardown(thread->attr.stack_base, thread->mmap_size); - } else { - // No need to free mapped space. Either there was no space mapped, or it is left for - // the pthread_join caller to clean up. - __exit(0); - } + // No need to free mapped space. Either there was no space mapped, or it is left for + // the pthread_join caller to clean up. + __exit(0); } diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h index 6ace30180..8da99dd36 100644 --- a/libc/bionic/pthread_internal.h +++ b/libc/bionic/pthread_internal.h @@ -29,6 +29,7 @@ #define _PTHREAD_INTERNAL_H_ #include +#include #include "private/bionic_tls.h" @@ -46,6 +47,13 @@ struct pthread_key_data_t { void* data; }; +enum ThreadJoinState { + THREAD_NOT_JOINED, + THREAD_EXITED_NOT_JOINED, + THREAD_JOINED, + THREAD_DETACHED +}; + struct pthread_internal_t { struct pthread_internal_t* next; struct pthread_internal_t* prev; @@ -74,6 +82,8 @@ struct pthread_internal_t { pthread_attr_t attr; + _Atomic(ThreadJoinState) join_state; + __pthread_cleanup_t* cleanup_stack; void* (*start_routine)(void*); diff --git a/libc/bionic/pthread_join.cpp b/libc/bionic/pthread_join.cpp index e3350efd3..15543b48e 100644 --- a/libc/bionic/pthread_join.cpp +++ b/libc/bionic/pthread_join.cpp @@ -44,16 +44,15 @@ int pthread_join(pthread_t t, void** return_value) { return ESRCH; } - if ((thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) != 0) { + ThreadJoinState old_state = THREAD_NOT_JOINED; + while ((old_state == THREAD_NOT_JOINED || old_state == THREAD_EXITED_NOT_JOINED) && + !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_JOINED)) { + } + + if (old_state == THREAD_DETACHED || old_state == THREAD_JOINED) { return EINVAL; } - if ((thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) != 0) { - return EINVAL; - } - - // Okay, looks like we can signal our intention to join. - thread->attr.flags |= PTHREAD_ATTR_FLAG_JOINED; tid = thread->tid; tid_ptr = &thread->tid; }