Skip to content

Fix null deref in forEachProperty when Proxy traps throw#29247

Closed
robobun wants to merge 1 commit intomainfrom
farm/eba0d65f/fix-foreach-property-proxy-throw
Closed

Fix null deref in forEachProperty when Proxy traps throw#29247
robobun wants to merge 1 commit intomainfrom
farm/eba0d65f/fix-foreach-property-proxy-throw

Conversation

@robobun
Copy link
Copy Markdown
Collaborator

@robobun robobun commented Apr 13, 2026

Fuzzilli fingerprint: 8bfeac75c4d58b34

What

Bun.inspect() / console.log() crashed with a null JSCell dereference when formatting an object whose prototype chain contains a Proxy with a throwing trap.

Minimal repro:

class Foo {
  get x() { throw new Error('boom'); }
}
const obj = new Foo();
Object.setPrototypeOf(obj, new Proxy(Object.getPrototypeOf(obj), {}));
console.log(obj);

Also crashes with a throwing getPrototypeOf trap:

class Foo {}
const obj = new Foo();
Object.setPrototypeOf(obj, new Proxy(Object.getPrototypeOf(obj), {
  getPrototypeOf() { throw new Error('boom'); },
}));
console.log(obj);

Why

In JSC__JSValue__forEachPropertyImpl:

  1. object->getPropertySlot() can throw via a Proxy [[Get]] trap and return false. The CLEAR_IF_EXCEPTION that was meant to swallow this sat after the early continue, so it was never reached and the pending exception leaked forward.
  2. iterating->getPrototype(globalObject) can throw via a Proxy [[GetPrototypeOf]] trap (or observe the leaked exception from (1)) and return an empty JSValue. Calling .getObject() on an empty JSValue dereferences a null JSCell.

Fix

  • Clear the exception before the continue.
  • Check the getPrototype() result (clear any exception, bail on empty / non-object) before dereferencing.

Added regression cases to the existing inspect.test.js crash-testing fixture list.

When inspecting an object whose prototype chain includes a Proxy,
forEachPropertyImpl could dereference a null JSCell in two places:

- getPropertySlot() can throw via a Proxy [[Get]] trap and return
  false. The CLEAR_IF_EXCEPTION that follows was placed after the
  early continue, so the pending exception leaked into subsequent
  operations.
- iterating->getPrototype() can throw via a Proxy [[GetPrototypeOf]]
  trap and return an empty JSValue. Calling .getObject() on an empty
  JSValue is a null dereference.

Clear the exception before the continue, and check the prototype
result before dereferencing it.
@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 13, 2026

Updated 10:05 PM PT - Apr 12th, 2026

@robobun, your commit c5a66bc has some failures in Build #45431 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 29247

That installs a local version of the PR into your bun-29247 executable, so you can run:

bun-29247 --bun

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ec0bd285-7a84-4634-9179-aaf8d193f8a1

📥 Commits

Reviewing files that changed from the base of the PR and between d7526e2 and c5a66bc.

📒 Files selected for processing (2)
  • src/bun.js/bindings/bindings.cpp
  • test/js/bun/util/inspect.test.js

Walkthrough

The PR modifies property iteration exception handling in bindings.cpp to improve prototype traversal robustness, and introduces crash-testing fixtures in the inspect test suite to validate edge cases involving property getters and prototype manipulation.

Changes

Cohort / File(s) Summary
Exception Handling in Property Iteration
src/bun.js/bindings/bindings.cpp
Refactored JSC__JSValue__forEachPropertyImpl to separate getPropertySlot into a boolean result with explicit exception clearing before checking the result; modified prototype-walk loop to retrieve the next prototype into a temporary variable with exception clearing before evaluating iteration conditions.
Crash-Testing Fixtures
test/js/bun/util/inspect.test.js
Added two new fixtures to test crash resilience: one where a property getter throws during inspection via prototype-to-Proxy mutation, and another where a prototype wrapped in a Proxy throws from getPrototypeOf(). Both fixtures are validated through existing Bun.inspect() exception handling loop.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main fix: preventing null dereference in forEachProperty when Proxy traps throw exceptions.
Description check ✅ Passed The description comprehensively covers all required template sections with clear problem statement, minimal reproductions, root cause analysis, fix explanation, and testing approach.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix null deref in forEachProperty when prototype is a Proxy #28325 - Fixes null deref in forEachProperty getPrototype call when prototype is a Proxy
  2. fix: clear exception before continue in property enumeration slow path #28530 - Moves CLEAR_IF_EXCEPTION before continue in getPropertySlot slow path
  3. Fix crash in forEachProperty when prototype chain contains a Proxy #28854 - Fixes crash in forEachProperty getPrototype with CLEAR_IF_EXCEPTION + null check
  4. Fix null deref in forEachProperty when Proxy prototype throws #28882 - Fixes both getPrototype null deref and napi_get_all_property_names Proxy crash
  5. Handle exception from getPrototype in forEachProperty #28918 - Fixes both getPrototype exception handling and adds CLEAR_IF_EXCEPTION in prototype counting
  6. Fix use-after-poison in forEachProperty prototype chain walk #28919 - Fixes getPrototype null deref with EnsureStillAliveScope and restructured loop
  7. fix null deref in forEachProperty prototype walk #28991 - Fixes both getPrototype null deref and getPropertySlot exception-before-continue
  8. Fix null deref in Bun.inspect when Proxy getPrototypeOf throws #29071 - Fixes getPrototype null deref with CLEAR_IF_EXCEPTION + null guard

