[JSC] DFG: clear a cloned block's source pointer when that source block is removed - #620
[JSC] DFG: clear a cloned block's source pointer when that source block is removed#620robobun wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe DFG graph now clears clone source references when assertion-enabled builds remove a basic block. A stress test exercises loop unrolling, late branch changes, and result equivalence. ChangesDFG clone source cleanup
Merge Risk: ⚪ Minimal · up to This change prevents stale clone-source pointers during graph cleanup and adds stress coverage for post-removal dumping. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed and relevant explanation of the problem, fix, reproduction, and verification. However, it omits the required Bugzilla link, review line, and file/function change list from the repository template. Resolution Add the bug title and Bugzilla URL, include a "Reviewed by NOBODY (OOPS!)." line or the applicable reviewer, and list each changed file with its affected functions or classes. Link the pull request to the associated Bugzilla issue and apply the required labels.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
…ck is removed Loop unrolling records in each clone the block it was cloned from (BasicBlock::cloneSource, ASSERT_ENABLED only). Graph::dumpBlockHeader() prints it. Nothing cleared the pointer when the source block left the graph, so a clone could outlive its source and the next graph dump read freed memory. CFGSimplificationPhase::mergeBlocks() kills the source block of an unrolled loop, and FTL::LowerDFGToB3::validateAIState() dumps the graph in the same plan. With --validateAbstractInterpreterState=1 an assert-enabled build reports a heap-use-after-free in BasicBlock::dump(). Clear the pointer in Graph::killBlock(), the one place a block leaves the graph. The dump then names the source block while it exists and says nothing once it does not.
99ada55 to
30ca760
Compare
There was a problem hiding this comment.
LGTM — thanks for reworking the test to use testLoopCount and compute the expected value from reference() instead of a pinned constant.
What was reviewed:
Graph::clearCloneSourceand thekillBlock(BlockIndex)call site: both correctly#if ASSERT_ENABLED-gated, so release builds compile no new code; the scan is O(blocks) per kill, debug-only.- Confirmed
killBlock(BasicBlock*)delegates tokillBlock(BlockIndex), so both overloads are covered by the one hook. - Verified
reference(i, late)matches the unrolled arithmetic ofhot(i, late)step-by-step, so the assertion is sound for anytestLoopCount.
Extended reasoning...
Overview
This PR fixes a debug-only dangling pointer in DFG's BasicBlock::cloneSource field (set by loop unrolling, read by graph dumps). Graph::killBlock(BlockIndex) now calls a new clearCloneSource() that walks m_blocks and nulls any cloneSource pointing at the block being freed. A comment is added on the field, and a new stress test exercises the FTL path where --validateAbstractInterpreterState=1 dumps the graph after CFG simplification has merged away an unrolled loop's source block. Since my earlier review, the author rewrote the test to drive iterations with testLoopCount and to compute the expected result via a hand-unrolled reference() function rather than a hard-coded constant, addressing the JSTests convention I flagged.
Security risks
None. The entire C++ change is gated behind #if ASSERT_ENABLED, so release/product builds compile no additional code. In assert-enabled builds it strictly removes a heap-use-after-free read in a diagnostic dump path. The new test is a pure JS stress test with no I/O.
Level of scrutiny
Low. Although this lives in Source/JavaScriptCore/dfg/, the modification is a minimal, obviously-correct pointer cleanup in debug-only bookkeeping — no changes to code generation, optimization, or runtime semantics. killBlock(BasicBlock*) forwards to killBlock(BlockIndex), so hooking the index overload covers both entry points; the null-check on dyingBlock guards the already-null slot case. The repo's CODEOWNERS is upstream WebKit's auto-reviewer list (it explicitly states contributors do not "own" components and names a @ WebKit/... team from a different org), so I did not treat it as a blocking ownership gate for this fork.
Other factors
I hand-checked the reference() closed form against hot()'s loop for j∈{0,1,2,3} in both late branches — they match, so the shouldBe(result, expected) assertion is valid regardless of testLoopCount's configured value. The test follows JSTests/README.md rules (throws on failure, no logging, testLoopCount-driven). My previous inline comment was optional and has been fully addressed by the force-pushed revision.
1 participant
Problem
--validateAbstractInterpreterState=1reportsAddressSanitizer: heap-use-after-free ... READ of size 4inBasicBlock::dump(), reached fromGraph::dumpBlockHeader()(dfg/DFGGraph.cpp:490) insideFTL::LowerDFGToB3::validateAIState(). The same plan freed that block earlier, inCFGSimplificationPhase::mergeBlocks().BasicBlock::cloneSource(dfg/DFGBasicBlock.h:279,ASSERT_ENABLEDonly) is a raw pointer to the block a clone came from. Loop unrolling sets it (dfg/DFGCloneHelper.h:125). Nothing cleared it when that block left the graph, so a clone could outlive its source and the next dump read freed memory.--dumpGraphAtEachPhase, verbose compilation, a validation failure report, aDFG_ASSERTreport.Fix
Graph::killBlock()clears the pointer in every block that still names the dying block. It is the one call that takes a single block out of the graph, andm_blocks[blockIndex] = nullptrfrees it there.BlockInsertionSet::execute()renumbers every block, so an index recorded at clone time names a different block later.JSTests/stress/loop-unrolling-dump-after-source-block-removal.js(new). On a Debug + ASAN shell built from this tree it reports the use-after-free unpatched and passes patched. 37reflect-*andloop-unrolling-*stress tests give identical results on both shells.Background
validateAbstractInterpreterStatechecks the abstract interpreter's state against what the FTL lowering believes. It callsGraph::dump()once before it reports anything, which is how a validation option reaches the block dumper.CloneHelper. Each copy keepscloneSourceso a dump can printBlock #12<-#5and a reader can tell which original block a clone came from.Notes
An internal differential audit of the JIT tiers found this. No user reported it and no issue is linked.
Reproduction,
jscshell from this tree (Debug + ASAN, soASSERT_ENABLED):Unpatched:
heap-use-after-free, 4 bytes inside a 304-byte region, exit 1. The read ism_indexinBasicBlock::dump(). Patched: prints nothing and exits 0. The test takes 3.2 s in that configuration, almost all of it the validator's per-node graph dump.BasicBlock::operator delete<-Graph::killBlock<-CFGSimplificationPhase::mergeBlocks<-Plan::compileInThreadImpl.BasicBlock::dump<-printInternal(PrintStream&, BasicBlock*)<-Graph::dumpBlockHeader<-Graph::dump<-FTL::LowerDFGToB3::validateAIState<-lowerDFGToB3<-Plan::compileInThreadImpl.cloneSource. What the bug costs is the DFG dump on the builds that have it, which is the tool a developer reaches for when the JIT misbehaves.ByteCodeParsercallskillBlockAndItsContents()beforem_blocks.removeLast(), and during parsing no clone exists yet.BlockInsertionSet::execute()only shrinks away entries it has already moved out, andGraph::freeDFGIRAfterLowering()drops the whole graph.JSTests/is in the sparse-checkout exclude list of.github/workflows/build-reusable.yml, so this repo's CI builds the shell but does not run the new test. I ran it withTools/Scripts/run-jsc-stress-testsagainst a local shell.cloneSourceandGraph::killBlock()are byte-identical in upstream WebKitmain, so the patch applies there unchanged and this delta can be dropped on the sync that carries it. The field came from WebKit/WebKit#43647 (293265@main) and has had no follow-up.testLoopCountiterations (150 with the thresholds in its header) and compares the result against the same arithmetic written without a loop, so it also fails on a miscompile of the unrolled loop.