a82eb9dd7f  [JSC] Array rematerialization should preserve double Array holes when having a bad time

The bug at a glance

A follow-up to the bad-time rematerialization fix: when a sunk double Array is rematerialized after a bad time, an unwritten element arrived as boxed NaN (the double hole default), matched no hole branch, and fell through to putDirectIndex, turning a hole into an own property and bypassing prototype indexed accessors. That is a correctness/soundness bug that lets fast paths (reverse) move raw holes without consulting accessors, a JIT security-relevant miscompile, so high.

operationPopulateObjectInOSR determined hole-ness from the array’s current indexing type and (for the pre-existing ArrayStorage branch) only recognized the empty JSValue. A double array’s hole default is boxed NaN, not empty, so after conversion to SlowPutArrayStorage the NaN was not treated as a hole and became a real own property; the ArrayStorage m_numValuesInVector was also not decremented, so hasHoles() lied. The fix keys hole-ness off the sunk indexing type and clears the vector slot while decrementing m_numValuesInVector.

Root cause

After commit eba64ef44d, a sunk array that is rematerialized during a bad time is built with the original contiguous structure, populated, then switched to SlowPutArrayStorage. operationPopulateObjectInOSR fills each element; holes are represented by the hole sentinel of the indexing type the array was sunk with - boxed NaN (PNaN) for a Double array, and the empty JSValue otherwise.

The pre-patch hole detection was wrong in two ways. It tested hasDouble(array->indexingType()) && value.isNumber() && std::isnan(value.asNumber()) and !value against the array’s current indexing type, but after the switch to SlowPutArrayStorage the array’s current type is ArrayStorage, not Double - so for a double array sunk with holes, the arriving boxed NaN matched neither the double branch (current type is no longer Double) nor the !value branches (the value is NaN, not empty). It therefore fell through to array->putDirectIndex(globalObject, index, value), which materializes index as a real own property holding NaN. That is a semantic escape: after a bad time, a hole at that index must forward to indexed accessors on the prototype chain, but the array now shadows the prototype with an own NaN property, so the prototype getter/setter is never consulted.

Separately, when the sentinel-filled contiguous butterfly was converted to ArrayStorage, the hole slot was counted in m_numValuesInVector. The prior ArrayStorage hole branch (added in eba64ef44d) cleared m_vector[index] but did not decrement m_numValuesInVector, so hasHoles() (which compares m_numValuesInVector to length) could read false. Array fast paths like Array.prototype.reverse then move the raw hole around without consulting prototype accessors - another accessor-bypass with wrong observable results.

The fix computes bool valueIsHole = hasDouble(materialization->indexingType()) ? value.isNumber() && isHole(value.asNumber()) : !value; - deriving hole-ness from the sunk indexing type (materialization->indexingType()), recognizing boxed NaN as the double hole via isHole(). All three hole branches now test valueIsHole. The ArrayStorage branch clears storage->m_vector[index] and decrements storage->m_numValuesInVector (with ASSERTs that the slot was non-empty and the count positive), so hasHoles() is accurate and fast paths correctly forward to prototype accessors.

Key code

Hole detection keyed to the sunk indexing type, with m_numValuesInVector fix (FTLOperations.cpp)

            bool valueIsHole = hasDouble(materialization->indexingType()) ? value.isNumber() && isHole(value.asNumber()) : !value;
            if (hasDouble(array->indexingType()) && valueIsHole) [[unlikely]]
                array->butterfly()->contiguousDouble().atUnsafe(index) = PNaN;
            else if ((hasInt32(array->indexingType()) || hasContiguous(array->indexingType())) && valueIsHole) [[unlikely]]
                array->butterfly()->contiguous().atUnsafe(index).setStartingValue(JSValue());
            else if (hasAnyArrayStorage(array->indexingType()) && valueIsHole) [[unlikely]] {
                ArrayStorage* storage = array->butterfly()->arrayStorage();
                ASSERT(storage->m_vector[index]);
                ASSERT(storage->m_numValuesInVector);
                storage->m_vector[index].clear();
                storage->m_numValuesInVector--;
            } else
                array->putDirectIndex(globalObject, index, value);

Patch walkthrough

• Source/JavaScriptCore/ftl/FTLOperations.cpp — operationPopulateObjectInOSR now computes valueIsHole from materialization->indexingType() (the sunk type): for a Double sink, a boxed NaN recognized by isHole() is a hole; otherwise the empty JSValue is. The double, int32/contiguous, and ArrayStorage branches all switch from their ad-hoc checks to valueIsHole. The ArrayStorage branch is expanded from a bare m_vector[index].clear() to also decrement m_numValuesInVector (guarded by ASSERT(storage->m_vector[index]) and ASSERT(storage->m_numValuesInVector)), fixing hasHoles() accounting so reverse and similar fast paths consult prototype indexed accessors instead of moving the raw hole.

Background

