From 5796b4cfd60046856b67b4abc9adb9e6eb8f0f76 Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Thu, 27 Jan 2022 08:08:08 -0800 Subject: [PATCH] Heap panic / abort cleanup Isolate NULL/panic test of _context to dev debug assert macro. Use abort instead of panic for case of caller providing non-heap address pointer. Added debug print. Improved get_unpoisoned_check_neighbors to print file/line when available. --- cores/esp8266/umm_malloc/umm_local.c | 23 ++++++---- cores/esp8266/umm_malloc/umm_malloc.cpp | 56 ++++++++++++++++------- cores/esp8266/umm_malloc/umm_malloc_cfg.h | 2 +- cores/esp8266/umm_malloc/umm_poison.c | 6 +-- 4 files changed, 56 insertions(+), 31 deletions(-) diff --git a/cores/esp8266/umm_malloc/umm_local.c b/cores/esp8266/umm_malloc/umm_local.c index 901fdfc63e..10769e1818 100644 --- a/cores/esp8266/umm_malloc/umm_local.c +++ b/cores/esp8266/umm_malloc/umm_local.c @@ -98,17 +98,20 @@ static void *get_unpoisoned_check_neighbors(void *vptr, const char *file, int li UMM_CRITICAL_DECL(id_poison); uint16_t c; bool poison = false; - umm_heap_context_t *_context = umm_get_ptr_context(vptr); - if (NULL == _context) { - panic(); - return NULL; + umm_heap_context_t *_context = _umm_get_ptr_context((void *)ptr); + if (_context) { + + /* Figure out which block we're in. Note the use of truncated division... */ + c = (ptr - (uintptr_t)(&(_context->heap[0]))) / sizeof(umm_block); + + UMM_CRITICAL_ENTRY(id_poison); + poison = + check_poison_block(&UMM_BLOCK(c)) && + check_poison_neighbors(_context, c); + UMM_CRITICAL_EXIT(id_poison); + } else { + DBGLOG_ERROR("\nPointer %p is not a Heap address.\n", vptr); } - /* Figure out which block we're in. Note the use of truncated division... */ - c = (ptr - (uintptr_t)(&(_context->heap[0]))) / sizeof(umm_block); - - UMM_CRITICAL_ENTRY(id_poison); - poison = check_poison_block(&UMM_BLOCK(c)) && check_poison_neighbors(_context, c); - UMM_CRITICAL_EXIT(id_poison); if (!poison) { if (file) { diff --git a/cores/esp8266/umm_malloc/umm_malloc.cpp b/cores/esp8266/umm_malloc/umm_malloc.cpp index 694478cd2b..85890d409c 100644 --- a/cores/esp8266/umm_malloc/umm_malloc.cpp +++ b/cores/esp8266/umm_malloc/umm_malloc.cpp @@ -82,6 +82,19 @@ extern "C" { // C extern void *UMM_MALLOC_CFG_HEAP_ADDR; // C extern uint32_t UMM_MALLOC_CFG_HEAP_SIZE; +#if 0 // Must be zero at release +#warning "Macro NON_NULL_CONTEXT_ASSERT() is active!" +/* + * Keep for future debug/maintenance of umm_malloc. Not needed in a + * regular/debug build. Call paths that use NON_NULL_CONTEXT_ASSERT logically + * guard against returning NULL. This macro double-checks that assumption during + * development. + */ +#define NON_NULL_CONTEXT_ASSERT() assert((NULL != _context)) +#else +#define NON_NULL_CONTEXT_ASSERT() (void)0 +#endif + #include "umm_local.h" // target-dependent supplemental /* ------------------------------------------------------------------------- */ @@ -212,21 +225,41 @@ int umm_get_heap_stack_index(void) { * realloc or free since you may not be in the right heap to handle it. * */ -static bool test_ptr_context(size_t which, void *ptr) { +static bool test_ptr_context(const size_t which, const void *const ptr) { return heap_context[which].heap && ptr >= (void *)heap_context[which].heap && ptr < heap_context[which].heap_end; } -static umm_heap_context_t *umm_get_ptr_context(void *ptr) { +/* + * Find Heap context by allocation address - may return NULL + */ +umm_heap_context_t *_umm_get_ptr_context(const void *const ptr) { for (size_t i = 0; i < UMM_NUM_HEAPS; i++) { if (test_ptr_context(i, ptr)) { return umm_get_heap_by_id(i); } } - panic(); + return NULL; +} + +/* + * Find Heap context by allocation address - must either succeed or abort + */ +static umm_heap_context_t *umm_get_ptr_context(const void *const ptr) { + umm_heap_context_t *const _context = _umm_get_ptr_context(ptr); + if (_context) { + return _context; + } + + [[maybe_unused]] uintptr_t sketch_ptr = (uintptr_t)ptr; + #if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) + sketch_ptr += sizeof(UMM_POISONED_BLOCK_LEN_TYPE) + UMM_POISON_SIZE_BEFORE; + #endif + DBGLOG_ERROR("\nPointer %p is not a Heap address.\n", (void *)sketch_ptr); + abort(); return NULL; } @@ -556,10 +589,7 @@ static void umm_free_core(umm_heap_context_t *_context, void *ptr) { uint16_t c; - if (NULL == _context) { - panic(); - return; - } + NON_NULL_CONTEXT_ASSERT(); STATS__FREE_REQUEST(id_free); /* @@ -649,12 +679,9 @@ static void *umm_malloc_core(umm_heap_context_t *_context, size_t size) { uint16_t cf; - STATS__ALLOC_REQUEST(id_malloc, size); + NON_NULL_CONTEXT_ASSERT(); - if (NULL == _context) { - panic(); - return NULL; - } + STATS__ALLOC_REQUEST(id_malloc, size); blocks = umm_blocks(size); @@ -902,10 +929,7 @@ void *umm_realloc(void *ptr, size_t size) { /* Need to be in the heap in which this block lives */ umm_heap_context_t *_context = umm_get_ptr_context(ptr); - if (NULL == _context) { - panic(); - return NULL; - } + NON_NULL_CONTEXT_ASSERT(); if (0 == size) { DBGLOG_DEBUG("realloc to 0 size, just free the block\n"); diff --git a/cores/esp8266/umm_malloc/umm_malloc_cfg.h b/cores/esp8266/umm_malloc/umm_malloc_cfg.h index 3ec44c1f40..c9c453b9c5 100644 --- a/cores/esp8266/umm_malloc/umm_malloc_cfg.h +++ b/cores/esp8266/umm_malloc/umm_malloc_cfg.h @@ -818,7 +818,7 @@ void IRAM_ATTR heap_vPortFree(void *ptr, const char *file, int line); #define dbg_heap_free(p) free(p) #endif -#elif defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) +#elif defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) // #elif for #ifdef DEBUG_ESP_OOM #include void *IRAM_ATTR heap_pvPortRealloc(void *ptr, size_t size, const char *file, int line); #define realloc(p,s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; heap_pvPortRealloc(p, s, mem_debug_file, __LINE__); }) diff --git a/cores/esp8266/umm_malloc/umm_poison.c b/cores/esp8266/umm_malloc/umm_poison.c index 1d5e7651de..49a2ed3528 100644 --- a/cores/esp8266/umm_malloc/umm_poison.c +++ b/cores/esp8266/umm_malloc/umm_poison.c @@ -138,10 +138,8 @@ static void *get_unpoisoned(void *vptr) { ptr -= (sizeof(UMM_POISONED_BLOCK_LEN_TYPE) + UMM_POISON_SIZE_BEFORE); umm_heap_context_t *_context = umm_get_ptr_context(vptr); - if (NULL == _context) { - panic(); - return NULL; - } + NON_NULL_CONTEXT_ASSERT(); + /* Figure out which block we're in. Note the use of truncated division... */ c = (ptr - (uintptr_t)(&(_context->heap[0]))) / sizeof(umm_block);