heap: Fix spurious heap_caps_check_integrity() errors in Comprehensive mode

Heap was not being locked before poisoning, so heap_caps_check_integrity()
would sometimes race with checking the poison bytes and print unnecessary
errors.

Details: https://esp32.com/viewtopic.php?f=2&t=3348&p=15732#p15732

components/heap/include/multi_heap.h

@@ -91,6 +91,8 @@ multi_heap_handle_t multi_heap_register(void *start, size_t size);
  *
  * The lock argument is supplied to the MULTI_HEAP_LOCK() and MULTI_HEAP_UNLOCK() macros, defined in multi_heap_platform.h.
  *
+ * The lock in question must be recursive.
+ *
  * When the heap is first registered, the associated lock is NULL.
  *
  * @param heap Handle to a registered heap.

components/heap/multi_heap.c

@@ -329,6 +329,16 @@ void multi_heap_set_lock(multi_heap_handle_t heap, void *lock)
     heap->lock = lock;
 }
 
+void inline multi_heap_internal_lock(multi_heap_handle_t heap)
+{
+    MULTI_HEAP_LOCK(heap->lock);
+}
+
+void inline multi_heap_internal_unlock(multi_heap_handle_t heap)
+{
+    MULTI_HEAP_UNLOCK(heap->lock);
+}
+
 void *multi_heap_malloc_impl(multi_heap_handle_t heap, size_t size)
 {
     heap_block_t *best_block = NULL;
@@ -341,7 +351,7 @@ void *multi_heap_malloc_impl(multi_heap_handle_t heap, size_t size)
         return NULL;
     }
 
-    MULTI_HEAP_LOCK(heap->lock);
+    multi_heap_internal_lock(heap);
 
     /* Find best free block to perform the allocation in */
     prev = &heap->first_block;
@@ -361,7 +371,7 @@ void *multi_heap_malloc_impl(multi_heap_handle_t heap, size_t size)
     }
 
     if (best_block == NULL) {
-        MULTI_HEAP_UNLOCK(heap->lock);
+        multi_heap_internal_unlock(heap);
         return NULL; /* No room in heap */
     }
 
@@ -376,7 +386,7 @@ void *multi_heap_malloc_impl(multi_heap_handle_t heap, size_t size)
         heap->minimum_free_bytes = heap->free_bytes;
     }
 
-    MULTI_HEAP_UNLOCK(heap->lock);
+    multi_heap_internal_unlock(heap);
 
     return best_block->data;
 }
@@ -389,7 +399,7 @@ void multi_heap_free_impl(multi_heap_handle_t heap, void *p)
         return;
     }
 
-    MULTI_HEAP_LOCK(heap->lock);
+    multi_heap_internal_lock(heap);
 
     assert_valid_block(heap, pb);
     MULTI_HEAP_ASSERT(!is_free(pb), pb); // block should not be free
@@ -420,7 +430,7 @@ void multi_heap_free_impl(multi_heap_handle_t heap, void *p)
         pb = merge_adjacent(heap, pb, next);
     }
 
-    MULTI_HEAP_UNLOCK(heap->lock);
+    multi_heap_internal_unlock(heap);
 }
 
 
@@ -451,7 +461,7 @@ void *multi_heap_realloc_impl(multi_heap_handle_t heap, void *p, size_t size)
         return NULL;
     }
 
-    MULTI_HEAP_LOCK(heap->lock);
+    multi_heap_internal_lock(heap);
     result = NULL;
 
     if (size <= block_data_size(pb)) {
@@ -461,7 +471,7 @@ void *multi_heap_realloc_impl(multi_heap_handle_t heap, void *p, size_t size)
     }
     else if (heap->free_bytes < size - block_data_size(pb)) {
         // Growing, but there's not enough total free space in the heap
-        MULTI_HEAP_UNLOCK(heap->lock);
+        multi_heap_internal_unlock(heap);
         return NULL;
     }
 
