Skip to content

Fix PropertySlot assertion by stripping static hash table attribute bits #28335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix PropertySlot assertion by stripping static hash table attribute bits #28335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8476e0e
Strip static hash table attribute bits before storing in PropertySlot…
robobun Mar 20, 2026
6e085da
Add regression test for spyOn on static hash table properties
robobun Mar 20, 2026
2834d49
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 20, 2026
72de067
Add stderr assertions to test
robobun Mar 20, 2026
fb78cf3
Use test.concurrent for independent subprocess tests
robobun Mar 20, 2026
e56ad3b
Simplify test to use bun:test spyOn directly instead of subprocess
robobun Mar 21, 2026
b35c001
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/bun.js/bindings/JSMockFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1529,12 +1529,12 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalOb
auto* mock = JSMockFunction::create(vm, globalObject, globalObject->mockModule.mockFunctionStructure.getInitializedOnMainThread(globalObject), CallbackKind::GetterSetter);
mock->spyTarget = JSC::Weak<JSObject>(object, &weakValueHandleOwner(), nullptr);
mock->spyIdentifier = propertyKey.isSymbol() ? Identifier::fromUid(vm, propertyKey.uid()) : Identifier::fromString(vm, propertyKey.publicName());
mock->spyAttributes = hasValue ? slot.attributes() : 0;
mock->spyAttributes = hasValue ? attributesForStructure(slot.attributes()) : 0;
unsigned attributes = 0;

if (hasValue && ((slot.attributes() & PropertyAttribute::Function) != 0 || (value.isCell() && value.isCallable()))) {
if (hasValue)
attributes = slot.attributes();
attributes = attributesForStructure(slot.attributes());

Comment on lines 1535 to 1538

@claude claude Bot Mar 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. Call spyOn(Bun, "gc"). Bun.gc is in the static hash table with PropertyAttribute::Function set.
  2. hasValue is set to true by object->getPropertySlot(...).
  3. Outer condition: hasValue is true AND the Function attribute check is true - we enter the block.
  4. Inner condition: hasValue is still true - the assignment always executes.
  5. 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.

mock->copyNameAndLength(vm, globalObject, value);

Expand All @@ -1553,7 +1553,7 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalOb
pushImpl(mock, globalObject, JSMockImplementation::Kind::Call, value);
} else {
if (hasValue)
attributes = slot.attributes();
attributes = attributesForStructure(slot.attributes());

attributes |= PropertyAttribute::Accessor;

Expand Down
4 changes: 2 additions & 2 deletions src/bun.js/bindings/NodeVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ bool NodeVMSpecialSandbox::getOwnPropertySlot(JSObject* cell, JSGlobalObject* gl
if (propertyName.uid()->utf8() == "globalThis") [[unlikely]] {
slot.disableCaching();
slot.setThisValue(thisObject);
slot.setValue(thisObject, slot.attributes(), thisObject);
slot.setValue(thisObject, 0, thisObject);
return true;
}

Expand All @@ -932,7 +932,7 @@ bool NodeVMGlobalObject::getOwnPropertySlot(JSObject* cell, JSGlobalObject* glob
if (notContextified && propertyName.uid()->utf8() == "globalThis") [[unlikely]] {
slot.disableCaching();
slot.setThisValue(thisObject);
slot.setValue(thisObject, slot.attributes(), thisObject->specialSandbox());
slot.setValue(thisObject, 0, thisObject->specialSandbox());
return true;
}

Expand Down
18 changes: 18 additions & 0 deletions test/js/bun/test/spyon-static-property.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { expect, spyOn, test } from "bun:test";

test("spyOn works on static hash table function properties", () => {
const spy = spyOn(Bun, "gc");
try {
Bun.gc(true);
expect(spy).toHaveBeenCalledTimes(1);
} finally {
spy.mockRestore();
}
});

test("spyOn preserves correct attributes after mockRestore", () => {
const spy = spyOn(Bun, "peek");
spy.mockRestore();
const p = Promise.resolve(42);
expect(Bun.peek(p)).toBe(42);
});
Loading