Double array hole sentinel (PNaN) — Double-indexed arrays represent holes as a canonical NaN (PNaN) stored in the double butterfly, because there is no empty JSValue in an unboxed double vector. When such an array is sunk and rematerialized, the recorded element for an unwritten index arrives as a boxed NaN, which must be recognized as a hole rather than a legitimate numeric value.

materialization->indexingType() vs array->indexingType() — materialization->indexingType() is the indexing type the array had when the allocation was sunk (Double, Int32, Contiguous); array->indexingType() is the live type after any bad-time switch to SlowPutArrayStorage. Hole-ness must be judged by the sunk type, since that dictates the sentinel encoding; the destination store is judged by the live type.

m_numValuesInVector / hasHoles() — ArrayStorage tracks how many vector slots hold real values in m_numValuesInVector. hasHoles() infers holes by comparing that count to the public length. If a cleared hole slot is not decremented, hasHoles() under-reports and fast paths (reverse, sort) skip the slow accessor-consulting path.

putDirectIndex accessor bypass — During a bad time, indexed stores/reads must route through prototype accessors. Calling putDirectIndex for what should be a hole installs a real own property, shadowing the prototype accessor - both a correctness bug and a soundness issue because optimized code assumed the accessor semantics.

Vulnerability window

1. Setup — opt() fills a double array leaving index 3 unwritten (a hole) until after cb(); FTL sinks the allocation after warmup.
2. Bad time — cb() defines get/set at index 3 on Array.prototype, entering a bad time; the sunk array is rematerialized as SlowPutArrayStorage.
3. Mis-populated hole — The hole for index 3 arrives as boxed NaN; pre-patch it matches no hole branch and putDirectIndex installs an own NaN property, and/or m_numValuesInVector is left too high.
4. Observable bug — a.hasOwnProperty(3) is true and the prototype getter is bypassed; reverse() moves the raw hole without consulting accessors.
5. Fix — valueIsHole keyed to the sunk indexing type recognizes boxed NaN; the ArrayStorage branch clears the slot and decrements m_numValuesInVector so accessors are consulted.

Proof of concept

VERBATIM excerpt of the added …-double-hole.js (trailing reverse()/element assertions elided for length; the int32-hole.js sibling is identical with integer values and the empty-JSValue sentinel). opt() leaves index 3 a double hole (PNaN) across the cb()/gc() that triggers the bad time and OSR exit, so the sunk double array is rematerialized with index 3 still holding the hole sentinel. Post-patch, index 3 is not an own property, reads 42 via the prototype getter, the store forwards to the prototype setter, and reverse() consults the accessors; pre-patch the hole became an own NaN property and hasHoles() mis-accounting let reverse move the raw hole.

//@ runDefault("--jitPolicyScale=0.1")

let trigger = false;
let getterCalls = 0;
let setterCalls = 0;

function cb() {
    if (trigger) {
        Object.defineProperty(Array.prototype, 3, {
            get() { getterCalls++; return 42; },
            set(value) { setterCalls++; },
            configurable: true
        });
    }
}
noInline(cb);

function collect() { gc(); }
noInline(collect);

function opt(escape) {
    let a = new Array(5);
    a[0] = 1.1;
    a[1] = 2.2;
    a[2] = 3.3;
    a[4] = 5.5;
    cb();
    collect();
    // Index 3 is written only after the cb() call so that the OSR exit triggered by having a bad
    // time during cb() rematerializes the sunk double Array while index 3 still holds the hole
    // default (PNaN).
    a[3] = 4.4;
    if (escape)
        return a;
    return 0;
}
noInline(opt);

for (let i = 0; i < 1000; i++)
    opt(!(i % 10));

trigger = true;
let a = opt(true);

if (a.hasOwnProperty(3))
    throw new Error("index 3 should not be an own property, got value: " + a[3]);
if (a[3] !== 42)
    throw new Error("a[3] should return 42 from the prototype getter, got: " + a[3]);
if (setterCalls !== 1)
    throw new Error("Array.prototype setter should have been called once, got: " + setterCalls);
a.reverse();

Exploitation

1. Reachability — Pure JS: array tiering plus Object.defineProperty of an indexed accessor on Array.prototype, exactly as in eba64ef44d. No special privileges.
2. Semantic escape — Force a double (or int32) array to be sunk with a hole, trigger a bad time so it rematerializes as SlowPutArrayStorage; the mis-detected hole becomes an own property, bypassing the prototype accessor the engine had already committed to honoring.
3. Primitive — Accessor-bypass plus incorrect hasHoles() accounting. The shipped tests demonstrate observable spec violations (own-property shadowing, reverse moving raw holes) rather than a direct memory-corruption primitive; the security risk is optimized code operating on an array whose real layout/semantics differ from what it assumed after the bad time.

Detection & hunting

For defenders and SOC / detection engineers:

• hasOwnProperty on a rematerialized hole —
• reverse/sort not consulting prototype accessors —
• ArrayStorage m_numValuesInVector inconsistency —

Audit directions

• Sentinel encoding per indexing type —
• ArrayStorage value accounting —
• Bad-time + hole interactions —
• Companion of eba64ef44d —