@@ -512,7 +522,7 @@ void *multi_heap_realloc_impl(multi_heap_handle_t heap, void *p, size_t size)
         heap->minimum_free_bytes = heap->free_bytes;
     }
 
-    MULTI_HEAP_UNLOCK(heap->lock);
+    multi_heap_internal_unlock(heap);
     return result;
 }
 
@@ -530,7 +540,7 @@ bool multi_heap_check(multi_heap_handle_t heap, bool print_errors)
     size_t total_free_bytes = 0;
     assert(heap != NULL);
 
-    MULTI_HEAP_LOCK(heap->lock);
+    multi_heap_internal_lock(heap);
 
     heap_block_t *prev = NULL;
     heap_block_t *prev_free = NULL;
@@ -593,7 +603,7 @@ bool multi_heap_check(multi_heap_handle_t heap, bool print_errors)
     }
 
  done:
-    MULTI_HEAP_UNLOCK(heap->lock);
+    multi_heap_internal_unlock(heap);
 
     return valid;
 }
@@ -602,7 +612,7 @@ void multi_heap_dump(multi_heap_handle_t heap)
 {
     assert(heap != NULL);
 
-    MULTI_HEAP_LOCK(heap->lock);
+    multi_heap_internal_lock(heap);
     printf("Heap start %p end %p\nFirst free block %p\n", &heap->first_block, heap->last_block, heap->first_block.next_free);
     for(heap_block_t *b = &heap->first_block; b != NULL; b = get_next_block(b)) {
         printf("Block %p data size 0x%08zx bytes next block %p", b, block_data_size(b), get_next_block(b));
@@ -612,7 +622,7 @@ void multi_heap_dump(multi_heap_handle_t heap)
             printf("\n");
         }
     }
-    MULTI_HEAP_UNLOCK(heap->lock);
+    multi_heap_internal_unlock(heap);
 }
 
 size_t multi_heap_free_size_impl(multi_heap_handle_t heap)
@@ -639,7 +649,7 @@ void multi_heap_get_info_impl(multi_heap_handle_t heap, multi_heap_info_t *info)
         return;
     }
 
