Bug 312681 – Incorrect Property Read in Megamorphic Cache due to ownProperty Flag Confusion in GetByIdWithThis

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

Created attachment 479197 [details]
poc.js

## Summary

In JavaScriptCore's megamorphic property cache, the `ownProperty` flag passed to `MegamorphicCache::initAsHit` is computed as `slot.slotBase() == thisValue` at JITOperations.cpp:441. For `GetByIdWithThis` (`super.x`), `thisValue` (the receiver) and `baseObject` (the lookup-start) are always distinct, so the flag is always `false` even when the property is an own property of `baseObject`. This causes the cache to store a raw pointer to `baseObject` as the holder instead of the sentinel value, so a subsequent megamorphic `GetById` on a different object with the same `StructureID` loads the property value from the stale, wrong object. The bug only manifests under JIT (the megamorphic cache fast path is JIT-only), demonstrating a clear optimized/unoptimized discrepancy.

**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

`getByIdMegamorphic` (JITOperations.cpp:441) populates the global `MegamorphicCache` with `ownProperty = (slot.slotBase() == thisValue)`. For `GetByIdWithThis`, `thisValue` is the method receiver while `baseObject` is the lookup-start (`homeObject.[[GetPrototypeOf]]()`); these are never equal, so `ownProperty` is always `false`. When property `x` is an own property of `baseObject` (meaning `slot.slotBase() == baseObject`), the cache stores `holder = baseObject` (a raw pointer) instead of the sentinel `JSCell::seenMultipleCalleeObjects()`. A later DFG/FTL-compiled `GetById` on a different object with the same `StructureID` uses `loadMegamorphicProperty`, which finds `holder != sentinel` and loads from the stale `baseObject`, returning the wrong property value.

### Detail

The buggy call (JITOperations.cpp:441):

```cpp
// Source/JavaScriptCore/jit/JITOperations.cpp:439-441
if (cacheable && slot.isCacheableValue() && slot.cachedOffset() <= MegamorphicCache::maxOffset) [[likely]] {
    if (slot.slotBase() == baseObject || !baseObject->structure()->isDictionary())
        vm.megamorphicCache()->initAsHit(baseObject->structureID(), uid, slot.slotBase(),
            slot.cachedOffset(), slot.slotBase() == thisValue);  // BUG
```

The correct version in `getByValMegamorphic` (JITOperations.cpp:3720-3722):

```cpp
        vm.megamorphicCache()->initAsHit(baseObject->structureID(), uid, slot.slotBase(),
            slot.cachedOffset(), slot.slotBase() == baseObject);  // CORRECT
```

`MegamorphicCache::initAsHit` (MegamorphicCache.h:219) delegates to `LoadEntry::initAsHit` (line 79), which stores either the sentinel or a raw holder pointer based on the flag:

```cpp
m_holder = (ownProperty) ? JSCell::seenMultipleCalleeObjects() : holder;
```

The JIT fast path `loadMegamorphicProperty` (AssemblyHelpers.cpp:548-552) uses a conditional move: if `holder == sentinel`, load from the current `baseGPR`; otherwise load from `holder`:

```cpp
loadPtr(Address(scratch3GPR, MegamorphicCache::LoadEntry::offsetOfHolder()), scratch2GPR);
auto missed = branchTestPtr(Zero, scratch2GPR);
moveConditionally64(Equal, scratch2GPR,
    TrustedImm32(std::bit_cast<uintptr_t>(JSCell::seenMultipleCalleeObjects())),
    baseGPR, scratch2GPR, scratch1GPR);
load16(Address(scratch3GPR, MegamorphicCache::LoadEntry::offsetOfOffset()), scratch2GPR);
loadProperty(scratch1GPR, scratch2GPR, JSValueRegs { resultGPR });
```

Both `compileGetByIdMegamorphic` and `compileGetByIdWithThisMegamorphic` in DFGSpeculativeJIT64.cpp (lines 7985–8029) use the same `loadMegamorphicProperty` call with `baseGPR`, and both write to the same global `MegamorphicCache` keyed by `{StructureID, uid}`. There is no per-access-kind separation.

**For `super.x` where `x` is an own property of `baseObject`:**
- `slot.slotBase() == baseObject` → true → correct `ownProperty` would be `true`
- Bug: `slot.slotBase() == thisValue` → always `false` for normal super access
- Cache stores `holder = baseObject` (raw pointer)

**Later, `readProp(victimObj)` via DFG `compileGetByIdMegamorphic`:**
- `victimObj.structureID == baseObject.structureID` (same shape)
- Cache hit: `holder = baseObject ≠ sentinel`
- Fast path loads from `baseObject.x` instead of `victimObj.x` → wrong value