🤖 Generated with Claude Code

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 13, 2026

Duplicate of #29071, which already fixes both the getPropertySlot exception-before-continue and the getPrototype null deref with effectively the same change. Closing in favor of that one.

@robobun robobun closed this Apr 13, 2026
Comment on lines 5290 to 5300
}

JSC::PropertySlot slot(object, PropertySlot::InternalMethodType::Get);
if (!object->getPropertySlot(globalObject, property, slot))
continue;
bool hasSlot = object->getPropertySlot(globalObject, property, slot);
// Ignore exceptions from "Get" proxy traps.
CLEAR_IF_EXCEPTION(scope);
if (!hasSlot)
continue;

if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) {
if (property == propertyNames->underscoreProto
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.

🟣 This PR fixes two Proxy-trap exception cases in forEachPropertyImpl (getPropertySlot and getPrototype), but a third case at line 5277 still uses RETURN_IF_EXCEPTION rather than CLEAR_IF_EXCEPTION, so a Proxy with a throwing ownKeys trap will propagate an exception out of the function. This is a pre-existing inconsistency, but the PR explicitly patches the same class of issue in the same function and misses this one case; Bun.inspect()/console.log() can throw a JS exception when inspecting an object whose prototype chain contains such a Proxy.

Extended reasoning...

What the bug is and how it manifests

In JSC__JSValue__forEachPropertyImpl, the slow-path prototype loop calls getOwnPropertyNames via iterating->methodTable()->getOwnPropertyNames(...) at line 5274. The very next line (5277) is RETURN_IF_EXCEPTION(scope, void()). If iterating is a Proxy whose ownKeys trap throws, this macro fires and returns early from forEachPropertyImpl with the exception still pending in the VM scope. The cleanup block at lines 5380-5383 that would normally clear remaining exceptions is never reached because of the early return.

The specific code path that triggers it

At line 5274-5277:
iterating->methodTable()->getOwnPropertyNames(iterating, globalObject, properties, DontEnumPropertiesMode::Include);
RETURN_IF_EXCEPTION(scope, void()); // propagates if ownKeys trap throws

Compare with the PR-added fixes just below at line 5295:
bool hasSlot = object->getPropertySlot(globalObject, property, slot);
CLEAR_IF_EXCEPTION(scope); // swallows Get-trap exceptions

And at line 5371:
CLEAR_IF_EXCEPTION(scope); // swallows getPrototypeOf-trap exceptions

Why existing code does not prevent it

The PR intentionally converts two RETURN_IF_EXCEPTION usages to CLEAR_IF_EXCEPTION with comments like 'Ignore exceptions from Proxy traps,' but the getOwnPropertyNames call at line 5274-5277 was not updated. The RETURN_IF_EXCEPTION there predates this PR and was simply overlooked.

What the impact would be

The C++ function is annotated [[ZIG_EXPORT(check_slow)]], so the generated Zig wrapper calls globalObject.hasException() after forEachPropertyImpl returns. When an exception is pending, it returns error.JSError. Zig callers in ConsoleObject.zig and pretty_format.zig use 'try value.forEachProperty(...)', so the error propagates up the call stack, causing Bun.inspect() or console.log() to throw a JS exception rather than silently skipping the problematic Proxy.

How to fix it

Replace RETURN_IF_EXCEPTION(scope, void()); at line 5277 with CLEAR_IF_EXCEPTION(scope); (optionally with a comment matching the pattern used for the other two cases). For consistency with the PR intent of swallowing proxy trap exceptions in inspect, clearing is the correct choice.

Step-by-step proof

  1. Create an object whose prototype is a Proxy with a throwing ownKeys trap:
    class Foo {}
    const obj = new Foo();
    Object.setPrototypeOf(obj, new Proxy(Object.getPrototypeOf(obj), {
    ownKeys() { throw new Error('boom'); }
    }));
    console.log(obj); // throws instead of silently continuing
  2. console.log calls Bun.inspect, which calls forEachPropertyImpl.
  3. In the slow-path loop, iterating is set to the Proxy.
  4. getOwnPropertyNames calls the ownKeys trap, which throws.
  5. RETURN_IF_EXCEPTION at line 5277 fires, returning early with exception pending.
  6. The Zig wrapper detects the pending exception and returns error.JSError.
  7. The 'try' in the Zig caller propagates this as a JS exception to user code, which is surprising and inconsistent behavior from an inspection-only function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant