Make threading helpers tests thread safe

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

include/mbedtls/threading.h

@@ -29,10 +29,11 @@ extern "C" {
 typedef struct mbedtls_threading_mutex_t {
     pthread_mutex_t MBEDTLS_PRIVATE(mutex);
 
-    /* is_valid is controlled by code in tests/src/threading_helpers - it will
+    /* WARNING - is_valid should only be accessed when holding the mutex lock in
-     * be 0 after a failed init or a free, and nonzero after a successful init.
+     * tests/src/threading_helpers.c, otherwise corruption can occur.
-     * This field is for testing only and thus not considered part of the
+     * is_valid will be 0 after a failed init or a free, and nonzero after a
-     * public API of Mbed TLS and may change without notice. */
+     * successful init. This field is for testing only and thus not considered
+     * part of the public API of Mbed TLS and may change without notice.*/
     char MBEDTLS_PRIVATE(is_valid);
 
 } mbedtls_threading_mutex_t;

tests/src/threading_helpers.c

@@ -77,6 +77,8 @@ typedef struct {
 } mutex_functions_t;
 static mutex_functions_t mutex_functions;
 
+mbedtls_threading_mutex_t mbedtls_test_mutex_mutex;
+
 /** The total number of calls to mbedtls_mutex_init(), minus the total number
  * of calls to mbedtls_mutex_free().
  *
@@ -88,6 +90,7 @@ static void mbedtls_test_mutex_usage_error(mbedtls_threading_mutex_t *mutex,
                                            const char *msg)
 {
     (void) mutex;
+
     if (mbedtls_test_info.mutex_usage_error == NULL) {
         mbedtls_test_info.mutex_usage_error = msg;
     }
@@ -112,73 +115,81 @@ static void mbedtls_test_wrap_mutex_init(mbedtls_threading_mutex_t *mutex)
 
 static void mbedtls_test_wrap_mutex_free(mbedtls_threading_mutex_t *mutex)
 {
-    switch (mutex->is_valid) {
+    if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) {
-        case MUTEX_FREED:
-            mbedtls_test_mutex_usage_error(mutex, "free without init or double free");
-            break;
-        case MUTEX_IDLE:
-            /* Do nothing. The underlying free function will reset is_valid
-             * to 0. */
-            break;
-        case MUTEX_LOCKED:
-            mbedtls_test_mutex_usage_error(mutex, "free without unlock");
-            break;
-        default:
-            mbedtls_test_mutex_usage_error(mutex, "corrupted state");
-            break;
-    }
 
-    /* Mark mutex as free'd first, because we need to release the mutex. If
+        switch (mutex->is_valid) {
-     * free fails, this could end up with inconsistent state. */
+            case MUTEX_FREED:
-    if (mutex->is_valid) {
+                mbedtls_test_mutex_usage_error(mutex, "free without init or double free");
-        mutex->is_valid = MUTEX_FREED;
+                break;
-        --live_mutexes;
+            case MUTEX_IDLE:
+                mutex->is_valid = MUTEX_FREED;
+                --live_mutexes;
+                break;
+            case MUTEX_LOCKED:
+                mbedtls_test_mutex_usage_error(mutex, "free without unlock");
+                break;
+            default:
+                mbedtls_test_mutex_usage_error(mutex, "corrupted state");
+                break;
+        }
+
+        mutex_functions.unlock(&mbedtls_test_mutex_mutex);
     }
     mutex_functions.free(mutex);
 }
 
 static int mbedtls_test_wrap_mutex_lock(mbedtls_threading_mutex_t *mutex)
 {
+    /* Lock the passed in mutex first, so that the only way to change the state
+     * is to hold the passed in and internal mutex - otherwise we create a race
+     * condition. */
     int ret = mutex_functions.lock(mutex);
-    switch (mutex->is_valid) {
+    if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) {
-        case MUTEX_FREED:
+        switch (mutex->is_valid) {
-            mbedtls_test_mutex_usage_error(mutex, "lock without init");
+            case MUTEX_FREED:
-            break;
+                mbedtls_test_mutex_usage_error(mutex, "lock without init");
-        case MUTEX_IDLE:
+                break;
-            if (ret == 0) {
+            case MUTEX_IDLE:
-                mutex->is_valid = MUTEX_LOCKED;
+                if (ret == 0) {
-            }
+                    mutex->is_valid = MUTEX_LOCKED;
-            break;
+                }
-        case MUTEX_LOCKED:
+                break;
-            mbedtls_test_mutex_usage_error(mutex, "double lock");
+            case MUTEX_LOCKED:
-            break;
+                mbedtls_test_mutex_usage_error(mutex, "double lock");
-        default:
+                break;
-            mbedtls_test_mutex_usage_error(mutex, "corrupted state");
+            default:
-            break;
+                mbedtls_test_mutex_usage_error(mutex, "corrupted state");
+                break;
+        }
+
+        mutex_functions.unlock(&mbedtls_test_mutex_mutex);
     }
     return ret;
 }
 
 static int mbedtls_test_wrap_mutex_unlock(mbedtls_threading_mutex_t *mutex)
 {
-    int ret = mutex_functions.unlock(mutex);
+    /* Lock the internal mutex first and change state, so that the only way to
-    switch (mutex->is_valid) {
+     * change the state is to hold the passed in and internal mutex - otherwise
-        case MUTEX_FREED:
+     * we create a race condition. */
-            mbedtls_test_mutex_usage_error(mutex, "unlock without init");
+    if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) {
-            break;
+        switch (mutex->is_valid) {
-        case MUTEX_IDLE:
+            case MUTEX_FREED:
-            mbedtls_test_mutex_usage_error(mutex, "unlock without lock");
+                mbedtls_test_mutex_usage_error(mutex, "unlock without init");
-            break;
+                break;
-        case MUTEX_LOCKED:
+            case MUTEX_IDLE:
-            if (ret == 0) {
+                mbedtls_test_mutex_usage_error(mutex, "unlock without lock");
+                break;
+            case MUTEX_LOCKED:
                 mutex->is_valid = MUTEX_IDLE;
-            }
+                break;
-            break;
+            default:
-        default:
+                mbedtls_test_mutex_usage_error(mutex, "corrupted state");
-            mbedtls_test_mutex_usage_error(mutex, "corrupted state");
+                break;
-            break;
+        }
+        mutex_functions.unlock(&mbedtls_test_mutex_mutex);
     }
-    return ret;
+    return mutex_functions.unlock(mutex);
 }
 
 void mbedtls_test_mutex_usage_init(void)
@@ -191,6 +202,8 @@ void mbedtls_test_mutex_usage_init(void)
     mbedtls_mutex_free = &mbedtls_test_wrap_mutex_free;
     mbedtls_mutex_lock = &mbedtls_test_wrap_mutex_lock;
     mbedtls_mutex_unlock = &mbedtls_test_wrap_mutex_unlock;
+
+    mutex_functions.init(&mbedtls_test_mutex_mutex);
 }
 
 void mbedtls_test_mutex_usage_check(void)