Merge branch 'fix/esp-event-security-fixes_v6.1' into 'release/v6.1'

fix(esp_event): multiple security and stability fixes (v6.1)

See merge request espressif/esp-idf!51046

components/esp_event/esp_event.c

@@ -548,6 +548,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)
@@ -587,6 +596,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
@@ -681,6 +694,8 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick
         // The event has already been unqueued, so ensure it gets executed.
         xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY);
 
+        bool exec = false;
+
         // check if the event retrieve from the queue is the internal event that is
         // triggered when a handler needs to be removed..
         if (post.base == esp_event_handler_cleanup) {
@@ -694,47 +709,45 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick
             if (ctx->legacy) {
                 free(ctx->handler_ctx);
             }
-        }
+        } else {
+            loop->running_task = xTaskGetCurrentTaskHandle();
 
-        loop->running_task = xTaskGetCurrentTaskHandle();
+            esp_event_handler_node_t *handler, *temp_handler;
+            esp_event_loop_node_t *loop_node, *temp_node;
+            esp_event_base_node_t *base_node, *temp_base;
+            esp_event_id_node_t *id_node, *temp_id_node;
 
-        bool exec = false;
-
-        esp_event_handler_node_t *handler, *temp_handler;
-        esp_event_loop_node_t *loop_node, *temp_node;
-        esp_event_base_node_t *base_node, *temp_base;
-        esp_event_id_node_t *id_node, *temp_id_node;
-
-        SLIST_FOREACH_SAFE(loop_node, &(loop->loop_nodes), next, temp_node) {
-            // Execute loop level handlers
-            SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) {
-                if (!handler->unregistered) {
-                    handler_execute(loop, handler, post);
-                    exec |= true;
-                }
-            }
-
-            SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) {
-                if (base_node->base == post.base) {
-                    // Execute base level handlers
-                    SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) {
-                        if (!handler->unregistered) {
-                            handler_execute(loop, handler, post);
-                            exec |= true;
-                        }
+            SLIST_FOREACH_SAFE(loop_node, &(loop->loop_nodes), next, temp_node) {
+                // Execute loop level handlers
+                SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) {
+                    if (!handler->unregistered) {
+                        handler_execute(loop, handler, post);
+                        exec |= true;
                     }
+                }
 
-                    SLIST_FOREACH_SAFE(id_node, &(base_node->id_nodes), next, temp_id_node) {
-                        if (id_node->id == post.id) {
-                            // Execute id level handlers
-                            SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) {
-                                if (!handler->unregistered) {
-                                    handler_execute(loop, handler, post);
-                                    exec |= true;
-                                }
+                SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) {
+                    if (base_node->base == post.base) {
+                        // Execute base level handlers
+                        SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) {
+                            if (!handler->unregistered) {
+                                handler_execute(loop, handler, post);
+                                exec |= true;
+                            }
+                        }
+
+                        SLIST_FOREACH_SAFE(id_node, &(base_node->id_nodes), next, temp_id_node) {
+                            if (id_node->id == post.id) {
+                                // Execute id level handlers
+                                SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) {
+                                    if (!handler->unregistered) {
+                                        handler_execute(loop, handler, post);
+                                        exec |= true;
+                                    }
+                                }
+                                // Skip to next base node
+                                break;
                             }
-                            // Skip to next base node
-                            break;
                         }
                     }
                 }
@@ -751,6 +764,7 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick
             remaining_ticks -= end - marker;
             // If the ticks to run expired, return to the caller
             if (remaining_ticks <= 0) {
+                loop->running_task = NULL;
                 xSemaphoreGiveRecursive(loop->mutex);
                 break;
             } else {
@@ -762,7 +776,7 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick
 
         xSemaphoreGiveRecursive(loop->mutex);
 
-        if (!exec) {
+        if (!exec && base != esp_event_handler_cleanup) {
             // No handlers were registered, not even loop/base level handlers
             ESP_LOGD(TAG, "no handlers have been registered for event %s:%"PRIu32" posted to loop %p", base, id, event_loop);
         }
@@ -779,7 +793,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);
@@ -808,6 +830,12 @@ esp_err_t esp_event_loop_delete(esp_event_loop_handle_t event_loop)
     // Drop existing posts on the queue
     esp_event_post_instance_t post;
     while (xQueueReceive(loop->queue, &post, 0) == pdTRUE) {
+        if (post.base == esp_event_handler_cleanup) {
+            esp_event_remove_handler_context_t* ctx = (esp_event_remove_handler_context_t*)post.data.ptr;
+            if (ctx->legacy) {
+                free(ctx->handler_ctx);
+            }
+        }
         post_instance_delete(&post);
     }
 
@@ -922,12 +950,26 @@ esp_err_t esp_event_handler_unregister_with_internal(esp_event_loop_handle_t eve
     /* remove the handler if the mutex is taken successfully.
      * otherwise it will be removed from the list later */
     esp_err_t res = ESP_FAIL;
-    if (xSemaphoreTake(loop->mutex, 0) == pdTRUE) {
-        res = loop_remove_handler(&remove_handler_ctx);
-        xSemaphoreGive(loop->mutex);
+    if (xSemaphoreTakeRecursive(loop->mutex, 0) == pdTRUE) {
+        /* We got the mutex. Check whether we are currently inside a handler
+         * callback for this loop (running_task is set while handler_execute()
+         * is active). If we are, we MUST NOT free the handler node immediately
+         * because handler_execute() will still write to handler->invoked /
+         * handler->time (profiling) after the callback returns. Use the
+         * deferred cleanup-event path instead so the node is only freed once
+         * the current dispatch iteration has fully completed. */
+        if (loop->running_task == xTaskGetCurrentTaskHandle()) {
+            res = find_and_unregister_handler(&remove_handler_ctx);
+        } else {
+            res = loop_remove_handler(&remove_handler_ctx);
+        }
+        xSemaphoreGiveRecursive(loop->mutex);
     } else {
+        /* Another task holds the mutex (e.g. the loop task is dispatching an
+         * event). Wait until it is released; by that point running_task will
+         * have been cleared, so direct removal is safe. */
         xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY);
-        res = find_and_unregister_handler(&remove_handler_ctx);
+        res = loop_remove_handler(&remove_handler_ctx);
         xSemaphoreGiveRecursive(loop->mutex);
     }
 
@@ -965,15 +1007,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) {
 #if CONFIG_ESP_EVENT_POST_FROM_ISR
         if (event_data_size > sizeof(post.data.val)) {
             post.data.ptr = calloc(1, event_data_size);
             if (post.data.ptr == NULL) {
-                return ESP_ERR_NO_MEM;
+                err = ESP_ERR_NO_MEM;
+                goto on_err;
             }
             post.data_allocated = true;
             memcpy(post.data.ptr, event_data, event_data_size);
@@ -986,7 +1038,8 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t
         void* event_data_copy = esp_event_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);
@@ -1028,14 +1081,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
@@ -1050,6 +1109,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));
 
@@ -1152,7 +1215,7 @@ esp_err_t esp_event_dump(FILE* file)
     portEXIT_CRITICAL(&s_event_loops_spinlock);
 
     // Print the contents of the buffer to the file
-    fprintf(file, buf);
+    fprintf(file, "%s", buf);
 
     // Free the allocated buffer
     free(buf);

components/esp_event/esp_event_private.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2018-2024 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2018-2026 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -20,6 +20,8 @@ bool esp_event_is_handler_registered(esp_event_loop_handle_t event_loop, esp_eve
     esp_event_id_node_t* id_node;
     esp_event_handler_node_t* handler;
 
+    xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY);
+
     SLIST_FOREACH(loop_node, &(loop->loop_nodes), next) {
         SLIST_FOREACH(handler, &(loop_node->handlers), next) {
             if (event_base == ESP_EVENT_ANY_BASE && event_id == ESP_EVENT_ANY_ID && handler->handler_ctx->handler == event_handler) {
@@ -52,6 +54,6 @@ bool esp_event_is_handler_registered(esp_event_loop_handle_t event_loop, esp_eve
     }
 
 out:
-    xSemaphoreGive(loop->mutex);
+    xSemaphoreGiveRecursive(loop->mutex);
     return result;
 }

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")

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 */

components/esp_event/test_apps/main/test_event_common.cpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2023-2026 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Unlicense OR CC0-1.0
  */
@@ -183,6 +183,30 @@ TEST_CASE("registering event twice with same handler yields updated handler arg"
     TEST_ASSERT_EQUAL(1, count_second);
 }
 
+static void self_unregister_legacy_handler(void* event_handler_arg, esp_event_base_t event_base, int32_t event_id, void* event_data)
+{
+    esp_event_loop_handle_t loop = (esp_event_loop_handle_t) event_handler_arg;
+    TEST_ESP_OK(esp_event_handler_unregister_with(loop, event_base, event_id, self_unregister_legacy_handler));
+}
+
+/* A legacy handler unregistering itself defers removal via an internal cleanup event
+ * left on the queue. Deleting the loop must drain it without leaking. */
+TEST_CASE("deleting loop with queued legacy cleanup event does not leak", "[event][linux]")
+{
+    EV_LoopFix loop_fix;
+
+    TEST_ESP_OK(esp_event_handler_register_with(loop_fix.loop,
+                                                s_test_base1,
+                                                TEST_EVENT_BASE1_EV1,
+                                                self_unregister_legacy_handler,
+                                                loop_fix.loop));
+
+    TEST_ESP_OK(esp_event_post_to(loop_fix.loop, s_test_base1, TEST_EVENT_BASE1_EV1, NULL, 0, portMAX_DELAY));
+
+    /* ZERO_DELAY runs a single event, leaving the deferred cleanup event queued. */
+    TEST_ESP_OK(esp_event_loop_run(loop_fix.loop, ZERO_DELAY));
+}
+
 TEST_CASE("registering event handler instance twice works", "[event][linux]")
 {
     EV_LoopFix loop_fix;