For regular `GetById`, `thisValue == baseValue == baseObject`, so the bug is invisible. It only surfaces for `GetByIdWithThis` where receiver and lookup-start are distinct.

### Trigger Conditions

1. A `super.x` access goes megamorphic (IC sees many structures).
2. `x` is an own property of the super-base (`Object.getPrototypeOf(HomeObject)`).
3. A separate `obj.x` megamorphic access (DFG/FTL) encounters an object with the same `StructureID` as the super-base.
4. The global megamorphic cache epoch matches (same GC cycle).

## Version

### Reproduced Version

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

## Reproduction Case

The bug is a JIT-only discrepancy: triggers under DFG/FTL (`jsc poc.js`), absent under the LLInt interpreter (`jsc --useJIT=false poc.js`).

### With JIT (release and debug)

```bash
jsc poc.js
```

```
FAIL: readProp(victimObj) = 0xcafe (expected 0xdead)
```

### Without JIT (`--useJIT=false`)

```bash
jsc --useJIT=false poc.js
```

```
PASS: readProp(victimObj) = 0xdead
```

No assertion failures fire in either build because the cache entry is structurally valid (`StructureID`, `uid`, `epoch` all match); only the holder semantics are wrong.

### PoC

```js
// Bug: JITOperations.cpp:441 — slot.slotBase() == thisValue should be == baseObject
// For GetByIdWithThis (super.x), thisValue != baseObject, so own properties of the
// lookup-start object are cached with a raw holder pointer instead of the sentinel,
// causing a later GetById on a different object of the same StructureID to read from
// the stale holder. Only triggers under JIT (megamorphic cache fast path).
//
// FAIL:  jsc poc.js
// PASS:  jsc --useJIT=false poc.js

function makeBecomeProto(val) {
    let obj = { x: val };
    Object.create(obj);  // BecomePrototype transition
    return obj;
}

let poisonObj = makeBecomeProto(0xcafe);
let victimObj = makeBecomeProto(0xdead);  // same StructureID as poisonObj

function readProp(obj) { return obj.x; }

// Drive readProp IC to megamorphic
for (let i = 0; i < 200; i++) { let o = { x: i }; o['w' + i] = i; Object.create(o); readProp(o); }
for (let i = 0; i < 5000; i++) readProp(makeBecomeProto(i));

// Drive super accessor IC to megamorphic
class Base { readSuper() { return super.x; } }
for (let i = 0; i < 200; i++) {
    let p = { x: i }; p['s' + i] = i; Object.create(p);
    Object.setPrototypeOf(Base.prototype, p);
    new Base().readSuper();
}

// Poison: super.x on poisonObj writes {StructureID, "x", holder=poisonObj} (wrong; should be sentinel)
gc();
Object.setPrototypeOf(Base.prototype, poisonObj);
new Base().readSuper();

// readProp(victimObj) hits the poisoned entry (same StructureID), reads from poisonObj
let result = readProp(victimObj);
print(result === 0xcafe
    ? "FAIL: readProp(victimObj) = 0x" + result.toString(16) + " (expected 0xdead)"
    : "PASS: readProp(victimObj) = 0x" + result.toString(16));
```

## Suggested Patch

```diff
--- a/Source/JavaScriptCore/jit/JITOperations.cpp
+++ b/Source/JavaScriptCore/jit/JITOperations.cpp
@@ -438,7 +438,7 @@ static ALWAYS_INLINE JSValue getByIdMegamorphic(JSGlobalObject* globalObject, VM
         if (hasProperty) {
             if (cacheable && slot.isCacheableValue() && slot.cachedOffset() <= MegamorphicCache::maxOffset) [[likely]] {
                 if (slot.slotBase() == baseObject || !baseObject->structure()->isDictionary())
-                    vm.megamorphicCache()->initAsHit(baseObject->structureID(), uid, slot.slotBase(), slot.cachedOffset(), slot.slotBase() == thisValue);
+                    vm.megamorphicCache()->initAsHit(baseObject->structureID(), uid, slot.slotBase(), slot.cachedOffset(), slot.slotBase() == baseObject);
```

Matches the correct logic already used in `getByValMegamorphic` (line 3722). Confirmed fix: applying this patch causes `jsc poc.js` to output `PASS`.

### Credit Information

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

[Radar WebKit Bug Importer]

<rdar://problem/175079685>

[Shu-yu Guo]

This is a correctness bug, not a security bug.

[Shu-yu Guo]

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

[EWS]

Committed 311692@main (9b1a02808762): <https://commits.webkit.org/311692@main>

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