Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heap panic / abort cleanup #8465

Merged
merged 5 commits into from
Mar 31, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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.
mhightower83 committed Jan 27, 2022
commit 5796b4cfd60046856b67b4abc9adb9e6eb8f0f76
23 changes: 13 additions & 10 deletions cores/esp8266/umm_malloc/umm_local.c
Original file line number Diff line number Diff line change
@@ -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) {
56 changes: 40 additions & 16 deletions cores/esp8266/umm_malloc/umm_malloc.cpp
Original file line number Diff line number Diff line change
@@ -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");
2 changes: 1 addition & 1 deletion cores/esp8266/umm_malloc/umm_malloc_cfg.h
Original file line number Diff line number Diff line change
@@ -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 <pgmspace.h>
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__); })
6 changes: 2 additions & 4 deletions cores/esp8266/umm_malloc/umm_poison.c
Original file line number Diff line number Diff line change
@@ -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);