Fix join and make Thread class thread safe

Add a mutex to the thread object to protect its internal data. Prevent
making OS calls with a thread ID that has been terminated. This thread
ID can be reused by another thread, leading to undefined behavior if it
is used after termination.

Update the function Thread::join to use a semaphore to
determine when the thread finishes. This both avoids polling and
prevents a freed TCB from being accessed.

Author: c1728p9

rtos/rtos/Thread.cpp

@@ -63,16 +63,21 @@ void Thread::constructor(Callback<void()> task,
 }
 
 osStatus Thread::start(Callback<void()> task) {
+    _mutex.lock();
+
     if (_tid != 0) {
+        _mutex.unlock();
         return osErrorParameter;
     }
 
 #if defined(__MBED_CMSIS_RTOS_CA9) || defined(__MBED_CMSIS_RTOS_CM)
-    _thread_def.pthread = (void (*)(const void *))Callback<void()>::thunk;
+    _thread_def.pthread = Thread::_thunk;
     if (_thread_def.stack_pointer == NULL) {
         _thread_def.stack_pointer = new uint32_t[_thread_def.stacksize/sizeof(uint32_t)];
-        if (_thread_def.stack_pointer == NULL)
+        if (_thread_def.stack_pointer == NULL) {
+            _mutex.unlock();
             return osErrorNoMemory;
+        }
     }
 
     //Fill the stack with a magic word for maximum usage checking
@@ -81,67 +86,118 @@ osStatus Thread::start(Callback<void()> task) {
     }
 #endif
     _task = task;
-    _tid = osThreadCreate(&_thread_def, &_task);
+    _tid = osThreadCreate(&_thread_def, this);
     if (_tid == NULL) {
         if (_dynamic_stack) delete[] (_thread_def.stack_pointer);
+        _mutex.unlock();
         return osErrorResource;
     }
+
+    _mutex.unlock();
     return osOK;
 }
 
 osStatus Thread::terminate() {
-    return osThreadTerminate(_tid);
+    osStatus ret;
+    _mutex.lock();
+
+    ret = osThreadTerminate(_tid);
+
+    _mutex.unlock();
+    return ret;
 }
 
 osStatus Thread::join() {
-    while (true) {
-        uint8_t state = get_state();
-        if (state == Thread::Inactive || state == osErrorParameter) {
-            return osOK;
-        }
-
-        osStatus status = yield();
-        if (status != osOK) {
-            return status;
-        }
+    int32_t ret = _join_sem.wait();
+    if (ret < 0) {
+        return osErrorOS;
     }
+    // Release sem so any other threads joining this thread wake up
+    _join_sem.release();
+    return osOK;
 }
 
 osStatus Thread::set_priority(osPriority priority) {
-    return osThreadSetPriority(_tid, priority);
+    osStatus ret;
+    _mutex.lock();
+
+    ret = osThreadSetPriority(_tid, priority);
+
+    _mutex.unlock();
+    return ret;
 }
 
 osPriority Thread::get_priority() {
-    return osThreadGetPriority(_tid);
+    osPriority ret;
+    _mutex.lock();
+
+    ret = osThreadGetPriority(_tid);
+
+    _mutex.unlock();
+    return ret;
 }
 
 int32_t Thread::signal_set(int32_t signals) {
+    // osSignalSet is thread safe as long as the underlying
+    // thread does not get terminated or return from main
     return osSignalSet(_tid, signals);
 }
 
 int32_t Thread::signal_clr(int32_t signals) {
+    // osSignalClear is thread safe as long as the underlying
+    // thread does not get terminated or return from main
     return osSignalClear(_tid, signals);
 }
 
 Thread::State Thread::get_state() {
 #if !defined(__MBED_CMSIS_RTOS_CA9) && !defined(__MBED_CMSIS_RTOS_CM)
 #ifdef CMSIS_OS_RTX
-    return ((State)_thread_def.tcb.state);
+    State status = Deleted;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        status = (State)_thread_def.tcb.state;
+    }
+
+    _mutex.unlock();
+    return status;
 #endif
 #else
-    uint8_t status;
-    status = osThreadGetState(_tid);
-    return ((State)status);
+    State status = Deleted;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        status = (State)osThreadGetState(_tid);
+    }
+
+    _mutex.unlock();
+    return status;
 #endif
 }
 
 uint32_t Thread::stack_size() {
 #ifndef __MBED_CMSIS_RTOS_CA9
 #if defined(CMSIS_OS_RTX) && !defined(__MBED_CMSIS_RTOS_CM)
-    return _thread_def.tcb.priv_stack;
+    uint32_t size = 0;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        size = _thread_def.tcb.priv_stack;
+    }
+
+    _mutex.unlock();
+    return size;
 #else
-    P_TCB tcb = rt_tid2ptcb(_tid);
-    return tcb->priv_stack;
+    uint32_t size = 0;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        P_TCB tcb = rt_tid2ptcb(_tid);
+        size = tcb->priv_stack;
+    }
+
+    _mutex.unlock();
+    return size;
 #endif
 #else
     return 0;
@@ -151,12 +207,28 @@ uint32_t Thread::stack_size() {
 uint32_t Thread::free_stack() {
 #ifndef __MBED_CMSIS_RTOS_CA9
 #if defined(CMSIS_OS_RTX) && !defined(__MBED_CMSIS_RTOS_CM)
-    uint32_t bottom = (uint32_t)_thread_def.tcb.stack;
-    return _thread_def.tcb.tsk_stack - bottom;
+    uint32_t size = 0;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        uint32_t bottom = (uint32_t)_thread_def.tcb.stack;
+        size = _thread_def.tcb.tsk_stack - bottom;
+    }
+
+    _mutex.unlock();
+    return size;
 #else
-    P_TCB tcb = rt_tid2ptcb(_tid);
-    uint32_t bottom = (uint32_t)tcb->stack;
-    return tcb->tsk_stack - bottom;
+    uint32_t size = 0;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        P_TCB tcb = rt_tid2ptcb(_tid);
+        uint32_t bottom = (uint32_t)tcb->stack;
+        size = tcb->tsk_stack - bottom;
+    }
+
+    _mutex.unlock();
+    return size;
 #endif
 #else
     return 0;
@@ -166,12 +238,28 @@ uint32_t Thread::free_stack() {
 uint32_t Thread::used_stack() {
 #ifndef __MBED_CMSIS_RTOS_CA9
 #if defined(CMSIS_OS_RTX) && !defined(__MBED_CMSIS_RTOS_CM)
-    uint32_t top = (uint32_t)_thread_def.tcb.stack + _thread_def.tcb.priv_stack;
-    return top - _thread_def.tcb.tsk_stack;
+    uint32_t size = 0;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        uint32_t top = (uint32_t)_thread_def.tcb.stack + _thread_def.tcb.priv_stack;
+        size = top - _thread_def.tcb.tsk_stack;
+    }
+
+    _mutex.unlock();
+    return size;
 #else
-    P_TCB tcb = rt_tid2ptcb(_tid);
-    uint32_t top = (uint32_t)tcb->stack + tcb->priv_stack;
-    return top - tcb->tsk_stack;
+    uint32_t size = 0;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        P_TCB tcb = rt_tid2ptcb(_tid);
+        uint32_t top = (uint32_t)tcb->stack + tcb->priv_stack;
+        size =  top - tcb->tsk_stack;
+    }
+
+    _mutex.unlock();
+    return size;
 #endif
 #else
     return 0;
@@ -181,16 +269,32 @@ uint32_t Thread::used_stack() {
 uint32_t Thread::max_stack() {
 #ifndef __MBED_CMSIS_RTOS_CA9
 #if defined(CMSIS_OS_RTX) && !defined(__MBED_CMSIS_RTOS_CM)
-    uint32_t high_mark = 0;
-    while (_thread_def.tcb.stack[high_mark] == 0xE25A2EA5)
-        high_mark++;
-    return _thread_def.tcb.priv_stack - (high_mark * 4);
+    uint32_t size = 0;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        uint32_t high_mark = 0;
+        while (_thread_def.tcb.stack[high_mark] == 0xE25A2EA5)
+            high_mark++;
+        size = _thread_def.tcb.priv_stack - (high_mark * 4);
+    }
+
+    _mutex.unlock();
+    return size;
 #else
-    P_TCB tcb = rt_tid2ptcb(_tid);
-    uint32_t high_mark = 0;
-    while (tcb->stack[high_mark] == 0xE25A2EA5)
-        high_mark++;
-    return tcb->priv_stack - (high_mark * 4);
+    uint32_t size = 0;
+    _mutex.lock();
+
+    if (_tid != NULL) {
+        P_TCB tcb = rt_tid2ptcb(_tid);
+        uint32_t high_mark = 0;
+        while (tcb->stack[high_mark] == 0xE25A2EA5)
+            high_mark++;
+        size = tcb->priv_stack - (high_mark * 4);
+    }
+
+    _mutex.unlock();
+    return size;
 #endif
 #else
     return 0;
@@ -218,6 +322,7 @@ void Thread::attach_idle_hook(void (*fptr)(void)) {
 }
 
 Thread::~Thread() {
+    // terminate is thread safe
     terminate();
 #ifdef __MBED_CMSIS_RTOS_CM
     if (_dynamic_stack) {
@@ -226,4 +331,14 @@ Thread::~Thread() {
 #endif
 }
 
+void Thread::_thunk(const void * thread_ptr)
+{
+    Thread *t = (Thread*)thread_ptr;
+    t->_task();
+    t->_mutex.lock();
+    t->_tid = (osThreadId)NULL;
+    t->_join_sem.release();
+    // rtos will release the mutex automatically
+}
+
 }

rtos/rtos/Thread.h

@@ -26,6 +26,8 @@
 #include "cmsis_os.h"
 #include "Callback.h"
 #include "toolchain.h"
+#include "Semaphore.h"
+#include "Mutex.h"
 
 namespace rtos {
 
@@ -205,6 +207,9 @@ class Thread {
         WaitingSemaphore,   /**< Waiting for a semaphore event to occur */
         WaitingMailbox,     /**< Waiting for a mailbox event to occur */
         WaitingMutex,       /**< Waiting for a mutex event to occur */
+
+        /* Not in sync with RTX below here */
+        Deleted,            /**< The task has been deleted */
     };
 
     /** State of this Thread
@@ -275,11 +280,14 @@ class Thread {
                      osPriority priority=osPriorityNormal,
                      uint32_t stack_size=DEFAULT_STACK_SIZE,
                      unsigned char *stack_pointer=NULL);
+    static void _thunk(const void * thread_ptr);
 
     mbed::Callback<void()> _task;
     osThreadId _tid;
     osThreadDef_t _thread_def;
     bool _dynamic_stack;
+    Semaphore _join_sem;
+    Mutex _mutex;
 };
 
 }