Move handling of mutex->is_valid into threading_helpers.c

This is now a field only used for testing.

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

include/mbedtls/threading.h

@@ -28,10 +28,13 @@ extern "C" {
 #include <pthread.h>
 typedef struct mbedtls_threading_mutex_t {
     pthread_mutex_t MBEDTLS_PRIVATE(mutex);
-    /* is_valid is 0 after a failed init or a free, and nonzero after a
-     * successful init. This field is not considered part of the public
-     * API of Mbed TLS and may change without notice. */
+
+    /* is_valid is controlled by code in tests/src/threading_helpers - it will
+     * be 0 after a failed init or a free, and nonzero after a 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;
 #endif
 

library/threading.c

@@ -56,28 +56,27 @@ static void threading_mutex_init_pthread(mbedtls_threading_mutex_t *mutex)
         return;
     }
 
-    /* A nonzero value of is_valid indicates a successfully initialized
-     * mutex. This is a workaround for not being able to return an error
-     * code for this function. The lock/unlock functions return an error
-     * if is_valid is nonzero. The Mbed TLS unit test code uses this field
-     * to distinguish more states of the mutex; see
-     * tests/src/threading_helpers for details. */
-    mutex->is_valid = pthread_mutex_init(&mutex->mutex, NULL) == 0;
+    /* One problem here is that calling lock on a pthread mutex without first
+     * having initialised it is undefined behaviour. Obviously we cannot check
+     * this here in a thread safe manner without a significant performance
+     * hit, so state transitions are checked in tests only via the is_valid
+     * varaible. Please make sure any new mutex that gets added is exercised in
+     * tests; see tests/src/threading_helpers for more details. */
+    (void) pthread_mutex_init(&mutex->mutex, NULL);
 }
 
 static void threading_mutex_free_pthread(mbedtls_threading_mutex_t *mutex)
 {
-    if (mutex == NULL || !mutex->is_valid) {
+    if (mutex == NULL) {
         return;
     }
 
     (void) pthread_mutex_destroy(&mutex->mutex);
-    mutex->is_valid = 0;
 }
 
 static int threading_mutex_lock_pthread(mbedtls_threading_mutex_t *mutex)
 {
-    if (mutex == NULL || !mutex->is_valid) {
+    if (mutex == NULL) {
         return MBEDTLS_ERR_THREADING_BAD_INPUT_DATA;
     }
 
@@ -90,7 +89,7 @@ static int threading_mutex_lock_pthread(mbedtls_threading_mutex_t *mutex)
 
 static int threading_mutex_unlock_pthread(mbedtls_threading_mutex_t *mutex)
 {
-    if (mutex == NULL || !mutex->is_valid) {
+    if (mutex == NULL) {
         return MBEDTLS_ERR_THREADING_BAD_INPUT_DATA;
     }
 

tests/src/threading_helpers.c

@@ -64,9 +64,9 @@ enum value_of_mutex_is_valid_field {
      * compatibility with threading_mutex_init_pthread() and
      * threading_mutex_free_pthread(). MUTEX_LOCKED could be any nonzero
      * value. */
-    MUTEX_FREED = 0, //!< Set by threading_mutex_free_pthread
-    MUTEX_IDLE = 1, //!< Set by threading_mutex_init_pthread and by our unlock
-    MUTEX_LOCKED = 2, //!< Set by our lock
+    MUTEX_FREED = 0, //! < Set by mbedtls_test_wrap_mutex_free
+    MUTEX_IDLE = 1, //! < Set by mbedtls_test_wrap_mutex_init and by mbedtls_test_wrap_mutex_unlock
+    MUTEX_LOCKED = 2, //! < Set by mbedtls_test_wrap_mutex_lock
 };
 
 typedef struct {
@@ -101,8 +101,12 @@ static void mbedtls_test_mutex_usage_error(mbedtls_threading_mutex_t *mutex,
 static void mbedtls_test_wrap_mutex_init(mbedtls_threading_mutex_t *mutex)
 {
     mutex_functions.init(mutex);
-    if (mutex->is_valid) {
+
+    if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) {
+        mutex->state = MUTEX_IDLE;
         ++live_mutexes;
+
+        mutex_functions.unlock(&mbedtls_test_mutex_mutex);
     }
 }
 
@@ -123,7 +127,11 @@ static void mbedtls_test_wrap_mutex_free(mbedtls_threading_mutex_t *mutex)
             mbedtls_test_mutex_usage_error(mutex, "corrupted state");
             break;
     }
+
+    /* Mark mutex as free'd first, because we need to release the mutex. If
+     * free fails, this could end up with inconsistent state. */
     if (mutex->is_valid) {
+        mutex->is_valid = MUTEX_FREED;
         --live_mutexes;
     }
     mutex_functions.free(mutex);
@@ -138,7 +146,7 @@ static int mbedtls_test_wrap_mutex_lock(mbedtls_threading_mutex_t *mutex)
             break;
         case MUTEX_IDLE:
             if (ret == 0) {
-                mutex->is_valid = 2;
+                mutex->is_valid = MUTEX_LOCKED;
             }
             break;
         case MUTEX_LOCKED: