Bug 312664 – Hole-to-NaN Conversion in DFG Object Allocation Sinking Phase

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

Created attachment 479189 [details]
poc.js

## Summary

The DFG Object Allocation Sinking phase in JavaScriptCore incorrectly handles holes in sunk double-indexed arrays. When a conditionally-written slot in a sunk double array is read on the path where the write did not happen, the PNaN hole marker (used internally by JSC to represent holes in double arrays) is boxed as `jsNumber(NaN)` instead of being recognized as a hole. This causes `arr[index]` to return `NaN` instead of `undefined`, and `index in arr` to return `true` instead of `false`. The bug affects both the FTL inline path (where a `NodeResultDouble` Phi resolves to PNaN and is boxed via `compileValueRep`) and the OSR exit materialization path (where the PNaN-as-NaN value is stored via `putDirectIndex`, triggering an unintended double-to-contiguous array type conversion and a stale write barrier check).

**While I think this is not a security bug, I thought it was safer to report this as security. Feel free to demote this to a bug.**

## Bug

### Summary

The root cause is in `DFGObjectAllocationSinkingPhase.cpp`'s `defaultFor()` lambda, which inserts `DoubleConstant(PNaN)` as the default value for unwritten slots in sunk double arrays. PNaN (Purified NaN = `0x7ff8000000000000`) has a dual meaning in JSC: as a raw double in butterfly storage it is the hole sentinel (empty slot), but when boxed as a JSValue via `jsNumber(PNaN)` or `boxDouble(pnan)` it becomes the floating-point value NaN. The sinking phase conflates these two roles. When a `NodeResultDouble` Phi merges the written value with PNaN at a control-flow join, the FTL type system treats PNaN as a real double and boxes it as NaN whenever the Phi result must appear as a JSValue, rather than recognizing it as a hole that should produce `undefined`.

### Detail

The `defaultFor()` lambda in the allocation sinking phase creates a PNaN constant for unwritten double array slots:

```cpp
// Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:2014-2031
auto defaultFor = [&](PromotedHeapLocation location) -> Node* {
    // For objects we track the transition state of the object (i.e. which properties are conditionally initialized).
    // Arrays have no such mechanism however they do have a hole value which we can use instead. This is fine because
    // if the hole conditionally makes it to a materialization it will be stored to the slot and it will appear as
    // if the slot was never written to.
    if (location.kind() == ArrayIndexedPropertyPLoc) {
        ASSERT(location.base()->op() == NewArrayWithButterfly);
        if (hasDouble(location.base()->indexingType())) {
            if (!m_PNaN)
                m_PNaN = m_insertionSet.insertConstant(0, m_graph.block(0)->at(0)->origin, jsNumber(PNaN), DoubleConstant);
            return m_PNaN;
        }
        if (!m_empty)
            m_empty = m_insertionSet.insertConstant(0, m_graph.block(0)->at(0)->origin, JSValue());
        return m_empty;
    }
    return m_bottom;
};
```

The comment assumes that PNaN will always reach user code via the raw-double butterfly store path (materialization), where the bit pattern `0x7ff8000000000000` genuinely represents a hole. However, this assumption is incorrect for the inline FTL path, where PNaN is converted to a JSValue and appears as NaN.

When a Phi node is created for a double array index at a control flow merge point, it is flagged with `NodeResultDouble`:

```cpp
// Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:2135-2145
Node* phiNode = m_graph.addNode(SpecHeapTop, Phi, block->at(0)->origin.withInvalidExit());

// It shouldn't be possible to get a butterfly here since it should also be a sink candidate.
ASSERT(location.kind() != ArrayButterflyPLoc);
if (location.kind() == ArrayIndexedPropertyPLoc && hasDouble(allocation.indexingType())) {
    ASSERT(allocation.kind() == Allocation::Kind::Array);
    phiNode->mergeFlags(NodeResultDouble);
} else
    phiNode->mergeFlags(NodeResultJS);
return phiNode;
```

Because the Phi has `NodeResultDouble`, the FTL lowers it to a B3 `Double`-typed phi (`createPhiVariables()` at `FTLLowerDFGToB3.cpp:452-454`). On the path where the conditional write did not happen, the phi's input from the `defaultFor` constant resolves to PNaN (double `0x7ff8000000000000`).

**Inline FTL path:**

Whenever the result of this `NodeResultDouble` Phi must be presented as a JSValue — for example, when `let v = arr[1]` stores the value into a JS variable — the FTL inserts a `ValueRep` node that calls `compileValueRep()`:

```cpp
// Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2333-2343
void compileValueRep()
{
    switch (m_node->child1().useKind()) {
    case DoubleRepUse: {
        LValue value = lowDouble(m_node->child1());

        if (abstractValue(m_node->child1()).couldBeType(SpecDoubleImpureNaN))
            value = m_out.purifyNaN(value);

        setJSValue(boxDouble(value));
        return;
    }
```

