[JSC] ConservativeRoots: apply the butterfly end-pointer rules to a JSCellButterfly only when it has no elements

[robobun]

Problem

• A Map or Set that code fills and empties in a loop can leak one table for every three or four insertions, plus the entries that were live at each rehash or clear(). On bun 1.4.3 canary (6a92015fc, linux x64 release), s.add(v); s.delete(prev) 3000 times leaves 929 Cell Butterfly cells after Bun.gc(true). A node:http server keeps about 240 of 1000 closed sockets alive, with their IncomingMessage and ServerResponse.
• The cause is the previous-block rule in ConservativeRoots::genericAddPointer (heap/ConservativeRoots.cpp:141). A stack word p <= blockFor(p) + sizeof(IndexingHeader) marks the last cell of the MarkedBlock before p when that block mayHaveIndexingHeader. In that build the JSC::VM object sits at the 16 KB boundary after the first block of 224-byte JSCellButterfly cells. VM* is in about 30 live stack slots at every collection, so that cell is marked in every collection.
• 224 bytes is a Map or Set table at its initial capacity. A replaced table points to the table that replaced it, so one immortal table keeps every later table of that Map or Set.

Fix

• Add mayBeReferencedByButterflyEndPointer(cellKind, cellSize) and use it for the three end-pointer gates: the previous block, the previous cell in the block, and the end+8 bound of a PreciseAllocation. Auxiliary passes at any size. JSCellWithIndexingHeader passes only when cellSize <= JSCellButterfly::offsetOfData(). JSCell never passes, as in [JSC] ConservativeRoots: no past-the-end butterfly slack for cells that cannot hold a butterfly (MarkedBlock rule + PreciseAllocation::contains) #398.
• Correct because JSCellWithIndexingHeader is JSCellButterfly only, and its Butterfly* is toButterfly() = cell + 16. That is past the last byte of the cell only when the cell has no elements (the 16-byte size class). For every other size it is an interior pointer, and the interior-pointer path already marks the cell.
• A precise JSCellButterfly keeps the exact one-past-the-end bound that [JSC] ConservativeRoots: no past-the-end butterfly slack for cells that cannot hold a butterfly (MarkedBlock rule + PreciseAllocation::contains) #398 kept for cells with trailing storage.
• Verified with a linux x64 release build of bun 9e1603604 pinned to autobuild-preview-pr-636-d4a52a7a. The Set repro above leaves 4 Cell Butterfly cells (3 before the loop), also with BUN_JSC_useJIT=0, BUN_JSC_useGenerationalGC=0 and BUN_JSC_useConcurrentGC=0. 0 to 1 of the 3000 values stay alive, for a Set and for a Map, and 1 of 1000 closed node:http sockets.
• The same scripts on the same machine with the official bun 1.4.2 release, which does not have this change: 934 cells, 930 of 3000 values, and 168 to 346 of 1000 sockets.

Background

• The collector scans the stack conservatively: each word that can be a pointer into a live cell marks that cell. A Butterfly* points after the IndexingHeader, so for a butterfly with no indexed storage it points up to 8 bytes past the end of its allocation. genericAddPointer therefore also marks the cell before the one such a pointer lands on. [JSC] ConservativeRoots: no past-the-end butterfly slack for cells that cannot hold a butterfly (MarkedBlock rule + PreciseAllocation::contains) #398 turned that off for plain JSCell blocks.
• JSCellButterfly is a cell with a butterfly layout: cell header, IndexingHeader, then the elements. It backs copy-on-write array literals and the tables of Map and Set. The collector visits its elements.
• A Map or Set replaces its table on rehash and on clear(). The old table keeps a pointer to the new table so that a live iterator can move to it, and it keeps its stale entries. Nothing else points to the old table.

Notes

How the cause was found (release build with symbols of bun 6a92015fc, linux x64):

