mimalloc: keep the idle sweep's state on the tld instead of in __thread variables (fixes Android SIGSEGV) - #38168
Conversation
…ad variables The hole-purging sweep kept its re-entrancy guard and per-sweep counters in __thread variables and read the guard inside mi_page_free_collect_ex. The Android target compiles __thread to emulated TLS, whose first access on a thread calls malloc, so the allocator re-entered itself from inside a page collect and recursed until the stack overflowed. Move that state onto the tld being swept (owner or parked thread), read the guard back through the calling thread's default theap, and make the sweep's own collects skip un-purging explicitly. Carried as a patch until the mimalloc pin moves. Fixes #38051
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 5:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit f4817d4 has some failures in 🧪 To try this PR locally: bunx bun-pr 38168That installs a local version of the PR into your bun-38168 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it patches the process-wide allocator's multi-threaded sweep path (including a semantic change to how _mi_page_purge_holes_in_progress() reads its guard on the scavenger thread), a human familiar with the fork's hole-purging feature should sign off.
Checked: patch applies cleanly to the pinned commit; every caller of _mi_page_purge_holes/_begin/_end is updated; _mi_page_free_collect_no_unpurge already exists in the pin.
The new mi_tld_t fields land in the zero-initialized tail — tld_empty/mi_process_tld_main positional initializers stop at memid, so zero (false/0) is correct for all four.
The scavenger-path guard weakening is compensated by the explicit _no_unpurge collects in theap.c/arena.c; a nested malloc on the scavenger only touches its own theaps, not the parked thread's pages.
Extended reasoning...
Overview
Adds a vendored patch to oven-sh/mimalloc (applied via the existing patches: mechanism in scripts/build/deps/mimalloc.ts) that moves four __thread variables backing the fork's idle hole-purging sweep — mi_purging_holes, mi_holes_sweep_full, mi_holes_sweep_skipped, mi_holes_sweep_visited — onto the mi_tld_t being swept, and threads that tld through _mi_page_purge_holes, _mi_page_purge_holes_begin/end, and mi_page_purge_holes_walk. The two sweep-internal collects switch from _mi_page_free_collect to the existing _mi_page_free_collect_no_unpurge. Root cause: on Android API < 29, __thread compiles to emulated TLS, whose first-touch allocation re-enters mimalloc from inside mi_page_free_collect_ex, causing unbounded recursion.
Security risks
None identified. This is an internal allocator refactor with no attacker-controlled inputs; the state relocation does not widen any trust boundary.
Level of scrutiny
High. This is the process-wide memory allocator, and the change touches concurrent state shared between an owner thread and a background scavenger. The scavenger-path behavior of _mi_page_purge_holes_in_progress() changes (it now reads the calling thread's own tld via _mi_theap_default(), so on the scavenger it returns false while sweeping a parked tld). The PR argues this is safe because (a) the sweep's own collects no longer depend on the guard, and (b) a nested malloc on the scavenger allocates from the scavenger's own theaps, not the pages under sweep. That argument reads correctly to me and I traced it through page.c:1213 and both callers, but it is exactly the kind of invariant a maintainer of this fork feature should confirm.
Other factors
- Verified the patch applies cleanly (
patch -p1 --dry-run) against the pinned source; no fuzz, no offsets. - Checked
init.c:tld_emptyandmi_process_tld_maininitialize positionally throughmemidand rely on zero-init for the tail, which is correct for the four new fields. - Confirmed the only remaining
mi_decl_threadinpage.cpost-patch is gone (all four removed);_mi_page_holes_count_page_freed/_ineligibleuse process-wide atomics directly, not thread-locals. - The removed
parg->tld != NULLcheck inmi_arena_page_purge_holes_atis safe: the sole caller now early-returns ontld == NULL, and the only call site (theap.c:238) always passes a non-NULLtld. - No automated test (justified: no CI target uses emulated TLS); the PR provides a linux-x64 reproducer and requests on-device confirmation.
|
For whoever does the sign-off on the hole-purging side, the one semantic change is what
Nested allocation inside a sweep is only reachable through a user output function on a mimalloc warning in the first place; on native-TLS targets the change is otherwise behavior-preserving (same discard and reuse counts before and after, see the description). |
Confirmed
Setup: an actual Android phone, no VM/emulator involved. The cross-compile toolchain (LLVM 21 + NDK r27c, Built
Before applying, checked whether the patch's target files (
const wasmoon = await import("wasmoon"); // wasmoon@1.16.0
const factory = new wasmoon.LuaFactory();
const lua = await factory.createEngine();
lua.doStringSync("x = 1 + 1");
console.log("lua ok, x =", lua.global.get("x"));
lua.global.close();Thanks for tracking this down — the emulated-TLS-recursion explanation matches everything I was seeing. |
|
Robobun push this as a PR to our mimalloc fork instead |
|
Done: oven-sh/mimalloc#17 (against Converted this PR to a draft. Once #17 is merged this becomes the pin bump (drop |
|
Folded into #37367: its pin (oven-sh/mimalloc |
|
Confirmed: |
…TLS crash (#37367) Moves the mimalloc pin to oven-sh/mimalloc `bun-dev3-v2` @ `be7eb3ff1`, which is: - oven-sh/mimalloc#15 — the fork synced with upstream `dev3` (261 commits: the #1271 audit fixes, the init/sub-process restructure, reworked theap teardown), fork features re-applied on top. Includes upstream's `thread_locals_get` fix: on Linux/Windows a thread that had used a non-main heap (JSC's structure heap, every `bun_alloc::Arena`) could NULL-deref in `mi_free`/teardown after mimalloc's own thread-done ran. Regression test added here (`worker_destruction.test.ts`, fails 4/4 on a debug build of main). - oven-sh/mimalloc#17 — the idle sweep's state lives on the tld instead of `__thread` variables. On emulated-TLS targets (our Android build, API < 29) the first `__thread` access mallocs, so reading the sweep guard from inside a page collect recursed until the stack was gone. Fixes #38051. - A rate-limited park (`purge_holes_min_interval`) is now swept when its window ends instead of at the scavenger's next unrelated wake (up to its 30 s safety net) — the end-of-burst park is the one that used to be left waiting. - A `MI_DEBUG_FULL`-only assertion exemption for the detached meta theap (aborted `test-heap-churn`/`test-heap-mt` in the fork's debug suite after the sync). Replaces #38168 and #38199 (both were `patches/mimalloc/*.patch` stop-gaps against the old pin). --- ### Measured (macOS arm64, release builds of this same commit, old pin vs new; paired rounds; memory is `Bun.unsafe.memoryFootprint`) | | Old → new | |---|---| | Retained by size class, 1-in-64 survivors | Identical | | Bare startup footprint | 6.54 → 6.75 MB (+0.2 MB, 5/5 pairs) | | express settled footprint after 300k requests, read after a forced full GC | 43.4 → 43.2 MB avg, 5 pairs (equal — the unforced reading that looked ~7 MB lower was GC timing, not the allocator) | | express peak footprint | No consistent direction | | express throughput | 89.5k → 90.1k rps (parity) | The startup-snapshot branch was also built against the equivalent (#16) and its suite passes; that surfaced one snapshot-writer fix, landed on that branch separately. CI is the regression sweep for this change.
Problem
bunmicro -profile) since the mimalloc pin bump in Update mimalloc to the upstream dev3 (v3.4.3) sync #36431 (Intermittent SIGSEGV on Android (arm64) since mimalloc dev3/v3.4.3 sync (#36431) #38051).__threadvariables (vendor/mimalloc/src/page.c:478-489) and reads the guard frommi_page_free_collect_ex(page.c:1213), i.e. from inside the allocator. Bun's Android target isaarch64-linux-android28, and for API < 29 clang lowers every__threadto emulated TLS:__emutls_get_addressmaterializes a thread's variables on first access withmalloc(), which on that target is mimalloc itself. Clang also speculates the read ahead of the last word of themi_page_has_purgedcheck, so it fires the first time a thread collects any page with an empty free list. If the size class emulated TLS asks for (128 bytes for its per-thread array, 16 for the flag) is itself sitting on an exhausted page at that moment, serving it collects that page, reads the still-unmaterialized guard again, and recurses until the stack is gone.__threadvariable (__mi_theap_main) during thread init, while the thread had no pages yet, which materialized each thread's emulated-TLS state at a harmless moment (confirmed with a breakpoint on__emutls_get_address:malloc(128)+malloc(23)from_mi_theap_default_set). The dev3 sync removed that variable, so the first emulated-TLS allocation moved into the collect path. glibc/musl/macOS/Windows use native TLS and are unaffected.Fix
patches/mimalloc/sweep-state-on-tld.patch(applied through the existingpatches:mechanism inscripts/build/deps/mimalloc.ts; robobun cannot push to oven-sh/mimalloc, so this is carried the same way earlier mimalloc fixes were until the pin moves): the state of a running sweep moves onto themi_tld_tbeing swept (holes_sweeping,holes_sweep_full,holes_sweep_skipped,holes_sweep_visited, appended to the zero-initialized tail of the struct)._mi_page_purge_holes_begin/endand_mi_page_purge_holestake that tld; both callers (theap.c,arena.c) already have it._mi_page_purge_holes_in_progress()now answers "is the calling thread inside a sweep of its own heaps" by reading_mi_theap_default()->tld->holes_sweeping. The default theap is the one thread-local every TLS model reads without allocating (a pthread key on Android). On the owner path this is exactly the old semantics; on the scavenger path the guard only ever protected the scavenger's own (never swept) pages, so reading false there is harmless.theap.c,arena.c) call_mi_page_free_collect_no_unpurgeexplicitly instead of relying on the guard, so the "do not un-purge what we are about to purge" behavior no longer depends on thread-local state at all.__threadvariable mimalloc still touches on Android is the message-output guard inoptions.c(recurse), which is only reached when mimalloc prints a warning or error.mi_page_free_collect_excontains no__emutls_get_addresscall; the binary references it only frommi_recurse_enter_prim/mi_recurse_exit_prim.-femulated-tls,MI_MALLOC_OVERRIDE,MI_TLS_MODEL_PTHREADS, compiler-rt's emutls):acd9924a) ok 10/10, current pin + this patch ok 10/10MI_TLS_MODEL_LOCALandMI_TLS_MODEL_PTHREADS, release andMI_DEBUG=3: a sweep discards 6 hole runs and the following allocations hand all 6 back (reuse_calls=6,purged_blocks=0), identical numbers before and after the patch; the stress runs clean under the new assertionsbun bd(MI_DEBUG=3) builds with the patch applied;test/js/bun/jsc/heapStats-mimalloc.test.tspasses; a script that churns memory on the main thread and a Worker runs clean with the scavenger sweeping the parked threads, withMIMALLOC_SCAVENGER=0(inline owner sweeps, 36 passes observed via breakpoints), and withMIMALLOC_PURGE_HOLES_MIN_INTERVAL=0 MIMALLOC_PURGE_HOLES_FULL_EVERY=1;process.versions.mimallocis unchangedtest/change: every CI platform uses native TLS, where the bug is unreachable, so nothing underbun testcan fail before and pass after. The C reproducer below is the regression test; it needs a from-source build of mimalloc with-femulated-tls. @jjtseng93, if you can run your 30xbunmicro -profileloop on an Android build of this branch, that would confirm it on the real target.patches:entry tomimalloc.ts; whichever lands second needs a one-line merge.Background
__threadvariable into a descriptor and replaces every access with a call to__emutls_get_address(compiler-rt), which keeps a per-thread array in a pthread key and allocates both the array and each variable's storage withmalloc()the first time a thread touches them. Native TLS is a register-relative load and never allocates, which is why the same code is fine on every other Bun target.mi_tld_t(one per thread) owning onemi_theap_tper heap the thread allocates from; pages belong to a theap.mi_on_thread_idle()walks a thread's pages and discards ("purges") the memory of free blocks that fill whole OS pages, taking those blocks off the free lists. When a page with purged holes later runs out of free blocks,mi_page_free_collect_exbrings one run back (_mi_page_unpurge_run). The guard exists so that ami_mallocnested inside the sweep itself (reachable from a warning's output hook) does not un-purge a page the sweep is in the middle of rewriting.mi_on_thread_idle_start()aroundepoll_wait, and the scavenger runs the sweep for parked threads. That is why the sweep already carried the target tld everywhere, and why per-thread state of the sweeping thread was the wrong place for this data even on native TLS.Reproducer (linux-x64, no device needed)
repro.c:Build against a mimalloc tree (
vendor/mimallocafterbun bdhas the patch applied; a plain extract of the pin does not):(
MI_TLS_MODEL_PTHREADSis whatprim-tls.hselects for__ANDROID__; the previous pin spells itMI_TLS_MODEL_DYNAMIC_PTHREADS.)nm repro | grep __emutls_vlists the__threadvariables that ended up emulated: 6 on the current pin, 2 after the patch (recurseand the never-read__mi_thread_id_helper).Results: current pin, exit 139 x10; previous pin,
okx10; patched,okx10.no test proof · iteration 0 · docs-only change; test-proof not applicable