mimalloc: clear theap->heap on heap delete so a cached theap cannot match a reused heap address (pin bump pending oven-sh/mimalloc#25)

[robobun]

Draft until oven-sh/mimalloc#24, #25 and #26 are merged. Then this becomes a two-line pin bump (MIMALLOC_COMMIT in scripts/build/deps/mimalloc.ts and the literal in test/js/node/process/process.test.js) and the patch file goes away. The patch is here so CI builds and tests the store on every platform in the meantime; it must not merge in this form, because oven-sh/mimalloc#25 rewrites the lines it patches and the next pin bump would fail git apply.

Problem

• The mimalloc dev3 sync (mimalloc: sync the fork with upstream dev3, fix the Android emulated-TLS crash #37367) carried upstream 36e8fc33, which dropped the theap->heap = NULL store from the heap-delete detach path (_mi_heap_detach_theaps, src/theap.c:656 at the pin). A thread's one-entry theap cache (_mi_theap_cached, checked only by theap->heap == heap in include/mimalloc/prim-tls.h:392) then matches a new heap created at the address of a destroyed one, and the thread allocates from a detached theap whose pages went back to the arena. Upstream had that store before, with a comment naming this exact ABA; the thread-exit path still has it.
• The fork's test-heap-aba (test-heap-aba: regression test for the theap cache ABA on a reused heap address (stacked on #25) mimalloc#26) shows it on the pin: a debug build segfaults in mi_heap_malloc, a release build hands out 127 of 1024 blocks that mi_heap_of attributes to no heap.
• Bun is not shown to reach this path: MimallocArena allocates only on the thread that created or last reset the heap (asserted in debug builds), and a debug build instrumented to abort on the stale cache hit ran 1630 tests without firing. This restores an allocator invariant Bun depends on; it is not tied to a Bun-level symptom.

Fix

• Restores the store under the tld lock, at the point where the thread-exit path already clears it. mi_heap_delete: detach the theaps before the pages are moved and free them after mimalloc#25 owns that change (it also keeps theap->tld valid until the theap struct is freed, for a cross-thread free that is mid-flight); test-heap-aba: regression test for the theap cache ABA on a reused heap address (stacked on #25) mimalloc#26 adds the regression test. This PR will pin the fork head that contains both.
• Correct because a detached theap then matches no heap, so the cache refreshes on the next use. Holding the tld lock keeps it clear of the heap-delete vs thread-exit race the fork tests (test-heap-delete-race): the owner cannot be collecting this theap while the lock is held, and after the unlink it no longer sees it.
• Verified: the fork suite on the Print import stack for errors during bun bun #25 tree with the test (Linux x64, Debug and Release). Bun: bun bd with the patch applied, then test/js/bun/transpiler, test/bundler/bundler_edgecase.test.ts, test/js/web/workers/worker.test.ts (the three terminate() races failures there are debug-build timing and identical on the unpatched pin).

Background

• mimalloc v3 splits a mi_heap_t into per-thread mi_theap_ts. _mi_heap_theap(heap) finds the calling thread's theap through a versioned thread-local slot (heap->theap), with _mi_theap_cached (the theap of the last heap this thread used) as a front cache keyed only by the theap->heap pointer. The slot is versioned, so only the cache can go stale.
• mi_heap_destroy detaches the heap's theaps from every thread's tld list and frees them. A detached theap stays alive while a thread's cache references it (the cache holds a refcount).
• Bun's MimallocArena (Rust) is mi_heap_new + mi_heap_malloc + mi_heap_destroy, used for parser and transpiler arenas. mi_heap_new allocates the heap struct from the main heap, which refreshes the creating thread's cache, so the same-thread destroy-then-new pattern Bun uses does not hit the ABA.

Notes

This came out of an investigation of Sentry BUN-4CG0 (SIGSEGV in JSC::MarkedBlock::Handle::sweep from LocalAllocator::tryAllocateIn, MarkedBlock::Header::m_vm NULL while the Handle is intact: the 16 KiB block reads back as zeros while JSC owns it). The event rate rose after the dev3 sync, but the signature is older than mimalloc as JSC's allocator: #24194 (2025-10, Bun 1.2.23 on macOS arm64, libpas-backed JSC) is the same frame chain. The cause of that crash is not established by this PR.

What was ruled out for BUN-4CG0, with evidence:

• The fork's hole-purging feature and its park handoff (free blocks inside a live page are discarded with MADV_FREE_REUSABLE; the sweep runs on the scavenger thread for a thread parked in kevent/epoll), which fork commit 6df95990 in the same sync made run on nearly every park: the sweep arithmetic (16 KiB and 4 KiB OS pages, large pages, the unformed tail), the park/wake handshake, the abandoned-page claim protocol, the used accounting, the arena purge and bitmap paths, the Bun side of the park contract (packages/bun-usockets/src/eventing/epoll_kqueue.c, signal handlers, spawn), and the 101 upstream commits after the sync point were each read end to end. No fault found.
• A JSC-like stress test against a MI_DEBUG_FULL build of the pinned fork (16 KiB aligned blocks, cross-thread frees, 9000 short-lived threads, heap create/destroy/delete, 770k park handoffs, 900k discards, emulated 16 KiB OS pages, variants for purge delay, commit on demand, reclaim on free): 100k iterations per variant, zero corruption.
• The theap cache ABA is the one defect that is new in the window. Its symptom chain would fit (a stale allocation lands in arena slices that are free, the JS thread reuses those slices for fresh MarkedBlock pages, the stale user zero-fills its block), but no Bun input is known to reach it: the instrumented build ran test/js/bun/transpiler, test/transpiler, test/js/web/workers, test/js/node/worker_threads, test/bundler/bun-build-api.test.ts, bundler_edgecase, bundler_splitting, bundler_bun, bundler_npm, test/cli/install/bun-install.test.ts, test/js/bun/glob and test/bake without a hit.
• No Bun test is included: the change is in a dependency and no Bun-level input reaches the path, so nothing fails before and passes after. The regression test lives in the fork.
• Patch files in patches/ need plain --- a/ headers; with a diff --git header git apply skips them silently from vendor/<dep> (build: stop git apply from silently skipping dep patches with a diff --git header #35099).

---

no test proof · iteration 0 · no src or test change; test-proof not applicable

[coderabbitai]

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

• Run review for free

_{On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file.}

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 37 seconds), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: acb38b37-1cd4-4b0f-bce9-ff56738596b3

📥 Commits

Reviewing files that changed from the base of the PR and between abe2ad4 and 068869b.

📒 Files selected for processing (2)

• patches/mimalloc/theap-cache-aba.patch
• scripts/build/deps/mimalloc.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 11:36 AM PT - Aug 22nd, 2026}

✅ @robobun, your commit b6904087e74e837749fa521aeeef48cdfb154471 passed in Build #103655! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 40101

That installs a local version of the PR into your bun-40101 executable, so you can run:

bun-40101 --bun

[robobun]

Status: draft. The fix lives in the fork: oven-sh/mimalloc#25 (the theap->heap = NULL store on the heap-delete detach, stacked on #24) and oven-sh/mimalloc#26 (the regression test test/test-heap-aba.c, stacked on #25). Once those merge, this PR turns into a two-line pin bump (MIMALLOC_COMMIT and the literal in test/js/node/process/process.test.js) and the patch file goes away. The patch is here only so CI exercises the store on every platform; it does not apply on top of #25, so it must not merge as is.

How it was reproduced: test-heap-aba (two threads: B creates heap H, A allocates from H, B destroys H and creates H2 at the same address, A allocates from H2). On the pinned fork a debug build segfaults in mi_heap_malloc and a release build hands out 127 of 1024 blocks that mi_heap_of attributes to no heap. On the #25 tree both are clean, and with #25's store reverted the test fails again.

Bun side: bun bd with the patch applied, then test/js/bun/transpiler, test/bundler/bundler_edgecase.test.ts, test/js/web/workers/worker.test.ts. Bun is not shown to reach the stale-cache path (see the PR body); the investigation record for BUN-4CG0 is in the Notes there.

[claude]

I reviewed this PR and didn't find any bugs. The patch format matches the repo's other dep patches (plain --- a/ / +++ b/ headers, applied via git apply --no-index from vendor/mimalloc/), and the patches field on the Dependency spec is an established pattern. Because this is a concurrency-sensitive change to the global allocator's heap-teardown path, a human look at the mimalloc reasoning would still be worthwhile.

What was reviewed

• Patch header format vs. fetch-cli.ts's git apply invocation and the PR's note about diff --git headers being silently skipped — this patch has no diff --git line, matching patches/highway/ and patches/lshpack/.
• Dependency.patches wiring in scripts/build/source.ts and fetch-cli.ts — patch content is hashed into the .ref identity, so adding it forces a re-fetch and non-zero git apply exit fails the build loudly.
• The added line mirrors the store the PR says _mi_tld_detach_theaps already performs, and uses the mi_atomic_store_ptr_release helper rather than a plain assignment.

Extended reasoning...

Overview

This PR adds a one-line patch to the vendored mimalloc allocator (patches/mimalloc/theap-cache-aba.patch) and registers it in scripts/build/deps/mimalloc.ts. The patch inserts mi_atomic_store_ptr_release(mi_heap_t, &theap->heap, NULL) into _mi_heap_detach_theaps immediately after the theap is unlinked from its tld list and before the tld lock is released. The stated goal is to close an ABA window where a thread's one-entry _mi_theap_cached slot (keyed only on theap->heap == heap) can match a freshly-created heap that reuses a destroyed heap's address, handing the thread a detached theap whose pages have been returned to the arena.

Security risks

None in the PR-mechanics sense — no untrusted input parsing, no auth, no network. The risk profile is entirely memory safety and concurrency correctness inside the process-wide allocator. If the added store were wrong (wrong lock held, racing with the owning thread's cache read, or the wrong ordering), the failure mode would be heap corruption rather than a security boundary bypass.

Level of scrutiny

High. mimalloc is Bun's global malloc replacement on Linux and the JS heap allocator everywhere; _mi_heap_detach_theaps runs during mi_heap_destroy, which MimallocArena calls on every parser/transpiler arena teardown. Even a one-line change here sits on a path exercised by essentially every request. The PR description is unusually thorough (Sentry correlation, upstream commit that dropped the store, a fork-level repro test in oven-sh/mimalloc#26, lock-safety argument, and an honest note that the Bun-level trigger is unproven). That raises confidence considerably, but per the review guidelines allocator/concurrency changes to a vendored dep are exactly the category where a maintainer should confirm the reasoning rather than an automated review approving on the diff alone.

Other factors

• I confirmed patches is a supported field on Dependency (source.ts:459) and is used identically by ~12 other deps (highway, libuv, tinycc, lsquic, etc.). fetch-cli.ts hashes patch content into the .ref stamp and pipes the CRLF-normalized body to git apply --ignore-whitespace --no-index - with cwd: vendor/mimalloc; a non-zero exit throws, so a mis-applying patch would fail the build rather than silently no-op.
• The patch header style (no diff --git line, plain --- a/src/theap.c) matches existing patches like patches/highway/silence-warnings.patch and patches/lshpack/bss-huff-tables.patch, and is consistent with the PR's own note that diff --git headers cause git apply from vendor/<dep> to treat paths as repo-relative and skip them.
• I could not verify the context lines against vendor/mimalloc/src/theap.c locally (the tarball is fetched at build time), but the PR states bun bd was run with the patch applied and several test suites passed, and a context mismatch would fail git apply loudly.
• No Bun-level test is added; the PR is upfront that no Bun input is known to reach the ABA and the repro lives in the fork (test-heap-aba.c). REVIEW.md's "every behavioral change ships an automated test" is in tension here, but for a dependency patch whose only known repro is at the C allocator level this seems like a reasonable, explicitly-stated exception for a maintainer to sign off on.

Deferring rather than approving because allocator lock-ordering and atomic-ordering claims ("the owner cannot be collecting this theap while the lock is held") are the kind of thing a human familiar with the mimalloc fork should confirm.

[robobun]

On the two points the review could not check locally:

• Patch context: verified against the pinned tarball. After bun bd with this branch, vendor/mimalloc/src/theap.c is byte-identical to the fork branch behind test-heap-aba: regression test for the theap cache ABA on a reused heap address (stacked on #25) mimalloc#26 (diff is empty), and the store appears once in _mi_tld_detach_theaps (upstream) and once in _mi_heap_detach_theaps (this patch).
• The lock argument: the thread-exit side that dereferences theap->heap is mi_thread_theaps_done (src/init.c), and it holds tld->theaps_lock for the whole loop over tld->theaps. The new store runs inside the same lock, after the theap is unlinked from that list, so the two cannot overlap and a later pass does not see the theap. The other readers of theap->heap that can run concurrently (_mi_page_associated_theap_peek, the cache check in _mi_heap_theap) already treat a mismatch or NULL as "no theap". The fork's test-heap-delete-race (20000 iterations of heap delete racing thread exit) passes with the change.

A maintainer look at the mimalloc side is welcome; the fork PR has the reproducer.

[robobun]

Superseded by #40138 (commit 861e9ae), which bumps mimalloc to oven-sh/mimalloc#27. That PR combines oven-sh/mimalloc#22 to #26 into one heap delete/destroy teardown protocol and replaces the per-reader fixes, including the #25 store this patch carried.

Verified at the pinned commit a178e44af6dc2845c793a39fd62da583b3e9f0ff:

• _mi_heap_detach_theaps (src/theap.c:703) stores theap->heap = NULL under the tld lock. Step 1 of the teardown protocol in src/heap.c:169 documents it. _mi_heap_theap (include/mimalloc/prim-tls.h:392) still matches the cache on theap->heap == heap, so a detached theap matches no heap, and not a new heap at the same address.
• test/test-heap-aba.c is in the fork and in the mi_static_tests list (CMakeLists.txt:880).
• patches/mimalloc/theap-cache-aba.patch no longer applies at that pin (git apply --check fails on the src/theap.c hunk). The theap->tld = NULL context line it anchored on is gone, and the store it added is already there.

Nothing left for this PR to carry. Closing.