diff --git a/include/mbedtls/threading.h b/include/mbedtls/threading.h index ed16a23b1..c136ea0b0 100644 --- a/include/mbedtls/threading.h +++ b/include/mbedtls/threading.h @@ -28,10 +28,13 @@ extern "C" { #include 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 diff --git a/library/threading.c b/library/threading.c index 52fe8fca9..d97f0cfe7 100644 --- a/library/threading.c +++ b/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; } diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index 6f405b00c..0ea1e98d8 100644 --- a/tests/src/threading_helpers.c +++ b/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: