JSObject::getPropertySlot: reload the structure before the prototype step - #390
JSObject::getPropertySlot: reload the structure before the prototype step#390robobun wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. Walkthrough
ChangesProperty lookup
Merge Risk: ⚪ Minimal · up to This localized fix reloads object structure after lazy-property reification, avoiding stale-structure access in the prototype lookup path; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs — the reload matches the established pattern in JITOperations.cpp and fixes the stale-structure assert. Deferring to a human because this touches the ALWAYS_INLINE prototype-chain loop that runs on essentially every property lookup, so the extra structureID().decode() per hop is worth a second pair of eyes on perf.
What was reviewed:
- Confirmed 4 identical "reload after getOwnNonIndexPropertySlot" sites in JITOperations.cpp (lines 389/730/920/3777) — this is the same fix.
- Confirmed
getNonIndexPropertySlotavoids the issue by re-reading the structure at loop top and usinggetPrototypeDirect(). - Verified the assert at StructureInlinesLight.h:56 is exactly
object->structure() == this, matching the described failure.
Extended reasoning...
Overview
Single-line fix to JSObject::getPropertySlot in Source/JavaScriptCore/runtime/JSObject.h: after getOwnNonIndexPropertySlot returns false, reload structure = object->structureID().decode() before calling structure->storedPrototype(object). The rest of the diff is a comment explaining why.
Correctness
The bug is real and well-explained: getOwnNonIndexPropertySlot → getOwnStaticPropertySlot can reify a lazy static-table property, transitioning the object to a new Structure, and still return false if the PropertyCallback throws. The old code then passed the stale structure to storedPrototype(object), which asserts object->structure() == this (StructureInlinesLight.h:56). I verified that JITOperations.cpp already applies exactly this reload in four places with the comment "Reload it again since static-class-table can cause transition", and that the sibling getNonIndexPropertySlot in JSObjectInlines.h re-reads the structure each iteration and uses getPrototypeDirect(), so it never held a stale pointer. The fix is the minimal, idiomatic version of the same defense.
Security risks
None introduced. If anything this closes a stale-pointer read in release builds (reading the prototype off a superseded Structure). No new attacker-controlled inputs, no new allocations.
Level of scrutiny
High. getPropertySlot is one of the hottest paths in JSC — it is ALWAYS_INLINE and executes for every non-cached property lookup that walks the prototype chain. Adding a second structureID().decode() per loop iteration is cheap (a load + mask, same as the one at the top of the loop), and the compiler may even be able to keep it in a register across the non-throwing common case, but any change to this loop deserves a human sign-off on the perf side and a decision on whether to upstream it to WebKit.
Other factors
No prior human reviews on the PR; only a coderabbit rate-limit notice. The bug hunting system found no issues. The PR description includes a deterministic repro and a Fuzzilli fingerprint. The change is already the current HEAD of the checkout (c3a156e), so it has presumably been building.
Preview Builds
|
c3a156e to
01c983d
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The reload matches the established pattern in JITOperations.cpp and getNonIndexPropertySlot. Because getPropertySlot is the hottest property-lookup path in JSC and this adds a decode per prototype step, a quick human sign-off on the perf/correctness trade-off would still be worthwhile.
What was reviewed:
- Confirmed the 4 identical
structure = object->structure()reloads in JITOperations.cpp:389/730/920/3777 that this mirrors. - Confirmed
getNonIndexPropertySlotalready re-reads viagetPrototypeDirect(), so this brings parity rather than diverging. - Checked that
storedPrototype(object)is the only downstream use ofstructureafter the call — the reload covers it fully.
Extended reasoning...
Overview
Single-line functional change (plus a 3-line comment) in JSObject::getPropertySlot (JSObject.h): after getOwnNonIndexPropertySlot returns false, re-read structure = object->structureID().decode() before calling structure->storedPrototype(object). This fixes a stale-structure read when a static-table lazy property's PropertyCallback reifies (transitioning the object) and then throws, causing getOwnNonIndexPropertySlot to return false with the object already transitioned. Fuzzilli found the resulting ASSERT(object->structure() == this) at StructureInlinesLight.h:56.
Security risks
None introduced. The change replaces a potentially-stale pointer read with a fresh one, which is strictly safer. No new user-controlled inputs, no allocation, no GC interaction beyond what already existed.
Level of scrutiny
High. JSObject::getPropertySlot is ALWAYS_INLINE and per the in-file comment is "very hot in variable lookup" — it's on the critical path for essentially every non-cached property access in the interpreter and JIT slow paths. Even a one-line change here can affect engine-wide performance. The extra structureID().decode() per prototype-chain step is cheap (one load + mask/lookup) and mirrors what the sibling getNonIndexPropertySlot already pays via getPrototypeDirect(), so it's very likely fine — but that's a judgment a human maintainer should make explicitly, not an automated reviewer.
Other factors
- The pattern is well-precedented: JITOperations.cpp has four identical post-
getOwnNonIndexPropertySlotreloads with the comment "Reload it again since static-class-table can cause transition", andgetNonIndexPropertySlotre-reads the structure viagetPrototypeDirect(). This PR closes the one remaining caller that reused the pre-call pointer. - No test added in-tree; the repro is Bun-specific (relies on the
Bunobject's lazy static properties). That's reasonable for this fork but worth a human ack. - No prior human or bot reviews on the thread other than a rate-limited CodeRabbit notice and a preview-build comment.
Given the criticality of the code path, deferring to a human for final sign-off rather than auto-approving.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
01c983d to
1394aa1
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The one-line reload matches the established pattern in JITOperations.cpp (four identical structure = object->structure() reloads after getOwnNonIndexPropertySlot) and getNonIndexPropertySlot (which re-reads via getPrototypeDirect()), and it resolves the ASSERT(object->structure() == this) at StructureInlinesLight.h:56. Because JSObject::getPropertySlot is the ALWAYS_INLINE hot path for property lookup in core JSC, a human sign-off is still worthwhile.
Checked that the reload uses the same object->structureID().decode() idiom as the load at the top of the loop, so no new failure mode is introduced.
Confirmed getNonIndexPropertySlot (the other branch of this loop) already re-reads the structure via getPrototypeDirect(), so it does not need the same fix.
Verified the checkNullStructure guard is not needed on the reload — the object had a valid structure two lines earlier and transitions never install a null one.
Extended reasoning...
Overview
This PR adds a single line to JSObject::getPropertySlot in Source/JavaScriptCore/runtime/JSObject.h: after getOwnNonIndexPropertySlot returns false, it reloads structure = object->structureID().decode() before calling structure->storedPrototype(object). The rationale is that getOwnNonIndexPropertySlot → getOwnStaticPropertySlot can reify a lazy static-table property, transitioning the object to a new Structure, and still return false if the PropertyCallback builder throws. The old code then passed the stale Structure pointer to storedPrototype(object), which asserts object->structure() == this (StructureInlinesLight.h:56).
Correctness
The fix is clearly correct and follows established precedent exactly. Four call sites in JITOperations.cpp (lines ~389, 730, 920, 3777) perform the identical reload with the comment "Reload it again since static-class-table can cause transition". The sibling getNonIndexPropertySlot in JSObjectInlines.h avoids the bug by calling object->getPrototypeDirect(), which reads structure() fresh. This PR brings the last remaining caller in line. The reload is strictly more defensive than the prior code — there is no scenario where re-reading the object's current structure produces a worse result than using a possibly-stale one. I also confirmed the checkNullStructure template guard need not be repeated: the object had a non-null structure immediately prior and structure transitions never install null.
Security risks
None introduced by this change. If anything, it removes a latent hazard: in release builds the old code read the prototype off a stale Structure, which for mono-proto structures would still return the same stored prototype (structure transitions preserve it), and for poly-proto reads object->getDirect(knownPolyProtoOffset) which doesn't depend on the stale pointer — so the practical release-build impact was likely benign, but the assert crash in debug/asserts builds was real and reproducible.
Level of scrutiny
High. JSObject::getPropertySlot is an ALWAYS_INLINE function on the hottest property-lookup path in JavaScriptCore — the header comment itself notes it is "very hot in variable lookup". Any change here affects every non-overridden property access in the engine and is inlined into many translation units. Even a one-line change on this path warrants a human maintainer's eyes, particularly regarding the (likely negligible) perf cost of one extra structureID().decode() per prototype-chain hop.
Other factors
The PR description is thorough, includes a deterministic Bun repro, and cites the exact assert location and the fuzzer fingerprint. The change is already merged to main (commit 1394aa1). No test is added in-tree, but the repro is Bun-specific (relies on the Bun global's lazy properties). No outstanding reviewer comments. Given the criticality of the code path, I'm deferring rather than auto-approving, though I have high confidence the change is correct.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
1394aa1 to
0c51423
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — the reload matches the established pattern in JITOperations.cpp and fixes the stale-structure assert. Because it adds a structure re-decode on every iteration of the hottest property-lookup loop in JSC, a human sign-off on the perf/correctness tradeoff is still worthwhile.
Checked that storedPrototype(object) at StructureInlinesLight.h:56 asserts object->structure() == this, which the stale pointer would violate.
Confirmed JITOperations.cpp reloads the structure after getOwnNonIndexPropertySlot in four places for the same reason.
Verified getNonIndexPropertySlot (the other prototype-walk path) already re-reads via getPrototypeDirect(), so this was the one remaining stale caller.
Extended reasoning...
Overview
This PR adds a single line (plus a comment) to JSObject::getPropertySlot in Source/JavaScriptCore/runtime/JSObject.h. After getOwnNonIndexPropertySlot returns false, it reloads structure = object->structureID().decode() before calling structure->storedPrototype(object) to advance the prototype-chain walk. The rationale is that reifying a static-table lazy property inside getOwnNonIndexPropertySlot can transition the object and still return false (when the PropertyCallback builder throws), leaving the local structure pointer stale. The stale pointer trips ASSERT(object->structure() == this) at StructureInlinesLight.h:56 in debug builds and reads the prototype off the wrong structure in release builds.
Security risks
None introduced by this change. The fix is strictly defensive — it re-reads fresh state instead of trusting a cached pointer across a call that can mutate the object. If anything, it closes a potential type-confusion / stale-read hazard in release builds where the prototype was being read off an outdated structure.
Level of scrutiny
High. JSObject::getPropertySlot is one of the hottest functions in JavaScriptCore — the header itself notes it is ALWAYS_INLINE because it "makes a big difference since this is function very hot in variable lookup." Every non-overridden property access on every object flows through this loop. Adding a structureID().decode() on each prototype-chain step is almost certainly cheap (and matches what JITOperations.cpp already does four times over), but any change on this path deserves a JSC maintainer's eye for perf implications and to confirm there is no cheaper placement (e.g., only reloading when hasStaticPropertyTable was true).
Other factors
- The fix follows clear precedent: JITOperations.cpp reloads the structure after
getOwnNonIndexPropertySlotin four separate places with the comment "Reload it again since static-class-table can cause transition", andgetNonIndexPropertySlotalready avoids the issue viagetPrototypeDirect(). - Found by Fuzzilli with a deterministic repro provided in the description, so the failure mode is well-understood.
- No test added in this repo (the repro depends on Bun-specific lazy properties on the
Bunobject). - The change is semantically correct and minimal; my hesitation to auto-approve is purely about the criticality of the code path, not about doubts in the fix itself.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
0c51423 to
37d85e8
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The one-line reload mirrors the established pattern in JITOperations.cpp and is clearly correct, but since it sits in JSObject::getPropertySlot — the ALWAYS_INLINE hot path for every property lookup in the VM — a human sign-off on the extra structureID().decode() per prototype step would still be worthwhile.
What was reviewed:
- Confirmed the reload matches the four identical "Reload it again since static-class-table can cause transition" sites in
JITOperations.cppand thatgetNonIndexPropertySlotalready avoids the stale pointer viagetPrototypeDirect(). - Checked that
storedPrototype(object)on the reloaded structure is safe (the assert it fixes is exactlyobject->structure() == this). - Verified the added decode is the same cheap load already done at the top of each loop iteration, so no new failure modes are introduced.
Extended reasoning...
Overview
This PR adds a single line (plus a three-line comment) to JSObject::getPropertySlot in Source/JavaScriptCore/runtime/JSObject.h. After getOwnNonIndexPropertySlot returns false, the loop now reloads structure = object->structureID().decode() before calling structure->storedPrototype(object). This fixes a Fuzzilli-discovered assertion at StructureInlinesLight.h:56 where reifying a lazy static property transitions the object but the builder throws, leaving the loop holding a stale Structure* when it steps to the prototype.
Security risks
None introduced by the change itself — it is strictly a defensive re-read of state the object already owns. The bug it fixes is arguably the security-relevant part: in release builds the old code read the prototype off a stale structure, which is the kind of type-confusion primitive fuzzers look for. The fix closes that, and the reload cannot make things worse than re-reading the current structure.
Level of scrutiny
High. getPropertySlot is ALWAYS_INLINE and on the hottest path in the engine — every non-indexed property access that walks the prototype chain goes through it. Even a one-line change here deserves a human look for perf implications (an extra structureID().decode() per prototype hop) and to confirm there is no subtler interaction with the checkNullStructure / debugLLIntGetById template variants. The correctness argument is strong and the pattern is copied verbatim from four sites in JITOperations.cpp, so the review should be quick, but it should be a human's call.
Other factors
- The PR description is thorough, with a deterministic repro and an explanation of both routes to the assert.
getNonIndexPropertySlot(the sibling loop this delegates to whenoverridesGetOwnPropertySlotis set) already avoids the bug by callingobject->getPrototypeDirect(), which re-reads the structure — so this change brings the two loops into agreement rather than introducing a novel approach.- No test is added; the repro depends on Bun-specific lazy builders (
Bun.sql/Bun.semver), so a JSC-only regression test would be hard to write. - Preview builds succeeded across four commits, indicating it compiles cleanly on all CI targets.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
37d85e8 to
8cb37ab
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The reload matches the four identical structure = object->structure() reloads after getOwnNonIndexPropertySlot in JITOperations.cpp and resolves the object->structure() == this assert in Structure::storedPrototype. Since getPropertySlot is the ALWAYS_INLINE prototype-chain fast path for essentially every uncached property read, a human look at the extra per-iteration structure decode would still be worthwhile.
Checked: the sibling loop getNonIndexPropertySlot already re-reads via object->getPrototypeDirect() so it isn't affected; the checkNullStructure guard sits before the call so the reloaded value going straight into storedPrototype matches the original null-handling; and the four JITOperations.cpp callers already reload, so this was the last stale-pointer holder.
Extended reasoning...
Overview
This PR adds a single line to JSObject::getPropertySlot in Source/JavaScriptCore/runtime/JSObject.h: after getOwnNonIndexPropertySlot returns false, it reloads structure = object->structureID().decode() before calling structure->storedPrototype(object). The stated reason is that reifying a static-table lazy property inside getOwnNonIndexPropertySlot can transition the object and still return false (when the PropertyCallback builder throws), leaving the local structure stale and tripping ASSERT(object->structure() == this) at StructureInlinesLight.h:56.
I verified the precedent: JITOperations.cpp performs this exact reload in four separate megamorphic slow-path loops (lines 389, 730, 920, 3777) with the comment "Reload it again since static-class-table can cause transition". I also confirmed getNonIndexPropertySlot in JSObjectInlines.h reads the prototype via object->getPrototypeDirect(), which re-fetches the structure, so it never had this issue. This PR brings the one remaining caller in line.
Security risks
None identified. The change reads the object's own current structure ID rather than a cached local — strictly more up-to-date data on the same object. It cannot introduce a type confusion or OOB; if anything it removes a stale-structure read that in release builds would fetch the prototype off the pre-transition structure.
Level of scrutiny
High. JSObject::getPropertySlot is ALWAYS_INLINE and sits on the generic property-lookup path taken by LLInt/baseline slow paths and C++ runtime callers for every uncached obj.prop read. Even a one-line addition here executes across the entire prototype chain for a large fraction of property accesses. The fix itself is mechanically obvious and mirrors established engine practice, but the location is performance-critical enough that it doesn't fit the "simple, mechanical, obvious" bar for auto-approval — a human familiar with JSC perf characteristics should confirm the extra decode per iteration is acceptable (it almost certainly is: same field just loaded, hot in cache, and JITOperations already does it).
Other factors
The PR description is unusually thorough — it includes the fuzzer fingerprint, a deterministic Bun repro, and a second route to the same failure. No test is added in this repo (the repro depends on Bun's lazy Bun.sql/Bun.semver builders). No prior human review comments are on the thread. Given the hot-path location I'm deferring rather than approving, but I see nothing wrong with the change.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
8cb37ab to
686a757
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The reload matches the established pattern in JITOperations.cpp and is clearly correct, but since getPropertySlot is the ALWAYS_INLINE hot path for every prototype-chain property lookup in JSC, a human sign-off on adding a load here would still be worthwhile.
Checked that structureID().decode() is the same cheap re-read used at the top of the loop, and that Structure::storedPrototype(object) at StructureInlinesLight.h:56 is the assert this fixes.
Confirmed the four JITOperations.cpp sites already reload with the cited comment, and getNonIndexPropertySlot re-reads the structure each iteration, so this was the one remaining stale-pointer caller.
Verified the reload cannot change behavior in the no-transition case — it just re-derives the same pointer.
Extended reasoning...
Overview
This PR adds a single-line structure reload in JSObject::getPropertySlot (Source/JavaScriptCore/runtime/JSObject.h) between the getOwnNonIndexPropertySlot miss and the structure->storedPrototype(object) prototype step, plus a three-line comment. The bug: getOwnNonIndexPropertySlot can reify a static-table lazy property (transitioning the object) and still return false when the PropertyCallback builder throws, leaving the loop holding a stale Structure* that trips ASSERT(object->structure() == this) at StructureInlinesLight.h:56 in debug and reads the prototype off the old structure in release.
Security risks
None. This is a defensive pointer refresh; it can only make the code more correct. There is no new attacker-controlled input, no allocation, no bounds change. The pre-fix release-build behavior (reading the prototype off the stale structure) was the riskier state.
Level of scrutiny
High — not because the change is complex, but because JSObject::getPropertySlot is ALWAYS_INLINE and, per the surrounding comment, "very hot in variable lookup". Every prototype-chain miss in the interpreter goes through this line. Adding even one load here is the kind of thing JSC maintainers historically weigh carefully. The PR description cites the four identical reloads already present in JITOperations.cpp ("Reload it again since static-class-table can cause transition"), which I confirmed, so there is direct precedent that this cost is acceptable — but a human should ratify applying that precedent to the hottest generic path.
Other factors
The fix is trivially correct: object->structureID().decode() re-reads a field already hot in cache and is identical to the load at the top of the loop. getNonIndexPropertySlot in JSObjectInlines.h already re-reads the structure each iteration, so this brings getPropertySlot in line with its sibling. The PR includes a deterministic repro and a well-argued root cause. The bug-hunting system found nothing. I am deferring solely on the "touches critical code paths" criterion, not on any correctness concern.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
686a757 to
9add1ce
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The one-line structure reload mirrors the established pattern in JITOperations.cpp ("Reload it again since static-class-table can cause transition") and fixes the ASSERT(object->structure() == this) trip at StructureInlinesLight.h:56. Because it sits on JSObject::getPropertySlot — the hot inlined prototype-walk path used by essentially every property miss — a human look is still worthwhile, mainly to confirm the extra structureID().decode() per prototype hop is acceptable.
What was reviewed:
- Confirmed the reload matches the four identical reloads in
JITOperations.cpp:389/730/920/3777and thatgetNonIndexPropertySlotalready re-reads viagetPrototypeDirect(). - Confirmed
Structure::storedPrototype(object)assertsobject->structure() == this, so the stale-pointer path described in the PR is real. - Checked that the reload only affects the local
structureused forstoredPrototype(); no other consumer of the pre-reload value remains in the loop body.
Extended reasoning...
Overview
The PR adds a single line (plus a comment) to JSObject::getPropertySlot in Source/JavaScriptCore/runtime/JSObject.h: after getOwnNonIndexPropertySlot returns false, it reloads structure = object->structureID().decode() before calling structure->storedPrototype(object). The rationale is that getOwnNonIndexPropertySlot → getOwnStaticPropertySlot can reify a lazy static-table property (transitioning the object's structure) and still return false when the PropertyCallback builder throws, leaving the loop holding a stale Structure* and tripping ASSERT(object->structure() == this) at StructureInlinesLight.h:56 (or, in release, reading the prototype off the stale structure).
I verified the claim: JITOperations.cpp already performs this exact reload in four megamorphic slow paths with the comment "Reload it again since static-class-table can cause transition", and the sibling getNonIndexPropertySlot in JSObjectInlines.h avoids the problem by calling object->getPrototypeDirect(), which re-reads the structure. So this change brings the one remaining caller in line with existing practice.
Security risks
None introduced. The change strictly refreshes a local pointer to the current structure before dereferencing it; it cannot expose new state or bypass any check. If anything it closes a (narrow) correctness gap where a stale mono-proto structure could be consulted after a transition.
Level of scrutiny
High. JSObject::getPropertySlot is ALWAYS_INLINE and sits on the hottest generic property-lookup path in JavaScriptCore — every prototype-chain miss in interpreted/baseline code flows through here. Even a one-line change here is inlined into a very large number of call sites and executed on every prototype hop. The change itself is mechanically simple and clearly follows the established JITOperations.cpp pattern, but the location is core-runtime-critical, which is why I'm deferring rather than auto-approving.
Other factors
- The fix is backed by a Fuzzilli finding with a deterministic repro described in the PR body, and the assert it trips (
StructureInlinesLight.h:56) matches exactly. - The added
structureID().decode()is cheap (a load + StructureID decode), and JITOperations.cpp already pays it on the equivalent paths, so perf impact should be negligible — but that judgment is better confirmed by a human familiar with JSC micro-benchmarks. - No behavior change on the common path where
getOwnNonIndexPropertySlotdoesn't transition: the reload returns the same structure. - No prior automated review from this bot on the PR; CodeRabbit had no actionable comments.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
9add1ce to
d0e68f3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The reload matches the established pattern in JITOperations.cpp and fixes a real stale-structure read. Because it adds a structure decode on every prototype-chain step inside the ALWAYS_INLINE getPropertySlot — one of the hottest paths in JSC — a human sign-off on the perf trade-off would still be worthwhile.
What was reviewed:
- Confirmed the four megamorphic slow paths in
JITOperations.cppalready do exactly this reload with the same rationale. - Confirmed
getNonIndexPropertySlotavoids the issue viagetPrototypeDirect(), so this was the one remaining caller reusing the pre-call structure. - Checked that the reloaded structure cannot be null (transition only), so the earlier
checkNullStructureguard need not be repeated.
Extended reasoning...
Overview
This PR adds a single functional line (plus a 3-line comment) to JSObject::getPropertySlot in Source/JavaScriptCore/runtime/JSObject.h. After getOwnNonIndexPropertySlot returns false, it reloads structure = object->structureID().decode() before calling structure->storedPrototype(object). The motivation is that getOwnNonIndexPropertySlot can reify a static-table lazy property (transitioning the object) and still return false when the PropertyCallback builder throws, leaving the local structure pointer stale. That trips ASSERT(object->structure() == this) at StructureInlinesLight.h:56 in debug builds and reads the prototype off the stale structure in release.
Security risks
None introduced. The change makes the code strictly more correct: reading the object's current structure is always at least as safe as reading a stale one. If anything, it closes a (narrow) window where release builds could walk the prototype chain via a stale structure's stored prototype.
Level of scrutiny
High. JSObject::getPropertySlot is ALWAYS_INLINE and sits on the hottest property-lookup path in the engine — the header comment itself notes it is "function very hot in variable lookup". Adding an extra structureID().decode() per prototype-chain iteration is almost certainly negligible (the same field was decoded a few instructions earlier and will be hot in cache), and four call sites in JITOperations.cpp already do the identical reload with the comment "Reload it again since static-class-table can cause transition". Still, for a change to this specific function, a human familiar with JSC perf characteristics should confirm the trade-off is acceptable rather than, say, gating the reload on TypeInfo::hasStaticPropertyTable.
Other factors
- The fix follows an established pattern with no ambiguity — I verified the four identical reloads in
JITOperations.cpp:389/730/920/3777and thatgetNonIndexPropertySlotinJSObjectInlines.hsidesteps the issue by usingobject->getPrototypeDirect()(which re-reads the structure). - The reload happens after the
checkNullStructureblock; sincegetOwnNonIndexPropertySlotcan only transition the structure (never null it), no repeat null check is needed. - The PR description includes a deterministic Bun repro and Fuzzilli fingerprint, and preview builds have been produced across multiple revisions.
- No prior human review on the timeline; only bot comments.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
…step getOwnNonIndexPropertySlot can reify a static-table lazy property, which transitions the object, and still return false when the PropertyCallback builder throws (setUpStaticFunctionSlot reports the slot as not found so the caller sees the exception). The inlined prototype-chain loop then called storedPrototype(object) on the structure it read before the call, tripping ASSERT(object->structure() == this) in StructureInlinesLight.h on asserts builds. The megamorphic slow paths in JITOperations.cpp already reload the structure after getOwnNonIndexPropertySlot for exactly this reason, and getNonIndexPropertySlot uses getPrototypeDirect() which re-reads it. Do the same in the one caller that reused the stale pointer. Seen by Fuzzilli in Bun: first read of a lazy Bun property whose builder runs JS that touches another lazy property on the same object and then throws, e.g. reading Bun.sql with a hooked Error superclass.
d0e68f3 to
5b4dfb3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM — one-line structure reload matching the established JITOperations.cpp pattern.
What was reviewed:
- Verified the four
structure = object->structure()reloads inJITOperations.cppand thatgetNonIndexPropertySlotusesgetPrototypeDirect()(re-reads structure), so this was the one caller reusing a stale pointer. - Confirmed the assert at
StructureInlinesLight.h:56isobject->structure() == this, which the reload satisfies. - The reload uses
object->structureID().decode(), identical to howstructureis loaded at the top of the loop;structureisn't used afterstoredPrototype()in the iteration, so scoping is correct.
Extended reasoning...
Overview
Single functional line added to JSObject::getPropertySlot in Source/JavaScriptCore/runtime/JSObject.h: after getOwnNonIndexPropertySlot returns false, reload structure = object->structureID().decode() before calling structure->storedPrototype(object). Plus a three-line explanatory comment.
Security risks
None introduced. The change strictly narrows a correctness gap — reading the prototype off a stale Structure* after the object transitioned. In release builds the old code could read the wrong prototype (mono/poly-proto decision made on a stale structure); the reload can only make this read more correct, never less. No new inputs, no attacker-controlled data flow changes.
Level of scrutiny
This is a hot inlined path in the core property-lookup machinery, which normally warrants care. However, the change is mechanical and pattern-matches four identical reloads already present in JITOperations.cpp ("Reload it again since static-class-table can cause transition") and mirrors how the sibling getNonIndexPropertySlot in JSObjectInlines.h avoids the issue via getPrototypeDirect(). A single extra structureID().decode() per prototype step is a field load + shift — negligible next to the existing structure->get() table lookup done each iteration. The reload uses the exact same expression as the initial load a few lines above, so there is no new failure mode.
Other factors
The PR is backed by a Fuzzilli-found assertion failure with a deterministic repro and a clear root-cause analysis (reify → transition → builder throws → setUpStaticFunctionSlot reports miss → stale-structure prototype read). Preview builds succeeded across multiple revisions. No prior human reviewer comments are outstanding, and no bugs were flagged by the bug-hunting pass. The checkNullStructure template guard is unaffected — a live object's structureID() is always valid, and structure is not reused after the storedPrototype() call within the iteration.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
|
Rebased onto Status on the bun side: the Bun-only half of the original problem (a debug-only exception report in the sql builders) landed separately in oven-sh/bun#29642, so oven-sh/bun#37001 now carries just this change's pin and one regression test. That test still aborts with the |
|
One more data point for review, on whether this reload is the only one needed. Using a builder that re-enters the object (reifies another static property, so the object transitions) and then throws, I went through the ways of reaching the lazy property on a bun debug build of plain |
1 participant
getOwnNonIndexPropertySlot can reify a static-table lazy property, which transitions the object, and still return false when the PropertyCallback builder throws (setUpStaticFunctionSlot reports the slot as not found so the caller observes the exception). The inlined prototype-chain loop in JSObject.h then called storedPrototype(object) on the structure it read before the call, tripping ASSERT(object->structure() == this) at StructureInlinesLight.h:56 on asserts builds. Release builds read the prototype off the stale structure instead.
The megamorphic slow paths in JITOperations.cpp already reload the structure after getOwnNonIndexPropertySlot with the comment "Reload it again since static-class-table can cause transition", and getNonIndexPropertySlot uses getPrototypeDirect() which re-reads it. This applies the same reload to the one caller that reused the stale pointer.
Found by Fuzzilli in Bun (fingerprint StructureInlinesLight.h(56), flaky under REPRL). Deterministic repro in Bun: first read of a lazy static-table property on the Bun object whose builder evaluates JS that first-touches another lazy property on the same object and then throws:
The class SQLError extends Error statement inside the bun:sql internal module hits the proxy trap while Bun.sql is being reified.
There is a second route to the same line, seen while tracing other fuzzer samples: a static-table lookup performed while an unrelated exception is already pending. reifyStaticProperty adds the property (transition), then setUpStaticFunctionSlot sees vm.exceptionForInspection() and reports a miss, so the loop again reaches storedPrototype() with the structure it read before the call. Bun hit that one through a debug-only exception report inside a builder reading process._fatalException; the Bun side is removing that report, and the reload here covers both routes since they converge on the same prototype step.