From 9947dfdc58ea685254a7d55e978dcf94a7958dc4 Mon Sep 17 00:00:00 2001 From: Konstantin Kondrashov Date: Tue, 7 Jul 2026 17:24:23 +0300 Subject: [PATCH] fix(esp_event): prevent UAF race between post and loop delete (SEC-222) esp_event_post_to() could access loop->queue / loop->mutex after esp_event_loop_delete() freed them when both ran concurrently. Introduce esp_event_loop_state_t with: - posts_in_flight: reference-count incremented atomically (under state.lock spinlock) before touching any loop resources, decremented on every exit path via goto on_err. - deleting: atomic_bool set by esp_event_loop_delete() to block new posts from entering the critical section. esp_event_loop_delete() sets deleting=true, then busy-waits (releasing and re-acquiring loop->mutex each tick) until posts_in_flight reaches zero before proceeding with teardown. esp_event_isr_post_to() performs a lock-free atomic_load of deleting as a best-effort guard; ISR context cannot participate in the spinlock protocol but the window is documented and accepted. --- components/esp_event/esp_event.c | 49 +++++++++++++++++-- .../main/esp_event_test.cpp | 9 ++++ .../private_include/esp_event_internal.h | 9 +++- 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 40ce60a0761..a1cef32f8e0 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -535,6 +535,15 @@ static esp_err_t find_and_unregister_handler(esp_event_remove_handler_context_t* return esp_event_post_to(ctx->loop, esp_event_handler_cleanup, 0, ctx, sizeof(esp_event_remove_handler_context_t), portMAX_DELAY); } +static bool event_loop_has_posts_in_flight(esp_event_loop_instance_t* loop) +{ + portENTER_CRITICAL(&loop->state.lock); + bool posts_in_flight = loop->state.posts_in_flight > 0; + portEXIT_CRITICAL(&loop->state.lock); + + return posts_in_flight; +} + /* ---------------------------- Public API --------------------------------- */ esp_err_t esp_event_loop_create(const esp_event_loop_args_t* event_loop_args, esp_event_loop_handle_t* event_loop) @@ -570,6 +579,10 @@ esp_err_t esp_event_loop_create(const esp_event_loop_args_t* event_loop_args, es goto on_err; } + loop->state.lock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED; + atomic_init(&loop->state.deleting, false); + loop->state.posts_in_flight = 0; + SLIST_INIT(&(loop->loop_nodes)); // Create the loop task if requested @@ -743,7 +756,15 @@ esp_err_t esp_event_loop_delete(esp_event_loop_handle_t event_loop) esp_event_loop_instance_t* loop = (esp_event_loop_instance_t*) event_loop; SemaphoreHandle_t loop_mutex = loop->mutex; - xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); + xSemaphoreTakeRecursive(loop_mutex, portMAX_DELAY); + + atomic_store(&loop->state.deleting, true); + while (event_loop_has_posts_in_flight(loop)) { + xSemaphoreGiveRecursive(loop_mutex); + // Wait for the posts in flight to finish + vTaskDelay(1); + xSemaphoreTakeRecursive(loop_mutex, portMAX_DELAY); + } #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING portENTER_CRITICAL(&s_event_loops_spinlock); @@ -934,15 +955,25 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t esp_event_loop_instance_t* loop = (esp_event_loop_instance_t*) event_loop; + portENTER_CRITICAL(&loop->state.lock); + if (atomic_load(&loop->state.deleting)) { + portEXIT_CRITICAL(&loop->state.lock); + return ESP_ERR_INVALID_STATE; + } + loop->state.posts_in_flight++; + portEXIT_CRITICAL(&loop->state.lock); + esp_event_post_instance_t post; memset((void*)(&post), 0, sizeof(post)); + esp_err_t err = ESP_OK; if (event_data != NULL && event_data_size != 0) { // Make persistent copy of event data on heap. void* event_data_copy = calloc(1, event_data_size); if (event_data_copy == NULL) { - return ESP_ERR_NO_MEM; + err = ESP_ERR_NO_MEM; + goto on_err; } memcpy(event_data_copy, event_data, event_data_size); @@ -987,14 +1018,20 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING atomic_fetch_add(&loop->events_dropped, 1); #endif - return ESP_ERR_TIMEOUT; + err = ESP_ERR_TIMEOUT; + goto on_err; } #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING atomic_fetch_add(&loop->events_received, 1); #endif - return ESP_OK; +on_err: + portENTER_CRITICAL(&loop->state.lock); + loop->state.posts_in_flight--; + portEXIT_CRITICAL(&loop->state.lock); + + return err; } #if CONFIG_ESP_EVENT_POST_FROM_ISR @@ -1009,6 +1046,10 @@ esp_err_t esp_event_isr_post_to(esp_event_loop_handle_t event_loop, esp_event_ba esp_event_loop_instance_t* loop = (esp_event_loop_instance_t*) event_loop; + if (atomic_load(&loop->state.deleting)) { + return ESP_ERR_INVALID_STATE; + } + esp_event_post_instance_t post; memset((void*)(&post), 0, sizeof(post)); diff --git a/components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp b/components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp index 692edd08505..b74e3f8ec6e 100644 --- a/components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp +++ b/components/esp_event/host_test/esp_event_unit_test/main/esp_event_test.cpp @@ -17,6 +17,7 @@ extern "C" { #include "Mocktask.h" #include "Mockqueue.h" +#include "Mockportmacro.h" } namespace { @@ -102,6 +103,8 @@ TEST_CASE("test esp_event_loop_create no_task(void)") xQueueTakeMutexRecursive_IgnoreAndReturn(0); xQueueGiveMutexRecursive_IgnoreAndReturn(0); xQueueReceive_IgnoreAndReturn(0); + vPortEnterCritical_Ignore(); + vPortExitCritical_Ignore(); esp_event_loop_handle_t loop = nullptr; esp_event_loop_args_t loop_args = test_event_get_default_loop_args(); @@ -115,6 +118,8 @@ TEST_CASE("test esp_event_loop_create no_task(void)") xQueueReceive_StopIgnore(); xQueueTakeMutexRecursive_StopIgnore(); xQueueGiveMutexRecursive_StopIgnore(); + vPortEnterCritical_StopIgnore(); + vPortExitCritical_StopIgnore(); } TEST_CASE("test esp_event_loop_create with_task(void)") @@ -125,6 +130,8 @@ TEST_CASE("test esp_event_loop_create with_task(void)") xQueueTakeMutexRecursive_IgnoreAndReturn(0); xQueueGiveMutexRecursive_IgnoreAndReturn(0); xQueueReceive_IgnoreAndReturn(0); + vPortEnterCritical_Ignore(); + vPortExitCritical_Ignore(); esp_event_loop_handle_t loop = nullptr; esp_event_loop_args_t loop_args = test_event_get_default_loop_args(); @@ -138,6 +145,8 @@ TEST_CASE("test esp_event_loop_create with_task(void)") xQueueReceive_StopIgnore(); xQueueTakeMutexRecursive_StopIgnore(); xQueueGiveMutexRecursive_StopIgnore(); + vPortEnterCritical_StopIgnore(); + vPortExitCritical_StopIgnore(); } TEST_CASE("registering with ANY_BASE but specific ID fails") diff --git a/components/esp_event/private_include/esp_event_internal.h b/components/esp_event/private_include/esp_event_internal.h index 76304c689ce..73306f405d1 100644 --- a/components/esp_event/private_include/esp_event_internal.h +++ b/components/esp_event/private_include/esp_event_internal.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2018-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2018-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -66,6 +66,12 @@ typedef struct esp_event_loop_node { typedef SLIST_HEAD(esp_event_loop_nodes, esp_event_loop_node) esp_event_loop_nodes_t; +typedef struct esp_event_loop_state { + portMUX_TYPE lock; /**< spinlock protecting deleting and posts_in_flight */ + atomic_bool deleting; /**< true when loop deletion has started; read lock-free from ISR */ + uint32_t posts_in_flight; /**< task-context posts that passed the post-entry gate */ +} esp_event_loop_state_t; + /// Event loop typedef struct esp_event_loop_instance { const char* name; /**< name of this event loop */ @@ -76,6 +82,7 @@ typedef struct esp_event_loop_instance { SemaphoreHandle_t mutex; /**< mutex for updating the events linked list */ esp_event_loop_nodes_t loop_nodes; /**< set of linked lists containing the registered handlers for the loop */ + esp_event_loop_state_t state; /**< loop deletion and post-entry state */ #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING atomic_uint_least32_t events_received; /**< number of events successfully posted to the loop */ atomic_uint_least32_t events_dropped; /**< number of events dropped due to queue being full */