From db49c531c188c1ef29320e6b6be043336429c4a6 Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Fri, 21 Apr 2023 14:31:44 -0700 Subject: [PATCH 1/2] umm_poison false positive from ISR The umm_poison logic runs outside the UMM_CRITICAL_* umbrella. When interrupt routines do alloc calls, it is possible to interrupt an in-progress allocation just before the poison is set, with a new alloc request resulting in a false "poison check fail" against the in-progress allocation. The SDK does mallocs from ISRs. SmartConfig can illustrate this issue. --- cores/esp8266/umm_malloc/umm_local.c | 8 ++++++++ cores/esp8266/umm_malloc/umm_local.h | 18 ++++++++++++++++- cores/esp8266/umm_malloc/umm_malloc.cpp | 3 +++ cores/esp8266/umm_malloc/umm_poison.c | 27 +++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletion(-) diff --git a/cores/esp8266/umm_malloc/umm_local.c b/cores/esp8266/umm_malloc/umm_local.c index b02e511cea..063cd66f5d 100644 --- a/cores/esp8266/umm_malloc/umm_local.c +++ b/cores/esp8266/umm_malloc/umm_local.c @@ -138,6 +138,8 @@ static void *get_unpoisoned_check_neighbors(void *vptr, const char *file, int li void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line) { void *ret; + POISON_CRITICAL_ENTRY(); + ptr = get_unpoisoned_check_neighbors(ptr, file, line); add_poison_size(&size); @@ -145,6 +147,8 @@ void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line) ret = get_poisoned(ret, size); + POISON_CRITICAL_EXIT(); + return ret; } @@ -152,8 +156,12 @@ void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line) void umm_poison_free_fl(void *ptr, const char *file, int line) { + POISON_CRITICAL_ENTRY(); + ptr = get_unpoisoned_check_neighbors(ptr, file, line); + POISON_CRITICAL_EXIT(); + umm_free(ptr); } #endif diff --git a/cores/esp8266/umm_malloc/umm_local.h b/cores/esp8266/umm_malloc/umm_local.h index c5dcffd73c..320f625848 100644 --- a/cores/esp8266/umm_malloc/umm_local.h +++ b/cores/esp8266/umm_malloc/umm_local.h @@ -48,13 +48,29 @@ static size_t umm_uadd_sat(const size_t a, const size_t b); static bool check_poison_neighbors(umm_heap_context_t *_context, uint16_t cur); #endif +#if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) +/* + The umm_poison logic runs outside the UMM_CRITICAL_* umbrella. When interrupt + routines do alloc calls, it is possible to interrupt an in-progress allocation + just before the poison is set, with a new alloc request resulting in a false + "poison check fail" against the in-progress allocation. The SDK does mallocs + from ISRs. SmartConfig can illustrate this issue. + + Add POISON_CRITICAL_* to skip check when not the first. +*/ +static int volatile umm_poison_critical __attribute__((section(".noinit"))); + +#define POISON_CRITICAL_ENTRY() (umm_poison_critical++) +#define POISON_CRITICAL_EXIT() (umm_poison_critical--) +#define POISON_CRITICAL_GET_LEVEL() (umm_poison_critical) +#endif + #if defined(UMM_STATS) || defined(UMM_STATS_FULL) void ICACHE_FLASH_ATTR umm_print_stats(int force); #endif - int ICACHE_FLASH_ATTR umm_info_safe_printf_P(const char *fmt, ...) __attribute__((format(printf, 1, 2))); #define UMM_INFO_PRINTF(fmt, ...) umm_info_safe_printf_P(PSTR(fmt),##__VA_ARGS__) diff --git a/cores/esp8266/umm_malloc/umm_malloc.cpp b/cores/esp8266/umm_malloc/umm_malloc.cpp index 4a1bdfc76c..78c648f45b 100644 --- a/cores/esp8266/umm_malloc/umm_malloc.cpp +++ b/cores/esp8266/umm_malloc/umm_malloc.cpp @@ -563,6 +563,9 @@ void ICACHE_MAYBE umm_init(void) { // Note, full_init must be true for the primary heap, DRAM. umm_init_heap(UMM_HEAP_DRAM, (void *)UMM_MALLOC_CFG_HEAP_ADDR, UMM_MALLOC_CFG_HEAP_SIZE, true); +#if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) + umm_poison_critical = 0; +#endif // upstream ref: // Initialize the heap from linker supplied values */ // umm_init_heap(UMM_MALLOC_CFG_HEAP_ADDR, UMM_MALLOC_CFG_HEAP_SIZE); diff --git a/cores/esp8266/umm_malloc/umm_poison.c b/cores/esp8266/umm_malloc/umm_poison.c index 9ebaafdd7a..3702e57d45 100644 --- a/cores/esp8266/umm_malloc/umm_poison.c +++ b/cores/esp8266/umm_malloc/umm_poison.c @@ -10,6 +10,7 @@ #define UMM_POISON_BLOCK_SIZE (UMM_POISON_SIZE_BEFORE + sizeof(UMM_POISONED_BLOCK_LEN_TYPE) + UMM_POISON_SIZE_AFTER) + /* * Yields the total size of a poison block of size `s`. * If `s` is 0, returns 0. @@ -75,6 +76,11 @@ static bool check_poison(const void *ptr, size_t poison_size, static bool check_poison_block(umm_block *pblock) { bool ok = true; + if (1 != POISON_CRITICAL_GET_LEVEL()) { + // Nested checks are not safe + return ok; + } + if (pblock->header.used.next & UMM_FREELIST_MASK) { DBGLOG_ERROR("check_poison_block is called for free block 0x%lx\n", (unsigned long)pblock); } else { @@ -160,12 +166,16 @@ static void *get_unpoisoned(void *vptr) { void *umm_poison_malloc(size_t size) { void *ret; + POISON_CRITICAL_ENTRY(); + add_poison_size(&size); ret = umm_malloc(size); ret = get_poisoned(ret, size); + POISON_CRITICAL_EXIT(); + return ret; } @@ -174,6 +184,8 @@ void *umm_poison_malloc(size_t size) { void *umm_poison_calloc(size_t num, size_t item_size) { void *ret; + POISON_CRITICAL_ENTRY(); + // Use saturated multiply. // Rely on umm_malloc to supply the fail response as needed. size_t size = umm_umul_sat(num, item_size); @@ -188,6 +200,8 @@ void *umm_poison_calloc(size_t num, size_t item_size) { ret = get_poisoned(ret, size); + POISON_CRITICAL_EXIT(); + return ret; } @@ -196,6 +210,8 @@ void *umm_poison_calloc(size_t num, size_t item_size) { void *umm_poison_realloc(void *ptr, size_t size) { void *ret; + POISON_CRITICAL_ENTRY(); + ptr = get_unpoisoned(ptr); add_poison_size(&size); @@ -203,6 +219,8 @@ void *umm_poison_realloc(void *ptr, size_t size) { ret = get_poisoned(ret, size); + POISON_CRITICAL_EXIT(); + return ret; } @@ -225,6 +243,13 @@ bool umm_poison_check(void) { bool ok = true; uint16_t cur; + if (0 != POISON_CRITICAL_GET_LEVEL()) { + // Nested checks are not safe. To arrive here implies we are in an + // interrupt context, and a previous sweep has not finished exiting. + return ok; + } + POISON_CRITICAL_ENTRY(); + UMM_CHECK_INITIALIZED(); UMM_CRITICAL_ENTRY(id_poison); @@ -246,6 +271,8 @@ bool umm_poison_check(void) { } UMM_CRITICAL_EXIT(id_poison); + POISON_CRITICAL_EXIT(); + return ok; } From a9c4ad3f8d31e73e57dc5f50f4582301dc84f2a0 Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Sat, 22 Apr 2023 17:38:23 -0700 Subject: [PATCH 2/2] Move get_poisoned() into umm_malloc() and umm_realloc(). Corrected missed edit in build path for UMM_REALLOC_MINIMIZE_COPY. --- cores/esp8266/umm_malloc/Notes.h | 11 ++++++ cores/esp8266/umm_malloc/umm_local.c | 15 +++----- cores/esp8266/umm_malloc/umm_local.h | 18 +-------- cores/esp8266/umm_malloc/umm_malloc.cpp | 29 +++++++++----- cores/esp8266/umm_malloc/umm_malloc_cfg.h | 8 ++++ cores/esp8266/umm_malloc/umm_poison.c | 46 ++++++----------------- 6 files changed, 56 insertions(+), 71 deletions(-) diff --git a/cores/esp8266/umm_malloc/Notes.h b/cores/esp8266/umm_malloc/Notes.h index af0852b430..5d076c4b67 100644 --- a/cores/esp8266/umm_malloc/Notes.h +++ b/cores/esp8266/umm_malloc/Notes.h @@ -341,5 +341,16 @@ Enhancement ideas: * For multiple Heaps builds, add a dedicated function that always reports DRAM results. + + April 22, 2023 + + The umm_poison logic runs outside the UMM_CRITICAL_* umbrella. When interrupt + routines do alloc calls, it is possible to interrupt an in-progress allocation + just before the poison is set, with a new alloc request resulting in a false + "poison check fail" against the in-progress allocation. The SDK does mallocs + from ISRs. SmartConfig can illustrate this issue. + + Move get_poisoned() within UMM_CRITICAL_* in umm_malloc() and umm_realloc(). + */ #endif diff --git a/cores/esp8266/umm_malloc/umm_local.c b/cores/esp8266/umm_malloc/umm_local.c index 063cd66f5d..c08e2a27ca 100644 --- a/cores/esp8266/umm_malloc/umm_local.c +++ b/cores/esp8266/umm_malloc/umm_local.c @@ -138,16 +138,15 @@ static void *get_unpoisoned_check_neighbors(void *vptr, const char *file, int li void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line) { void *ret; - POISON_CRITICAL_ENTRY(); - ptr = get_unpoisoned_check_neighbors(ptr, file, line); add_poison_size(&size); ret = umm_realloc(ptr, size); - - ret = get_poisoned(ret, size); - - POISON_CRITICAL_EXIT(); + /* + "get_poisoned" is now called from umm_realloc while still in a critical + section. Before umm_realloc returned, the pointer offset was adjusted to + the start of the requested buffer. + */ return ret; } @@ -156,12 +155,8 @@ void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line) void umm_poison_free_fl(void *ptr, const char *file, int line) { - POISON_CRITICAL_ENTRY(); - ptr = get_unpoisoned_check_neighbors(ptr, file, line); - POISON_CRITICAL_EXIT(); - umm_free(ptr); } #endif diff --git a/cores/esp8266/umm_malloc/umm_local.h b/cores/esp8266/umm_malloc/umm_local.h index 320f625848..c5dcffd73c 100644 --- a/cores/esp8266/umm_malloc/umm_local.h +++ b/cores/esp8266/umm_malloc/umm_local.h @@ -48,29 +48,13 @@ static size_t umm_uadd_sat(const size_t a, const size_t b); static bool check_poison_neighbors(umm_heap_context_t *_context, uint16_t cur); #endif -#if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) -/* - The umm_poison logic runs outside the UMM_CRITICAL_* umbrella. When interrupt - routines do alloc calls, it is possible to interrupt an in-progress allocation - just before the poison is set, with a new alloc request resulting in a false - "poison check fail" against the in-progress allocation. The SDK does mallocs - from ISRs. SmartConfig can illustrate this issue. - - Add POISON_CRITICAL_* to skip check when not the first. -*/ -static int volatile umm_poison_critical __attribute__((section(".noinit"))); - -#define POISON_CRITICAL_ENTRY() (umm_poison_critical++) -#define POISON_CRITICAL_EXIT() (umm_poison_critical--) -#define POISON_CRITICAL_GET_LEVEL() (umm_poison_critical) -#endif - #if defined(UMM_STATS) || defined(UMM_STATS_FULL) void ICACHE_FLASH_ATTR umm_print_stats(int force); #endif + int ICACHE_FLASH_ATTR umm_info_safe_printf_P(const char *fmt, ...) __attribute__((format(printf, 1, 2))); #define UMM_INFO_PRINTF(fmt, ...) umm_info_safe_printf_P(PSTR(fmt),##__VA_ARGS__) diff --git a/cores/esp8266/umm_malloc/umm_malloc.cpp b/cores/esp8266/umm_malloc/umm_malloc.cpp index 78c648f45b..6923081874 100644 --- a/cores/esp8266/umm_malloc/umm_malloc.cpp +++ b/cores/esp8266/umm_malloc/umm_malloc.cpp @@ -563,9 +563,6 @@ void ICACHE_MAYBE umm_init(void) { // Note, full_init must be true for the primary heap, DRAM. umm_init_heap(UMM_HEAP_DRAM, (void *)UMM_MALLOC_CFG_HEAP_ADDR, UMM_MALLOC_CFG_HEAP_SIZE, true); -#if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) - umm_poison_critical = 0; -#endif // upstream ref: // Initialize the heap from linker supplied values */ // umm_init_heap(UMM_MALLOC_CFG_HEAP_ADDR, UMM_MALLOC_CFG_HEAP_SIZE); @@ -912,6 +909,8 @@ void *umm_malloc(size_t size) { ptr = umm_malloc_core(_context, size); + ptr = POISON_CHECK_SET_POISON(ptr, size); + UMM_CRITICAL_EXIT(id_malloc); return ptr; @@ -1071,7 +1070,7 @@ void *umm_realloc(void *ptr, size_t size) { // Case 2 - block + next block fits EXACTLY } else if ((blockSize + nextBlockSize) == blocks) { DBGLOG_DEBUG("exact realloc using next block - %i\n", blocks); - umm_assimilate_up(c); + umm_assimilate_up(_context, c); STATS__FREE_BLOCKS_UPDATE(-nextBlockSize); blockSize += nextBlockSize; @@ -1090,8 +1089,9 @@ void *umm_realloc(void *ptr, size_t size) { STATS__FREE_BLOCKS_UPDATE(-prevBlockSize); STATS__FREE_BLOCKS_ISR_MIN(); blockSize += prevBlockSize; + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); // Fix allocation so ISR poison check is good UMM_CRITICAL_SUSPEND(id_realloc); - memmove((void *)&UMM_DATA(c), ptr, curSize); + UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); UMM_CRITICAL_RESUME(id_realloc); // Case 5 - prev block + block + next block fits @@ -1111,8 +1111,9 @@ void *umm_realloc(void *ptr, size_t size) { #else blockSize += (prevBlockSize + nextBlockSize); #endif + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); UMM_CRITICAL_SUSPEND(id_realloc); - memmove((void *)&UMM_DATA(c), ptr, curSize); + UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); UMM_CRITICAL_RESUME(id_realloc); @@ -1122,8 +1123,9 @@ void *umm_realloc(void *ptr, size_t size) { void *oldptr = ptr; if ((ptr = umm_malloc_core(_context, size))) { DBGLOG_DEBUG("realloc %i to a bigger block %i, copy, and free the old\n", blockSize, blocks); + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); UMM_CRITICAL_SUSPEND(id_realloc); - memcpy(ptr, oldptr, curSize); + UMM_POISON_MEMCPY(ptr, oldptr, curSize); UMM_CRITICAL_RESUME(id_realloc); umm_free_core(_context, oldptr); } else { @@ -1184,8 +1186,9 @@ void *umm_realloc(void *ptr, size_t size) { blockSize = blocks; #endif } + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); UMM_CRITICAL_SUSPEND(id_realloc); - memmove((void *)&UMM_DATA(c), ptr, curSize); + UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); UMM_CRITICAL_RESUME(id_realloc); } else if (blockSize >= blocks) { // 2 @@ -1201,8 +1204,9 @@ void *umm_realloc(void *ptr, size_t size) { void *oldptr = ptr; if ((ptr = umm_malloc_core(_context, size))) { DBGLOG_DEBUG("realloc %d to a bigger block %d, copy, and free the old\n", blockSize, blocks); + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); UMM_CRITICAL_SUSPEND(id_realloc); - memcpy(ptr, oldptr, curSize); + UMM_POISON_MEMCPY(ptr, oldptr, curSize); UMM_CRITICAL_RESUME(id_realloc); umm_free_core(_context, oldptr); } else { @@ -1226,8 +1230,9 @@ void *umm_realloc(void *ptr, size_t size) { void *oldptr = ptr; if ((ptr = umm_malloc_core(_context, size))) { DBGLOG_DEBUG("realloc %d to a bigger block %d, copy, and free the old\n", blockSize, blocks); + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); UMM_CRITICAL_SUSPEND(id_realloc); - memcpy(ptr, oldptr, curSize); + UMM_POISON_MEMCPY(ptr, oldptr, curSize); UMM_CRITICAL_RESUME(id_realloc); umm_free_core(_context, oldptr); } else { @@ -1253,6 +1258,8 @@ void *umm_realloc(void *ptr, size_t size) { STATS__FREE_BLOCKS_MIN(); + ptr = POISON_CHECK_SET_POISON(ptr, size); + /* Release the critical section... */ UMM_CRITICAL_EXIT(id_realloc); @@ -1261,6 +1268,7 @@ void *umm_realloc(void *ptr, size_t size) { /* ------------------------------------------------------------------------ */ +#if !defined(UMM_POISON_CHECK) && !defined(UMM_POISON_CHECK_LITE) void *umm_calloc(size_t num, size_t item_size) { void *ret; @@ -1276,6 +1284,7 @@ void *umm_calloc(size_t num, size_t item_size) { return ret; } +#endif /* ------------------------------------------------------------------------ */ diff --git a/cores/esp8266/umm_malloc/umm_malloc_cfg.h b/cores/esp8266/umm_malloc/umm_malloc_cfg.h index 449d395fc2..14d2055ebd 100644 --- a/cores/esp8266/umm_malloc/umm_malloc_cfg.h +++ b/cores/esp8266/umm_malloc/umm_malloc_cfg.h @@ -618,6 +618,9 @@ extern bool umm_poison_check(void); // Local Additions to better report location in code of the caller. void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line); void umm_poison_free_fl(void *ptr, const char *file, int line); +#define POISON_CHECK_SET_POISON(p, s) get_poisoned(p, s) +#define UMM_POISON_SKETCH_PTR(p) ((void*)((uintptr_t)p + sizeof(UMM_POISONED_BLOCK_LEN_TYPE) + UMM_POISON_SIZE_BEFORE)) +#define UMM_POISON_SKETCH_PTRSZ(s) (s - sizeof(UMM_POISONED_BLOCK_LEN_TYPE) - UMM_POISON_SIZE_BEFORE - UMM_POISON_SIZE_AFTER) #if defined(UMM_POISON_CHECK_LITE) /* * We can safely do individual poison checks at free and realloc and stay @@ -637,8 +640,13 @@ void umm_poison_free_fl(void *ptr, const char *file, int line); #else #define POISON_CHECK() 1 #define POISON_CHECK_NEIGHBORS(c) do {} while (false) +#define POISON_CHECK_SET_POISON(p, s) (p) +#define UMM_POISON_SKETCH_PTR(p) (p) +#define UMM_POISON_SKETCH_PTRSZ(s) (s) #endif +#define UMM_POISON_MEMMOVE(t, p, s) memmove(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(s)) +#define UMM_POISON_MEMCPY(t, p, s) memcpy(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(s)) #if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) /* diff --git a/cores/esp8266/umm_malloc/umm_poison.c b/cores/esp8266/umm_malloc/umm_poison.c index 3702e57d45..ca41cabf4f 100644 --- a/cores/esp8266/umm_malloc/umm_poison.c +++ b/cores/esp8266/umm_malloc/umm_poison.c @@ -10,7 +10,6 @@ #define UMM_POISON_BLOCK_SIZE (UMM_POISON_SIZE_BEFORE + sizeof(UMM_POISONED_BLOCK_LEN_TYPE) + UMM_POISON_SIZE_AFTER) - /* * Yields the total size of a poison block of size `s`. * If `s` is 0, returns 0. @@ -76,11 +75,6 @@ static bool check_poison(const void *ptr, size_t poison_size, static bool check_poison_block(umm_block *pblock) { bool ok = true; - if (1 != POISON_CRITICAL_GET_LEVEL()) { - // Nested checks are not safe - return ok; - } - if (pblock->header.used.next & UMM_FREELIST_MASK) { DBGLOG_ERROR("check_poison_block is called for free block 0x%lx\n", (unsigned long)pblock); } else { @@ -166,15 +160,14 @@ static void *get_unpoisoned(void *vptr) { void *umm_poison_malloc(size_t size) { void *ret; - POISON_CRITICAL_ENTRY(); - add_poison_size(&size); ret = umm_malloc(size); - - ret = get_poisoned(ret, size); - - POISON_CRITICAL_EXIT(); + /* + "get_poisoned" is now called from umm_malloc while still in a critical + section. Before umm_malloc returned, the pointer offset was adjusted to + the start of the requested buffer. + */ return ret; } @@ -184,24 +177,19 @@ void *umm_poison_malloc(size_t size) { void *umm_poison_calloc(size_t num, size_t item_size) { void *ret; - POISON_CRITICAL_ENTRY(); - // Use saturated multiply. // Rely on umm_malloc to supply the fail response as needed. size_t size = umm_umul_sat(num, item_size); + size_t request_sz = size; add_poison_size(&size); ret = umm_malloc(size); if (NULL != ret) { - memset(ret, 0x00, size); + memset(ret, 0x00, request_sz); } - ret = get_poisoned(ret, size); - - POISON_CRITICAL_EXIT(); - return ret; } @@ -210,16 +198,15 @@ void *umm_poison_calloc(size_t num, size_t item_size) { void *umm_poison_realloc(void *ptr, size_t size) { void *ret; - POISON_CRITICAL_ENTRY(); - ptr = get_unpoisoned(ptr); add_poison_size(&size); ret = umm_realloc(ptr, size); - - ret = get_poisoned(ret, size); - - POISON_CRITICAL_EXIT(); + /* + "get_poisoned" is now called from umm_realloc while still in a critical + section. Before umm_realloc returned, the pointer offset was adjusted to + the start of the requested buffer. + */ return ret; } @@ -243,13 +230,6 @@ bool umm_poison_check(void) { bool ok = true; uint16_t cur; - if (0 != POISON_CRITICAL_GET_LEVEL()) { - // Nested checks are not safe. To arrive here implies we are in an - // interrupt context, and a previous sweep has not finished exiting. - return ok; - } - POISON_CRITICAL_ENTRY(); - UMM_CHECK_INITIALIZED(); UMM_CRITICAL_ENTRY(id_poison); @@ -271,8 +251,6 @@ bool umm_poison_check(void) { } UMM_CRITICAL_EXIT(id_poison); - POISON_CRITICAL_EXIT(); - return ok; }