Merge branch 'master' into pr-heap-refactor3

mhightower83 · mhightower83 · commit 0ce295eefe95 · 2023-04-28T06:51:49.000-07:00
Updated umm_poison move from umm_malloc_cfg.h to umm_malloc_cfgport.h
diff --git 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
@@ -142,8 +142,11 @@ void *umm_poison_realloc_flc(void *ptr, size_t size, const char *file, int line,
 
     add_poison_size(&size);
     ret = umm_realloc(ptr, size);
-
-    ret = get_poisoned(ret, size);
+    /*
+      "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;
 }
diff --git a/cores/esp8266/umm_malloc/umm_malloc.cpp b/cores/esp8266/umm_malloc/umm_malloc.cpp
@@ -909,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;
@@ -1068,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;
 
@@ -1087,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
@@ -1108,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);
 
@@ -1119,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 {
@@ -1181,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
@@ -1198,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 {
@@ -1223,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 {
@@ -1250,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);
 
@@ -1258,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;
 
@@ -1273,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_cfgport.h b/cores/esp8266/umm_malloc/umm_malloc_cfgport.h
@@ -308,6 +308,9 @@ extern void *umm_poison_calloc(size_t num, size_t size);
  */
 extern void *umm_poison_realloc_flc(void *ptr, size_t size, const char *file, int line, const void *caller);
 extern void  umm_poison_free_flc(void *ptr, const char *file, int line, const void *caller);
+#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)
 // No meaningful information is conveyed with panic() for fail. Save space used abort().
 #define POISON_CHECK_NEIGHBORS(c) \
     do { \
@@ -325,15 +328,23 @@ extern void  umm_poison_free_flc(void *ptr, const char *file, int line, const vo
 extern void *umm_poison_realloc(void *ptr, size_t size);
 extern void  umm_poison_free(void *ptr);
 extern bool  umm_poison_check(void);
+#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)
 /* Not normally enabled. A full heap poison check may exceed 10us. */
 #define POISON_CHECK() umm_poison_check()
 #define POISON_CHECK_NEIGHBORS(c) do {} while (false)
 
 #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
@@ -165,8 +165,11 @@ void *umm_poison_malloc(size_t size) {
     add_poison_size(&size);
 
     ret = umm_malloc(size);
-
-    ret = get_poisoned(ret, size);
+    /*
+      "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;
 }
@@ -179,17 +182,16 @@ void *umm_poison_calloc(size_t num, size_t item_size) {
     // 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);
-
     return ret;
 }
 
@@ -205,8 +207,11 @@ void *umm_poison_realloc(void *ptr, size_t size) {
 
     add_poison_size(&size);
     ret = umm_realloc(ptr, size);
-
-    ret = get_poisoned(ret, size);
+    /*
+      "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;
 }