`boxDouble(PNaN)` produces a JSValue encoding of NaN — the number `NaN` — not the `undefined` value that the spec requires for a hole. The `purifyNaN` call does not help here because PNaN is already the canonical NaN; the fundamental problem is that a raw-double hole sentinel is being used in a JSValue context.

**OSR Exit Materialization Path:**

When the array escapes and must be materialized during OSR exit, the sunk Phi value (PNaN, in `DataFormatDouble`) is first boxed by the exit compiler:

```cpp
// Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:91-98
case DataFormatDouble: {
    jit.moveDoubleTo64(FPRInfo::fpRegT0, scratch1);
    jit.move64ToDouble(value, FPRInfo::fpRegT0);
    jit.purifyNaN(FPRInfo::fpRegT0, FPRInfo::fpRegT0);
    jit.boxDouble(FPRInfo::fpRegT0, value);
    jit.move64ToDouble(scratch1, FPRInfo::fpRegT0);
    break;
}
```

`purifyNaN(PNaN) == PNaN`, so `boxDouble` still produces a NaN JSValue. Then `operationPopulateObjectInOSR` stores this boxed NaN via `putDirectIndex`:

```cpp
// Source/JavaScriptCore/ftl/FTLOperations.cpp:93-107
for (unsigned i = materialization->properties().size(); i--;) {
    const ExitPropertyValue& property = materialization->properties()[i];
    if (property.location().kind() != ArrayIndexedPropertyPLoc)
        continue;

    JSValue value = JSValue::decode(values[i]);
    unsigned index = property.location().info();

    array->putDirectIndex(globalObject, index, value);
    scope.assertNoExceptionExceptTermination();
}

// This might be unnecessary because operationMaterializeObjectInOSR does DeferGCForAWhile but its better to be safe.
if (hasContiguous(materialization->indexingType()))
    vm.writeBarrier(array);
```

Inside `setIndexQuickly`, the NaN JSValue is a number (`v.isNumber()` is true), so it passes the first guard. Then the NaN check `value != value` is true and triggers `convertDoubleToContiguousWhilePerformingSetIndex`:

```cpp
// Source/JavaScriptCore/runtime/JSObject.h:511-525
case ALL_DOUBLE_INDEXING_TYPES: {
    ASSERT(i < butterfly->vectorLength());
    if (!v.isNumber()) {
        convertDoubleToContiguousWhilePerformingSetIndex(vm, i, v);
        return;
    }
    double value = v.asNumber();
    if (value != value) {  // true for NaN
        convertDoubleToContiguousWhilePerformingSetIndex(vm, i, v);
        return;
    }
    butterfly->contiguousDouble().at(this, i) = value;
    if (i >= butterfly->publicLength())
        butterfly->setPublicLength(i + 1);
    break;
}
```

This converts the array from Double to Contiguous indexing and stores NaN as a JSValue. The subsequent write barrier check at line 106 uses `materialization->indexingType()` (the stale Double type recorded at compile time), so `hasContiguous()` returns false and the write barrier is **not** issued for the now-Contiguous array.

### Trigger Conditions

1. A `new Array(n)` with double values must be created in a hot loop, making it a candidate for the DFG Object Allocation Sinking phase.
2. At least one array index must have a conditional write (e.g., `if (flag) arr[1] = 2.2`), creating a Phi between the written double value and PNaN (the hole default).
3. The code must reach FTL compilation tier (requires approximately 100,000+ iterations in a release build).
4. A cold code path must read from the conditionally-written slot on the iteration where the write did NOT happen, causing the Phi to resolve to PNaN.
5. The `useArrayAllocationSinking` option must be enabled (it is by default).

## Version

### Reproduced Version

- `main` branch latest commit (2026/04/18): `a4390137a403`

## Reproduction Case

The PoC runs the same array operation twice — once interpreted and once after FTL compilation with allocation sinking — to make the discrepancy immediately visible.

### PoC

```bash
jsc poc.js          # release build
# jsc poc.js with loop bound 900000 for debug builds
```

```javascript
// Unoptimized
function baseline() {
    let arr = new Array(4);
    arr[0] = 1.1;
    arr[2] = 3.3;
    return [arr[1], 1 in arr];
}
const [baseV, baseHas] = baseline();
print("interpreter: arr[1] =", baseV, "| 1 in arr =", baseHas);

// FTL with allocation sinking — even iterations leave arr[1] as a hole
// (adjust loop bound to 900000 for debug builds)
let ftlV, ftlHas;
for (let i = 0; i < 500000; i++) {
    let arr = new Array(4);
    arr[0] = 1.1;
    arr[2] = 3.3;
    if (i & 1) arr[1] = 2.2;
    arr[0] = i + 0.5;
    if (i === 499410) { ftlV = arr[1]; ftlHas = 1 in arr; break; }
}
print("optimized:   arr[1] =", ftlV, "| 1 in arr =", ftlHas);
```

