fix: zero the new TLS slots after growth (first import from Bun, #78) - #148
Merged
Conversation
imported from oven-sh/mimalloc @ d078ad06 (src/threadlocal.c), MIT. mi_thread_locals_expand grew the slot array with rezalloc and assumed the new slots were zero. They are not. _mi_theap_realloc_zero starts its zero-fill from the block's OLD USABLE size, not from the size originally requested -- it cannot do better, since the requested size is not tracked in release builds -- so the slack between the requested size and the bin size is carried across verbatim, and that slack was never initialized by anyone. Confirmed deterministically on our tree, not taken on faith: mi_malloc(40) yields usable 48, and after mi_rezalloc(p,112) the eight bytes [40,48) still read 0xAA. It matters because _mi_thread_local_get validates a slot ONLY by comparing its version lane against the key's version, and versions are small sequential counters. A garbage lane equal to a live key's version makes the adjacent garbage lane be returned as a cached mi_theap_t* -- application bytes dereferenced as a theap, which mimalloc then writes page queues and stats through. This is the third bug in this one function: #128 B3 (array allocated from a destroyable heap) and now this. They are independent; the B3 fix does not address this, and Bun still has B3. test/test-rezalloc-slack.c pins the zero-fill boundary that makes the memzero necessary. Note honestly what it is NOT: it does not fail without this fix, since mi_thread_locals_expand is static and the corruption is only observable through a heap->theap lookup returning garbage. Bun's own reproducer is probabilistic ("roughly once per several hundred process runs") and pthread-only. So #66 C.2's "import only with a test that fails without it" is not strictly met here; the mechanism is pinned deterministically instead and the deviation is recorded rather than glossed over. The test also caught my own wrong model on first run: I predicted the in-place realloc branch's boundary, but newsize > usable takes the move path, which zeroes then copies min(newsize, old_usable) bytes over the top -- so the ENTIRE slack survives, not slack minus one word. Co-Authored-By: Claude <noreply@anthropic.com>
CI caught this: mi_malloc carries mi_attr_alloc_size(1), so with _FORTIFY_SOURCE glibc's fortified memset sees a 40-byte object and aborts with '*** buffer overflow detected ***' when the test writes the usable slack. Writing up to mi_usable_size is legal for mimalloc; the compiler has no way to know. Volatile byte loop instead. Windows/MinGW never showed it -- three Linux jobs did. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Aug 2, 2026
zackees
added a commit
that referenced
this pull request
Aug 2, 2026
) (#149) #78 and #66 require every import to carry three things: an in-code "imported from <repo> @ <sha>, <license>" comment, a row in MIMALLOC_FORKS.md, and its own commit. #148 shipped the code comment and the commit but NOT the MIMALLOC_FORKS.md row -- my omission, and exactly the provenance gap the rule exists to prevent. We intend to upstream to Microsoft, and they will not take code whose origin we cannot state. Adds: - MIMALLOC_FORKS.md row for the TLS slot zeroing, marked IMPORTED (#148), with the deterministic confirmation and the symmetry worth remembering: Bun fixed the zeroing, we fixed the same function's provenance bug (#128 B3), and each fork still had the other's half until now. - MIMALLOC_FORKS.md row for the page-map os_align over-count, scored 4 and marked NOT yet taken, stating plainly that the over-count is reproduced but the corruption is not. - README "Adopted from other forks" row for the same import. Also corrects two rows that had gone stale: - the __GCC__ row said "report tracked in #114"; it is now filed as microsoft/mimalloc#1349 with a 44x thread-churn measurement. - zero-tracking said "on v4 only"; the v4 line was dissolved into main in #129. Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Aug 2, 2026
zackees
added a commit
that referenced
this pull request
Sep 2, 2026
…w notes Opus review (PR #305): - B1: TLS-slot zeroing was attributed to d078ad06 ("macOS VM tag default"), not the actual fix. Verified against oven-sh/mimalloc directly (`git show --stat afb41757 d078ad06`): afb41757 ("threadlocal: zero new slots explicitly when expanding the slot array", src/threadlocal.c) is the real commit. Fixed in README.md (text + href) and MIMALLOC_FORKS.md:325, where the error originated (PR #148's body). - B2: "upstream is 10" -> 1000ms. Verified `git show 6def7be9:src/options.c` at the pin: `purge_delay` upstream default is 1000ms, not 10. - B4: dropped the "(open, stacked on #299)" parenthetical from the purge_holes row now that #299 is merged; PR stays draft until #302 merges. - M1: the scavenger row's deviation (c) had it backwards -- the park protocol IS Bun's, imported as part of this PR. Replaced with the real deviation: mi_subproc_t's new fields are appended at the struct tail rather than mid-struct, which is where Bun's placement would have shifted `stats` (the free path touches it, ~2 ns/alloc+free). - Minor: "Our PR" -> "Landed in"; added the regen command (`uv run ci/bench_hole_purging.py --build-dir ... --include-dir include --out-dir .github/assets [--table]`) after the source sentence; added a sentence to the v3-only note that upstream dev3 is itself still pre-release, linking docs/fork-divergence.md#how-v3-was-validated; added "The scavenger is on in both runs; the chart isolates hole purging" to the prose; re-rendered both RSS SVGs for the new title. `check_doc_snippets.py` and `pytest ci/tests` (235 tests) both pass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YT4jVokb2gdT8ngFrH8i8S
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
1 participant
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.
The first genuine import from the Bun fork (#78), and a real memory-corruption bug we ship today.
imported from oven-sh/mimalloc @ d078ad06 (src/threadlocal.c), MITThe bug
mi_thread_locals_expandgrew the slot array withrezallocand assumed the new slots were zero. They are not._mi_theap_realloc_zerostarts its zero-fill from the block's old usable size, not from the size originally requested — it cannot do better, since the requested size isn't tracked in release builds — so the slack between requested and bin size is carried across verbatim. That slack was never initialized by anyone; it holds whatever the previous tenant of the page left.Confirmed deterministically on our own tree rather than taken on faith:
Why it is dangerous
_mi_thread_local_getvalidates a slot only by comparing itsversionlane against the key's version — and versions are small sequential counters. A garbage lane that happens to equal a live key's version makes the adjacent garbage lane be returned as a cachedmi_theap_t*. Application bytes get dereferenced as a theap, and mimalloc then writes page queues and stat counters through it.This is the third bug found in this one function: #128 B3 (array allocated from a user-destroyable heap) and now this. They're independent — the B3 fix does not address this one, and Bun still has B3 while we still had this.
On the C.2 rule, stated plainly
test/test-rezalloc-slack.cpins the zero-fill boundary that makes the memzero necessary. It does not fail without this fix:mi_thread_locals_expandis static, and the corruption is only observable through a heap→theap lookup returning garbage. Bun's own reproducer is probabilistic ("roughly once per several hundred process runs") and pthread-only, so it is not usable in CI.So #66 C.2 — import only with a test that fails without it — is not strictly met here. The mechanism is pinned deterministically instead, and I'd rather record the deviation than pretend the box is ticked.
The test also caught my own error on its first run: I predicted the in-place realloc branch's boundary, but
newsize > usabletakes the move path, which zeroes and then copiesmin(newsize, old_usable)bytes over the top — so the entire slack survives, not slack-minus-one-word.20/20 locally. Second commit regenerates the vendored amalgamation.