Fix PropertySlot assertion by stripping static hash table attribute bits#28335
Fix PropertySlot assertion by stripping static hash table attribute bits#28335robobun wants to merge 7 commits into
Conversation
|
Updated 6:20 PM PT - Mar 25th, 2026
❌ @autofix-ci[bot], your commit b35c001 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28335That installs a local version of the PR into your bun-28335 --bun |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughReplaced direct slot attribute usage with structure-derived attributes in spy setup; changed Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/test/spyon-static-property.test.ts`:
- Around line 26-29: The test captures stdout, stderr, and exitCode but never
asserts stderr — add an assertion that stderr is empty (e.g.,
expect(stderr.trim()).toBe("") or equivalent) alongside the existing stdout and
exitCode assertions to improve failure diagnostics; apply this change for both
occurrences in the spyon-static-property.test (the blocks that destructure const
[stdout, stderr, exitCode] and currently assert stdout and exitCode).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 377ebf4e-0552-4db5-a35a-f44e488d9b3e
📒 Files selected for processing (3)
src/bun.js/bindings/JSMockFunction.cppsrc/bun.js/bindings/NodeVM.cpptest/js/bun/test/spyon-static-property.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted correctness fix matching JSC conventions.
Extended reasoning...
Overview
This PR modifies three files: JSMockFunction.cpp (3 hunks), NodeVM.cpp (2 hunks), and a new test file. All changes are narrow bug fixes.
Security risks
No security-sensitive code is touched. The changes are internal JSC property attribute handling, not auth, crypto, or permissions.
Level of scrutiny
Moderate — the changes touch JSC binding internals, but each change is a one-liner substitution. attributesForStructure() is the canonical JSC way to strip upper-bit flags before storing attributes in a PropertySlot or Structure; this is exactly what JSC's own Lookup.h does at lines 462, 488, 496, 503. The NodeVM fix of passing 0 instead of a stale slot.attributes() is correct: globalThis is a plain value property with no special attributes. Both fixes are well-motivated by the described assertion failure.
Other factors
The one bug flagged by the hunter is a pre-existing redundant inner if (hasValue) guard nested inside an if (hasValue && ...) block — harmless dead code that does not affect runtime behavior. The actual fix inside that guard is correct. Regression tests are included that exercise both the function-property spy path and the mockRestore attribute-preservation path.
| if (hasValue && ((slot.attributes() & PropertyAttribute::Function) != 0 || (value.isCell() && value.isCallable()))) { | ||
| if (hasValue) | ||
| attributes = slot.attributes(); | ||
| attributes = attributesForStructure(slot.attributes()); | ||
|
|
There was a problem hiding this comment.
🟡 The inner if (hasValue) on line 1536 of JSMockFunction.cpp is always true and is dead code - the outer if (hasValue && ...) condition already guarantees hasValue is truthy before reaching it. This is a pre-existing issue; the PR correctly updated the assignment inside that guard from slot.attributes() to attributesForStructure(slot.attributes()), but left the redundant guard in place.
Extended reasoning...
In JSMock__jsSpyOn (JSMockFunction.cpp), the spying logic for callable values contains a doubly-nested hasValue check. The outer condition is: if (hasValue && ((slot.attributes() & PropertyAttribute::Function) != 0 || (value.isCell() && value.isCallable()))). Inside that block there is another: if (hasValue) attributes = attributesForStructure(slot.attributes());
Why the inner check is always true: C++ short-circuit evaluation means the && operator in the outer condition only evaluates the right-hand side when hasValue is truthy. Therefore, any code inside the outer if block already has hasValue == true as a guaranteed precondition. The inner if (hasValue) can never be false when reached.
Code path that triggers it: Any call to spyOn() where the target property exists (hasValue = true) and the property is either flagged as Function in the static hash table or holds a callable value (e.g., spyOn(Bun, "gc")) will enter the outer branch and then unconditionally execute the inner assignment.
Why existing code does not prevent it: The redundant guard predates this PR. The PR only changed the RHS of the assignment from slot.attributes() to attributesForStructure(slot.attributes()) - a correct and necessary fix - while preserving the existing (redundant) if (hasValue) wrapper structure.
Impact: At runtime this is completely harmless: attributes is always assigned attributesForStructure(slot.attributes()) when the outer condition holds. However, the dead inner guard is misleading - a future maintainer refactoring the outer condition (e.g., separating the hasValue check) might incorrectly rely on the inner if as a safety net, leading to a latent logic error where attributes silently stays at 0 when it should be set.
Step-by-step proof:
- Call spyOn(Bun, "gc"). Bun.gc is in the static hash table with PropertyAttribute::Function set.
- hasValue is set to true by object->getPropertySlot(...).
- Outer condition: hasValue is true AND the Function attribute check is true - we enter the block.
- Inner condition: hasValue is still true - the assignment always executes.
- The inner if guard can never skip the assignment; it is dead code.
Fix: Simply remove the inner if (hasValue) guard, leaving the assignment as an unconditional statement within the outer if block.
There was a problem hiding this comment.
LGTM — all prior feedback has been addressed.
Extended reasoning...
Overview
This PR touches three files: JSMockFunction.cpp (spyOn attribute handling), NodeVM.cpp (globalThis slot attributes), and a new test file. The changes are minimal and surgical — replacing raw slot.attributes() with attributesForStructure(slot.attributes()) in two places and using 0 for the NodeVM globalThis slot.
Security risks
None. This is a bug fix in property attribute bit manipulation with no security surface.
Level of scrutiny
Low. The fix follows the exact same pattern JSC itself uses throughout Lookup.h (lines 462, 488, 496, 503, etc.). The root cause (passing raw static hash table attribute bits into PropertySlot::setValue) is well-documented in the PR description and the fix is straightforward.
Other factors
My previous inline comments are fully addressed in the current commit: both tests use test.concurrent, both include expect(stderr).toBe("") assertions, and CodeRabbit's nit was resolved. The only outstanding observation — the pre-existing redundant if (hasValue) inner guard — is harmless at runtime and predates this PR. The regression tests cover the exact Fuzzilli-found crash scenario with Bun.gc and Bun.peek.
There was a problem hiding this comment.
LGTM — prior feedback addressed, fix is correct and well-scoped.
Extended reasoning...
Overview
This PR fixes a JSC assertion failure (PropertySlot.h:219) in two places: JSMockFunction.cpp (spyOn on static hash table properties) and NodeVM.cpp (globalThis slot handling). The change is minimal — 4 lines of production code changed, plus regression tests.
Security risks
No security implications. The fix masks attribute bits before storing them, matching how JSC itself handles these in Lookup.h. No auth, crypto, or permission-sensitive code is touched.
Level of scrutiny
Low risk. The fix follows established JSC conventions (attributesForStructure() is the correct call to strip hash table bits before storing), is isolated to two specific code paths, and the root cause is clearly documented. Regression tests exercise both paths and would crash in debug builds without the fix.
Other factors
Prior inline comments from my earlier review (dead if (hasValue) guard nit, test.concurrent guideline) were fully addressed in commits 4a75d44 and 96def6f. All reviewer threads are resolved. CI passed across platforms. The dead inner if (hasValue) guard I noted is pre-existing, harmless, and does not affect correctness of this fix.
There was a problem hiding this comment.
LGTM — all prior nits addressed.
Extended reasoning...
Overview
This PR modifies three files: JSMockFunction.cpp (strips static hash table attribute bits via attributesForStructure() when saving spyAttributes and computing attributes in spyOn), NodeVM.cpp (uses 0 instead of slot.attributes() for the globalThis plain-value slot in two getOwnPropertySlot overrides), and adds test/js/bun/test/spyon-static-property.test.ts with two regression tests.
Security risks
No security concerns. The changes are isolated to mock/spy internals and a VM sandbox property lookup path. No auth, crypto, or permission-sensitive code is touched.
Level of scrutiny
Low-to-medium. The fix is mechanical and well-precedented — JSC itself calls attributesForStructure() in Lookup.h at exactly the equivalent call sites. The root cause (assertion failure from raw slot.attributes() containing high bits like PropertyAttribute::Function) is clearly explained and the fix is minimal. The NodeVM change using 0 is correct because globalThis is a plain value with no special attributes.
Other factors
All prior review comments have been resolved: test.concurrent was applied in commit 27c7dd1, stderr assertions were added in commit 4a75d44 (and reformatted in 7e1cea4). The dead if (hasValue) inner guard I noted is a pre-existing nit with no runtime impact. No bugs found by the bug hunting system. CI passed on the relevant builds.
|
CI failures in build #40464 are all pre-existing — the same debian/ubuntu/darwin test steps fail identically on the recently merged PR #28337 (build #40404). Alpine-x64 passes; alpine-aarch64 and alpine-x64-baseline are flaky (same code, different outcome). No failures are introduced by this PR. |
…/Structure PropertySlot::setValue asserts that attributes have no static hash table bits (bits 8+) and no Accessor flag. Two places passed raw slot.attributes() which could contain Function, DOMAttribute, or other high bits: - NodeVM.cpp: getOwnPropertySlot for 'globalThis' passed slot.attributes() to setValue, but the slot was freshly constructed with potentially stale attributes. Use 0 since globalThis is a plain value property. - JSMockFunction.cpp: spyOn saved and restored slot.attributes() without stripping static hash table bits via attributesForStructure(). When spying on properties from static hash tables (e.g. Bun.gc), the Function bit would persist through putDirect and later trigger the assertion.
robobun
force-pushed
the
farm/2e6b20d2/fix-property-slot-attributes
branch
from
7e1cea4 to
b35c001
Compare
There was a problem hiding this comment.
LGTM — both prior nits addressed, fix correctly applies attributesForStructure() following JSC's own pattern.
Extended reasoning...
Overview
This PR touches three files: JSMockFunction.cpp (spyOn attribute masking), NodeVM.cpp (globalThis slot attributes), and a new regression test file. All changes are tightly scoped to a single bug fix.
Security Risks
No security-sensitive code paths are involved. The change deals with internal JSC property attribute bit masking, not auth, crypto, or permissions.
Level of Scrutiny
The fix is low-risk and mechanical: it applies attributesForStructure() — a JSC utility that masks to the low 8 bits — in two places where raw slot.attributes() was previously passed. JSC's own Lookup.h uses this pattern consistently (lines 462, 488, 496, 503). The NodeVM.cpp change uses 0 for a plain value property where no attributes apply. Both changes are correct by construction.
Other Factors
My two prior review comments have been addressed: test.concurrent was added per project CLAUDE.md guidelines, and stderr assertions were added per CodeRabbit's nit. The redundant inner if (hasValue) guard I noted is a pre-existing dead-code issue that is harmless at runtime. CI has passed. No blocking reviews remain.
1 participant
Fuzzilli found a flaky assertion failure at
PropertySlot.h(219):PropertySlot::setValueasserts that property attributes contain no static hash table bits (bits 8+, likeFunction,DOMAttribute,PropertyCallback, etc.) and noAccessorflag. JSC's own code always strips these viaattributesForStructure()before storing (seeLookup.hlines 462, 488, 496, 503, etc.), but two places in Bun's bindings passed rawslot.attributes():JSMockFunction.cpp (
spyOn): When spying on a property from a static hash table (e.g.Bun.gc),slot.attributes()contains theFunctionbit (bit 8). This was passed directly toputDirectand saved inspyAttributesfor latermockRestore, propagating invalid attribute bits into the Structure and triggering the assertion on subsequent property lookups.NodeVM.cpp (
getOwnPropertySlotforglobalThis): Passedslot.attributes()from an uninitialized/stale slot tosetValue. Use0sinceglobalThisis a plain value property with no special attributes.Fix: call
attributesForStructure()to mask to the low 8 bits inspyOn, and use0for the NodeVMglobalThisslot.Verified: CI Format ✅, Lint ✅, Pipeline ✅, Buildkite Build #40400 compiling on latest commit 4a75d44. Prior Build #40398 was canceled (superseded), not failed. Diff is clean — 3 files changed, all scoped to the fix. Two regression tests exercise spyOn on static hash table functions (Bun.gc, Bun.peek), assert spy call recording, mockRestore, and post-restore functionality; these would crash on main in debug builds. CodeRabbit stderr-assertion nit resolved.
Verified by robobun: CI on commit 4a75d44 passed all build/test jobs across all platforms (Build #40400 shows canceled but all individual jobs succeeded before supersede). Latest commit 96def6f (Build #40409) only adds test.concurrent and stderr assertions to the test file. Format ✅, Lint ✅. Diff is clean — no TODO/FIXME/HACK. Tests exercise spyOn on static hash table functions (Bun.gc, Bun.peek) which would crash with an assertion failure on main in debug builds. CodeRabbit stderr nit resolved; Claude review threads are nits about pre-existing dead code. No blocking reviews.
Verified by robobun (iteration 3): Commit 27c7dd1, Build #40463 in progress — Lint JS ✅, pipeline ✅, Format pending. Prior Build #40409 failures were solely bundler_compile.test.ts (unrelated to mock/NodeVM changes). Diff is clean (3 files, no TODO/FIXME/HACK). Tests run in-process: spyOn(Bun, "gc") asserts call tracking + mockRestore, spyOn(Bun, "peek") asserts restore preserves functionality — both exercise the exact assertion-failure path fixed. Code changes follow JSC's own attributesForStructure() pattern from Lookup.h. No blocking reviews.