From 76d819044417151bf6d2c56b022dc72c01b2c039 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 18 Sep 2017 14:51:51 +1000 Subject: [PATCH 1/2] multi_heap: Print the problem address when aborting due to heap corruption New multi_heap code has proven effective at aborting when buffer overruns occur, but it's currently hard to debug the stack traces from these failures. --- components/heap/multi_heap.c | 53 ++++++++++++++---------- components/heap/multi_heap_platform.h | 28 ++++++++++++- docs/api-reference/system/heap_debug.rst | 6 ++- 3 files changed, 63 insertions(+), 24 deletions(-) diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index 723d53407..1d2607913 100644 --- a/components/heap/multi_heap.c +++ b/components/heap/multi_heap.c @@ -144,12 +144,14 @@ static inline size_t block_data_size(const heap_block_t *block) /* Check a block is valid for this heap. Used to verify parameters. */ static void assert_valid_block(const heap_t *heap, const heap_block_t *block) { - assert(block >= &heap->first_block && block <= heap->last_block); /* block should be in heap */ + MULTI_HEAP_ASSERT(block >= &heap->first_block && block <= heap->last_block, + block); // block not in heap if (heap < (const heap_t *)heap->last_block) { const heap_block_t *next = get_next_block(block); - assert(next >= &heap->first_block && next <= heap->last_block); + MULTI_HEAP_ASSERT(next >= &heap->first_block && next <= heap->last_block, block); // Next block not in heap if (is_free(block)) { - assert(block->next_free >= &heap->first_block && block->next_free <= heap->last_block); + // Check block->next_free is valid + MULTI_HEAP_ASSERT(block->next_free >= &heap->first_block && block->next_free <= heap->last_block, &block->next_free); } } } @@ -168,10 +170,11 @@ static heap_block_t *get_prev_free_block(heap_t *heap, const heap_block_t *block assert(block != &heap->first_block); /* can't look for a block before first_block */ for (heap_block_t *b = &heap->first_block; b != NULL && b < block; b = b->next_free) { - assert(is_free(b)); + MULTI_HEAP_ASSERT(is_free(b), b); // Block should be free if (b->next_free == NULL || b->next_free >= block) { if (is_free(block)) { - assert(b->next_free == block); /* if block is on freelist, 'b' should be the item before it. */ + /* if block is on freelist, 'b' should be the item before it. */ + MULTI_HEAP_ASSERT(b->next_free == block, &b->next_free); } return b; /* b is the last free block before 'block' */ } @@ -199,7 +202,7 @@ static heap_block_t *merge_adjacent(heap_t *heap, heap_block_t *a, heap_block_t return b; } - assert(get_next_block(a) == b); + MULTI_HEAP_ASSERT(get_next_block(a) == b, a); // Blocks should be in order bool free = is_free(a) && is_free(b); /* merging two free blocks creates a free block */ if (!free && (is_free(a) || is_free(b))) { @@ -208,18 +211,20 @@ static heap_block_t *merge_adjacent(heap_t *heap, heap_block_t *a, heap_block_t */ heap_block_t *free_block = is_free(a) ? a : b; heap_block_t *prev_free = get_prev_free_block(heap, free_block); - assert(free_block->next_free > prev_free); + MULTI_HEAP_ASSERT(free_block->next_free > prev_free, &free_block->next_free); // Next free block should be after prev one prev_free->next_free = free_block->next_free; heap->free_bytes -= block_data_size(free_block); } a->header = b->header & NEXT_BLOCK_MASK; - assert(a->header != 0); + MULTI_HEAP_ASSERT(a->header != 0, a); if (free) { a->header |= BLOCK_FREE_FLAG; - assert(b->next_free == NULL || b->next_free > a); - assert(b->next_free == NULL || b->next_free > b); + if (b->next_free != NULL) { + MULTI_HEAP_ASSERT(b->next_free > a, &b->next_free); + MULTI_HEAP_ASSERT(b->next_free > b, &b->next_free); + } a->next_free = b->next_free; /* b's header can be put into the pool of free bytes */ @@ -245,8 +250,8 @@ static heap_block_t *merge_adjacent(heap_t *heap, heap_block_t *a, heap_block_t */ static void split_if_necessary(heap_t *heap, heap_block_t *block, size_t size, heap_block_t *prev_free_block) { - assert(!is_free(block)); /* split_if_necessary doesn't expect a free block */ - assert(size <= block_data_size(block)); /* can't grow a block this way! */ + MULTI_HEAP_ASSERT(!is_free(block), block); // split block shouldn't be free + MULTI_HEAP_ASSERT(size <= block_data_size(block), block); // size should be valid size = ALIGN_UP(size); /* can't split the head or tail block */ @@ -266,7 +271,9 @@ static void split_if_necessary(heap_t *heap, heap_block_t *block, size_t size, h if (prev_free_block == NULL) { prev_free_block = get_prev_free_block(heap, block); } - assert(prev_free_block->next_free > new_block); /* prev_free_block should point to a free block after new_block */ + /* prev_free_block should point to a free block after new_block */ + MULTI_HEAP_ASSERT(prev_free_block->next_free > new_block, + &prev_free_block->next_free); // free blocks should be in order new_block->next_free = prev_free_block->next_free; prev_free_block->next_free = new_block; heap->free_bytes += block_data_size(new_block); @@ -277,7 +284,7 @@ size_t multi_heap_get_allocated_size_impl(multi_heap_handle_t heap, void *p) heap_block_t *pb = get_block(p); assert_valid_block(heap, pb); - assert(!is_free(pb)); + MULTI_HEAP_ASSERT(!is_free(pb), pb); // block should be free return block_data_size(pb); } @@ -339,7 +346,8 @@ void *multi_heap_malloc_impl(multi_heap_handle_t heap, size_t size) /* Find best free block to perform the allocation in */ prev = &heap->first_block; for (heap_block_t *b = heap->first_block.next_free; b != NULL; b = b->next_free) { - assert(is_free(b)); + MULTI_HEAP_ASSERT(b > prev, &prev->next_free); // free blocks should be ascending in address + MULTI_HEAP_ASSERT(is_free(b), b); // block should be free size_t bs = block_data_size(b); if (bs >= size && bs < best_size) { best_block = b; @@ -384,15 +392,16 @@ void multi_heap_free_impl(multi_heap_handle_t heap, void *p) MULTI_HEAP_LOCK(heap->lock); assert_valid_block(heap, pb); - assert(!is_free(pb)); - assert(!is_last_block(pb)); - assert(pb != &heap->first_block); + MULTI_HEAP_ASSERT(!is_free(pb), pb); // block should not be free + MULTI_HEAP_ASSERT(!is_last_block(pb), pb); // block should not be last block + MULTI_HEAP_ASSERT(pb != &heap->first_block, pb); // block should not be first block heap_block_t *next = get_next_block(pb); /* Update freelist pointers */ heap_block_t *prev_free = get_prev_free_block(heap, pb); - assert(prev_free->next_free == NULL || prev_free->next_free > pb); + // freelist validity check + MULTI_HEAP_ASSERT(prev_free->next_free == NULL || prev_free->next_free > pb, &prev_free->next_free); pb->next_free = prev_free->next_free; prev_free->next_free = pb; @@ -428,7 +437,8 @@ void *multi_heap_realloc_impl(multi_heap_handle_t heap, void *p, size_t size) } assert_valid_block(heap, pb); - assert(!is_free(pb) && "realloc arg should be allocated"); + // non-null realloc arg should be allocated + MULTI_HEAP_ASSERT(!is_free(pb), pb); if (size == 0) { /* note: calling multi_free_impl() here as we've already been @@ -646,7 +656,8 @@ void multi_heap_get_info_impl(multi_heap_handle_t heap, multi_heap_info_t *info) } info->minimum_free_bytes = heap->minimum_free_bytes; - assert(info->total_free_bytes == heap->free_bytes); + // heap has wrong total size (address printed here is not indicative of the real error) + MULTI_HEAP_ASSERT(info->total_free_bytes == heap->free_bytes, heap); MULTI_HEAP_UNLOCK(heap->lock); diff --git a/components/heap/multi_heap_platform.h b/components/heap/multi_heap_platform.h index 2c4d9f96f..874fd535b 100644 --- a/components/heap/multi_heap_platform.h +++ b/components/heap/multi_heap_platform.h @@ -18,6 +18,7 @@ #include #include #include +#include /* Because malloc/free can happen inside an ISR context, we need to use portmux spinlocks here not RTOS mutexes */ @@ -40,11 +41,36 @@ #define MULTI_HEAP_PRINTF ets_printf #define MULTI_HEAP_STDERR_PRINTF(MSG, ...) ets_printf(MSG, __VA_ARGS__) -#else +inline static void multi_heap_assert(bool condition, const char *format, int line, intptr_t address) +{ + /* Can't use libc assert() here as it calls printf() which can cause another malloc() for a newlib lock. + + Also, it's useful to be able to print the memory address where corruption was detected. + */ +#ifndef NDEBUG + if(!condition) { +#ifndef CONFIG_OPTIMIZATION_ASSERTIONS_SILENT + ets_printf(format, line, address); +#endif // CONFIG_OPTIMIZATION_ASSERTIONS_SILENT + abort(); + } +#else // NDEBUG + (void) condition; +#endif // NDEBUG +} + +#define MULTI_HEAP_ASSERT(CONDITION, ADDRESS) \ + multi_heap_assert((CONDITION), "CORRUPT HEAP: multi_heap.c:%d detected at 0x%08x\n", \ + __LINE__, (intptr_t)(ADDRESS)) + +#else // ESP_PLATFORM + +#include #define MULTI_HEAP_PRINTF printf #define MULTI_HEAP_STDERR_PRINTF(MSG, ...) fprintf(stderr, MSG, __VA_ARGS__) #define MULTI_HEAP_LOCK(PLOCK) #define MULTI_HEAP_UNLOCK(PLOCK) +#define MULTI_HEAP_ASSERT(CONDITION, ADDRESS) assert((CONDITION) && "Heap corrupt") #endif diff --git a/docs/api-reference/system/heap_debug.rst b/docs/api-reference/system/heap_debug.rst index 478f602fa..b93a3272b 100644 --- a/docs/api-reference/system/heap_debug.rst +++ b/docs/api-reference/system/heap_debug.rst @@ -38,7 +38,9 @@ Assertions The heap implementation (``multi_heap.c``, etc.) includes a lot of assertions which will fail if the heap memory is corrupted. To detect heap corruption most effectively, ensure that assertions are enabled in ``make menuconfig`` under ``Compiler options``. -It's also possible to manually check heap integrity by calling the ``heap_caps_check_integrity()`` function (see below). This function checks all of requested heap memory for integrity, and can be used even if assertions are disabled. +If a heap integrity assertion fails, a line will be printed like ``CORRUPT HEAP: multi_heap.c:225 detected at 0x3ffbb71c``. The memory address which is printed is the address of the heap structure which has corrupt content. + +It's also possible to manually check heap integrity by calling the ``heap_caps_check_integrity()`` function (see below). This function checks all of requested heap memory for integrity, and can be used even if assertions are disabled. If the integrity check prints an error, it will contain the address(es) of corrupt heap structures. Configuration ^^^^^^^^^^^^^ @@ -81,7 +83,7 @@ Finding Heap Corruption Memory corruption can be one of the hardest classes of bugs to find and fix, as one area of memory can be corrupted from a totally different place. Some tips: -- If you can find the address (in memory) which is being corrupted, you can set a watchpoint on this address via JTAG to have the CPU halt when it is written to. +- Once you know the address (in memory) which is being corrupted, you can set a watchpoint on this address via JTAG to have the CPU halt when it is written to. - If you don't have JTAG, but you do know roughly when the corruption happens, then you can set a watchpoint in software. A fatal exception will occur when the watchpoint triggers. For example ``esp_set_watchpoint(0, (void *)addr, 4, ESP_WATCHPOINT_STORE``. Note that the watchpoint is set on the current running CPU only, so if you don't know which CPU is corrupting memory then you will need to call this function on both CPUs. - For buffer overflows, `heap tracing`_ in ``HEAP_TRACE_ALL`` mode lets you see which callers allocate the memory address(es) immediately before the address which is being corrupted. There is a strong chance this is the code which overflows the buffer. From 959462ffb6c25026c85833d3d5fbb723ae7ac5cd Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 18 Sep 2017 16:54:28 +1000 Subject: [PATCH 2/2] multi_heap_poisoning: Use MULTI_HEAP_STDERR_PRINTF (ets_printf) to print heap errors Needed because normal printf() can trigger a malloc() (for standard stream locks) which then re-triggers this check. --- components/heap/multi_heap_poisoning.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/heap/multi_heap_poisoning.c b/components/heap/multi_heap_poisoning.c index 2ca17e0e2..a91446f09 100644 --- a/components/heap/multi_heap_poisoning.c +++ b/components/heap/multi_heap_poisoning.c @@ -92,7 +92,7 @@ static poison_head_t *verify_allocated_region(void *data, bool print_errors) /* check if the beginning of the data was overwritten */ if (head->head_canary != HEAD_CANARY_PATTERN) { if (print_errors) { - printf("CORRUPT HEAP: Bad head at %p. Expected 0x%08x got 0x%08x\n", &head->head_canary, + MULTI_HEAP_STDERR_PRINTF("CORRUPT HEAP: Bad head at %p. Expected 0x%08x got 0x%08x\n", &head->head_canary, HEAD_CANARY_PATTERN, head->head_canary); } return NULL; @@ -142,7 +142,7 @@ static bool verify_fill_pattern(void *data, size_t size, bool print_errors, bool while (size >= 4) { if (*p != EXPECT_WORD) { if (print_errors) { - printf("Invalid data at %p. Expected 0x%08x got 0x%08x\n", p, EXPECT_WORD, *p); + MULTI_HEAP_STDERR_PRINTF("CORRUPT HEAP: Invalid data at %p. Expected 0x%08x got 0x%08x\n", p, EXPECT_WORD, *p); } valid = false; } @@ -159,7 +159,7 @@ static bool verify_fill_pattern(void *data, size_t size, bool print_errors, bool for (int i = 0; i < size; i++) { if (p[i] != (uint8_t)EXPECT_WORD) { if (print_errors) { - printf("Invalid data at %p. Expected 0x%02x got 0x%02x\n", p, (uint8_t)EXPECT_WORD, *p); + MULTI_HEAP_STDERR_PRINTF("CORRUPT HEAP: Invalid data at %p. Expected 0x%02x got 0x%02x\n", p, (uint8_t)EXPECT_WORD, *p); } valid = false; } @@ -315,7 +315,7 @@ bool multi_heap_internal_check_block_poisoning(void *start, size_t size, bool is /* block can be bigger than alloc_size, for reasons of alignment & fragmentation, but block can never be smaller than head->alloc_size... */ if (print_errors) { - printf("CORRUPT HEAP: Size at %p expected <=0x%08x got 0x%08x\n", &head->alloc_size, + MULTI_HEAP_STDERR_PRINTF("CORRUPT HEAP: Size at %p expected <=0x%08x got 0x%08x\n", &head->alloc_size, size - POISON_OVERHEAD, head->alloc_size); } return false;