diff --git a/components/heap/heap_caps.c b/components/heap/heap_caps.c index 501ebe6392..7f8578fe62 100644 --- a/components/heap/heap_caps.c +++ b/components/heap/heap_caps.c @@ -122,7 +122,9 @@ HEAP_IRAM_ATTR static void *heap_caps_malloc_base( size_t size, uint32_t caps) { void *ret = NULL; - if (size == 0 || MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size) > HEAP_SIZE_MAX ) { + // remove block owner size to HEAP_SIZE_MAX rather than adding the block owner size + // to size to prevent overflows. + if (size == 0 || size > MULTI_HEAP_REMOVE_BLOCK_OWNER_SIZE(HEAP_SIZE_MAX) ) { // Avoids int overflow when adding small numbers to size, or // calculating 'end' from start+size, by limiting 'size' to the possible range return NULL; @@ -412,7 +414,9 @@ HEAP_IRAM_ATTR static void *heap_caps_realloc_base( void *ptr, size_t size, uint return NULL; } - if (MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size) > HEAP_SIZE_MAX) { + // remove block owner size to HEAP_SIZE_MAX rather than adding the block owner size + // to size to prevent overflows. + if (size > MULTI_HEAP_REMOVE_BLOCK_OWNER_SIZE(HEAP_SIZE_MAX)) { return NULL; } @@ -421,6 +425,7 @@ HEAP_IRAM_ATTR static void *heap_caps_realloc_base( void *ptr, size_t size, uint if(esp_ptr_in_diram_iram((void *)ptr)) { uint32_t *dram_addr = (uint32_t *)ptr; dram_ptr = (void *)dram_addr[-1]; + dram_ptr = MULTI_HEAP_REMOVE_BLOCK_OWNER_OFFSET(dram_ptr); heap = find_containing_heap(dram_ptr); assert(heap != NULL && "realloc() pointer is outside heap areas"); @@ -435,6 +440,12 @@ HEAP_IRAM_ATTR static void *heap_caps_realloc_base( void *ptr, size_t size, uint assert(heap != NULL && "realloc() pointer is outside heap areas"); } + // shift ptr by block owner offset. Since the ptr returned to the user + // does not include the block owner bytes (that are located at the + // beginning of the allocated memory) we have to add them back before + // processing the realloc. + ptr = MULTI_HEAP_REMOVE_BLOCK_OWNER_OFFSET(ptr); + // are the existing heap's capabilities compatible with the // requested ones? bool compatible_caps = (caps & get_all_caps(heap)) == caps; @@ -466,8 +477,10 @@ HEAP_IRAM_ATTR static void *heap_caps_realloc_base( void *ptr, size_t size, uint } assert(old_size > 0); - memcpy(new_p, ptr, MIN(size, old_size)); - heap_caps_free(ptr); + // do not copy the block owner bytes + memcpy(new_p, MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(ptr), MIN(size, old_size)); + // add the block owner bytes to ptr since they are removed in heap_caps_free + heap_caps_free(MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(ptr)); return new_p; } @@ -571,11 +584,13 @@ void heap_caps_get_info( multi_heap_info_t *info, uint32_t caps ) multi_heap_info_t hinfo; multi_heap_get_info(heap->heap, &hinfo); - info->total_free_bytes += hinfo.total_free_bytes; - info->total_allocated_bytes += hinfo.total_allocated_bytes; + info->total_free_bytes += hinfo.total_free_bytes - MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(0); + info->total_allocated_bytes += (hinfo.total_allocated_bytes - + hinfo.allocated_blocks * MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(0)); info->largest_free_block = MAX(info->largest_free_block, hinfo.largest_free_block); - info->minimum_free_bytes += hinfo.minimum_free_bytes; + info->largest_free_block -= info->largest_free_block ? MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(0) : 0; + info->minimum_free_bytes += hinfo.minimum_free_bytes - MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(0); info->allocated_blocks += hinfo.allocated_blocks; info->free_blocks += hinfo.free_blocks; info->total_blocks += hinfo.total_blocks; @@ -654,6 +669,9 @@ void heap_caps_dump_all(void) size_t heap_caps_get_allocated_size( void *ptr ) { + // add the block owner bytes back to ptr before handing over + // to multi heap layer. + ptr = MULTI_HEAP_REMOVE_BLOCK_OWNER_OFFSET(ptr); heap_t *heap = find_containing_heap(ptr); assert(heap); size_t size = multi_heap_get_allocated_size(heap->heap, ptr); @@ -696,8 +714,9 @@ HEAP_IRAM_ATTR void *heap_caps_aligned_alloc(size_t alignment, size_t size, uint //Heap has at least one of the caps requested. If caps has other bits set that this prio //doesn't cover, see if they're available in other prios. if ((get_all_caps(heap) & caps) == caps) { - //Just try to alloc, nothing special. - ret = multi_heap_aligned_alloc(heap->heap, MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size), alignment); + // Just try to alloc, nothing special. Provide the size of the block owner + // as an offset to prevent a miscalculation of the alignment. + ret = multi_heap_aligned_alloc_offs(heap->heap, MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size), alignment, MULTI_HEAP_BLOCK_OWNER_SIZE()); if (ret != NULL) { MULTI_HEAP_SET_BLOCK_OWNER(ret); ret = MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(ret); diff --git a/components/heap/include/multi_heap.h b/components/heap/include/multi_heap.h index e2aa6672d7..fb7b8797b2 100644 --- a/components/heap/include/multi_heap.h +++ b/components/heap/include/multi_heap.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -179,6 +179,17 @@ typedef struct { */ void multi_heap_get_info(multi_heap_handle_t heap, multi_heap_info_t *info); +/** + * @brief Perform an aligned allocation from the provided offset + * + * @param heap The heap in which to perform the allocation + * @param size The size of the allocation + * @param alignment How the memory must be aligned + * @param offset The offset at which the alignment should start + * @return void* The ptr to the allocated memory + */ +void *multi_heap_aligned_alloc_offs(multi_heap_handle_t heap, size_t size, size_t alignment, size_t offset); + #ifdef __cplusplus } #endif diff --git a/components/heap/linker.lf b/components/heap/linker.lf index 7109ae3814..b40ea44cce 100644 --- a/components/heap/linker.lf +++ b/components/heap/linker.lf @@ -48,6 +48,9 @@ entries: multi_heap_poisoning:multi_heap_get_allocated_size (noflash) multi_heap_poisoning:multi_heap_internal_check_block_poisoning (noflash) multi_heap_poisoning:multi_heap_internal_poison_fill_region (noflash) + multi_heap_poisoning:multi_heap_aligned_alloc_offs (noflash) + else: + multi_heap:multi_heap_aligned_alloc_offs (noflash) if HEAP_POISONING_COMPREHENSIVE = y: multi_heap_poisoning:verify_fill_pattern (noflash) diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index e6e95c966c..9a1a93d7ce 100644 --- a/components/heap/multi_heap.c +++ b/components/heap/multi_heap.c @@ -26,7 +26,14 @@ /* Defines compile-time configuration macros */ #include "multi_heap_config.h" -#if (!defined MULTI_HEAP_POISONING) && (!defined CONFIG_HEAP_TLSF_USE_ROM_IMPL) +#if (!defined MULTI_HEAP_POISONING) + +void *multi_heap_aligned_alloc_offs(multi_heap_handle_t heap, size_t size, size_t alignment, size_t offset) +{ + return multi_heap_aligned_alloc_impl_offs(heap, size, alignment, offset); +} + +#if (!defined CONFIG_HEAP_TLSF_USE_ROM_IMPL) /* if no heap poisoning, public API aliases directly to these implementations */ void *multi_heap_malloc(multi_heap_handle_t heap, size_t size) __attribute__((alias("multi_heap_malloc_impl"))); @@ -60,7 +67,9 @@ size_t multi_heap_minimum_free_size(multi_heap_handle_t heap) void *multi_heap_get_block_address(multi_heap_block_handle_t block) __attribute__((alias("multi_heap_get_block_address_impl"))); -#endif + +#endif // !CONFIG_HEAP_TLSF_USE_ROM_IMPL +#endif // !MULTI_HEAP_POISONING #define ALIGN(X) ((X) & ~(sizeof(void *)-1)) #define ALIGN_UP(X) ALIGN((X)+sizeof(void *)-1) diff --git a/components/heap/multi_heap_platform.h b/components/heap/multi_heap_platform.h index a5ce50208a..f2d1516b25 100644 --- a/components/heap/multi_heap_platform.h +++ b/components/heap/multi_heap_platform.h @@ -72,6 +72,7 @@ inline static void multi_heap_assert(bool condition, const char *format, int lin #define MULTI_HEAP_REMOVE_BLOCK_OWNER_OFFSET(HEAD) ((TaskHandle_t*)(HEAD) - 1) #define MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(SIZE) ((SIZE) + sizeof(TaskHandle_t)) #define MULTI_HEAP_REMOVE_BLOCK_OWNER_SIZE(SIZE) ((SIZE) - sizeof(TaskHandle_t)) +#define MULTI_HEAP_BLOCK_OWNER_SIZE() sizeof(TaskHandle_t) #else #define MULTI_HEAP_SET_BLOCK_OWNER(HEAD) #define MULTI_HEAP_GET_BLOCK_OWNER(HEAD) (NULL) @@ -79,6 +80,7 @@ inline static void multi_heap_assert(bool condition, const char *format, int lin #define MULTI_HEAP_REMOVE_BLOCK_OWNER_OFFSET(HEAD) (HEAD) #define MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(SIZE) (SIZE) #define MULTI_HEAP_REMOVE_BLOCK_OWNER_SIZE(SIZE) (SIZE) +#define MULTI_HEAP_BLOCK_OWNER_SIZE() 0 #endif // CONFIG_HEAP_TASK_TRACKING #else // MULTI_HEAP_FREERTOS diff --git a/components/heap/multi_heap_poisoning.c b/components/heap/multi_heap_poisoning.c index 6a1db4d70f..faa8567986 100644 --- a/components/heap/multi_heap_poisoning.c +++ b/components/heap/multi_heap_poisoning.c @@ -205,6 +205,11 @@ void block_absorb_post_hook(void *start, size_t size, bool is_free) #endif void *multi_heap_aligned_alloc(multi_heap_handle_t heap, size_t size, size_t alignment) +{ + return multi_heap_aligned_alloc_offs(heap, size, alignment, 0); +} + +void *multi_heap_aligned_alloc_offs(multi_heap_handle_t heap, size_t size, size_t alignment, size_t offset) { if (!size) { return NULL; @@ -216,7 +221,7 @@ void *multi_heap_aligned_alloc(multi_heap_handle_t heap, size_t size, size_t ali multi_heap_internal_lock(heap); poison_head_t *head = multi_heap_aligned_alloc_impl_offs(heap, size + POISON_OVERHEAD, - alignment, sizeof(poison_head_t)); + alignment, offset + sizeof(poison_head_t)); uint8_t *data = NULL; if (head != NULL) { data = poison_allocated_region(head, size); diff --git a/components/heap/test_apps/heap_tests/main/CMakeLists.txt b/components/heap/test_apps/heap_tests/main/CMakeLists.txt index 9e9d865371..0ed19610ce 100644 --- a/components/heap/test_apps/heap_tests/main/CMakeLists.txt +++ b/components/heap/test_apps/heap_tests/main/CMakeLists.txt @@ -7,7 +7,8 @@ set(src_test "test_heap_main.c" "test_malloc_caps.c" "test_malloc.c" "test_realloc.c" - "test_runtime_heap_reg.c") + "test_runtime_heap_reg.c" + "test_task_tracking.c") idf_component_register(SRCS ${src_test} INCLUDE_DIRS "." 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 new file mode 100644 index 0000000000..99215969dc --- /dev/null +++ b/components/heap/test_apps/heap_tests/main/test_task_tracking.c @@ -0,0 +1,98 @@ +/* + * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Unlicense OR CC0-1.0 + */ +#include "unity.h" +#include "stdio.h" + +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" +#include "esp_heap_caps.h" +#include "esp_heap_task_info.h" + +// This test only apply when task tracking is enabled +#if defined(CONFIG_HEAP_TASK_TRACKING) + +#define MAX_TASK_NUM 10 // Max number of per tasks info that it can store +#define MAX_BLOCK_NUM 10 // Max number of per block info that it can store +#define ALLOC_BYTES 36 + +static void check_heap_task_info(TaskHandle_t taskHdl) +{ + size_t num_totals = 0; + heap_task_totals_t s_totals_arr[MAX_TASK_NUM]; + heap_task_block_t s_block_arr[MAX_BLOCK_NUM]; + + heap_task_info_params_t heap_info = {0}; + heap_info.caps[0] = MALLOC_CAP_32BIT; // Gets heap info with CAP_32BIT capabilities + heap_info.mask[0] = MALLOC_CAP_32BIT; + heap_info.tasks = NULL; // Passing NULL captures heap info for all tasks + heap_info.num_tasks = 0; + heap_info.totals = s_totals_arr; // Gets task wise allocation details + heap_info.num_totals = &num_totals; + heap_info.max_totals = MAX_TASK_NUM; // Maximum length of "s_totals_arr" + heap_info.blocks = s_block_arr; // Gets block wise allocation details. For each block, gets owner task, address and size + heap_info.max_blocks = MAX_BLOCK_NUM; // Maximum length of "s_block_arr" + + heap_caps_get_per_task_info(&heap_info); + + bool task_found = false; + for (int i = 0 ; i < *heap_info.num_totals; i++) { + // the prescheduler allocs and free are stored as a + // task with a handle set to 0, avoid calling pcTaskGetName + // in that case. + if (heap_info.totals[i].task != 0 && (uint32_t*)(heap_info.totals[i].task) == (uint32_t*)taskHdl) { + task_found = true; + // check the number of byte allocated according to the task tracking feature + // and make sure it matches the expected value. The size returned by the + // 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_ASSERT_TRUE(task_found); +} + +static void test_task(void *args) +{ + void *ptr = heap_caps_malloc(ALLOC_BYTES, MALLOC_CAP_32BIT); + if (ptr == NULL) { + abort(); + } + + // unlock main too check task tracking feature + xTaskNotifyGive((TaskHandle_t)args); + + // wait for main to delete this task + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); +} + +/* This test will create a task, wait for the task to allocate / free memory + * so it is added to the task tracking info in the heap component and then + * call heap_caps_get_per_task_info() and make sure a task with the name test_task + * is in the list, and that the right ALLOC_BYTES are shown. + * + * Note: The memory allocated in the task is not freed for the sake of the test + * so it is normal that memory leak will be reported by the test environment. It + * shouldn't be more than the byte allocated by the task + associated metadata + */ +TEST_CASE("heap task tracking reports created task", "[heap]") +{ + TaskHandle_t test_task_handle; + + xTaskCreate(&test_task, "test_task", 3072, (void *)xTaskGetCurrentTaskHandle(), 5, &test_task_handle); + + // wait for task to allocate memory and give the hand back to the test + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + + // check that the task is referenced in the list of task + // by the task tracking feature. Check the number of bytes + // the task has allocated and make sure it is matching the + // expected value. + check_heap_task_info(test_task_handle); + + // delete the task. + vTaskDelete(test_task_handle); +} + +#endif // CONFIG_HEAP_TASK_TRACKING diff --git a/components/heap/test_apps/heap_tests/pytest_heap.py b/components/heap/test_apps/heap_tests/pytest_heap.py index c3684eff2d..4bed368aba 100644 --- a/components/heap/test_apps/heap_tests/pytest_heap.py +++ b/components/heap/test_apps/heap_tests/pytest_heap.py @@ -13,7 +13,7 @@ from pytest_embedded import Dut [ 'no_poisoning', 'light_poisoning', - 'comprehensive_poisoning' + 'comprehensive_poisoning', ] ) def test_heap_poisoning(dut: Dut) -> None: diff --git a/components/heap/test_apps/heap_tests/sdkconfig.ci.no_poisoning b/components/heap/test_apps/heap_tests/sdkconfig.ci.no_poisoning index 657a760928..58c6da62c4 100644 --- a/components/heap/test_apps/heap_tests/sdkconfig.ci.no_poisoning +++ b/components/heap/test_apps/heap_tests/sdkconfig.ci.no_poisoning @@ -1,3 +1,5 @@ CONFIG_HEAP_POISONING_DISABLED=y CONFIG_HEAP_POISONING_LIGHT=n CONFIG_HEAP_POISONING_COMPREHENSIVE=n + +CONFIG_HEAP_TASK_TRACKING=y # to make sure the config doesn't induce unexpected behavior