From 3774637518fdb218ae899e991113d4459095de88 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 12 Nov 2023 19:05:57 +0000 Subject: [PATCH] Make threading helpers tests thread safe Signed-off-by: Paul Elliott --- include/mbedtls/threading.h | 9 +-- tests/src/threading_helpers.c | 115 +++++++++++++++++++--------------- 2 files changed, 69 insertions(+), 55 deletions(-) diff --git a/include/mbedtls/threading.h b/include/mbedtls/threading.h index c136ea0b0..cdfa7d69e 100644 --- a/include/mbedtls/threading.h +++ b/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 - * 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. */ + /* WARNING - is_valid should only be accessed when holding the mutex lock in + * tests/src/threading_helpers.c, otherwise corruption can occur. + * is_valid 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; diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index 0ea1e98d8..0ffffbfd5 100644 --- a/tests/src/threading_helpers.c +++ b/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) { - 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; - } + if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) { - /* 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; + switch (mutex->is_valid) { + case MUTEX_FREED: + mbedtls_test_mutex_usage_error(mutex, "free without init or double free"); + break; + 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) { - case MUTEX_FREED: - mbedtls_test_mutex_usage_error(mutex, "lock without init"); - break; - case MUTEX_IDLE: - if (ret == 0) { - mutex->is_valid = MUTEX_LOCKED; - } - break; - case MUTEX_LOCKED: - mbedtls_test_mutex_usage_error(mutex, "double lock"); - break; - default: - mbedtls_test_mutex_usage_error(mutex, "corrupted state"); - break; + if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) { + switch (mutex->is_valid) { + case MUTEX_FREED: + mbedtls_test_mutex_usage_error(mutex, "lock without init"); + break; + case MUTEX_IDLE: + if (ret == 0) { + mutex->is_valid = MUTEX_LOCKED; + } + break; + case MUTEX_LOCKED: + mbedtls_test_mutex_usage_error(mutex, "double lock"); + break; + default: + 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); - switch (mutex->is_valid) { - case MUTEX_FREED: - mbedtls_test_mutex_usage_error(mutex, "unlock without init"); - break; - case MUTEX_IDLE: - mbedtls_test_mutex_usage_error(mutex, "unlock without lock"); - break; - case MUTEX_LOCKED: - if (ret == 0) { + /* Lock the internal mutex first and change state, so that the only way to + * change the state is to hold the passed in and internal mutex - otherwise + * we create a race condition. */ + if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) { + switch (mutex->is_valid) { + case MUTEX_FREED: + mbedtls_test_mutex_usage_error(mutex, "unlock without init"); + break; + case MUTEX_IDLE: + mbedtls_test_mutex_usage_error(mutex, "unlock without lock"); + break; + case MUTEX_LOCKED: mutex->is_valid = MUTEX_IDLE; - } - break; - default: - mbedtls_test_mutex_usage_error(mutex, "corrupted state"); - break; + break; + default: + mbedtls_test_mutex_usage_error(mutex, "corrupted state"); + 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)