From cda232027e6f64cd6bb8bdf394a86bf1f885a174 Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Sat, 8 Jul 2023 06:18:46 -0700 Subject: [PATCH 1/3] Fixes occasional UMM_POISON failure Bug introduced with PR fix #8914. When a reallocated pointer could not grow in place, a replacement allocation was created. Then UMM_POISON was written to the wrong block. --- cores/esp8266/umm_malloc/umm_malloc.cpp | 10 +++++----- cores/esp8266/umm_malloc/umm_malloc_cfg.h | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cores/esp8266/umm_malloc/umm_malloc.cpp b/cores/esp8266/umm_malloc/umm_malloc.cpp index 6923081874..72f13826d0 100644 --- a/cores/esp8266/umm_malloc/umm_malloc.cpp +++ b/cores/esp8266/umm_malloc/umm_malloc.cpp @@ -928,7 +928,7 @@ void *umm_realloc(void *ptr, size_t size) { uint16_t c; - size_t curSize; + [[maybe_unused]] size_t curSize; UMM_CHECK_INITIALIZED(); @@ -1089,7 +1089,7 @@ 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 + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); // Fix new allocation so poison check from an ISR passes. UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); @@ -1123,7 +1123,7 @@ 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); + (void)POISON_CHECK_SET_POISON(ptr, size); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMCPY(ptr, oldptr, curSize); UMM_CRITICAL_RESUME(id_realloc); @@ -1204,7 +1204,7 @@ 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); + (void)POISON_CHECK_SET_POISON(ptr, size); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMCPY(ptr, oldptr, curSize); UMM_CRITICAL_RESUME(id_realloc); @@ -1230,7 +1230,7 @@ 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); + (void)POISON_CHECK_SET_POISON(ptr, size); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMCPY(ptr, oldptr, curSize); UMM_CRITICAL_RESUME(id_realloc); diff --git a/cores/esp8266/umm_malloc/umm_malloc_cfg.h b/cores/esp8266/umm_malloc/umm_malloc_cfg.h index 14d2055ebd..5e460cb429 100644 --- a/cores/esp8266/umm_malloc/umm_malloc_cfg.h +++ b/cores/esp8266/umm_malloc/umm_malloc_cfg.h @@ -620,7 +620,10 @@ 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) +#define UMM_POISON_SKETCH_PTRSZ(p) (*(UMM_POISONED_BLOCK_LEN_TYPE *)p) +#define UMM_POISON_MEMMOVE(t, p, s) memmove(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(p)) +#define UMM_POISON_MEMCPY(t, p, s) memcpy(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(p)) + #if defined(UMM_POISON_CHECK_LITE) /* * We can safely do individual poison checks at free and realloc and stay @@ -641,13 +644,10 @@ void umm_poison_free_fl(void *ptr, const char *file, int line); #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) +#define UMM_POISON_MEMMOVE(t, p, s) memmove((t), (p), (s)) +#define UMM_POISON_MEMCPY(t, p, s) memcpy((t), (p), (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) /* * Overhead adjustments needed for free_blocks to express the number of bytes From 834644c638c44b8ef1a1d33e745957fdf5bbd0a6 Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Sun, 9 Jul 2023 13:28:32 -0700 Subject: [PATCH 2/3] Fix umm_poison data corruption on realloc when memory move is used. Bug introduced with PR fix #8914 --- cores/esp8266/umm_malloc/umm_malloc.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/umm_malloc/umm_malloc.cpp b/cores/esp8266/umm_malloc/umm_malloc.cpp index 72f13826d0..2e850977f8 100644 --- a/cores/esp8266/umm_malloc/umm_malloc.cpp +++ b/cores/esp8266/umm_malloc/umm_malloc.cpp @@ -1089,7 +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 new allocation so poison check from an ISR passes. + // Fix new allocation such that poison checks from an ISR pass. + size_t super_size = (blockSize * sizeof(umm_block)) - (sizeof(((umm_block *)0)->header)); + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), super_size); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); @@ -1111,7 +1113,8 @@ void *umm_realloc(void *ptr, size_t size) { #else blockSize += (prevBlockSize + nextBlockSize); #endif - POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); + size_t super_size = (blockSize * sizeof(umm_block)) - (sizeof(((umm_block *)0)->header)); + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), super_size); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); @@ -1186,7 +1189,9 @@ void *umm_realloc(void *ptr, size_t size) { blockSize = blocks; #endif } - POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); + // Fix new allocation such that poison checks from an ISR pass. + size_t super_size = (blockSize * sizeof(umm_block)) - (sizeof(((umm_block *)0)->header)); + POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), super_size); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); From 8118998a9ac6ea034e61741cbbf3b2fa9f720f7f Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Sun, 9 Jul 2023 14:42:51 -0700 Subject: [PATCH 3/3] refactored to resolve unused error in some build contexts --- cores/esp8266/umm_malloc/umm_malloc.cpp | 9 +++------ cores/esp8266/umm_malloc/umm_malloc_cfg.h | 8 +++++++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/cores/esp8266/umm_malloc/umm_malloc.cpp b/cores/esp8266/umm_malloc/umm_malloc.cpp index 2e850977f8..e130862cf7 100644 --- a/cores/esp8266/umm_malloc/umm_malloc.cpp +++ b/cores/esp8266/umm_malloc/umm_malloc.cpp @@ -1090,8 +1090,7 @@ void *umm_realloc(void *ptr, size_t size) { STATS__FREE_BLOCKS_ISR_MIN(); blockSize += prevBlockSize; // Fix new allocation such that poison checks from an ISR pass. - size_t super_size = (blockSize * sizeof(umm_block)) - (sizeof(((umm_block *)0)->header)); - POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), super_size); + POISON_CHECK_SET_POISON_BLOCKS((void *)&UMM_DATA(c), blockSize); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); @@ -1113,8 +1112,7 @@ void *umm_realloc(void *ptr, size_t size) { #else blockSize += (prevBlockSize + nextBlockSize); #endif - size_t super_size = (blockSize * sizeof(umm_block)) - (sizeof(((umm_block *)0)->header)); - POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), super_size); + POISON_CHECK_SET_POISON_BLOCKS((void *)&UMM_DATA(c), blockSize); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); @@ -1190,8 +1188,7 @@ void *umm_realloc(void *ptr, size_t size) { #endif } // Fix new allocation such that poison checks from an ISR pass. - size_t super_size = (blockSize * sizeof(umm_block)) - (sizeof(((umm_block *)0)->header)); - POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), super_size); + POISON_CHECK_SET_POISON_BLOCKS((void *)&UMM_DATA(c), blockSize); UMM_CRITICAL_SUSPEND(id_realloc); UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize); ptr = (void *)&UMM_DATA(c); diff --git a/cores/esp8266/umm_malloc/umm_malloc_cfg.h b/cores/esp8266/umm_malloc/umm_malloc_cfg.h index 5e460cb429..bcc355f893 100644 --- a/cores/esp8266/umm_malloc/umm_malloc_cfg.h +++ b/cores/esp8266/umm_malloc/umm_malloc_cfg.h @@ -619,7 +619,12 @@ extern bool umm_poison_check(void); 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 POISON_CHECK_SET_POISON_BLOCKS(p, s) \ + do { \ + size_t super_size = (s * sizeof(umm_block)) - (sizeof(((umm_block *)0)->header)); \ + get_poisoned(p, super_size); \ + } while (false) +#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(p) (*(UMM_POISONED_BLOCK_LEN_TYPE *)p) #define UMM_POISON_MEMMOVE(t, p, s) memmove(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(p)) #define UMM_POISON_MEMCPY(t, p, s) memcpy(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(p)) @@ -644,6 +649,7 @@ void umm_poison_free_fl(void *ptr, const char *file, int line); #define POISON_CHECK() 1 #define POISON_CHECK_NEIGHBORS(c) do {} while (false) #define POISON_CHECK_SET_POISON(p, s) (p) +#define POISON_CHECK_SET_POISON_BLOCKS(p, s) #define UMM_POISON_MEMMOVE(t, p, s) memmove((t), (p), (s)) #define UMM_POISON_MEMCPY(t, p, s) memcpy((t), (p), (s)) #endif