Skip to content

Commit

Permalink
lib/pthread: Fix create/join race in thread lifecycle
Browse files Browse the repository at this point in the history
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 zephyrproject-rtos#56163

Signed-off-by: Andy Ross <andyross@google.com>
  • Loading branch information
andyross committed May 10, 2023
1 parent c897766 commit 48596a1
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 15 deletions.
1 change: 0 additions & 1 deletion lib/posix/posix_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
21 changes: 7 additions & 14 deletions lib/posix/pthread.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;
}
Expand All @@ -413,8 +409,6 @@ void pthread_exit(void *retval)
pthread_mutex_unlock(&self->state_lock);
pthread_mutex_destroy(&self->state_lock);

pthread_cond_destroy(&self->state_cond);

k_thread_abort((k_tid_t)self);
}

Expand All @@ -436,12 +430,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) {
if (status != NULL) {
*status = pthread->retval;
Expand Down Expand Up @@ -479,10 +475,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;
Expand Down

0 comments on commit 48596a1

Please sign in to comment.