From 7492c862af90d8a8d9920f30b3f652287ef12436 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 4 Oct 2023 14:42:00 +0200 Subject: [PATCH] feat(heap): Dissociate heap poisoning from task tracking In order to enable CONFIG_HEAP_TASK_TRACKING, some kind of poisoning had to be enabled (!HEAP_POISONING_DISABLED). However since those functionalities don't seem to be related in any way, this commit decouple them by removing MULTI_HEAP_BLOCK_OWNER from poison_head_t in multi_heap_poisoning.c and handling the block ownership in heap_caps.c instead. Note that handling task tracking in multi_heap.c would necessitate updating the ROM implementation of multi_heap.c as well. For this reason, the task tracking feature has to be handled in heap_caps.c. --- components/heap/Kconfig | 1 - components/heap/heap_caps.c | 31 ++++++++++++------- components/heap/heap_task_info.c | 3 +- components/heap/linker.lf | 1 - components/heap/multi_heap.c | 8 +---- components/heap/multi_heap_internal.h | 3 -- components/heap/multi_heap_platform.h | 18 +++++++---- components/heap/multi_heap_poisoning.c | 9 +----- .../heap_task_tracking/sdkconfig.defaults | 1 - 9 files changed, 34 insertions(+), 41 deletions(-) diff --git a/components/heap/Kconfig b/components/heap/Kconfig index 1a05ddd78c9..8de0f77519f 100644 --- a/components/heap/Kconfig +++ b/components/heap/Kconfig @@ -63,7 +63,6 @@ menu "Heap memory debugging" config HEAP_TASK_TRACKING bool "Enable heap task tracking" - depends on !HEAP_POISONING_DISABLED help Enables tracking the task responsible for each heap allocation. diff --git a/components/heap/heap_caps.c b/components/heap/heap_caps.c index 9df5fb212ea..501ebe6392e 100644 --- a/components/heap/heap_caps.c +++ b/components/heap/heap_caps.c @@ -122,7 +122,7 @@ HEAP_IRAM_ATTR static void *heap_caps_malloc_base( size_t size, uint32_t caps) { void *ret = NULL; - if (size == 0 || size > HEAP_SIZE_MAX ) { + if (size == 0 || MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(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; @@ -164,17 +164,20 @@ HEAP_IRAM_ATTR static void *heap_caps_malloc_base( size_t size, uint32_t caps) //This is special, insofar that what we're going to get back is a DRAM address. If so, //we need to 'invert' it (lowest address in DRAM == highest address in IRAM and vice-versa) and //add a pointer to the DRAM equivalent before the address we're going to return. - ret = multi_heap_malloc(heap->heap, size + 4); // int overflow checked above - + ret = multi_heap_malloc(heap->heap, MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size) + 4); // int overflow checked above if (ret != NULL) { + MULTI_HEAP_SET_BLOCK_OWNER(ret); + ret = MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(ret); uint32_t *iptr = dram_alloc_to_iram_addr(ret, size + 4); // int overflow checked above CALL_HOOK(esp_heap_trace_alloc_hook, iptr, size, caps); return iptr; } } else { //Just try to alloc, nothing special. - ret = multi_heap_malloc(heap->heap, size); + ret = multi_heap_malloc(heap->heap, MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size)); if (ret != NULL) { + MULTI_HEAP_SET_BLOCK_OWNER(ret); + ret = MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(ret); CALL_HOOK(esp_heap_trace_alloc_hook, ret, size, caps); return ret; } @@ -382,10 +385,10 @@ HEAP_IRAM_ATTR void heap_caps_free( void *ptr) uint32_t *dramAddrPtr = (uint32_t *)ptr; ptr = (void *)dramAddrPtr[-1]; } - - heap_t *heap = find_containing_heap(ptr); + void *block_owner_ptr = MULTI_HEAP_REMOVE_BLOCK_OWNER_OFFSET(ptr); + heap_t *heap = find_containing_heap(block_owner_ptr); assert(heap != NULL && "free() target pointer is outside heap areas"); - multi_heap_free(heap->heap, ptr); + multi_heap_free(heap->heap, block_owner_ptr); CALL_HOOK(esp_heap_trace_free_hook, ptr); } @@ -409,7 +412,7 @@ HEAP_IRAM_ATTR static void *heap_caps_realloc_base( void *ptr, size_t size, uint return NULL; } - if (size > HEAP_SIZE_MAX) { + if (MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size) > HEAP_SIZE_MAX) { return NULL; } @@ -439,8 +442,10 @@ HEAP_IRAM_ATTR static void *heap_caps_realloc_base( void *ptr, size_t size, uint if (compatible_caps && !ptr_in_diram_case) { // try to reallocate this memory within the same heap // (which will resize the block if it can) - void *r = multi_heap_realloc(heap->heap, ptr, size); + void *r = multi_heap_realloc(heap->heap, ptr, MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size)); if (r != NULL) { + MULTI_HEAP_SET_BLOCK_OWNER(r); + r = MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(r); CALL_HOOK(esp_heap_trace_alloc_hook, r, size, caps); return r; } @@ -652,7 +657,7 @@ size_t heap_caps_get_allocated_size( void *ptr ) heap_t *heap = find_containing_heap(ptr); assert(heap); size_t size = multi_heap_get_allocated_size(heap->heap, ptr); - return size; + return MULTI_HEAP_REMOVE_BLOCK_OWNER_SIZE(size); } HEAP_IRAM_ATTR void *heap_caps_aligned_alloc(size_t alignment, size_t size, uint32_t caps) @@ -672,7 +677,7 @@ HEAP_IRAM_ATTR void *heap_caps_aligned_alloc(size_t alignment, size_t size, uint return NULL; } - if (size > HEAP_SIZE_MAX) { + if (MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(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 heap_caps_alloc_failed(size, caps, __func__); @@ -692,8 +697,10 @@ HEAP_IRAM_ATTR void *heap_caps_aligned_alloc(size_t alignment, size_t size, uint //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, size, alignment); + ret = multi_heap_aligned_alloc(heap->heap, MULTI_HEAP_ADD_BLOCK_OWNER_SIZE(size), alignment); if (ret != NULL) { + MULTI_HEAP_SET_BLOCK_OWNER(ret); + ret = MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(ret); CALL_HOOK(esp_heap_trace_alloc_hook, ret, size, caps); return ret; } diff --git a/components/heap/heap_task_info.c b/components/heap/heap_task_info.c index 54c6fb346c2..83368e5db4a 100644 --- a/components/heap/heap_task_info.c +++ b/components/heap/heap_task_info.c @@ -68,8 +68,7 @@ size_t heap_caps_get_per_task_info(heap_task_info_params_t *params) } void *p = multi_heap_get_block_address(b); // Safe, only arithmetic size_t bsize = multi_heap_get_allocated_size(heap, p); // Validates - TaskHandle_t btask = (TaskHandle_t)multi_heap_get_block_owner(b); - + TaskHandle_t btask = MULTI_HEAP_GET_BLOCK_OWNER(p); // Accumulate per-task allocation totals. if (params->totals) { size_t i; diff --git a/components/heap/linker.lf b/components/heap/linker.lf index 7d30fc20c70..7109ae3814a 100644 --- a/components/heap/linker.lf +++ b/components/heap/linker.lf @@ -45,7 +45,6 @@ entries: multi_heap_poisoning:multi_heap_aligned_free (noflash) multi_heap_poisoning:multi_heap_realloc (noflash) multi_heap_poisoning:multi_heap_get_block_address (noflash) - multi_heap_poisoning:multi_heap_get_block_owner (noflash) 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) diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index 05477806d42..e6e95c966c7 100644 --- a/components/heap/multi_heap.c +++ b/components/heap/multi_heap.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -60,12 +60,6 @@ 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"))); - -void *multi_heap_get_block_owner(multi_heap_block_handle_t block) -{ - return NULL; -} - #endif #define ALIGN(X) ((X) & ~(sizeof(void *)-1)) diff --git a/components/heap/multi_heap_internal.h b/components/heap/multi_heap_internal.h index b34c3393c61..2fd09a9ad06 100644 --- a/components/heap/multi_heap_internal.h +++ b/components/heap/multi_heap_internal.h @@ -87,6 +87,3 @@ bool multi_heap_is_free(const multi_heap_block_handle_t block); /* Get the data address of a heap block */ void *multi_heap_get_block_address(multi_heap_block_handle_t block); - -/* Get the owner identification for a heap block */ -void *multi_heap_get_block_owner(multi_heap_block_handle_t block); diff --git a/components/heap/multi_heap_platform.h b/components/heap/multi_heap_platform.h index 35c2a33373e..a5ce50208a5 100644 --- a/components/heap/multi_heap_platform.h +++ b/components/heap/multi_heap_platform.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -66,14 +66,20 @@ inline static void multi_heap_assert(bool condition, const char *format, int lin #ifdef CONFIG_HEAP_TASK_TRACKING #include -#define MULTI_HEAP_BLOCK_OWNER TaskHandle_t task; -#define MULTI_HEAP_SET_BLOCK_OWNER(HEAD) (HEAD)->task = xTaskGetCurrentTaskHandle() -#define MULTI_HEAP_GET_BLOCK_OWNER(HEAD) ((HEAD)->task) +#define MULTI_HEAP_SET_BLOCK_OWNER(HEAD) *((TaskHandle_t*)HEAD) = xTaskGetCurrentTaskHandle() +#define MULTI_HEAP_GET_BLOCK_OWNER(HEAD) *((TaskHandle_t*)HEAD) +#define MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(HEAD) ((TaskHandle_t*)(HEAD) + 1) +#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)) #else -#define MULTI_HEAP_BLOCK_OWNER #define MULTI_HEAP_SET_BLOCK_OWNER(HEAD) #define MULTI_HEAP_GET_BLOCK_OWNER(HEAD) (NULL) -#endif +#define MULTI_HEAP_ADD_BLOCK_OWNER_OFFSET(HEAD) (HEAD) +#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) +#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 3b608e0b247..6a1db4d70f0 100644 --- a/components/heap/multi_heap_poisoning.c +++ b/components/heap/multi_heap_poisoning.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -51,7 +51,6 @@ typedef struct { uint32_t head_canary; - MULTI_HEAP_BLOCK_OWNER size_t alloc_size; } poison_head_t; @@ -72,7 +71,6 @@ __attribute__((noinline)) static uint8_t *poison_allocated_region(poison_head_t poison_tail_t *tail = (poison_tail_t *)(data + alloc_size); head->alloc_size = alloc_size; head->head_canary = HEAD_CANARY_PATTERN; - MULTI_HEAP_SET_BLOCK_OWNER(head); uint32_t tail_canary = TAIL_CANARY_PATTERN; if ((intptr_t)tail % sizeof(void *) == 0) { @@ -351,11 +349,6 @@ void *multi_heap_get_block_address(multi_heap_block_handle_t block) return head + sizeof(poison_head_t); } -void *multi_heap_get_block_owner(multi_heap_block_handle_t block) -{ - return MULTI_HEAP_GET_BLOCK_OWNER((poison_head_t*)multi_heap_get_block_address_impl(block)); -} - multi_heap_handle_t multi_heap_register(void *start, size_t size) { #ifdef SLOW diff --git a/examples/system/heap_task_tracking/sdkconfig.defaults b/examples/system/heap_task_tracking/sdkconfig.defaults index 9f0f338bdf0..9c49654ecda 100644 --- a/examples/system/heap_task_tracking/sdkconfig.defaults +++ b/examples/system/heap_task_tracking/sdkconfig.defaults @@ -1,2 +1 @@ -CONFIG_HEAP_POISONING_LIGHT=y CONFIG_HEAP_TASK_TRACKING=y