From c7328f1cc05847b812d2b3c81b7697f454fc5e85 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 19 Apr 2021 15:59:44 +1000 Subject: [PATCH 1/2] freertos: Fix race condition returning incorrect TCB on unpinned tasks Noted as a problem with thread local storage returning a different task's pointers, but some other were APIs also accessing current task unsafely. Regression in FreeRTOS 10 update a3c90bf59add307d3a41f5fb4c17385ee67dcf20 --- components/freertos/tasks.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 94aa827038..5053c8ba16 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -231,7 +231,12 @@ count overflows. */ * task should be used in place of the parameter. This macro simply checks to * see if the parameter is NULL and returns a pointer to the appropriate TCB. */ -#define prvGetTCBFromHandle( pxHandle ) ( ( ( pxHandle ) == NULL ) ? (TaskHandle_t)pxCurrentTCB[xPortGetCoreID()] : ( (TaskHandle_t)pxHandle ) ) +#if portNUM_PROCESSORS > 1 +/* In SMP, we need to disable interrupts if getting the current task handle outside a critical section. Calling xTaskGetCurrentTaskHandle() ensures this. */ +#define prvGetTCBFromHandle( pxHandle ) ( ( ( pxHandle ) == NULL ) ? xTaskGetCurrentTaskHandle() : ( (TaskHandle_t)pxHandle ) ) +#else +#define prvGetTCBFromHandle( pxHandle ) ( ( ( pxHandle ) == NULL ) ? (TaskHandle_t) pxCurrentTCB[0] : ( (TaskHandle_t)pxHandle ) ) +#endif /* The item value of the event list item is normally used to hold the priority of the task to which it belongs (coded to allow it to be held in reverse @@ -4449,7 +4454,7 @@ TCB_t *pxTCB; } /*-----------------------------------------------------------*/ -#if ( ( INCLUDE_xTaskGetCurrentTaskHandle == 1 ) || ( configUSE_MUTEXES == 1 ) ) +#if ( ( INCLUDE_xTaskGetCurrentTaskHandle == 1 ) || ( configUSE_MUTEXES == 1 ) || (portNUM_PROCESSORS > 1) ) TaskHandle_t xTaskGetCurrentTaskHandle( void ) { From 79ae7a7bd968af1ea6bbf8aa9d0db5bb2739bc73 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 19 Apr 2021 20:01:24 +1000 Subject: [PATCH 2/2] freertos: test: Add stress test for thread local storage (Fails without the fix applied in parent commit.) --- .../pthread/test/test_pthread_local_storage.c | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/components/pthread/test/test_pthread_local_storage.c b/components/pthread/test/test_pthread_local_storage.c index 114c33b893..ebcaafcbe8 100644 --- a/components/pthread/test/test_pthread_local_storage.c +++ b/components/pthread/test/test_pthread_local_storage.c @@ -4,6 +4,7 @@ #include "freertos/FreeRTOS.h" #include "freertos/task.h" #include "test_utils.h" +#include "esp_system.h" TEST_CASE("pthread local storage basics", "[pthread]") { @@ -128,3 +129,36 @@ static void task_test_pthread_destructor(void *v_key) thread_test_pthread_destructor(v_key); vTaskDelete(NULL); } + +#define STRESS_NUMITER 2000000 +#define STRESS_NUMTASKS 16 + +static void *thread_stress_test(void *v_key) +{ + pthread_key_t key = (pthread_key_t) v_key; + void *tls_value = (void *)esp_random(); + + pthread_setspecific(key, tls_value); + + for(int i = 0; i < STRESS_NUMITER; i++) { + TEST_ASSERT_EQUAL_HEX32(pthread_getspecific(key), tls_value); + } + + return NULL; +} + + +// This test case added to reproduce issues with unpinned tasks and TLS +TEST_CASE("pthread local storage stress test", "[pthread]") +{ + pthread_key_t key = -1; + pthread_t threads[STRESS_NUMTASKS] = { 0 }; + TEST_ASSERT_EQUAL(0, pthread_key_create(&key, test_pthread_destructor)); + + for (int i = 0; i < STRESS_NUMTASKS; i++) { + TEST_ASSERT_EQUAL(0, pthread_create(&threads[i], NULL, thread_stress_test, (void *)key)); + } + for (int i = 0; i < STRESS_NUMTASKS; i++) { + TEST_ASSERT_EQUAL(0, pthread_join(threads[i], NULL)); + } +}