test-heap-aba: regression test for the theap cache ABA on a reused heap address (stacked on #25) - #26
Closed
robobun wants to merge 5 commits into
Closed
Conversation
…oncurrent mi_heap_delete can free the heap mi_heap_delete claims each page of the heap, re-points page->heap to the main heap, and then frees the heap struct. A mi_free from another thread runs concurrently with that. Two places on its path read page->heap at a point where the deleter does not wait for them: - mi_stat_free (MI_STAT>0) read page->heap->subproc before it owns the page. The heap struct can be freed between the two loads, and the free list link is written over heap->subproc (its first field), so the next load is garbage and subproc->theap_meta faults. This is the SIGSEGV in test-heap-mt (heap-free-during-delete-overlap) under load. The stats now find the subproc through the page's arena, which is constant for the lifetime of the page (and through the current subproc for an OS allocated page), and the meta theap test compares the page's thread id with MI_THREADID_DETACHED directly. - mi_arenas_page_free_prim read page->heap->subproc after it cleared the page's bit in the heap's arena_pages. That bit is what the deleter waits on, so the heap can be gone by then. Read the subproc once, before the bit is cleared. Document the rule on mi_page_heap and on the page->heap field.
…e is freed mi_bitmap_clear updates the chunkmap of the bitmap after it clears the bit. The bit is what a concurrent mi_heap_delete waits on before it frees the heap and its arena_pages (mi_heap_visit_page_at), so that chunkmap update could touch a freed arena_pages. Add mi_bitmap_clear_no_chunkmap, which leaves the chunkmap bit set (allowed: it means the chunk may have bits set, and the pages bitmap is only ever visited, never searched), and use it in mi_arenas_page_free_prim. Narrow the comment on mi_page_heap to what the delete actually waits for.
…em after mi_heap_delete (and mi_heap_destroy) freed the theaps of the heap before it walked the pages, and the detach marked a theap by clearing theap->tld. A thread that used the heap before still reaches its theap for the heap through the heap's thread local while it frees blocks of the heap during the delete (_mi_page_associated_theap_peek on the reclaim and re-abandon paths of mi_free_try_collect_mt). That theap is then detached, with a NULL tld that the callers dereference, or already freed. Now the detach clears theap->heap instead (as _mi_tld_detach_theaps does on thread exit), which is what the peek compares, so a free that runs after the detach does not get the theap anymore. The theap struct and its tld stay valid until after the page walk, for a free that obtained the theap just before the detach: the walk waits for such a free while it owns the page. The statistics are still merged before the walk, so the page statistics of the heap are unchanged. _mi_heap_theap_peek returns NULL for a detached theap as well instead of asserting. test-heap-mt gets a variant where the pages are abandoned (the allocating thread exited) and every freer allocated from the heap once. It crashed in the first iteration before (mi_theap_matches_thread through _mi_arenas_page_try_reabandon_to_mapped).
Armed after pthread_create, the thread can have exited before the store and then never parks, and the wait for it spins forever. Seen once as a 1500s ctest timeout under a loaded ctest -j4.
…ap address Every thread keeps the theap of the heap it last used in `_mi_theap_cached`, and `_mi_heap_theap` takes that cache whenever `theap->heap` equals the requested heap. Upstream 36e8fc3 ("improve concurrent heap_delete and thread termination") dropped the `theap->heap = NULL` store from the heap-delete detach path, which the old `_mi_theap_free` had with a comment that it avoids an ABA where the cache holds a heap address that a newly allocated heap reuses. The previous commit restores that store (under the tld lock, as the thread-exit path does). This test pins it: B: H = mi_heap_new() A: mi_heap_malloc(H) -> A caches its theap for H B: mi_heap_destroy(H); H2 = mi_heap_new() -> H2 == H (LIFO reuse of the block) A: mi_heap_malloc(H2) -> must come from a theap of H2 Without the store a debug build segfaults in mi_heap_malloc (the cached theap is detached, `tld == NULL`) and a release build hands out 127 of 1024 blocks that `mi_heap_of` attributes to no heap: memory that went back to the arena. With the store every block belongs to H2.
robobun
force-pushed
the
robobun/theap-cache-aba
branch
from
August 22, 2026 18:16
684f33b to
94540d7
Compare
robobun
changed the title
robobun
added a commit
to oven-sh/bun
that referenced
this pull request
Aug 22, 2026
…en-sh/mimalloc#25, CI stand-in) The dev3 sync (#37367) carried upstream 36e8fc33, which dropped the `theap->heap = NULL` store from the heap-delete detach path. A thread's one-entry theap cache is keyed only by that pointer, so a heap created at the address of a destroyed one matched the stale entry and the thread allocated from a detached theap whose pages had gone back to the arena. Carry the store as a patch on the current pin so CI builds and tests it on every platform. oven-sh/mimalloc#25 owns the change in the fork and oven-sh/mimalloc#26 adds the regression test; this becomes a pin bump once they are merged, and the patch is removed then.
This was referenced Aug 23, 2026
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #25 (which is stacked on #24): their four commits come first, merge them first. This PR is the last commit: a regression test.
Problem
_mi_heap_theapserves a heap from the thread's_mi_theap_cachedentry whenevertheap->heapequals the requested heap (include/mimalloc/prim-tls.h:392). Upstream 36e8fc3 ("improve concurrent heap_delete and thread termination", in the dev3 sync of Merge upstream dev3 (261 commits): #1271 audit fixes, init/subproc restructure #15) removed thetheap->heap = NULLstore from the heap-delete detach path (_mi_heap_detach_theaps). The old_mi_theap_freehad that store with a comment that it avoids an ABA when the cache holds a heap address that a new heap reuses. The thread-exit path (_mi_tld_detach_theaps) still clears it.mi_heap_newreuses H's block (LIFO). A's nextmi_heap_malloc(H2)hits the cache (theap->heap == H == H2) and allocates from a detached theap whose page queues point at pages that were returned to the arena. A debug build segfaults. A release build hands out blocks inside free arena slices, or inside a page that by then belongs to another heap, and corrupts whatever reused that memory.Fix
test/test-heap-aba.c: the two-thread sequence above, 64 rounds, 16 allocations per round, each block checked withmi_heap_of. Registered inmi_static_tests.MI_DEBUG_FULL) and Release. With mi_heap_delete: detach the theaps before the pages are moved and free them after #25's store replaced by the pre-mi_heap_delete: detach the theaps before the pages are moved and free them after #25theap->tld = NULLline, the Release build reports 127 blocks attributed to no heap (exit 1) and the Debug build segfaults. On the unmodified pin6a14aee2the same. The rest of the suite on the mi_heap_delete: detach the theaps before the pages are moved and free them after #25 tree is unchanged (test-purge-holesunformed-tailfails on this Linux x64 machine with and without the test, as noted in Do not dereference page->heap on the cross-thread free path while mi_heap_delete can free the heap #24 and mi_heap_delete: detach the theaps before the pages are moved and free them after #25).Background
mi_heap_tis shared by threads; each thread allocates from it through its ownmi_theap_t. The theap for a (heap, thread) pair is found through a versioned thread-local slot (heap->theap), with_mi_theap_cachedas a one-entry front cache keyed only by thetheap->heappointer. The slot is versioned, so only the cache can go stale.