[JSC] ConservativeRoots: no past-the-end butterfly slack for cells that cannot hold a butterfly (MarkedBlock rule + PreciseAllocation::contains) - #398
Conversation
…n blocks that can hold butterflies genericAddPointer() ends with a rule for butterfly pointers that point just past the previous object: a candidate in [cellStart, cellStart + sizeof(IndexingHeader)] that did not resolve to a live cell marks the cell to its left. The equivalent cross-block case a few lines up is gated on mayHaveIndexingHeader(cellKind); the within-block case was not, so it also fired in plain JSCell blocks, where no butterfly can live and a pointer at or just past a (dead) cell's start says nothing about its left neighbour. Effect in a JSCell block today: any stack/register word equal to deadCell or deadCell+8 pins the live cell immediately before it. Observed in Bun: a callee-saved register spilled by a long-lived native frame held such a value for the whole program, so a JSArray allocated next to that slot survived every full collection until the frame unwound. Gate the rule on mayHaveIndexingHeader(cellKind) like its sibling.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The change is small and mirrors the existing mayHaveIndexingHeader guard on the cross-block-boundary path, but because it narrows conservative root scanning in the GC — the direction where a missed case means use-after-free — a human look would still be worthwhile.
What was reviewed:
- Confirmed the new guard matches the sibling check at ConservativeRoots.cpp:141 and uses the same
cellKindalready loaded for this block. - Verified
mayHaveIndexingHeader(HeapCell.h:108) admitsAuxiliaryandJSCellWithIndexingHeader, so butterfly-bearing blocks keep the past-the-end rule. - Checked that
tryPointer's live-and-!mayHaveIndexingHeaderearly return already made this rule unreachable for live JSCell neighbours, supporting the PR's inconsistency argument.
Extended reasoning...
Overview
This PR adds a single mayHaveIndexingHeader(cellKind) conjunct to the final branch of ConservativeRoots::genericAddPointer(), which handles the "butterfly pointer sits just past the end of the previous cell" case within a MarkedBlock. The equivalent cross-block-boundary case ~50 lines above already carries this guard; this brings the two paths into alignment. The effect is that in plain HeapCell::JSCell blocks, a stack word landing on (or up to sizeof(IndexingHeader) past) a dead slot no longer conservatively marks the live cell to its left.
Security risks
The change makes conservative root scanning strictly less conservative for one cell kind. The upside (fixing a spurious retention that broke a Bun NAPI finalizer test) is benign; the downside, if the invariant "no code path holds a one-past-the-end pointer to a plain JSCell as its only root" is ever violated, is a live object being collected — i.e., use-after-free. The PR description argues this invariant holds (JIT/LLInt keep base pointers; the live-neighbour early return already skipped this rule inconsistently), and I found nothing contradicting that, but validating it exhaustively across LLInt/Baseline/DFG/FTL/B3 codegen and C++ runtime helpers is beyond what an automated pass can guarantee.
Level of scrutiny
High. ConservativeRoots::genericAddPointer runs on every word of every scanned stack/register set during every GC, and errors here manifest as rare, build-layout-dependent memory corruption. Even a one-line change in this function warrants review by someone with authority over JSC GC invariants.
Other factors
- The diff is mechanically correct:
cellKindis loaded fromcandidate->handle().cellKind()a few lines above and is the right block's kind; short-circuit ordering means the new predicate is evaluated first and cheaply. mayHaveIndexingHeader(HeapCell.h:108-111) coversAuxiliaryandJSCellWithIndexingHeader, matching the comment's intent — butterfly/auxiliary storage retains the past-the-end inference.- The PR description's forensic trace (stale callee-saved
r12spill mapping to dead-neighbour+8) is detailed and internally consistent, and the fix is the minimal change that addresses it. - No test is added; the motivating repro depends on exact stack layout and allocation order on a specific darwin-x64 build, so a deterministic regression test is likely impractical.
Given the criticality of the code path and the fact that the change relaxes a safety net, I'm deferring rather than approving.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe conservative root scan now applies cell-kind-specific allocation bounds and restricts butterfly end-pointer fallback marking to cells that may have an ChangesConservative root scanning
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
Preview Builds
|
No-Verification-Needed: diagnostics-only branch
…s that cannot hold a butterfly either The precise-allocation branch of genericAddPointer() uses PreciseAllocation::contains(), which accepts pointers up to sizeof(IndexingHeader) past the end of the cell for the same butterfly-end-pointer reason. It applied that slack to every precise allocation, including lower-tier precise cells of plain JSCell types (the first few cells of each IsoSubspace), so a word equal to cellEnd..cellEnd+8 pinned such a cell. That is the case actually hit in Bun (a 16-byte lower-tier-precise JSArray at ...c8, word == cell+24): gate the slack on mayHaveIndexingHeader(cellKind) here too, and treat only [cell, cellEnd) as a reference otherwise.
dylan-conway
changed the title
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it narrows the conservative-root acceptance window in the GC — where a mistake means premature collection / UAF rather than a leak — a human sign-off from someone with JSC heap ownership would still be worthwhile.
What was reviewed:
- The within-block
mayHaveIndexingHeader(cellKind)guard mirrors the existing previous-block guard;cellAlign()still marks interior JSCell pointers. - The PreciseAllocation branch:
aboveLowerBoundmatchescontains()'s lower bound (>= cell()), and the new upper bound< cell() + cellSize()drops only the+sizeof(IndexingHeader)slack frombelowUpperBoundfor non-butterfly kinds. - Confirmed
mayHaveIndexingHeadercoversAuxiliaryandJSCellWithIndexingHeader, so butterfly-carrying allocations keep the past-the-end rule.
Extended reasoning...
Overview
Two hunks in Source/JavaScriptCore/heap/ConservativeRoots.cpp::genericAddPointer():
- MarkedBlock within-block fallback — the "mark the cell to the left" rule for a candidate word in
[alignedPointer, alignedPointer + sizeof(IndexingHeader)]is now gated onmayHaveIndexingHeader(cellKind), matching the pre-existing gate on the cross-block-boundary variant ~50 lines above. In plainHeapCell::JSCellblocks the left-neighbour inference no longer fires; interior pointers still resolve viacellAlign(). - PreciseAllocation (second commit on the branch, not covered by the description text) — for allocations whose
cellKindcannot carry an IndexingHeader, the containment test is tightened fromcontains()(which allowsptr <= end + 8) toaboveLowerBound(ptr) && ptr < cell() + cellSize(). Butterfly-capable kinds keepcontains()unchanged.
Both changes strictly reduce the set of stack words that mark a cell.
Security risks
There is no injection/auth/data-exposure surface here. The relevant risk is memory safety: conservative scanning errs toward keeping things alive, and this PR removes some of that slack. If any codegen path held only a one-past-the-end (or end + 8) pointer to a plain JSCell-kind cell as its sole reference, that cell could now be collected while still reachable. The description argues convincingly that JIT/LLInt keep base pointers and that the existing live-neighbour early return already made this rule inconsistent for JSCell blocks — but the failure mode is UAF, so this deserves a human reviewer who owns this code.
Level of scrutiny
High. ConservativeRoots is the last line of defence for stack-referenced cells across all JIT tiers; a regression here manifests as rare, allocation-order-dependent crashes that are extremely hard to reproduce (as the PR's own debugging story demonstrates in the opposite direction). The diff is small and the reasoning is solid, but this is not a mechanical or config-level change.
Other factors
- The bug hunting system found no issues.
- I verified
PreciseAllocation::aboveLowerBound/belowUpperBound/containsinPreciseAllocation.h— the new strict check reuses the same lower bound and only drops the+ sizeOfIndexingHeaderupper slack, so no off-by-one vs. the existing helper. mayHaveIndexingHeader(HeapCell.h) returns true forAuxiliaryandJSCellWithIndexingHeader, so all butterfly-bearing allocations retain the original behaviour.- No prior human review or approval on the timeline yet; only bot comments.
…the PreciseAllocation half of oven-sh/WebKit#398) No-Verification-Needed: dependency pin bump; built in CI
…the PreciseAllocation half of oven-sh/WebKit#398) No-Verification-Needed: dependency pin bump; built in CI
…non-butterfly precise cells Review feedback: dropping [end, end+8] for JSCell-kind precise allocations also dropped the exact one-past-the-end pointer, which compiler-rewritten loops over trailing in-cell storage (DirectArguments, JSLexicalEnvironment, JSBigInt digits, large JSFinalObject inline storage, Wasm instances) can legitimately materialise. Only the +1..+8 IndexingHeader slack is butterfly-specific, so accept [cell, end] and reject (end, end+8].
| // case, this is pointing to the object to the right of the one we should be marking. As with the | ||
| // previous-block case above, only blocks whose cells can carry an IndexingHeader have such pointers. | ||
| if (mayHaveIndexingHeader(cellKind) && candidate->candidateAtomNumber(alignedPointer) > 0 && pointer <= alignedPointer + sizeof(IndexingHeader)) | ||
| tryPointer(alignedPointer - candidate->cellSize()); |
There was a problem hiding this comment.
🟡 Commit 9b999ae relaxed the PreciseAllocation branch to <= end because compiler-rewritten loops over trailing in-cell storage (DirectArguments, JSLexicalEnvironment, JSBigInt digits, large JSFinalObject inline storage) can materialise the exact one-past-the-end pointer as the only live reference — but that same rationale applies to those types when they live in a MarkedBlock with objectSize == cellSize, and here the new mayHaveIndexingHeader(cellKind) guard now drops that case (when the right neighbour is dead, tryPointer(alignedPointer - cellSize) used to mark the cell; now it's skipped). Consider allowing the exact-boundary case here too, e.g. (mayHaveIndexingHeader(cellKind) || pointer == alignedPointer) && ..., or add a comment explaining why the asymmetry vs. the PreciseAllocation path is intentional.
Extended reasoning...
What changed and where the asymmetry appears
Commit 9b999ae (the review-feedback follow-up on this PR) changed the PreciseAllocation non-butterfly bound from pointer < cell + cellSize to pointer <= cell + cellSize, with the explicit rationale:
compiler-rewritten loops over trailing in-cell storage (DirectArguments, JSLexicalEnvironment, JSBigInt digits, large JSFinalObject inline storage, Wasm instances) can legitimately materialise [the exact one-past-the-end pointer]. Only the +1..+8 IndexingHeader slack is butterfly-specific, so accept [cell, end] and reject (end, end+8].
That fix was applied only to the attemptLarge hunk. The MarkedBlock hunk at line 197 still gates the entire left-neighbour fallback on mayHaveIndexingHeader(cellKind), so for a non-butterfly block a stack word that lands exactly on cell + cellSize no longer marks cell.
Why the same rationale reaches the MarkedBlock path
The types listed in 9b999ae's message all live in MarkedBlocks when small enough. MarkedSpace's precise size classes cover every multiple of sizeStep = 16 up to preciseCutoff, so e.g. a DirectArguments or JSLexicalEnvironment whose allocationSize(N) is a multiple of 16 gets a block where objectSize == cellSize exactly. A C++-compiler-rewritten loop over its trailing storage can then hold only cell + cellSize — which is the start of the right-hand neighbour cell.
Step-by-step trace at line 197
Take a non-butterfly block (cellKind == JSCell), a live cell C at atom N, a dead cell at atom N+1, and a stack word pointer == start(N+1) == C + cellSize:
isJSCellKind(cellKind)is true andpointeris atom-aligned, sotryPointer(pointer)runs on cellN+1. It's dead →isLive == false→ returnsfalse.alignedPointer = cellAlign(pointer) = start(N+1) = pointer.tryPointer(alignedPointer)again hits deadN+1→ returnsfalse.- Pre-PR:
candidateAtomNumber(alignedPointer) > 0 && pointer <= alignedPointer + 8→ true →tryPointer(alignedPointer - cellSize)=tryPointer(C)→Cis marked. - Post-PR:
mayHaveIndexingHeader(cellKind)isfalse→ the whole condition short-circuits →Cis not marked.
So this PR removes the case where the MarkedBlock path did handle the exact one-past-the-end pointer, while the review-feedback commit on the same PR asserts that case matters for the PreciseAllocation path.
Why the PR description's defence doesn't cover this
The PR description says "plain JSCells are never referenced by an end/past-the-end pointer as their only reference (JIT/LLInt keep the base)". That addresses JIT-generated code, but 9b999ae's rationale is about C++-compiler-rewritten loops over trailing in-cell storage — a different codepath, and one the author accepted as valid one commit later. Either that rationale applies to both allocation paths (in which case line 197 should also let pointer == alignedPointer through), or it applies to neither (in which case 9b999ae is unnecessary). Applying it to only one hunk looks like an oversight.
There is a plausible argument for keeping the asymmetry: accepting pointer == alignedPointer as a reference to the previous MarkedBlock cell reintroduces a false-positive path (a stale base pointer to a dead cell N+1 marks its live predecessor N) that PreciseAllocation doesn't have because precise cells have no adjacent neighbour. And the MarkedBlock one-past-the-end case was already unreliable pre-PR — a live right neighbour short-circuits at the isLive && !mayHaveIndexingHeader early-return before ever reaching the fallback. But if that tradeoff is the intent, it's worth a comment; "was already broken half the time" is a weak reason to remove the half that worked.
Suggested fix
if ((mayHaveIndexingHeader(cellKind) || pointer == alignedPointer)
&& candidate->candidateAtomNumber(alignedPointer) > 0
&& pointer <= alignedPointer + sizeof(IndexingHeader))
tryPointer(alignedPointer - candidate->cellSize());This mirrors the PreciseAllocation path's <= end: for non-butterfly blocks, only the exact boundary (pointer == alignedPointer, i.e. end of the left cell) triggers the fallback; the (end, end+8] IndexingHeader slack stays gated on mayHaveIndexingHeader.
Severity
Nit. The trigger is narrow (objectSize == cellSize exactly, C++ compiler drops the base pointer across a GC safepoint, right neighbour dead), there's no known repro (the Bun repro was end+8 in a PreciseAllocation), and the pre-PR MarkedBlock behaviour was already inconsistent. But it's exactly the class of feedback the author already acted on in 9b999ae for the other hunk, so worth either fixing symmetrically or documenting why not.
…rfly slack for non-butterfly cells) oven-sh/WebKit#398 is merged; move the pin from the PR preview build to the fork-main autobuild. No-Verification-Needed: dependency pin bump
1 participant
What
ConservativeRoots::genericAddPointer()honours "a butterfly may pointsizeof(IndexingHeader)past the end of its allocation" in two places, and in both it applied that slack regardless of whether the candidate cell can carry an IndexingHeader at all:MarkedBlock, within a block — a candidate in
[cellStart, cellStart + sizeof(IndexingHeader)]that didn't resolve to a live cell marks the cell to its left:The cross-block version of the same rule ~50 lines up is already gated on
mayHaveIndexingHeader(cellKind); this one wasn't. → gate it the same way (6cb6d08).PreciseAllocation —
attemptLarge()usesPreciseAllocation::contains(), whosebelowUpperBound()isptr <= end + 8for every precise allocation, including lower-tier precise cells of plain JSCell types (JSCell::numberOfLowerTierPreciseCells = 8— the first few cells of each IsoSubspace). → for kinds that can't carry an IndexingHeader, accept only[cell, cellEnd); butterfly-capable kinds keepcontains()(e501c5c).contains()/belowUpperBound()themselves are unchanged (still used for the coarse range prefilter).Butterfly-capable allocations (Auxiliary / JSCellWithIndexingHeader) are untouched in both places — they need
end+8becauseButterfly*for a zero-vector-length butterfly without an indexing header is computed one header past the end. Plain JSCells are never referenced by an end/past-the-end pointer as their only reference (JIT/LLInt keep the base), and in the MarkedBlock case the behaviour was already inconsistent (a live right-hand neighbour short-circuits before the rule). Interior pointers still mark their own cell everywhere; that's the irreducible part of a conservative scan and is not changed.Why (how this was found)
Bun's
test/napi/napi.test.ts › napi_reference_unref is blocked from finalizers in experimental modulesstarted failing deterministically on darwin-x64 after an unrelated startup-order change (oven-sh/bun#37075). The test allocates aJSArrayof three finalizable objects via an addon, drops it, callsgc()(collectNow(Sync, Full)) from the entry script, and expects the finalizers to have run. They didn't; a secondgc()after the entry script returned to the event loop did.Instrumented builds (GC-debugging heap snapshot + in-process probes keyed on a file so argv/env stayed byte-identical — oven-sh/bun#37214) showed on the failing hosts:
SlotVisitor::append(const ConservativeRoots&);stack[…] = 0x34320fc24e0forArray @0x34320fc24c8—cell + 24=end + 8— atfp-0x20of Bun's CommonJS entry-module generator frame (push r12in its prologue =JSModuleLoader::makeModule'sr12at call time), a frame that lives for the whole entry script. The array's address is ≡ 8 mod 16 → a lower-tier PreciseAllocation → accepted by path (2). (Path (1) is the same defect for MarkedBlock cells; a first preview build with only (1) fixed left the failure intact, which is how (2) was pinned down.)Deterministic per binary+env+argv (same spilled value, same allocation/reuse sequence), flipped by any byte of either.
Risk
Only pointers that land on
end..end+8of a non-butterfly cell (precise) or on a dead right-hand neighbour's first 8 bytes (MarkedBlock) stop counting as references. No JSC code hands those out as a cell's sole reference.