Fix null deref in forEachProperty when Proxy traps throw#29247
Fix null deref in forEachProperty when Proxy traps throw#29247
Conversation
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.
|
Updated 10:05 PM PT - Apr 12th, 2026
❌ @robobun, your commit c5a66bc has some failures in 🧪 To try this PR locally: bunx bun-pr 29247That installs a local version of the PR into your bun-29247 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe 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
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #29071, which already fixes both the |
| } | ||
|
|
||
| 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 |
There was a problem hiding this comment.
🟣 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
- 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 - console.log calls Bun.inspect, which calls forEachPropertyImpl.
- In the slow-path loop, iterating is set to the Proxy.
- getOwnPropertyNames calls the ownKeys trap, which throws.
- RETURN_IF_EXCEPTION at line 5277 fires, returning early with exception pending.
- The Zig wrapper detects the pending exception and returns error.JSError.
- 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.
Fuzzilli fingerprint:
8bfeac75c4d58b34What
Bun.inspect()/console.log()crashed with a nullJSCelldereference when formatting an object whose prototype chain contains a Proxy with a throwing trap.Minimal repro:
Also crashes with a throwing
getPrototypeOftrap:Why
In
JSC__JSValue__forEachPropertyImpl:object->getPropertySlot()can throw via a Proxy[[Get]]trap and returnfalse. TheCLEAR_IF_EXCEPTIONthat was meant to swallow this sat after the earlycontinue, so it was never reached and the pending exception leaked forward.iterating->getPrototype(globalObject)can throw via a Proxy[[GetPrototypeOf]]trap (or observe the leaked exception from (1)) and return an emptyJSValue. Calling.getObject()on an emptyJSValuedereferences a nullJSCell.Fix
continue.getPrototype()result (clear any exception, bail on empty / non-object) before dereferencing.Added regression cases to the existing
inspect.test.jscrash-testing fixture list.