From 6d3684345d61ea750fd36e6aa68d48397d925fd1 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 9 May 2023 16:26:43 -0700 Subject: [PATCH] lib/pthread: Fix create/join race in thread lifecycle The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes #56163 Signed-off-by: Andy Ross --- lib/posix/posix_internal.h | 1 - lib/posix/pthread.c | 21 +++++++-------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/lib/posix/posix_internal.h b/lib/posix/posix_internal.h index 4f68003fe945a1f..95d9772210de9b5 100644 --- a/lib/posix/posix_internal.h +++ b/lib/posix/posix_internal.h @@ -54,7 +54,6 @@ struct posix_thread { /* Pthread State */ enum pthread_state state; pthread_mutex_t state_lock; - pthread_cond_t state_cond; }; typedef struct pthread_key_obj { diff --git a/lib/posix/pthread.c b/lib/posix/pthread.c index 15efcf568d3ac67..4ae6459631ff839 100644 --- a/lib/posix/pthread.c +++ b/lib/posix/pthread.c @@ -159,7 +159,6 @@ int pthread_create(pthread_t *newthread, const pthread_attr_t *_attr, k_spinlock_key_t key; uint32_t pthread_num; k_spinlock_key_t cancel_key; - pthread_condattr_t cond_attr; struct posix_thread *thread; const struct pthread_attr *attr = (const struct pthread_attr *)_attr; @@ -207,7 +206,6 @@ int pthread_create(pthread_t *newthread, const pthread_attr_t *_attr, thread->state = attr->detachstate; pthread_mutex_unlock(&thread->state_lock); - pthread_cond_init(&thread->state_cond, &cond_attr); sys_slist_init(&thread->key_list); *newthread = pthread_num; @@ -274,7 +272,6 @@ int pthread_cancel(pthread_t pthread) } else { thread->retval = PTHREAD_CANCELED; thread->state = PTHREAD_EXITED; - pthread_cond_broadcast(&thread->state_cond); } pthread_mutex_unlock(&thread->state_lock); @@ -395,7 +392,6 @@ void pthread_exit(void *retval) if (self->state == PTHREAD_JOINABLE) { self->state = PTHREAD_EXITED; self->retval = retval; - pthread_cond_broadcast(&self->state_cond); } else { self->state = PTHREAD_TERMINATED; } @@ -417,8 +413,6 @@ void pthread_exit(void *retval) pthread_mutex_destroy(&self->state_lock); } - pthread_cond_destroy(&self->state_cond); - k_thread_abort(&self->thread); } @@ -440,12 +434,14 @@ int pthread_join(pthread_t thread, void **status) return ESRCH; } - pthread_mutex_lock(&pthread->state_lock); - - if (pthread->state == PTHREAD_JOINABLE) { - pthread_cond_wait(&pthread->state_cond, &pthread->state_lock); + if (pthread->state == PTHREAD_DETACHED) { + return EINVAL; } + k_thread_join(&pthread->thread, K_FOREVER); + + pthread_mutex_lock(&pthread->state_lock); + if (pthread->state == PTHREAD_EXITED) { pthread->state = PTHREAD_TERMINATED; if (status != NULL) { @@ -484,10 +480,7 @@ int pthread_detach(pthread_t thread) switch (pthread->state) { case PTHREAD_JOINABLE: pthread->state = PTHREAD_DETACHED; - /* Broadcast the condition. - * This will make threads waiting to join this thread continue. - */ - pthread_cond_broadcast(&pthread->state_cond); + z_thread_wake_joiners(&pthread->thread); break; case PTHREAD_EXITED: pthread->state = PTHREAD_TERMINATED;