From f21b8bbf557cb5bf8b9ab3f788247dd52ce5d99b Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Thu, 16 May 2019 11:51:57 +0800 Subject: [PATCH 1/4] esp_event: fix issue with post data preparation Fixes an issue with post instance data preparation. Currently, there is no way to check if event data has really been set during handler execution preparation. When data is not allocated from the heap, user could have passed 0x0 which can lead to failed checks. This also implements using the already allocated data memory for posting events from non-ISR functions when data size is less than the capacity. --- components/esp_event/esp_event.c | 51 +++++++++++++------ .../private_include/esp_event_internal.h | 3 +- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index e89b7e3caa..f9704977d1 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -132,8 +132,18 @@ static void handler_execute(esp_event_loop_instance_t* loop, esp_event_handler_i #endif // Execute the handler #if CONFIG_ESP_EVENT_POST_FROM_ISR - (*(handler->handler))(handler->arg, post.base, post.id, post.data_allocd ? post.data.ptr : &post.data.val); -#else + void* data_ptr = NULL; + + if (post.data_set) { + if (post.data_allocated) { + data_ptr = post.data.ptr; + } else { + data_ptr = &post.data.val; + } + } + + (*(handler->handler))(handler->arg, post.base, post.id, data_ptr); +#else (*(handler->handler))(handler->arg, post.base, post.id, post.data); #endif @@ -380,7 +390,7 @@ static void loop_node_remove_all_handler(esp_event_loop_node_t* loop_node) static void inline __attribute__((always_inline)) post_instance_delete(esp_event_post_instance_t* post) { #if CONFIG_ESP_EVENT_POST_FROM_ISR - if (post->data_allocd && post->data.ptr) { + if (post->data_allocated && post->data.ptr) { free(post->data.ptr); } #else @@ -740,20 +750,28 @@ 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; esp_event_post_instance_t post; - memset((void*)(&(post.data)), 0, sizeof(post.data)); + memset((void*)(&post), 0, sizeof(post)); 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; - } - - memcpy(event_data_copy, event_data, event_data_size); #if CONFIG_ESP_EVENT_POST_FROM_ISR - post.data.ptr = event_data_copy; - post.data_allocd = true; + if(event_data_size > sizeof(post.data.val)) { +#endif + // 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; + } + + memcpy(event_data_copy, event_data, event_data_size); +#if CONFIG_ESP_EVENT_POST_FROM_ISR + post.data.ptr = event_data_copy; + post.data_allocated = true; + } else { + memcpy(&post.data.val, event_data, event_data_size); + post.data_allocated = false; + } + post.data_set = true; #else post.data = event_data_copy; #endif @@ -816,7 +834,7 @@ 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; esp_event_post_instance_t post; - memset((void*)(&(post.data)), 0, sizeof(post.data)); + memset((void*)(&post), 0, sizeof(post)); if (event_data_size > sizeof(post.data.val)) { return ESP_ERR_INVALID_ARG; @@ -824,7 +842,8 @@ esp_err_t esp_event_isr_post_to(esp_event_loop_handle_t event_loop, esp_event_ba if (event_data != NULL && event_data_size != 0) { memcpy((void*)(&(post.data.val)), event_data, event_data_size); - post.data_allocd = false; + post.data_allocated = false; + post.data_set = true; } post.base = event_base; post.id = event_id; diff --git a/components/esp_event/private_include/esp_event_internal.h b/components/esp_event/private_include/esp_event_internal.h index d5bbb7fac1..fb970a8068 100644 --- a/components/esp_event/private_include/esp_event_internal.h +++ b/components/esp_event/private_include/esp_event_internal.h @@ -97,7 +97,8 @@ typedef void* esp_event_post_data_t; /// Event posted to the event queue typedef struct esp_event_post_instance { #if CONFIG_ESP_EVENT_POST_FROM_ISR - bool data_allocd; /**< indicates whether data is alloc'd */ + bool data_allocated; /**< indicates whether data is allocated from heap */ + bool data_set; /**< indicates if data is null */ #endif esp_event_base_t base; /**< the event base */ int32_t id; /**< the event id */ From 4042902402ab94eb71e1b946a2d27bdcdc3057ca Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Mon, 20 May 2019 19:31:45 +0800 Subject: [PATCH 2/4] esp_event: always alloc data when not posting from isr --- components/esp_event/esp_event.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index f9704977d1..61a372a176 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -753,24 +753,17 @@ esp_err_t esp_event_post_to(esp_event_loop_handle_t event_loop, esp_event_base_t memset((void*)(&post), 0, sizeof(post)); if (event_data != NULL && event_data_size != 0) { -#if CONFIG_ESP_EVENT_POST_FROM_ISR - if(event_data_size > sizeof(post.data.val)) { -#endif - // Make persistent copy of event data on heap. - void* event_data_copy = calloc(1, event_data_size); + // 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; - } - - memcpy(event_data_copy, event_data, event_data_size); -#if CONFIG_ESP_EVENT_POST_FROM_ISR - post.data.ptr = event_data_copy; - post.data_allocated = true; - } else { - memcpy(&post.data.val, event_data, event_data_size); - post.data_allocated = false; + if (event_data_copy == NULL) { + return ESP_ERR_NO_MEM; } + + memcpy(event_data_copy, event_data, event_data_size); +#if CONFIG_ESP_EVENT_POST_FROM_ISR + post.data.ptr = event_data_copy; + post.data_allocated = true; post.data_set = true; #else post.data = event_data_copy; From 136e5bc32dd9f28d90c56ff4dc7ee865c6a9d84c Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Mon, 20 May 2019 19:28:35 +0800 Subject: [PATCH 3/4] esp_event: style fixes --- components/esp_event/esp_event.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index 61a372a176..d5b5d66c51 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -170,7 +170,7 @@ static esp_err_t handler_instances_add(esp_event_handler_instances_t* handlers, handler_instance->handler = handler; handler_instance->arg = handler_arg; - if(SLIST_EMPTY(handlers)) { + if (SLIST_EMPTY(handlers)) { SLIST_INSERT_HEAD(handlers, handler_instance, next); } else { @@ -473,16 +473,16 @@ esp_err_t esp_event_loop_create(const esp_event_loop_args_t* event_loop_args, es return ESP_OK; on_err: - if(loop->queue != NULL) { + if (loop->queue != NULL) { vQueueDelete(loop->queue); } - if(loop->mutex != NULL) { + if (loop->mutex != NULL) { vSemaphoreDelete(loop->mutex); } #ifdef CONFIG_ESP_EVENT_LOOP_PROFILING - if(loop->profiling_mutex != NULL) { + if (loop->profiling_mutex != NULL) { vSemaphoreDelete(loop->profiling_mutex); } #endif @@ -507,7 +507,7 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick TickType_t marker = xTaskGetTickCount(); TickType_t end = 0; -#if( configUSE_16_BIT_TICKS == 1 ) +#if (configUSE_16_BIT_TICKS == 1) int32_t remaining_ticks = ticks_to_run; #else int64_t remaining_ticks = ticks_to_run; @@ -542,7 +542,7 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick } SLIST_FOREACH(id_node, &(base_node->id_nodes), next) { - if(id_node->id == post.id) { + if (id_node->id == post.id) { // Execute id level handlers SLIST_FOREACH(handler, &(id_node->handlers), next) { handler_execute(loop, handler, post); From 8ed07d0bb2e9faea1cdf863a67f34799be937f86 Mon Sep 17 00:00:00 2001 From: Renz Christian Bagaporo Date: Mon, 20 May 2019 20:56:59 +0800 Subject: [PATCH 4/4] esp_event: check that event data is prepared properly --- components/esp_event/test/test_event.c | 31 ++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/components/esp_event/test/test_event.c b/components/esp_event/test/test_event.c index 3b2ff0c68f..c5dcc868da 100644 --- a/components/esp_event/test/test_event.c +++ b/components/esp_event/test/test_event.c @@ -1149,6 +1149,37 @@ TEST_CASE("events are dispatched in the order they are registered", "[event]") } #if CONFIG_ESP_EVENT_POST_FROM_ISR +TEST_CASE("can properly prepare event data posted to loop", "[event]") +{ + 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)); + + esp_event_post_instance_t post; + esp_event_loop_instance_t* loop_def = (esp_event_loop_instance_t*) loop; + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV1, NULL, 0, portMAX_DELAY)); + TEST_ASSERT_EQUAL(pdTRUE, xQueueReceive(loop_def->queue, &post, portMAX_DELAY)); + TEST_ASSERT_EQUAL(false, post.data_set); + TEST_ASSERT_EQUAL(false, post.data_allocated); + TEST_ASSERT_EQUAL(NULL, post.data.ptr); + + int sample = 0; + TEST_ASSERT_EQUAL(ESP_OK, esp_event_isr_post_to(loop, s_test_base1, TEST_EVENT_BASE1_EV1, &sample, sizeof(sample), NULL)); + TEST_ASSERT_EQUAL(pdTRUE, xQueueReceive(loop_def->queue, &post, portMAX_DELAY)); + TEST_ASSERT_EQUAL(true, post.data_set); + TEST_ASSERT_EQUAL(false, post.data_allocated); + TEST_ASSERT_EQUAL(false, post.data.val); + + TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_delete(loop)); + + TEST_TEARDOWN(); +} + TEST_CASE("can post events from interrupt handler", "[event]") { SemaphoreHandle_t sem = xSemaphoreCreateBinary();