From 26160a217e3a2953fd0b2eabbde1075e3bb46941 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Tue, 9 Apr 2024 12:23:14 +0200 Subject: [PATCH 1/3] fix(heap): Add block owner to allocs in heap_caps_init Add the block owner field in the memory allocated in heap_caps_init() to avoid parsing error wheen using the task tracking feature. Closes https://github.com/espressif/esp-idf/issues/13467 --- components/heap/heap_caps_init.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/components/heap/heap_caps_init.c b/components/heap/heap_caps_init.c index 134378af00..27f55e8ff4 100644 --- a/components/heap/heap_caps_init.c +++ b/components/heap/heap_caps_init.c @@ -61,7 +61,7 @@ void heap_caps_init(void) num_regions = soc_get_available_memory_regions(regions); // the following for loop will calculate the number of possible heaps - // based on how many regions were coalesed. + // based on how many regions were coalesced. size_t num_heaps = num_regions; //The heap allocator will treat every region given to it as separate. In order to get bigger ranges of contiguous memory, @@ -75,7 +75,7 @@ void heap_caps_init(void) b->size += a->size; // remove one heap from the number of heaps as - // 2 regions just got coalesed. + // 2 regions just got coalesced. num_heaps--; } } @@ -124,8 +124,14 @@ void heap_caps_init(void) heap_t *heaps_array = NULL; for (size_t i = 0; i < num_heaps; i++) { if (heap_caps_match(&temp_heaps[i], MALLOC_CAP_8BIT|MALLOC_CAP_INTERNAL)) { - /* use the first DRAM heap which can fit the data */ - heaps_array = multi_heap_malloc(temp_heaps[i].heap, sizeof(heap_t) * num_heaps); + /* use the first DRAM heap which can fit the data. + * the allocated block won't include the block owner bytes since this operation + * is done by the top level API heap_caps_malloc(). So we need to add it manually + * after successful allocation. Allocate extra 4 bytes for that purpose. */ + heaps_array = multi_heap_malloc(temp_heaps[i].heap, MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(sizeof(heap_t) * num_heaps)); + assert(heaps_array != NULL); + MULTI_HEAP_SET_BLOCK_OWNER(heaps_array); + heaps_array = (heap_t *)MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(heaps_array); if (heaps_array != NULL) { break; } @@ -178,7 +184,7 @@ bool heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, * cannot be added twice. In fact, registering the same memory region as a heap twice * would cause a corruption and then an exception at runtime. * - * the existing heap region s(tart) e(nd) + * the existing heap region start end * |----------------------| * * 1.add region (e1 Date: Wed, 10 Apr 2024 11:23:07 +0200 Subject: [PATCH 2/3] test(heap): Extend task tracking test with task handle check Add a test to make sure that the task handles returned in the task tracking information are valid task handles. To verify that, feed the task name returned by pcTaskGetName() using the task handle under test to xTaskGetHandle() and make sure the task handle returned matches the one under test. --- .../test_apps/heap_tests/main/test_task_tracking.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/components/heap/test_apps/heap_tests/main/test_task_tracking.c b/components/heap/test_apps/heap_tests/main/test_task_tracking.c index 99215969dc..96a370528d 100644 --- a/components/heap/test_apps/heap_tests/main/test_task_tracking.c +++ b/components/heap/test_apps/heap_tests/main/test_task_tracking.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -49,6 +49,16 @@ static void check_heap_task_info(TaskHandle_t taskHdl) // heap_caps_get_per_task_info includes the size of the block owner (4 bytes) TEST_ASSERT(heap_info.totals[i].size[0] == ALLOC_BYTES + 4); } + + // test that if not 0, the task handle corresponds to an actual task. + // this test is to make sure no rubbish is stored as a task handle. + if (heap_info.totals[i].task != 0) { + // feeding the task name returned by pcTaskGetName() to xTaskGetHandle(). + // xTaskGetHandle would return the task handler used as parameter in + // pcTaskGetName if the task handle is valid. Otherwise, it will return + // NULL or just crash if the pointer to the task name is complete nonsense. + TEST_ASSERT_EQUAL(heap_info.totals[i].task, xTaskGetHandle(pcTaskGetName(heap_info.totals[i].task))); + } } TEST_ASSERT_TRUE(task_found); } From 3e214bc2f41202c91fa6aae0483025529b9d7a13 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Fri, 12 Apr 2024 12:21:13 +0200 Subject: [PATCH 3/3] fix(heap): Loop break on failed alloc don't check for heaps_array != NULL in the loop. The check is done after the loop since it is allowed for the allocation to fail until finding aa ssuitable heap. --- components/heap/heap_caps_init.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/components/heap/heap_caps_init.c b/components/heap/heap_caps_init.c index 27f55e8ff4..063af207a8 100644 --- a/components/heap/heap_caps_init.c +++ b/components/heap/heap_caps_init.c @@ -129,15 +129,14 @@ void heap_caps_init(void) * is done by the top level API heap_caps_malloc(). So we need to add it manually * after successful allocation. Allocate extra 4 bytes for that purpose. */ heaps_array = multi_heap_malloc(temp_heaps[i].heap, MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(sizeof(heap_t) * num_heaps)); - assert(heaps_array != NULL); - MULTI_HEAP_SET_BLOCK_OWNER(heaps_array); - heaps_array = (heap_t *)MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(heaps_array); if (heaps_array != NULL) { break; } } } assert(heaps_array != NULL); /* if NULL, there's not enough free startup heap space */ + MULTI_HEAP_SET_BLOCK_OWNER(heaps_array); + heaps_array = (heap_t *)MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(heaps_array); memcpy(heaps_array, temp_heaps, sizeof(heap_t)*num_heaps);