From 4c97906fc868ef17f4566298b35f533f4a733378 Mon Sep 17 00:00:00 2001 From: Xentec Date: Tue, 1 Oct 2019 01:31:09 +0200 Subject: [PATCH 1/4] esp_event: fix crash when unregistering a handler instance in itself When a handler instance is the last one in the list und unregisters itself, the handler iterator will be invalidated by entering free'd memory. Same applies for event base and id, if they become empty. Merges https://github.com/espressif/esp-idf/pull/4139 --- components/esp_event/esp_event.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 557cfba0d3..9ed30a04ed 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -526,30 +526,30 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick bool exec = false; - esp_event_handler_instance_t *handler; + esp_event_handler_instance_t *handler, *temp_handler; esp_event_loop_node_t *loop_node; - esp_event_base_node_t *base_node; - esp_event_id_node_t *id_node; + esp_event_base_node_t *base_node, *temp_base; + esp_event_id_node_t *id_node, *temp_id_node; SLIST_FOREACH(loop_node, &(loop->loop_nodes), next) { // Execute loop level handlers - SLIST_FOREACH(handler, &(loop_node->handlers), next) { + SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) { handler_execute(loop, handler, post); exec |= true; } - SLIST_FOREACH(base_node, &(loop_node->base_nodes), next) { + SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) { if (base_node->base == post.base) { // Execute base level handlers - SLIST_FOREACH(handler, &(base_node->handlers), next) { + SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) { handler_execute(loop, handler, post); exec |= true; } - SLIST_FOREACH(id_node, &(base_node->id_nodes), next) { + 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(handler, &(id_node->handlers), next) { + SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) { handler_execute(loop, handler, post); exec |= true; } From 3c253295eb51f5800715393f47512816d2970cd4 Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Fri, 18 Oct 2019 12:54:42 +0800 Subject: [PATCH 2/4] esp_event: iterate loop nodes safely as well --- components/esp_event/esp_event.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 9ed30a04ed..be55d952f5 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -527,11 +527,11 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick bool exec = false; esp_event_handler_instance_t *handler, *temp_handler; - esp_event_loop_node_t *loop_node; + 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(loop_node, &(loop->loop_nodes), next) { + 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) { handler_execute(loop, handler, post); From 521f436315f3944c36706c718925a753ccf091ec Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Tue, 8 Oct 2019 15:32:51 +0800 Subject: [PATCH 3/4] esp_event: test that handlers can unregister themselves --- components/esp_event/test/test_event.c | 67 ++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/components/esp_event/test/test_event.c b/components/esp_event/test/test_event.c index c016f48f3c..9a4b1a400c 100644 --- a/components/esp_event/test/test_event.c +++ b/components/esp_event/test/test_event.c @@ -203,6 +203,7 @@ static void test_event_simple_handler_registration_task(void* args) vTaskDelete(NULL); } + static void test_handler_post_w_task(void* event_handler_arg, esp_event_base_t event_base, int32_t event_id, void* event_data) { simple_arg_t* arg = (simple_arg_t*) event_handler_arg; @@ -254,6 +255,17 @@ static void test_handler_post_wo_task(void* event_handler_arg, esp_event_base_t } } +static void test_handler_unregister_itself(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_data; + int* unregistered = (int*) event_handler_arg; + + (*unregistered) += (event_base == s_test_base1 ? 0 : 10) + event_id + 1; + + // Unregister this handler for this event + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_unregister_with(*loop, event_base, event_id, test_handler_unregister_itself)); +} + static void test_post_from_handler_loop_task(void* args) { esp_event_loop_handle_t event_loop = (esp_event_loop_handle_t) args; @@ -468,6 +480,61 @@ TEST_CASE("can unregister handler", "[event]") TEST_TEARDOWN(); } +TEST_CASE("handler can unregister itself", "[event]") +{ + /* this test aims to verify that handlers can unregister themselves */ + + TEST_SETUP(); + + esp_event_loop_handle_t loop; + esp_event_loop_args_t loop_args = test_event_get_default_loop_args(); + + loop_args.task_name = NULL; + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_create(&loop_args, &loop)); + + int unregistered = 0; + + /* + * s_test_base1, ev1 = 1 + * s_test_base1, ev2 = 2 + * s_test_base2, ev1 = 11 + * s_test_base2, ev2 = 12 + */ + int expected_unregistered = 0; + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_register_with(loop, s_test_base1, TEST_EVENT_BASE1_EV1, test_handler_unregister_itself, &unregistered)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_register_with(loop, s_test_base1, TEST_EVENT_BASE1_EV2, test_handler_unregister_itself, &unregistered)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_register_with(loop, s_test_base2, TEST_EVENT_BASE2_EV1, test_handler_unregister_itself, &unregistered)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_handler_register_with(loop, s_test_base2, TEST_EVENT_BASE2_EV2, test_handler_unregister_itself, &unregistered)); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV2, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_run(loop, pdMS_TO_TICKS(10))); + expected_unregistered = 2; // base1, ev2 + TEST_ASSERT_EQUAL(expected_unregistered, unregistered); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV1, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base2, TEST_EVENT_BASE2_EV1, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_run(loop, pdMS_TO_TICKS(10))); + expected_unregistered += 1 + 11; // base1, ev1 + base2, ev1 + TEST_ASSERT_EQUAL(expected_unregistered, unregistered); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base2, TEST_EVENT_BASE2_EV2, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_run(loop, pdMS_TO_TICKS(10))); + expected_unregistered += 12; // base2, ev2 + TEST_ASSERT_EQUAL(expected_unregistered, unregistered); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV1, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV2, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base2, TEST_EVENT_BASE2_EV1, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base2, TEST_EVENT_BASE2_EV2, &loop, sizeof(loop), portMAX_DELAY)); + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_run(loop, pdMS_TO_TICKS(10))); + TEST_ASSERT_EQUAL(expected_unregistered, unregistered); // all handlers unregistered + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_delete(loop)); + + TEST_TEARDOWN(); +} + TEST_CASE("can exit running loop at approximately the set amount of time", "[event]") { /* this test aims to verify that running loop does not block indefinitely in cases where From 1ed77e3a8d8769f4f8d12f515b6818787f725867 Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Fri, 25 Oct 2019 13:14:05 +0800 Subject: [PATCH 4/4] esp_event: remove extra line from source file --- components/esp_event/test/test_event.c | 1 - 1 file changed, 1 deletion(-) diff --git a/components/esp_event/test/test_event.c b/components/esp_event/test/test_event.c index 9a4b1a400c..c756c8641d 100644 --- a/components/esp_event/test/test_event.c +++ b/components/esp_event/test/test_event.c @@ -203,7 +203,6 @@ static void test_event_simple_handler_registration_task(void* args) vTaskDelete(NULL); } - static void test_handler_post_w_task(void* event_handler_arg, esp_event_base_t event_base, int32_t event_id, void* event_data) { simple_arg_t* arg = (simple_arg_t*) event_handler_arg;