7937fd75ae  [JSC] Map / Set iterator next operation should not touch JSMapIterator / JSSetIterator directly

The bug at a glance

This is a JIT-correctness hardening of the DFG/FTL model for Map/Set iterator advancement. Pre-patch, MapIteratorNext mutated the JSMapIterator/JSSetIterator in place and was declared with a NodeResultBoolean while forbidding OSR exit mid-operation; that coupling made allocation-sinking impossible and left the advance non-atomic w.r.t. OSR exit. The class of defect (an iterator left partially- or double-advanced across an OSR exit or a sunk-object materialization) is a soundness bug that can desynchronize the iterator’s storage/entry state; graded medium as a correctness/robustness fix without a demonstrated in-the-wild memory-corruption exploit.

The refactor makes MapIteratorNext stateless: instead of the node reading-and-writing the iterator, the parser now loads the (Storage, IteratedObject, Entry) internal fields explicitly, computes the advanced (storage, entry) tuple, and commits it back via PutInternalField nodes emitted AFTER the key/value loads. The irreversible advance is published exactly once, only after everything else in next() is done, so an OSR exit or object-allocation-sinking escape mid-way can never leave the iterator partially or doubly advanced.

Root cause

OBSERVED: MapIteratorNext’s node result changes from NodeResultBoolean to a tuple (DFGNodeType.h: macro(MapIteratorNext, 0), plus isTuple()/tupleSize()==2 entries in DFGNode.h). The operation now returns a (storage, entry) pair via ExtractFromTuple(0)/ExtractFromTuple(1), and ‘done’ is computed separately as CompareEqPtr(newStorage, orderedHashTableSentinel).

OBSERVED: In DFGByteCodeParser.cpp, the intrinsic and handleIteratorNext paths no longer pass the iterator object to MapIteratorNext with MapIteratorObjectUse/SetIteratorObjectUse. Instead they emit GetInternalField for Storage, IteratedObject and Entry, feed those to MapIteratorNext (Edge(storageField), Edge(iteratedObjectField, MapObjectUse/SetObjectUse), Edge(entryField)), then emit PutInternalField(storageFieldIndex)/PutInternalField(entryFieldIndex) to commit the advance – crucially placed AFTER the MapIteratorKey/MapIteratorValue loads. The done-block writes the sentinel into the Storage field.

OBSERVED: The advanced pair is carried to successor blocks through private tmps (allocatePrivateTmps(2), tmpStorage/tmpEntry) with a set()+flush() so ’the successor reads go through a phi merge rather than short-circuiting to the producer, which would create cross-block edges that violate the CPS validator.’ The old code had a comment ‘Now MapIterator status gets mutated. So we must not do OSRExit unless it is throwing an exception’ and used m_exitOK=true;addToGraph(ExitOK); the new code replaces that with emitExitOK() at the block boundaries because the mutation is no longer implicit inside MapIteratorNext.

OBSERVED: DFGClobberize.h no longer def()s a HeapLocation for MapIteratorNext (‘We do not def here as it is tuple result’) and stops writing the iterator-fields heap for MapIteratorNext; MapIteratorKey/MapIteratorValue now key their HeapLocation on (storageEdge, entryEdge) and select the heap via node->bucketOwnerType() rather than the iterator use-kind.

OBSERVED: The runtime operations operationMapIteratorNext/operationSetIteratorNext change signature from (VM*, JSCell*) returning EncodedJSValue to (VM*, JSCell* storageCell, JSCell* iteratedObjectCell, int32_t entry) returning UGPRPair. They handle the sentinel and null-storage cases, then call JSMap/JSSet::Helper::transitAndNext(vm, storageRef, entry) and return (result.storage, result.entry+1) – i.e. the advance is expressed as a pure function of inputs rather than a mutation of the iterator cell.

INFERRED: Because the old model mutated JSMapIterator in place and disallowed OSR exit mid-op, DFG/FTL could not treat the iterator as a sinkable allocation, and any exit between the implicit advance and the value load risked observing an inconsistent iterator. Making the advance a tuple-producing, explicitly-committed operation lets ObjectAllocationSinking eliminate the iterator in for-of and guarantees the single PutInternalField commit is the only observable state change, closing the partial/double-advance window.

Key code

Parser: advance via tuple, then commit AFTER the value loads (verbatim, DFGByteCodeParser.cpp)

Node* tuple = addToGraph(MapIteratorNext, Edge(storageField), Edge(iteratedObjectField, MapObjectUse), Edge(entryField));

Node* newStorage = addToGraph(ExtractFromTuple, OpInfo(0), tuple);
newStorage->setResult(NodeResultJS);
Node* newEntry = addToGraph(ExtractFromTuple, OpInfo(1), tuple);
newEntry->setResult(NodeResultInt32);
// ... key/value loaded from tmpStorage/tmpEntry in successor block ...
// Commit the advanced state after the loads, so a key/value OSR exit cannot
// leave the iterator partially advanced.
Node* iterator = get(bytecode.m_iterator);
addToGraph(PutInternalField, OpInfo(storageFieldIndex), iterator, keyStorage);
addToGraph(PutInternalField, OpInfo(entryFieldIndex), iterator, keyEntry);

Patch walkthrough

• Source/JavaScriptCore/dfg/DFGNodeType.h — Changes MapIteratorNext from NodeResultBoolean to a tuple result (0 flags), so the node produces a (storage, entry) pair instead of a boolean done-flag.
• Source/JavaScriptCore/dfg/DFGNode.h — Registers MapIteratorNext as a tuple node: adds it to isTuple() and to tupleSize() (returning 2), and adds MapIteratorKey/MapIteratorValue to hasBucketOwnerType() so those nodes now carry a Map-vs-Set discriminator in OpInfo instead of via the iterator use-kind.
• Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp — Rewrites both the JSMapIterator{Next,Key,Value} intrinsics and handleIteratorNext. Now emits explicit GetInternalField(Storage/IteratedObject/Entry), a MapIteratorNext taking those edges, ExtractFromTuple for the new storage/entry, and PutInternalField commits placed AFTER the key/value loads; done is CompareEqPtr against orderedHashTableSentinel. Uses allocatePrivateTmps(2) + flush() to pass the advanced pair across blocks safely, and emitExitOK() at block boundaries now that the advance is no longer an implicit mutation.
• Source/JavaScriptCore/dfg/DFGClobberize.h — MapIteratorNext no longer read/write/def()s the iterator-fields heap (it is now a tuple result); MapIteratorKey/MapIteratorValue def HeapLocation keyed on (storageEdge, entryEdge) and choose JSMapFields/JSSetFields via bucketOwnerType(). This lets CSE key the loads on storage+entry rather than the iterator identity.
• Source/JavaScriptCore/dfg/DFGFixupPhase.cpp — Updates edge fixups: MapIteratorNext fixes child2 to Map/SetObjectUse and child3 to Int32Use (child1 storage left UntypedUse so the empty-marker flows without a speculation check) and sets the tuple result flags (NodeResultJS, NodeResultInt32); MapIteratorKey/Value fix child1 to KnownCellUse (storage) and child2 to Int32Use (entry).
• Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h / DFGPredictionPropagationPhase.cpp — Teach the abstract interpreter and prediction propagation about the tuple: setTypeForTupleNode(node,0,SpecCellOther)/setNonCellTypeForTupleNode(node,1,SpecInt32Only) and setTuplePredictions(SpecCellOther, SpecInt32Only), then clearForNode.
• Source/JavaScriptCore/dfg/DFGOperations.cpp / DFGOperations.h — operationMapIteratorNext/operationSetIteratorNext become pure (VM*, storageCell, iteratedObjectCell, entry) -> UGPRPair functions that resolve the sentinel/null-storage cases and call JSMap/JSSet::Helper::transitAndNext, returning (storage, entry+1) rather than mutating the iterator cell.
• Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp / *32_64.cpp / *64.cpp / DFGSpeculativeJIT.h — compileMapIteratorKey/Value now take (storage, entry) operands directly instead of speculating the iterator object and loading its Entry/Storage fields; adds jsValueTupleResultWithoutUsingChildren and reworks cellTupleResultWithoutUsingChildren for the tuple result of MapIteratorNext.
• Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp — FTL lowerings for compileMapIteratorNext/Key/Value are updated to the tuple / (storage,entry) model to match the DFG.

Background

JSMapIterator / JSSetIterator internal fields — Map/Set iterators are JSInternalFieldObjectImpl instances with internal fields including Storage (the ordered-hash-table butterfly, or a sentinel), IteratedObject (the JSMap/JSSet), and Entry (the current index). next() must advance Entry and follow storage transitions when the table was rehashed.

OSR exit and atomicity — An OSR exit bails from optimized code back to the baseline interpreter at a bytecode checkpoint. If optimized code has already applied a side effect (advancing the iterator) but then exits before producing the value, re-execution in baseline could re-advance – skipping or duplicating an element. The old model forbade OSR exit mid-next(); the new model instead commits the advance atomically after the loads so exit-then-reexecute is safe.

ObjectAllocationSinking — A DFG/FTL phase that avoids allocating an object whose effects can be represented by its field values, materializing it lazily only if it escapes. For a for-of loop the JSMapIterator can be sunk entirely – but only if the compiler can see all field modifications as explicit PutInternalField nodes, which is exactly what this patch exposes (benefit #1 in the commit message).

Tuple result nodes / ExtractFromTuple — Some DFG nodes produce more than one value (e.g. StringIteratorNext). They are marked isTuple() with a tupleSize and per-index result flags; consumers pull individual results via ExtractFromTuple(index). MapIteratorNext is converted into such a node, returning (storage, entry).

orderedHashTableSentinel — A VM-wide sentinel cell stored into the iterator’s Storage field to mark iteration complete. ‘done’ is now computed as CompareEqPtr(newStorage, sentinel) instead of being a boolean returned from the mutating operation; the done-block writes the sentinel via PutInternalField.

transitAndNext — JSMap/JSSet::Helper::transitAndNext(vm, storageRef, entry) walks any obsolete/rehashed storage chain from the given entry and returns the next live (storage, entry). Expressing advancement as this pure helper keyed on explicit inputs is what makes the DFG operation stateless and re-runnable.

Vulnerability window

1. Prior model — MapIteratorNext mutates JSMapIterator/JSSetIterator in place, returns a boolean done, and forbids OSR exit mid-operation (per the removed ‘must not do OSRExit’ comments).
2. Limitations — The implicit mutation blocks ObjectAllocationSinking of the iterator and couples the irreversible advance to a point where an OSR exit or escape could observe inconsistent state.
3. Redesign — bug 315650 / rdar://178034413: model next() statelessly, returning an advanced (storage, entry) tuple committed via explicit PutInternalField after the loads.
4. Implementation — Node becomes a tuple; parser emits Get/Put InternalField + ExtractFromTuple; operations become pure (VM*, storage, iterated, entry) -> UGPRPair; clobberize/fixup/AI/prediction/JIT/FTL all updated (315330@main).
5. Verification — Added JSTests/stress/map-set-iterator-next-deferred-commit.js exercises sunk-iterator for-of, escape-after-partial-advance, OSR-exit-after-advance, and mutation-during-iteration.

Proof of concept

Verbatim excerpt (case 3) of the added regression test JSTests/stress/map-set-iterator-next-deferred-commit.js. It advances a Map values-iterator once, then forces an OSR exit immediately after the advance and escapes the iterator. The assertions require the escaped iterator to resume from exactly the next live entry (1, then 2, then done) – i.e. the advance must be neither lost nor duplicated across the exit. The full file also covers sunk-iterator for-of (case 1), escape-after-partial-advance for Map and Set (cases 2/2b), and rehash+delete mutation during iteration (case 4). It is a correctness regression test, not a crash reproducer.

// 3. Force an OSR exit immediately after an advance. The iterator must not be left
//    partially advanced: re-running from the checkpoint after the exit must continue
//    the iteration cleanly without skipping or repeating an element.
(function () {
    var map = new Map([[0, 0], [1, 1], [2, 2]]);
    var mapIteratorPrototype = map.values().__proto__;

    function test(map, flag) {
        var iterator = map.values();
        iterator.next();
        if (flag) {
            OSRExit();
            return iterator;
        }
        return iterator.next().value;
    }
    noInline(test);

    for (var i = 0; i < testLoopCount; ++i)
        shouldBe(test(map, false), 1);

    var iterator = test(map, true);
    shouldBe(iterator.__proto__, mapIteratorPrototype);
    shouldBe(iterator.next().value, 1);
    shouldBe(iterator.next().value, 2);
    shouldBe(iterator.next().done, true);
})();

Exploitation

1. Nature — This is a JIT model-soundness change. The pre-patch hazard is a Map/Set iterator being left in an inconsistent (partially- or double-advanced) state across an OSR exit or an allocation-sinking materialization, which corrupts iteration order/results.
2. Toward corruption — INFERRED/speculative: iterator storage/entry desynchronization in JIT-compiled for-of could, in a worst case, drive out-of-step reads of hash-table storage. The patch provides no exploit and no memory-corruption test; treat any memory-safety claim as unproven.
3. Honest assessment — Best characterized as correctness-only in the observed artifacts (all added tests are value-equality assertions). No crash-only PoC or shaped primitive is demonstrated.

Detection & hunting

For defenders and SOC / detection engineers:

• for-of result divergence under JIT —
• OSR-exit stress on iterators —
• Allocation-sinking assertions —

Audit directions

• Other stateful iterator nodes —
• Tuple-node plumbing —
• transitAndNext / obsolete-storage chains —
• clobberize HeapLocation keys —