Result:

```
interpreter: arr[1] = undefined | 1 in arr = false
optimized:   arr[1] = NaN | 1 in arr = true
```

## Suggested Patch

### Diff

```diff
diff --git a/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp b/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
index bf59f3ddfbe1..8168061a2335 100644
--- a/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
@@ -2018,11 +2018,6 @@ escapeChildren:
             // if the slot was never written to.
             if (location.kind() == ArrayIndexedPropertyPLoc) {
                 ASSERT(location.base()->op() == NewArrayWithButterfly);
-                if (hasDouble(location.base()->indexingType())) {
-                    if (!m_PNaN)
-                        m_PNaN = m_insertionSet.insertConstant(0, m_graph.block(0)->at(0)->origin, jsNumber(PNaN), DoubleConstant);
-                    return m_PNaN;
-                }
                 if (!m_empty)
                     m_empty = m_insertionSet.insertConstant(0, m_graph.block(0)->at(0)->origin, JSValue());
                 return m_empty;
@@ -2136,11 +2131,7 @@ escapeChildren:
 
                 // It shouldn't be possible to get a butterfly here since it should also be a sink candidate.
                 ASSERT(location.kind() != ArrayButterflyPLoc);
-                if (location.kind() == ArrayIndexedPropertyPLoc && hasDouble(allocation.indexingType())) {
-                    ASSERT(allocation.kind() == Allocation::Kind::Array);
-                    phiNode->mergeFlags(NodeResultDouble);
-                } else
-                    phiNode->mergeFlags(NodeResultJS);
+                phiNode->mergeFlags(NodeResultJS);
                 return phiNode;
             });
 
@@ -3057,7 +3048,6 @@ escapeChildren:
 
     Node* m_bottom { nullptr };
     Node* m_empty { nullptr };
-    Node* m_PNaN { nullptr };
 };
 
 }
diff --git a/Source/JavaScriptCore/ftl/FTLOperations.cpp b/Source/JavaScriptCore/ftl/FTLOperations.cpp
index 3a1e27fd4ccf..03a28b468d9d 100644
--- a/Source/JavaScriptCore/ftl/FTLOperations.cpp
+++ b/Source/JavaScriptCore/ftl/FTLOperations.cpp
@@ -103,7 +103,7 @@ JSC_DEFINE_NOEXCEPT_JIT_OPERATION(operationPopulateObjectInOSR, void, (JSGlobalO
         }
 
         // This might be unnecessary because operationMaterializeObjectInOSR does DeferGCForAWhile but its better to be safe.
-        if (hasContiguous(materialization->indexingType()))
+        if (hasContiguous(array->indexingType()))
             vm.writeBarrier(array);
         break;
     }
```

### Explanation

**Hunk 1 — `defaultFor()` in `DFGObjectAllocationSinkingPhase.cpp`:** Removes the special `hasDouble` branch that inserts `DoubleConstant(PNaN)` as the default for unwritten double array slots. After the fix, double arrays use the same `JSValue()` (empty) default that non-double arrays already use. The empty JSValue is a genuine absent-element sentinel: when the Phi resolves to it on the unwritten path, JSValue context reads correctly return `undefined` and `HasProperty` correctly returns `false`.

**Hunk 2 — Phi result type in `DFGObjectAllocationSinkingPhase.cpp`:** Removes the `NodeResultDouble` flag for `ArrayIndexedPropertyPLoc` Phis in double arrays. With the empty JSValue as the default, the Phi now merges a double value (written path) with a non-double value (unwritten path), so it must be `NodeResultJS`. The written double will be boxed before entering the Phi, which is the correct representation.

**Hunk 3 — Remove `m_PNaN` member:** The `m_PNaN` field is now unused and can be removed.

**Hunk 4 — Write barrier in `FTLOperations.cpp`:** After `putDirectIndex` stores NaN into a Double array it can trigger `convertDoubleToContiguousWhilePerformingSetIndex`, changing the array's indexing type to Contiguous. The subsequent write barrier check must query the array's **current** indexing type rather than the stale type recorded at compile time.

Note: Hunk 4 addresses a secondary consequence of the primary bug. If Hunk 1–3 are applied, PNaN is never stored as a NaN JSValue, so the type conversion no longer occurs. Hunk 4 is a defense-in-depth fix for any future case where a NaN JSValue reaches `putDirectIndex` on a materialized Double array.

### Credit Information

Reporter credit: Junyoung Park (@candymate) of KAIST Hacking Lab

[Radar WebKit Bug Importer]

<rdar://problem/175064220>

[Shu-yu Guo]

This is a correctness bug.

[Shu-yu Guo]

Pull request: https://github.com/WebKit/WebKit/pull/63170

[EWS]

Committed 311962@main (289a55e3913b): <https://commits.webkit.org/311962@main>

Reviewed commits have been landed. Closing PR #63170 and removing active labels.