[JSC] IRO's ArithAbs transform can pin freed range bound proofs (cherry-pick WebKit/WebKit#72679) - #530
Conversation
https://bugs.webkit.org/show_bug.cgi?id=322801 Reviewed by NOBODY (OOPS!). Since 319726@main, rangeFor() returns RangeBounds whose proof field is a raw pointer into the queried node's relationship Vector. The ArithAbs transform is the one caller that runs executeNode() between rangeFor() and pinRangeBounds(), and executeNode(ArithAbs) can grow that same Vector: when the abs input is proven equal to a constant, setOneSide()'s Equal-edge propagation appends a derived relationship to the input's list. The append can reallocate the Vector, and pinRangeBoundProof() then reads the proofs out of freed memory and merges NodeMustGenerate into whatever nodes it finds there. Capture the proof's left and right nodes by value in RangeBound instead of pointing into the relationship map. Nodes are stable, so the captured proof remains valid no matter how executeNode() mutates the map. Without the fix, the added stress test crashes with a heap-use-after-free on ASAN builds; on regular builds the stale read goes unnoticed because the freed buffer still holds the old bytes. Test: JSTests/stress/dfg-integer-range-optimization-arith-abs-proof-use-after-free.js * JSTests/stress/dfg-integer-range-optimization-arith-abs-proof-use-after-free.js: Added. (foo): * Source/JavaScriptCore/dfg/DFGIntegerRangeOptimizationPhase.cpp: (cherry picked from commit cc6f247)
|
Warning Review limit reached
On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file. Or wait 3 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a clean upstream cherry-pick, but since it touches DFG JIT internals (a security-sensitive area) and both changed paths are covered by CODEOWNERS, a human look is still worthwhile.
What was reviewed:
- Confirmed
pinRangeBoundProofstill handles the no-proof case:proofLeft/proofRightdefault tonullptrand are each null-checked, so removing the oldif (!bound.proof) return;guard is safe. - Verified the ArithAbs path calls
executeNode()betweenrangeFor()andpinRangeBounds(), which can grow the relationshipVector— the capturedNode*values are arena-stable across that mutation, unlike the oldRelationship*. - Checked the new stress test against JSTests/README.md rules: uses
testLoopCount, throws on failure, no success logging,//@ runDefaultheader present.
Extended reasoning...
Overview
This PR cherry-picks upstream WebKit commit cc6f247 into the Bun fork. It changes Source/JavaScriptCore/dfg/DFGIntegerRangeOptimizationPhase.cpp to fix a heap-use-after-free: RangeBound previously held a const Relationship* pointing into a Vector<Relationship> in m_relationships. In the ArithAbs transform, executeNode() runs between rangeFor() (which captured the pointer) and pinRangeBounds() (which dereferenced it), and executeNode() can append to that same vector via setRelationship/setOneSide, reallocating it. The fix copies relationship.left().node() and relationship.right().node() into the RangeBound by value at capture time — DFG Node* are arena-allocated and stable for the phase — and pinRangeBoundProof() reads those directly. A regression test is added under JSTests/stress/.
Security risks
The bug being fixed is itself security-relevant: a UAF in a JIT compiler phase that then calls mergeFlags(NodeMustGenerate) on whatever pointers happen to be in freed memory. The fix reduces risk and does not introduce new attack surface. I did not spot any way the new code could dereference an invalid pointer: both proofLeft and proofRight are initialized to nullptr and null-checked before use, and they are only ever assigned Node* values obtained from live Relationship entries during iteration.
Level of scrutiny
Medium-high. The diff is small (~15 lines of production code) and mechanical, and it was already reviewed and landed upstream. However, it lives in the DFG optimizer — miscompiles here can lead to incorrect codegen or exploitable behavior — and both Source/JavaScriptCore/** and JSTests/** are listed in .github/CODEOWNERS under @ WebKit/jsc-reviewers. Per the approval guidelines, CODEOWNER-covered and security-sensitive paths should get a human sign-off rather than an automated approval.
Other factors
The new stress test follows the JSTests conventions (testLoopCount, throw-on-failure, no success output, //@ runDefault directive with lowered warm-up thresholds and concurrent JIT disabled). The removal of the early if (!bound.proof) return; is behaviorally equivalent because the two new fields default to nullptr and are individually guarded. No other callers of RangeBound::proof exist in the file, so the rename is complete.
Preview Builds
|
Cherry-pick of WebKit/WebKit#72679 (upstream commit
cc6f2479e114d710ff3657fba7d322d50f34f7a0, https://bugs.webkit.org/show_bug.cgi?id=322801). The commit applies cleanly onmain. The author and the commit message are unchanged.Problem
rangeFor()inDFGIntegerRangeOptimizationPhase.cppreturnsRangeBoundvalues whoseprooffield is a raw pointer into the queried node's relationshipVector.ArithAbstransform callsexecuteNode()betweenrangeFor()andpinRangeBounds(). When the abs input is proven equal to a constant,setOneSide()appends a derived relationship to that sameVector. The append can reallocate it.pinRangeBoundProof()then reads the proof out of freed memory and mergesNodeMustGenerateinto whatever nodes it finds there. ASAN builds report a heap-use-after-free.Fix
RangeBoundstores the proof's left and rightNode*by value (proofLeft,proofRight) instead of a pointer into the relationship map.executeNode()mutates the map.JSTests/stress/dfg-integer-range-optimization-arith-abs-proof-use-after-free.js(added by the upstream commit). It crashes on an unfixed ASAN build.Background
pinRangeBounds()marks the nodes that a proof depends on asNodeMustGenerate, so a later DCE pass cannot remove them after IRO relied on them.Relationshipobjects live in per-nodeVectors insidem_relationships. AVectormoves its elements when it grows, so a pointer to an element is only valid until the next append.