From 5fa986c8cb77c081ab5046cdd8e519c1a1f4560b Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 10 Nov 2023 14:05:09 +0000 Subject: [PATCH 1/6] Move handling of mutex->is_valid into threading_helpers.c This is now a field only used for testing. Signed-off-by: Paul Elliott --- include/mbedtls/threading.h | 9 ++++++--- library/threading.c | 21 ++++++++++----------- tests/src/threading_helpers.c | 18 +++++++++++++----- 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/include/mbedtls/threading.h b/include/mbedtls/threading.h index ed16a23b1a..c136ea0b0d 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 52fe8fca99..d97f0cfe78 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 6f405b00c6..0ea1e98d82 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: From 3774637518fdb218ae899e991113d4459095de88 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 12 Nov 2023 19:05:57 +0000 Subject: [PATCH 2/6] 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 c136ea0b0d..cdfa7d69e5 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 0ea1e98d82..0ffffbfd54 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) From 9e25936241c1e6096779899ddc1fc1cb068ed93f Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Wed, 15 Nov 2023 11:33:32 +0000 Subject: [PATCH 3/6] Rename mutex->is_valid to mutex->state Rename struct member to make it more representative of its current use. Signed-off-by: Paul Elliott --- include/mbedtls/threading.h | 6 +++--- tests/src/threading_helpers.c | 16 ++++++++-------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/threading.h b/include/mbedtls/threading.h index cdfa7d69e5..b504233bdc 100644 --- a/include/mbedtls/threading.h +++ b/include/mbedtls/threading.h @@ -29,12 +29,12 @@ extern "C" { typedef struct mbedtls_threading_mutex_t { pthread_mutex_t MBEDTLS_PRIVATE(mutex); - /* WARNING - is_valid should only be accessed when holding the mutex lock in + /* WARNING - state 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 + * state 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); + char MBEDTLS_PRIVATE(state); } mbedtls_threading_mutex_t; #endif diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index 0ffffbfd54..385a079261 100644 --- a/tests/src/threading_helpers.c +++ b/tests/src/threading_helpers.c @@ -58,8 +58,8 @@ * indicate the exact location of the problematic call. To locate the error, * use a debugger and set a breakpoint on mbedtls_test_mutex_usage_error(). */ -enum value_of_mutex_is_valid_field { - /* Potential values for the is_valid field of mbedtls_threading_mutex_t. +enum value_of_mutex_state_field { + /* Potential values for the state field of mbedtls_threading_mutex_t. * Note that MUTEX_FREED must be 0 and MUTEX_IDLE must be 1 for * compatibility with threading_mutex_init_pthread() and * threading_mutex_free_pthread(). MUTEX_LOCKED could be any nonzero @@ -117,12 +117,12 @@ static void mbedtls_test_wrap_mutex_free(mbedtls_threading_mutex_t *mutex) { if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) { - switch (mutex->is_valid) { + switch (mutex->state) { case MUTEX_FREED: mbedtls_test_mutex_usage_error(mutex, "free without init or double free"); break; case MUTEX_IDLE: - mutex->is_valid = MUTEX_FREED; + mutex->state = MUTEX_FREED; --live_mutexes; break; case MUTEX_LOCKED: @@ -145,13 +145,13 @@ static int mbedtls_test_wrap_mutex_lock(mbedtls_threading_mutex_t *mutex) * condition. */ int ret = mutex_functions.lock(mutex); if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) { - switch (mutex->is_valid) { + switch (mutex->state) { case MUTEX_FREED: mbedtls_test_mutex_usage_error(mutex, "lock without init"); break; case MUTEX_IDLE: if (ret == 0) { - mutex->is_valid = MUTEX_LOCKED; + mutex->state = MUTEX_LOCKED; } break; case MUTEX_LOCKED: @@ -173,7 +173,7 @@ static int mbedtls_test_wrap_mutex_unlock(mbedtls_threading_mutex_t *mutex) * 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) { + switch (mutex->state) { case MUTEX_FREED: mbedtls_test_mutex_usage_error(mutex, "unlock without init"); break; @@ -181,7 +181,7 @@ static int mbedtls_test_wrap_mutex_unlock(mbedtls_threading_mutex_t *mutex) mbedtls_test_mutex_usage_error(mutex, "unlock without lock"); break; case MUTEX_LOCKED: - mutex->is_valid = MUTEX_IDLE; + mutex->state = MUTEX_IDLE; break; default: mbedtls_test_mutex_usage_error(mutex, "corrupted state"); From f25d83112389ec4b4cc23ae6c005c56fec53841f Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 23 Nov 2023 18:49:43 +0000 Subject: [PATCH 4/6] Ensure mutex test mutex gets free'd Signed-off-by: Paul Elliott --- programs/ssl/ssl_test_lib.c | 3 +++ tests/include/test/helpers.h | 12 ++++++++++-- tests/src/threading_helpers.c | 10 ++++++++++ tests/suites/host_test.function | 4 ++++ 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/programs/ssl/ssl_test_lib.c b/programs/ssl/ssl_test_lib.c index 6e0c6153f1..b49dd67c26 100644 --- a/programs/ssl/ssl_test_lib.c +++ b/programs/ssl/ssl_test_lib.c @@ -435,6 +435,9 @@ int test_hooks_failure_detected(void) void test_hooks_free(void) { +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + mbedtls_test_mutex_usage_end(); +#endif } #endif /* MBEDTLS_TEST_HOOKS */ diff --git a/tests/include/test/helpers.h b/tests/include/test/helpers.h index ba117fbdfc..4708df1632 100644 --- a/tests/include/test/helpers.h +++ b/tests/include/test/helpers.h @@ -240,10 +240,18 @@ int mbedtls_test_hexcmp(uint8_t *a, uint8_t *b, #endif #if defined(MBEDTLS_TEST_MUTEX_USAGE) -/** Permanently activate the mutex usage verification framework. See - * threading_helpers.c for information. */ +/** + * Activate the mutex usage verification framework. See threading_helpers.c for + * information. + * */ void mbedtls_test_mutex_usage_init(void); +/** + * Deactivate the mutex usage verification framework. See threading_helpers.c + * for information. + */ +void mbedtls_test_mutex_usage_end(void); + /** Call this function after executing a test case to check for mutex usage * errors. */ void mbedtls_test_mutex_usage_check(void); diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index 385a079261..434d124f18 100644 --- a/tests/src/threading_helpers.c +++ b/tests/src/threading_helpers.c @@ -228,4 +228,14 @@ void mbedtls_test_mutex_usage_check(void) mbedtls_test_info.mutex_usage_error = NULL; } +void mbedtls_test_mutex_usage_end(void) +{ + mbedtls_mutex_init = mutex_functions.init; + mbedtls_mutex_free = mutex_functions.free; + mbedtls_mutex_lock = mutex_functions.lock; + mbedtls_mutex_unlock = mutex_functions.unlock; + + mutex_functions.free(&mbedtls_test_mutex_mutex); +} + #endif /* MBEDTLS_TEST_MUTEX_USAGE */ diff --git a/tests/suites/host_test.function b/tests/suites/host_test.function index d8ff49ef17..cc286973cf 100644 --- a/tests/suites/host_test.function +++ b/tests/suites/host_test.function @@ -772,6 +772,10 @@ int execute_tests(int argc, const char **argv) mbedtls_fprintf(stdout, " (%u / %u tests (%u skipped))\n", total_tests - total_errors, total_tests, total_skipped); +#if defined(MBEDTLS_TEST_MUTEX_USAGE) + mbedtls_test_mutex_usage_end(); +#endif + #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && \ !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) #if defined(MBEDTLS_MEMORY_DEBUG) From 8c6d332c44bd4a212119c278f3f8a21fb420257d Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 23 Nov 2023 18:53:13 +0000 Subject: [PATCH 5/6] Fix comment typos Signed-off-by: Paul Elliott --- library/threading.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/threading.c b/library/threading.c index d97f0cfe78..873b5077b8 100644 --- a/library/threading.c +++ b/library/threading.c @@ -59,9 +59,9 @@ static void threading_mutex_init_pthread(mbedtls_threading_mutex_t *mutex) /* 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. */ + * hit, so state transitions are checked in tests only via the state + * variable. Please make sure any new mutex that gets added is exercised in + * tests; see tests/src/threading_helpers.c for more details. */ (void) pthread_mutex_init(&mutex->mutex, NULL); } From 392ed3fe7fe9d0ed7769fbb3ad9a6329114a7e1e Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 24 Nov 2023 15:48:28 +0000 Subject: [PATCH 6/6] Add better documentation for mbedtls_test_mutex_mutex Signed-off-by: Paul Elliott --- tests/src/threading_helpers.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index 434d124f18..5fbf65b2da 100644 --- a/tests/src/threading_helpers.c +++ b/tests/src/threading_helpers.c @@ -77,12 +77,30 @@ typedef struct { } mutex_functions_t; static mutex_functions_t mutex_functions; +/** + * The mutex used to guard live_mutexes below and access to the status variable + * in every mbedtls_threading_mutex_t. + * Note that we are not reporting any errors when locking and unlocking this + * mutex. This is for a couple of reasons: + * + * 1. We have no real way of reporting any errors with this mutex - we cannot + * report it back to the caller, as the failure was not that of the mutex + * passed in. We could fail the test, but again this would indicate a problem + * with the test code that did not exist. + * + * 2. Any failure to lock is unlikely to be intermittent, and will thus not + * give false test results - the overall result would be to turn off the + * testing. This is not a situation that is likely to happen with normal + * testing and we still have TSan to fall back on should this happen. + */ 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(). +/** + * The total number of calls to mbedtls_mutex_init(), minus the total number + * of calls to mbedtls_mutex_free(). * - * Reset to 0 after each test case. + * Do not read or write without holding mbedtls_test_mutex_mutex (above). Reset + * to 0 after each test case. */ static int live_mutexes;