-    MULTI_HEAP_LOCK(heap->lock);
+    multi_heap_internal_lock(heap);
     for(heap_block_t *b = get_next_block(&heap->first_block); !is_last_block(b); b = get_next_block(b)) {
         info->total_blocks++;
         if (is_free(b)) {
@@ -659,6 +669,6 @@ void multi_heap_get_info_impl(multi_heap_handle_t heap, multi_heap_info_t *info)
     // heap has wrong total size (address printed here is not indicative of the real error)
     MULTI_HEAP_ASSERT(info->total_free_bytes == heap->free_bytes, heap);
 
-    MULTI_HEAP_UNLOCK(heap->lock);
+    multi_heap_internal_unlock(heap);
 
 }

components/heap/multi_heap_internal.h

@@ -38,3 +38,10 @@ bool multi_heap_internal_check_block_poisoning(void *start, size_t size, bool is
    Called when merging blocks, to overwrite the old block header.
 */
 void multi_heap_internal_poison_fill_region(void *start, size_t size, bool is_free);
+
+/* Allow heap poisoning to lock/unlock the heap to avoid race conditions
+   if multi_heap_check() is running concurrently.
+*/
+void multi_heap_internal_lock(multi_heap_handle_t heap);
+
+void multi_heap_internal_unlock(multi_heap_handle_t heap);

components/heap/multi_heap_poisoning.c

@@ -173,16 +173,18 @@ static bool verify_fill_pattern(void *data, size_t size, bool print_errors, bool
 
 void *multi_heap_malloc(multi_heap_handle_t heap, size_t size)
 {
+    multi_heap_internal_lock(heap);
     poison_head_t *head = multi_heap_malloc_impl(heap, size + POISON_OVERHEAD);
-    if (head == NULL) {
-        return NULL;
-    }
-    uint8_t *data = poison_allocated_region(head, size);
+    uint8_t *data = NULL;
+    if (head != NULL) {
+        data = poison_allocated_region(head, size);
 #ifdef SLOW
-    /* check everything we got back is FREE_FILL_PATTERN & swap for MALLOC_FILL_PATTERN */
-    assert( verify_fill_pattern(data, size, true, true, true) );
+        /* check everything we got back is FREE_FILL_PATTERN & swap for MALLOC_FILL_PATTERN */
+        assert( verify_fill_pattern(data, size, true, true, true) );
 #endif
+    }
 
+    multi_heap_internal_unlock(heap);
     return data;
 }
 
@@ -191,6 +193,8 @@ void multi_heap_free(multi_heap_handle_t heap, void *p)
     if (p == NULL) {
         return;
     }
+    multi_heap_internal_lock(heap);
+
     poison_head_t *head = verify_allocated_region(p, true);
     assert(head != NULL);
 
@@ -200,11 +204,15 @@ void multi_heap_free(multi_heap_handle_t heap, void *p)
            head->alloc_size + POISON_OVERHEAD);
     #endif
     multi_heap_free_impl(heap, head);
+
+    multi_heap_internal_unlock(heap);
 }
 
 void *multi_heap_realloc(multi_heap_handle_t heap, void *p, size_t size)
 {
     poison_head_t *head = NULL;
+    poison_head_t *new_head;
+    void *result = NULL;
 
     if (p == NULL) {
         return multi_heap_malloc(heap, size);
@@ -218,14 +226,18 @@ void *multi_heap_realloc(multi_heap_handle_t heap, void *p, size_t size)
     head = verify_allocated_region(p, true);
     assert(head != NULL);
 
+    multi_heap_internal_lock(heap);
+
 #ifndef SLOW
-    poison_head_t *new_head = multi_heap_realloc_impl(heap, head, size + POISON_OVERHEAD);
-    if (new_head == NULL) { // new allocation failed, everything stays as-is
-        return NULL;
+    new_head = multi_heap_realloc_impl(heap, head, size + POISON_OVERHEAD);
+    if (new_head != NULL) {
+        /* For "fast" poisoning, we only overwrite the head/tail of the new block so it's safe
+           to poison, so no problem doing this even if realloc resized in place.
+        */
+        result = poison_allocated_region(new_head, size);
     }
-    return poison_allocated_region(new_head, size);
 #else // SLOW
-    /* When slow poisoning is enabled, it becomes very fiddly to try and correctly fill memory when reallocing in place
+    /* When slow poisoning is enabled, it becomes very fiddly to try and correctly fill memory when resizing in place
        (where the buffer may be moved (including to an overlapping address with the old buffer), grown, or shrunk in
        place.)
 
@@ -233,15 +245,17 @@ void *multi_heap_realloc(multi_heap_handle_t heap, void *p, size_t size)
     */
     size_t orig_alloc_size = head->alloc_size;
 
-    poison_head_t *new_head = multi_heap_malloc_impl(heap, size + POISON_OVERHEAD);
-    if (new_head == NULL) {
-        return NULL;
+    new_head = multi_heap_malloc_impl(heap, size + POISON_OVERHEAD);
+    if (new_head != NULL) {
+        result = poison_allocated_region(new_head, size);
+        memcpy(result, p, MIN(size, orig_alloc_size));
+        multi_heap_free(heap, p);
     }
-    void *new_data = poison_allocated_region(new_head, size);
-    memcpy(new_data, p, MIN(size, orig_alloc_size));
-    multi_heap_free(heap, p);
-    return new_data;
 #endif
+
+    multi_heap_internal_unlock(heap);
+
+    return result;
 }
 
 size_t multi_heap_get_allocated_size(multi_heap_handle_t heap, void *p)

docs/api-reference/system/heap_debug.rst

@@ -52,7 +52,7 @@ Memory corruption can be one of the hardest classes of bugs to find and fix, as
 - Adding regular calls to :cpp:func:`heap_caps_check_integrity_all` or :cpp:func:`heap_caps_check_integrity_addr` in your code will help you pin down the exact time that the corruption happened. You can move these checks around to "close in on" the section of code that corrupted the heap.
 - Based on the memory address which is being corrupted, you can use :ref:`JTAG debugging <jtag-debugging-introduction>` to set a watchpoint on this address and have the CPU halt when it is written to.
 - If you don't have JTAG, but you do know roughly when the corruption happens, then you can set a watchpoint in software just beforehand via :cpp:func:`esp_set_watchpoint`. A fatal exception will occur when the watchpoint triggers. For example ``esp_set_watchpoint(0, (void *)addr, 4, ESP_WATCHPOINT_STORE``. Note that watchpoints are per-CPU and are set on the current running CPU only, so if you don't know which CPU is corrupting memory then you will need to call this function on both CPUs.
-- For buffer overflows, `heap tracing`_ in ``HEAP_TRACE_ALL`` mode lets you see which callers are allocating from heap. If you can find the function which allocates memory with an address immediately before the address which is corrupted, this will probably be the function which overflows the buffer.
+- For buffer overflows, `heap tracing`_ in ``HEAP_TRACE_ALL`` mode lets you see which callers are allocating which addresses from the heap. See `Heap Tracing To Find Heap Corruption` for more details.
 
 Configuration
 ^^^^^^^^^^^^^
@@ -68,29 +68,51 @@ This is the default level. No special heap corruption features are enabled, but
 
 If assertions are enabled, an assertion will also trigger if a double-free occurs (the same memory is freed twice).
 
-Light impact
+Calling :cpp:func:`heap_caps_check_integrity` in Basic mode will check the integrity of all heap structures, and print errors if any appear to be corrupted.
+
+Light Impact
 ++++++++++++
 
-At this level, heap memory is additionally "poisoned" with head and tail "canary bytes" before and after each block which is allocated. If an application writes outside the bounds of the allocated buffer, the canary bytes will be corrupted and the integrity check will fail.
+At this level, heap memory is additionally "poisoned" with head and tail "canary bytes" before and after each block which is allocated. If an application writes outside the bounds of allocated buffers, the canary bytes will be corrupted and the integrity check will fail.
 
-"Basic" heap corruption checks can also detect most out of bounds writes, but this setting is more precise as even a single byte overrun will always be detected. With Basic heap checks, the number of overrun bytes before a failure is detected will depend on the properties of the heap.
+The head canary word is 0xABBA1234 (3412BAAB in byte order), and the tail canary word is 0xBAAD5678 (7856ADBA in byte order).
 
-Similar to other heap checks, these "canary bytes" are checked via assertion whenever memory is freed and can also be checked manually via :cpp:func:`heap_caps_check_integrity` or related functions.
+"Basic" heap corruption checks can also detect most out of bounds writes, but this setting is more precise as even a single byte overrun can be detected. With Basic heap checks, the number of overrun bytes before a failure is detected will depend on the properties of the heap.
+
+Enabling "Light Impact" checking increases memory usage, each individual allocation will use 9 to 12 additional bytes of memory (depending on alignment).
+
+Each time ``free()`` is called in Light Impact mode, the head and tail canary bytes of the buffer being freed are checked against the expected values.
+
+When :cpp:func:`heap_caps_check_integrity` is called, all allocated blocks of heap memory have their canary bytes checked against the expected values.
+
+In both cases, the check is that the first 4 bytes of an allocated block (before the buffer returned to the user) should be the word 0xABBA1234. Then the last 4 bytes of the allocated block (after the buffer returned to the user) should be the word 0xBAAD5678.
+
+Different values usually indicate buffer underrun or overrun, respectively.
 
-This level increases memory usage, each individual allocation will use 9 to 12 additional bytes of memory (depending on alignment).
 
 Comprehensive
 +++++++++++++
 
 This level incorporates the "light impact" detection features plus additional checks for uninitialised-access and use-after-free bugs. In this mode, all freshly allocated memory is filled with the pattern 0xCE, and all freed memory is filled with the pattern 0xFE.
 
-If an application crashes reading/writing an address related to 0xCECECECE when this setting is enabled, this indicates it has read uninitialized memory. The application should be changed to either use calloc() (which zeroes memory), or initialize the memory before using it. The value 0xCECECECE may also be seen in stack-allocated automatic variables, because in IDF most task stacks are originally allocated from the heap and in C stack memory is uninitialized by default.
+Enabling "Comprehensive" detection has a substantial runtime performance impact (as all memory needs to be set to the allocation patterns each time a malloc/free completes, and the memory also needs to be checked each time.) However it allows easier detection of memory corruption bugs which are much more subtle to find otherwise. It is recommended to only enable this mode when debugging, not in production.
 
-If an application crashes reading/writing an address related to 0xFEFEFEFE, this indicates it is reading heap memory after it has been freed (a "use after free bug".) The application should be changed to not access heap memory after it has been freed.
+Crashes in Comprehensive Mode
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-If the IDF heap allocator fails because the pattern 0xFEFEFEFE was not found in freed memory then this indicates the app has a use-after-free bug where it is writing to memory which has already been freed.
+If an application crashes reading/writing an address related to 0xCECECECE in Comprehensive mode, this indicates it has read uninitialized memory. The application should be changed to either use calloc() (which zeroes memory), or initialize the memory before using it. The value 0xCECECECE may also be seen in stack-allocated automatic variables, because in IDF most task stacks are originally allocated from the heap and in C stack memory is uninitialized by default.
 
-Enabling "Comprehensive" detection has a substantial runtime performance impact (as all memory needs to be set to the allocation patterns each time a malloc/free completes, and the memory also needs to be checked each time.)
+If an application crashes and the exception register dump indicates that some addresses or values were 0xFEFEFEFE, this indicates it is reading heap memory after it has been freed (a "use after free bug".) The application should be changed to not access heap memory after it has been freed.
+
+If a call to malloc() or realloc() causes a crash because it expected to find the pattern 0xFEFEFEFE in free memory and a different pattern was found, then this indicates the app has a use-after-free bug where it is writing to memory which has already been freed.
+
+Manual Heap Checks in Comprehensive Mode
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Calls to :cpp:func:`heap_caps_check_integrity` may print errors relating to 0xFEFEFEFE, 0xABBA1234 or 0xBAAD5678. In each case the checker is expecting to find a given pattern, and will error out if this is not found:
+
+- For free heap blocks, the checker expects to find all bytes set to 0xFE. Any other values indicate a use-after-free bug where free memory has been incorrectly overwritten.
+- For allocated heap blocks, the behaviour is the same as for `Light Impact` mode. The canary bytes 0xABBA1234 and 0xBAAD5678 are checked at the head and tail of each allocated buffer, and any variation indicates a buffer overrun/underrun.
 
 .. _heap-tracing:
 
@@ -183,6 +205,17 @@ Finally, the total number of 'leaked' bytes (bytes allocated but not freed while
 
 A warning will be printed if the trace buffer was not large enough to hold all the allocations which happened. If you see this warning, consider either shortening the tracing period or increasing the number of records in the trace buffer.
 
+Heap Tracing To Find Heap Corruption
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When a region in heap is corrupted, it may be from some other part of the program which allocated memory at a nearby address.
+
+If you have some idea at what time the corruption occured, enabling heap tracing in ``HEAP_TRACE_ALL`` mode allows you to record all of the functions which allocated memory, and the addresses where they were corrupted.
+
+Using heap tracing in this way is very similar to memory leak detection as described above. For memory which is allocated and not freed, the output 
+
+Heap tracing can also be used to help track down heap corruption. By using 
+
 Performance Impact
 ^^^^^^^^^^^^^^^^^^