• generateHeapSnapshotForDebugging() after the Set loop: the kept tables have no incoming edge and no root entry. JSCellButterfly::visitChildren reports its elements with appendValuesHidden, and conservative roots report nothing. The kept range starts at the last 224-byte cell of one MarkedBlock and covers every later table.
• lldb, stopped after the loop: vm = 0x4dbdcf80000 in Bun::evaluateCommonJSModuleOnce and this = 0x4dbdcf80000 in JSC::VM::drainMicrotasks. The first kept table is at 0x4dbdcf7ff20, the last cell of the block 0x4dbdcf7c000, which ends at 0x4dbdcf80000. 33 live stack words hold 0x4dbdcf80000. No stack word points into the table.
• The closed [JSC] ConservativeRoots: only a zero-length JSCellButterfly is referenced from past its end #637 has a gdb session on ConservativeRoots::add that shows the same thing from the other side: after the scan of the register state the root set holds exactly one cell, VM - 0xe0, and none on the fixed build.
• The frames that own the VM* words include JSC::VM::drainMicrotasks, JSC::JSModuleLoader::makeModule and bun's EventLoop::tick, so the word is on the stack in every collection that runs while JS runs.

The rule has a second trigger that needs no special layout. A stack word that points at the start of a JSCellButterfly also marked the cell on its left (the third gate). Consecutive tables of one Map or Set are neighbors, and so are the tables of Maps and Sets that are created in a row. JSTests/stress files for this case and for the 16-byte cell that keeps the rules are linked in the comments below. The bun-side test for it is the "conservative roots" case in test/js/bun/jsc/bun-jsc.test.ts (oven-sh/bun#42460).

Measurements from the four reports that ended here are in the comments below and in oven-sh/bun#42460: 2832 bytes per add + delete cycle with two live 2 KB entries, 55 MB for 30000 set() calls with a clear() after every third, and 1549 to 1657 of 2400 aborted node:http requests.

[github-actions]

Preview Builds

Commit	Release	Date
d4a52a7a	autobuild-preview-pr-636-d4a52a7a	2026-09-12 10:18:10 UTC

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

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

Or wait 8 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 0ba55be0-729c-4658-bbd6-17e4edaa3a58

📥 Commits

Reviewing files that changed from the base of the PR and between cf1b36e and d4a52a7.

📒 Files selected for processing (1)

• Source/JavaScriptCore/heap/ConservativeRoots.cpp

---

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

[claude]

I reviewed this PR and didn't find any bugs. Because it tightens the conservative root scanner in a way that could turn a false positive into a missed root if the layout assumption ever slips, a human familiar with JSC GC should confirm the reasoning before it lands.

What was reviewed:

• Verified HeapCell::JSCellWithIndexingHeader is used only by immutableButterflyAuxiliarySpace / JSCellButterfly, so the "JSCellWithIndexingHeader ⇒ JSCellButterfly" assumption holds today.
• Checked JSCellButterfly::toButterfly() = this + offsetOfData(); for any cell with ≥1 element that lands strictly inside the allocation, so the interior-pointer path in genericAddPointer still marks it.
• Confirmed MarkedBlock::cellSize() and PreciseAllocation::cellSize() exist and are the right accessors at each of the three call sites.

Extended reasoning...

Overview

This PR touches a single file, Source/JavaScriptCore/heap/ConservativeRoots.cpp, adding a helper mayBeReferencedByButterflyEndPointer(kind, cellSize) and swapping it in for mayHaveIndexingHeader(kind) at the three "butterfly end-pointer" gates in genericAddPointer: the PreciseAllocation bounds check, the previous-MarkedBlock probe, and the previous-cell-in-block probe. For HeapCell::JSCellWithIndexingHeader the new predicate only passes when cellSize <= JSCellButterfly::offsetOfData() (i.e., a zero-element JSCellButterfly), otherwise it falls back to the existing behavior. The intent is to stop unrelated stack words that happen to land in the first 8 bytes after a JSCellButterfly block from pinning its last cell — the leak the PR describes for Map/Set tables.

Security risks

The conservative root scanner is what keeps stack-referenced heap cells alive across GC. Narrowing any gate here risks under-marking: if a Butterfly* to a JSCellButterfly could ever legitimately point past the end of the allocation for a cell whose size exceeds offsetOfData(), this change would let that cell be collected while still referenced, yielding a use-after-free. The argument that this cannot happen rests on toButterfly() being cell + offsetOfData() and allocationSize(n) = offsetOfData() + n*8, which I verified in JSCellButterfly.h; for n ≥ 1 the butterfly pointer is strictly interior. I also confirmed that JSCellWithIndexingHeader is only ever assigned via immutableButterflyHeapCellType → immutableButterflyAuxiliarySpace, and only JSCellButterfly::subspaceFor returns that space. There is no injection, auth, or data-exposure surface here — the risk is purely memory safety.

Level of scrutiny

High. This is core GC root-scanning logic that CLAUDE.md explicitly calls out as underpinning C++ integration safety, and Source/JavaScriptCore is CODEOWNERS-covered by @ WebKit/jsc-reviewers. The change is small and the reasoning reads correctly, but the failure mode (a rare UAF that only manifests when a JSCellButterfly's Butterfly* is the sole live stack reference at collection time) would be very hard to diagnose. It also encodes a layout invariant — that no JSCellWithIndexingHeader type other than JSCellButterfly exists, and that its Butterfly* is always this + offsetOfData() — that a future change elsewhere could silently break. A human JSC/GC reviewer should sign off on that invariant.

Other factors

The PR is still a draft and the author is explicitly waiting on a Bun release build to validate the fix against the repro. No regression test is added under JSTests/stress/. The change follows the pattern of the earlier #398 (which excluded plain JSCell from these gates) and the three call-site edits are mechanically consistent, so I have reasonable confidence it is correct — but not enough to bypass human review on a GC-safety path.

[robobun]

#637 was the same fix from another report. I closed it in favor of this PR. Two things from it may be useful here.

• The diffs differ in one place. For the 16-byte JSCellButterfly, [JSC] ConservativeRoots: only a zero-length JSCellButterfly is referenced from past its end #637 accepted only a pointer at exactly the end of the cell (toButterfly() is cell + 16, never cell + 24). This PR keeps the full sizeof(IndexingHeader) slack for that cell. Both fix the Map and Set tables, and this one is the more conservative of the two.
• The [JSC] ConservativeRoots: only a zero-length JSCellButterfly is referenced from past its end #637 body has a gdb session on ConservativeRoots::add. After the scan of the 40-byte register state, the root set holds exactly one cell, VM - 0xe0. On the fixed build it holds none, and the VM still follows a table block.

A test that does not depend on the memory layout is linked from oven-sh/bun#42460. It fails on release, ASAN debug and Windows builds that lack the fix, and it passes against this PR's preview build.

[robobun]

I found the same bug from a third report (a node:http server that keeps the requests of aborted clients alive) and arrived at the same fix. This PR covers it, so I did not open another one. One thing from my branch may help.

A JSTests/stress test. Branch robobun/44d9bcf4/conservative-roots-cell-butterfly-past-the-end, commit 25e4fbc, one new file: JSTests/stress/conservative-roots-cell-butterfly-past-the-end.js. The commit has no engine change, so it cherry-picks cleanly.

• It covers the third gate of this PR, the previous cell in the block. The VM* case in the PR body is the first gate. The third gate does not need a special memory layout: a stack word that points at the start of any JSCellButterfly also marked the cell on its left.
• The test allocates 200 Sets with one key each and drops every second one. It iterates the other 100 with nested forEach calls, so that the stack references the storage of each kept Set. Then it calls gc() and counts the dropped keys through WeakRef.
• bin/jsc of autobuild-cf1b36ec8703 (release and debug ASAN): Error: 99 of 100 dropped keys are still alive, also with --useJIT=0.
• bin/jsc of autobuild-preview-pr-636-d4a52a7a (debug ASAN): the test passes, also with --useJIT=0.

The same test as a bun:test case is linked from oven-sh/bun#42460. It fails on a debug ASAN build of bun that lacks this change, so it can show the failing side there.

My diff used the exact pointer for the 16-byte cell (pointer == cell + JSCellButterfly::offsetOfData()), as #637 did. This PR keeps the full slack for that cell, which is the more conservative choice. Upstream WebKit main has the same three gates as cf1b36ec8703.

Numbers for the node:http report: a release build of bun 471b5868b6 against cf1b36ec8703 plus the three gates keeps 1 of 1500 aborted requests. A control build of the same configuration without them keeps 574 to 809.

[robobun]

I found the same bug from a fourth report and arrived at the same predicate at the same three gates. This PR covers it, so I did not open another one. In that report only the first Map or Set of a process whose table reaches that cell is hit, and it keeps the values too: 30000 set() calls with a clear() after every third leave 55 MB alive after the Map is dropped.

Three things that are not in the thread yet.

1. A test for the side of the predicate that keeps the rules. Branch robobun/40c859ac/cell-butterfly-past-the-end, commit ebea517, one new file: JSTests/stress/empty-cell-butterfly-is-kept-alive-by-its-end-pointer.js. The commit has no engine change, so it cherry-picks cleanly.

• The two comments above discuss how much slack the 16-byte cell needs. No test shows that it needs any. I built a shell in which JSCellWithIndexingHeader fails the predicate at every size. Under --scribbleFreeCells=1 --useZombieMode=1 --collectContinuously=1 --sweepSynchronously=1 it gives the same result as a shell with this PR's predicate on each of the 291 existing stress tests with keys, cow or copy-on-write in their names.
• The new test fails on that shell in 10 of 10 runs (bad array length 3134975728, or a segfault). With this PR's predicate it passes 10 of 10 runs, about 2 s each.
• The reference it protects is real. CommonSlowPaths::allocateNewArrayBuffer() (runtime/CommonSlowPaths.h:241) calls JSArray::createWithButterfly(vm, nullptr, originalStructure, immutableButterfly->toButterfly()). The DFG and FTL fast paths of Object.keys(), Object.getOwnPropertySymbols() and Reflect.ownKeys() reach it through operationNewArrayBuffer with the cached names butterfly of the Structure, after the last use of the object. When the object was the only owner of its Structure, the end pointer is the only reference to a names butterfly with no elements while the JSArray is allocated.

2. Upstream has the left-neighbor case too. A JSCOnly build of WebKit/WebKit main at 70bb79bb0b (libpas, no mimalloc, no fork patches) fails the left-neighbor stress test from the same branch (e5c4bd3, map-set-table-is-not-kept-alive-by-a-pointer-to-its-right-neighbor.js: 64 pairs of Maps or Sets, nested forEach, heap growth). The heap grows by 34.07 MB against a limit of 8 MB, also with --useJIT=0. With #398 and this predicate applied to that tree it grows by 7 KB. So the change applies upstream as it is. That test is the same idea as conservative-roots-cell-butterfly-past-the-end.js from the comment above. One of the two is enough.

3. A control build for the fourth report. bun 1d487c40d9 built twice as release-local against cf1b36ec8703, with and without the three gates. Nothing else differs.

• Without: +13.3 MB per 20000 set() + delete() cycles of the first Map, +55.0 MB for the clear() loop, +19.4 MB for a Set with add() + delete(). With: +0.0 MB for each.
• At the conservative scan of the unfixed build, 30 stack words and one saved register hold the VM*. The frames that own them include JSC::VM::drainMicrotasks, JSC::JSModuleLoader::makeModule and bun's EventLoop::tick. So the word is on the stack in every collection that runs while JS runs.