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 zephyrproject-rtos#56163

Signed-off-by: Andy Ross <[email protected]>

Author: andyross

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 {

lib/posix/pthread.c

@@ -183,7 +183,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;
 
@@ -218,8 +217,6 @@ int pthread_create(pthread_t *newthread, const pthread_attr_t *_attr,
 	thread->cancel_pending = 0;
 	k_spin_unlock(&thread->cancel_lock, cancel_key);
 
-	pthread_cond_init(&thread->state_cond, &cond_attr);
-
 	*newthread = pthread_num;
 	k_thread_create(&thread->thread, attr->stack, attr->stacksize,
 			(k_thread_entry_t)zephyr_thread_wrapper, (void *)arg, NULL, threadroutine,
@@ -283,7 +280,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);
 
@@ -405,7 +401,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 {
 		destroy = true;
 		self->state = PTHREAD_TERMINATED;
@@ -428,8 +423,6 @@ void pthread_exit(void *retval)
 		pthread_mutex_destroy(&self->state_lock);
 	}
 
-	pthread_cond_destroy(&self->state_cond);
-
 	k_thread_abort(&self->thread);
 }
 
@@ -452,12 +445,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) {
 		destroy = true;
 		pthread->state = PTHREAD_TERMINATED;
@@ -498,10 +493,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:
 		join = true;