From 4824325fe4aee5ca08de230f09c5bd4777dbcfec Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Fri, 27 Oct 2023 15:06:58 +0200 Subject: [PATCH] fix(heap): Fix bugs in heap task tracking Update task tracking feature to fix bugs introduced when decoupling task tracking from heap poisoning. Closes https://github.com/espressif/esp-idf/issues/12498 Closes https://github.com/espressif/esp-idf/issues/12493 --- components/heap/heap_caps.c | 37 +++++++++++++++++++------- components/heap/include/multi_heap.h | 13 ++++++++- components/heap/linker.lf | 3 +++ components/heap/multi_heap.c | 13 +++++++-- components/heap/multi_heap_platform.h | 2 ++ components/heap/multi_heap_poisoning.c | 7 ++++- 6 files changed, 62 insertions(+), 13 deletions(-) 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);