From e66c841c7362ea0a549d7df383dc1e990b03677d Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 13 Feb 2024 15:33:26 +0000 Subject: [PATCH 1/5] Make internal test info accessor functions static. Signed-off-by: Paul Elliott --- tests/src/helpers.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/src/helpers.c b/tests/src/helpers.c index b9233be956..b810253e6c 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -50,7 +50,7 @@ mbedtls_test_result_t mbedtls_test_get_result(void) return result; } -void mbedtls_test_set_result(mbedtls_test_result_t result, const char *test, +static void mbedtls_test_set_result(mbedtls_test_result_t result, const char *test, int line_no, const char *filename) { /* Internal function only - mbedtls_test_info_mutex should be held prior @@ -144,7 +144,7 @@ unsigned long mbedtls_test_get_step(void) return step; } -void mbedtls_test_reset_step(void) +static void mbedtls_test_reset_step(void) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ @@ -178,7 +178,7 @@ void mbedtls_test_get_line1(char *line) #endif /* MBEDTLS_THREADING_C */ } -void mbedtls_test_set_line1(const char *line) +static void mbedtls_test_set_line1(const char *line) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ @@ -203,7 +203,7 @@ void mbedtls_test_get_line2(char *line) #endif /* MBEDTLS_THREADING_C */ } -void mbedtls_test_set_line2(const char *line) +static void mbedtls_test_set_line2(const char *line) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ @@ -255,7 +255,7 @@ unsigned mbedtls_test_get_case_uses_negative_0(void) return test_case_uses_negative_0; } -void mbedtls_test_set_case_uses_negative_0(unsigned uses) +static void mbedtls_test_set_case_uses_negative_0(unsigned uses) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ From 264e21011acbf06701c1e5da17b3fdbf3c8589ad Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 15 Feb 2024 12:28:56 +0000 Subject: [PATCH 2/5] Rename internal test info data accessors Rename internal test info data accessors by adding _internal to mark them as obviously internal. Add to the intial comment block to further explain the mutex locking policy. Signed-off-by: Paul Elliott --- tests/src/helpers.c | 50 +++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/tests/src/helpers.c b/tests/src/helpers.c index b810253e6c..9053aac4b7 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -31,7 +31,17 @@ mbedtls_threading_mutex_t mbedtls_test_info_mutex; #endif /* MBEDTLS_THREADING_C */ /*----------------------------------------------------------------------------*/ -/* Mbedtls Test Info accessors */ +/* Mbedtls Test Info accessors + * + * NOTE - there are two types of accessors here; the _internal functions, which + * are expected to be used from this module *only*. These do not attempt to lock + * the mbedtls_test_info_mutex, as they are designed to be called from within + * public functions which do lock the mutex first (if mutexes are enabled). + * The main reason for this is the need to set some sets of test data + * atomically, without releasing the mutex inbetween to prevent race conditions + * resulting in mixed test info. The other public accessors have prototypes in + * the header and have to lock the mutex for safety as we obviously cannot + * control where they are called from. */ mbedtls_test_result_t mbedtls_test_get_result(void) { @@ -50,8 +60,8 @@ mbedtls_test_result_t mbedtls_test_get_result(void) return result; } -static void mbedtls_test_set_result(mbedtls_test_result_t result, const char *test, - int line_no, const char *filename) +static void mbedtls_test_set_result_internal(mbedtls_test_result_t result, const char *test, + int line_no, const char *filename) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ @@ -144,7 +154,7 @@ unsigned long mbedtls_test_get_step(void) return step; } -static void mbedtls_test_reset_step(void) +static void mbedtls_test_reset_step_internal(void) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ @@ -178,7 +188,7 @@ void mbedtls_test_get_line1(char *line) #endif /* MBEDTLS_THREADING_C */ } -static void mbedtls_test_set_line1(const char *line) +static void mbedtls_test_set_line1_internal(const char *line) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ @@ -203,7 +213,7 @@ void mbedtls_test_get_line2(char *line) #endif /* MBEDTLS_THREADING_C */ } -static void mbedtls_test_set_line2(const char *line) +static void mbedtls_test_set_line2_internal(const char *line) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ @@ -255,7 +265,7 @@ unsigned mbedtls_test_get_case_uses_negative_0(void) return test_case_uses_negative_0; } -static void mbedtls_test_set_case_uses_negative_0(unsigned uses) +static void mbedtls_test_set_case_uses_negative_0_internal(unsigned uses) { /* Internal function only - mbedtls_test_info_mutex should be held prior * to calling this function. */ @@ -350,7 +360,7 @@ static void mbedtls_test_fail_internal(const char *test, int line_no, const char if (mbedtls_test_info.result != MBEDTLS_TEST_RESULT_FAILED) { /* If we have already recorded the test as having failed then don't * overwrite any previous information about the failure. */ - mbedtls_test_set_result(MBEDTLS_TEST_RESULT_FAILED, test, line_no, filename); + mbedtls_test_set_result_internal(MBEDTLS_TEST_RESULT_FAILED, test, line_no, filename); } } @@ -373,7 +383,7 @@ void mbedtls_test_skip(const char *test, int line_no, const char *filename) mbedtls_mutex_lock(&mbedtls_test_info_mutex); #endif /* MBEDTLS_THREADING_C */ - mbedtls_test_set_result(MBEDTLS_TEST_RESULT_SKIPPED, test, line_no, filename); + mbedtls_test_set_result_internal(MBEDTLS_TEST_RESULT_SKIPPED, test, line_no, filename); #ifdef MBEDTLS_THREADING_C mbedtls_mutex_unlock(&mbedtls_test_info_mutex); @@ -386,13 +396,13 @@ void mbedtls_test_info_reset(void) mbedtls_mutex_lock(&mbedtls_test_info_mutex); #endif /* MBEDTLS_THREADING_C */ - mbedtls_test_set_result(MBEDTLS_TEST_RESULT_SUCCESS, 0, 0, 0); - mbedtls_test_reset_step(); - mbedtls_test_set_line1(NULL); - mbedtls_test_set_line2(NULL); + mbedtls_test_set_result_internal(MBEDTLS_TEST_RESULT_SUCCESS, 0, 0, 0); + mbedtls_test_reset_step_internal(); + mbedtls_test_set_line1_internal(NULL); + mbedtls_test_set_line2_internal(NULL); #if defined(MBEDTLS_BIGNUM_C) - mbedtls_test_set_case_uses_negative_0(0); + mbedtls_test_set_case_uses_negative_0_internal(0); #endif #ifdef MBEDTLS_THREADING_C @@ -424,11 +434,11 @@ int mbedtls_test_equal(const char *test, int line_no, const char *filename, (void) mbedtls_snprintf(buf, sizeof(buf), "lhs = 0x%016llx = %lld", value1, (long long) value1); - mbedtls_test_set_line1(buf); + mbedtls_test_set_line1_internal(buf); (void) mbedtls_snprintf(buf, sizeof(buf), "rhs = 0x%016llx = %lld", value2, (long long) value2); - mbedtls_test_set_line2(buf); + mbedtls_test_set_line2_internal(buf); } #ifdef MBEDTLS_THREADING_C @@ -462,11 +472,11 @@ int mbedtls_test_le_u(const char *test, int line_no, const char *filename, (void) mbedtls_snprintf(buf, sizeof(buf), "lhs = 0x%016llx = %llu", value1, value1); - mbedtls_test_set_line1(buf); + mbedtls_test_set_line1_internal(buf); (void) mbedtls_snprintf(buf, sizeof(buf), "rhs = 0x%016llx = %llu", value2, value2); - mbedtls_test_set_line2(buf); + mbedtls_test_set_line2_internal(buf); } #ifdef MBEDTLS_THREADING_C @@ -500,11 +510,11 @@ int mbedtls_test_le_s(const char *test, int line_no, const char *filename, (void) mbedtls_snprintf(buf, sizeof(buf), "lhs = 0x%016llx = %lld", (unsigned long long) value1, value1); - mbedtls_test_set_line1(buf); + mbedtls_test_set_line1_internal(buf); (void) mbedtls_snprintf(buf, sizeof(buf), "rhs = 0x%016llx = %lld", (unsigned long long) value2, value2); - mbedtls_test_set_line2(buf); + mbedtls_test_set_line2_internal(buf); } #ifdef MBEDTLS_THREADING_C From 114ed5ef1ee57f881d208f3f69772225c54a6f4b Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 13 Feb 2024 15:35:14 +0000 Subject: [PATCH 3/5] Fix missing mutex lock for mutex usage error Although this can only be read in a situation where threads should have already stopped, best to fix this as its public. Signed-off-by: Paul Elliott --- tests/src/helpers.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/src/helpers.c b/tests/src/helpers.c index 9053aac4b7..8e1d564145 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -229,7 +229,19 @@ static void mbedtls_test_set_line2_internal(const char *line) #if defined(MBEDTLS_TEST_MUTEX_USAGE) const char *mbedtls_test_get_mutex_usage_error(void) { - return mbedtls_test_info.mutex_usage_error; + const char *usage_error; + +#ifdef MBEDTLS_THREADING_C + mbedtls_mutex_lock(&mbedtls_test_info_mutex); +#endif /* MBEDTLS_THREADING_C */ + + usage_error = mbedtls_test_info.mutex_usage_error; + +#ifdef MBEDTLS_THREADING_C + mbedtls_mutex_unlock(&mbedtls_test_info_mutex); +#endif /* MBEDTLS_THREADING_C */ + + return usage_error; } void mbedtls_test_set_mutex_usage_error(const char *msg) From ba536dc1dbd74d9d8870a4f874bf82e92859559e Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 13 Feb 2024 15:36:47 +0000 Subject: [PATCH 4/5] Lock test mutex before doing mutex usage check Although this again should only happen post all threads stopping, guard this just in case things change. Signed-off-by: Paul Elliott --- tests/src/threading_helpers.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index ff0c712faf..5eac02909e 100644 --- a/tests/src/threading_helpers.c +++ b/tests/src/threading_helpers.c @@ -317,22 +317,26 @@ void mbedtls_test_mutex_usage_init(void) void mbedtls_test_mutex_usage_check(void) { - if (live_mutexes != 0) { - /* A positive number (more init than free) means that a mutex resource - * is leaking (on platforms where a mutex consumes more than the - * mbedtls_threading_mutex_t object itself). The rare case of a - * negative number means a missing init somewhere. */ - mbedtls_fprintf(stdout, "[mutex: %d leaked] ", live_mutexes); - live_mutexes = 0; - mbedtls_test_set_mutex_usage_error("missing free"); + if (mutex_functions.lock(&mbedtls_test_mutex_mutex) == 0) { + if (live_mutexes != 0) { + /* A positive number (more init than free) means that a mutex resource + * is leaking (on platforms where a mutex consumes more than the + * mbedtls_threading_mutex_t object itself). The rare case of a + * negative number means a missing init somewhere. */ + mbedtls_fprintf(stdout, "[mutex: %d leaked] ", live_mutexes); + live_mutexes = 0; + mbedtls_test_set_mutex_usage_error("missing free"); + } + if (mbedtls_test_get_mutex_usage_error() != NULL && + mbedtls_test_get_result() != MBEDTLS_TEST_RESULT_FAILED) { + /* Functionally, the test passed. But there was a mutex usage error, + * so mark the test as failed after all. */ + mbedtls_test_fail("Mutex usage error", __LINE__, __FILE__); + } + mbedtls_test_set_mutex_usage_error(NULL); + + mutex_functions.unlock(&mbedtls_test_mutex_mutex); } - if (mbedtls_test_get_mutex_usage_error() != NULL && - mbedtls_test_get_result() != MBEDTLS_TEST_RESULT_FAILED) { - /* Functionally, the test passed. But there was a mutex usage error, - * so mark the test as failed after all. */ - mbedtls_test_fail("Mutex usage error", __LINE__, __FILE__); - } - mbedtls_test_set_mutex_usage_error(NULL); } void mbedtls_test_mutex_usage_end(void) From 9011dae0c18d6195b756d43b823265a9b0a685e4 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sat, 24 Feb 2024 10:57:22 +0000 Subject: [PATCH 5/5] Improve documentation / comments Signed-off-by: Paul Elliott --- tests/src/helpers.c | 17 ++++++++--------- tests/src/threading_helpers.c | 4 ++-- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/tests/src/helpers.c b/tests/src/helpers.c index 8e1d564145..24334228ff 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -33,15 +33,14 @@ mbedtls_threading_mutex_t mbedtls_test_info_mutex; /*----------------------------------------------------------------------------*/ /* Mbedtls Test Info accessors * - * NOTE - there are two types of accessors here; the _internal functions, which - * are expected to be used from this module *only*. These do not attempt to lock - * the mbedtls_test_info_mutex, as they are designed to be called from within - * public functions which do lock the mutex first (if mutexes are enabled). - * The main reason for this is the need to set some sets of test data - * atomically, without releasing the mutex inbetween to prevent race conditions - * resulting in mixed test info. The other public accessors have prototypes in - * the header and have to lock the mutex for safety as we obviously cannot - * control where they are called from. */ + * NOTE - there are two types of accessors here: public accessors and internal + * accessors. The public accessors have prototypes in helpers.h and lock + * mbedtls_test_info_mutex (if mutexes are enabled). The _internal accessors, + * which are expected to be used from this module *only*, do not lock the mutex. + * These are designed to be called from within public functions which already + * hold the mutex. The main reason for this difference is the need to set + * multiple test data values atomically (without releasing the mutex) to prevent + * race conditions. */ mbedtls_test_result_t mbedtls_test_get_result(void) { diff --git a/tests/src/threading_helpers.c b/tests/src/threading_helpers.c index 5eac02909e..c1686c21ad 100644 --- a/tests/src/threading_helpers.c +++ b/tests/src/threading_helpers.c @@ -321,8 +321,8 @@ void mbedtls_test_mutex_usage_check(void) if (live_mutexes != 0) { /* A positive number (more init than free) means that a mutex resource * is leaking (on platforms where a mutex consumes more than the - * mbedtls_threading_mutex_t object itself). The rare case of a - * negative number means a missing init somewhere. */ + * mbedtls_threading_mutex_t object itself). The (hopefully) rare + * case of a negative number means a missing init somewhere. */ mbedtls_fprintf(stdout, "[mutex: %d leaked] ", live_mutexes); live_mutexes = 0; mbedtls_test_set_mutex_usage_error("missing free");