Fix null pointer crash in lazy property reification during deepEquals#28550
Fix null pointer crash in lazy property reification during deepEquals#28550
Conversation
|
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 (3)
WalkthroughChanged lazy property getter wrappers to guard falsy decoded values and return Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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/deep-equals-lazy-property-crash.test.ts`:
- Around line 9-17: The test currently asserts the absence of a crash string
(e.g., not.toContain("null pointer") ) which is forbidden; instead modify the
child code (the snippet that defines function F4 and calls
Bun.deepEquals(Uint8Array, Bun)) to emit a deterministic success marker (e.g.,
console.log("TEST_OK")) when it finishes, then update the assertions to first
expect stdout to contain that marker and only after that assert exitCode === 0;
remove any negative assertions checking for crash text and apply the same change
to the other occurrences referenced (lines 24-27).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ceb2fbe6-dc7a-468a-bf89-befe1ddea287
📒 Files selected for processing (3)
src/bun.js/bindings/BunObject+exports.hsrc/bun.js/bindings/BunObject.cpptest/js/bun/deep-equals-lazy-property-crash.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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 `@src/bun.js/webview/WebViewEventTarget.h`:
- Line 45: Add the missing WebCore::BunWebViewEventTargetInterfaceType enum
entry to EventTargetInterfaces.h and add a matching case in
EventTargetFactory.cpp inside the toJS(WebCore::EventTargetInterface) switch so
the eventTargetInterface() implementation in WebViewEventTarget.h (which returns
WebCore::BunWebViewEventTargetInterfaceType) compiles; ensure the new enum value
is appended or numbered consistently with existing values (after
XMLHttpRequestUploadEventTargetInterfaceType) and implement the toJS branch to
construct/return the appropriate JS wrapper for
BunWebViewEventTargetInterfaceType so the mapping between enum and factory is
complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fb13b20-0db1-4341-abf3-c268f62c3e0e
📒 Files selected for processing (2)
src/bun.js/webview/WebViewEventTarget.htest/js/bun/deep-equals-lazy-property-crash.test.ts
| { | ||
| } | ||
|
|
||
| WebCore::EventTargetInterface eventTargetInterface() const final { return WebCore::BunWebViewEventTargetInterfaceType; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if BunWebViewEventTargetInterfaceType exists in the enum definition
echo "=== Searching for BunWebViewEventTargetInterfaceType in EventTargetInterfaces.h ==="
rg -n 'BunWebViewEventTargetInterfaceType' --type=cpp --type=h
echo ""
echo "=== Checking EventTargetFactory.cpp for dispatch handling ==="
rg -n -A2 'BunWebViewEventTargetInterfaceType' src/bun.js/bindings/webcore/EventTargetFactory.cpp
echo ""
echo "=== Full EventTargetInterface enum ==="
rg -n -A 100 'enum EventTargetInterface' src/bun.js/bindings/webcore/EventTargetInterfaces.h | head -150Repository: oven-sh/bun
Length of output: 4534
🏁 Script executed:
#!/bin/bash
echo "=== Full search for BunWebViewEventTargetInterfaceType in codebase ==="
rg 'BunWebViewEventTargetInterfaceType' -t cpp -t h
echo ""
echo "=== Get complete EventTargetInterface enum ==="
rg -n 'enum EventTargetInterface' -A 200 src/bun.js/bindings/webcore/EventTargetInterfaces.h | tail -100
echo ""
echo "=== Check if enum continues beyond visible output ==="
wc -l src/bun.js/bindings/webcore/EventTargetInterfaces.h
echo ""
echo "=== Look for BUN-specific enum extensions or overrides ==="
fd -e h -e cpp . src/bun.js/webview/ | xargs rg -l 'EventTargetInterface'
echo ""
echo "=== Check EventTargetFactory.cpp for total number of cases ==="
rg -c 'case.*EventTargetInterfaceType' src/bun.js/bindings/webcore/EventTargetFactory.cppRepository: oven-sh/bun
Length of output: 4867
BunWebViewEventTargetInterfaceType enum value is not defined—code will not compile.
The eventTargetInterface() method returns WebCore::BunWebViewEventTargetInterfaceType, but this enum value does not exist in EventTargetInterfaces.h. The enum ends at XMLHttpRequestUploadEventTargetInterfaceType = 74 with no entry for BunWebViewEventTargetInterfaceType. Additionally, EventTargetFactory.cpp has no dispatch case for this interface type (74 cases total, none matching this type).
This is a compilation error. Add BunWebViewEventTargetInterfaceType to the enum in EventTargetInterfaces.h and add a corresponding case in EventTargetFactory.cpp's toJS() switch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/webview/WebViewEventTarget.h` at line 45, Add the missing
WebCore::BunWebViewEventTargetInterfaceType enum entry to
EventTargetInterfaces.h and add a matching case in EventTargetFactory.cpp inside
the toJS(WebCore::EventTargetInterface) switch so the eventTargetInterface()
implementation in WebViewEventTarget.h (which returns
WebCore::BunWebViewEventTargetInterfaceType) compiles; ensure the new enum value
is appended or numbered consistently with existing values (after
XMLHttpRequestUploadEventTargetInterfaceType) and implement the toJS branch to
construct/return the appropriate JS wrapper for
BunWebViewEventTargetInterfaceType so the mapping between enum and factory is
complete.
|
|
||
| // definition of the C++ wrapper to call the Zig function | ||
| #define DEFINE_ZIG_BUN_OBJECT_GETTER_WRAPPER(name) static JSC::JSValue BunObject_lazyPropCb_wrap_##name(JSC::VM &vm, JSC::JSObject *object) { \ | ||
| return JSC::JSValue::decode(BunObject_lazyPropCb_##name(object->globalObject(), object)); \ | ||
| JSC::JSValue result = JSC::JSValue::decode(BunObject_lazyPropCb_##name(object->globalObject(), object)); \ | ||
| if (!result) [[unlikely]] return JSC::jsUndefined(); \ | ||
| return result; \ | ||
| } \ |
There was a problem hiding this comment.
🔴 The fix converts a hard crash into silent, permanent corruption of the Bun global object: JSC's PropertyCallback machinery unconditionally calls putDirect(vm, propName, result) with the callback return value before any exception check, so returning jsUndefined() on failure causes JSC to permanently store undefined for properties like Bun.$, Bun.sql, Bun.SQL, and Bun.postgres. Since these properties have DontDelete, they cannot be reset — the Bun namespace is irrecoverably broken for the rest of that process. Consider checking for a pending exception inside the callbacks before returning, or skipping putDirect when a VM exception is already pending.
Extended reasoning...
Root cause: putDirect is called unconditionally before the exception is checked
JSC's PropertyCallback handling in Lookup.h (WebKit commit fc9f2fa) is:
if (value.attributes() & PropertyAttribute::PropertyCallback) {
JSValue result = value.lazyPropertyCallback()(vm, &thisObj);
thisObj.putDirect(vm, propertyName, result, attributesForStructure(value.attributes()));
return;
}There is no exception guard between the callback invocation and putDirect. The PR description itself confirms this: "This empty value was passed directly to putDirect()". Before the fix, passing JSValue{} (null cell) to putDirect called isGetterSetter() on a null pointer, causing a hard crash. After the fix, passing jsUndefined() to putDirect stores undefined permanently and strips the PropertyCallback attribute — no crash, but no more lazy reification either.
Step-by-step proof of the corruption scenario
- Deep JS recursion (
new F4()) causes a first JSC stack-overflow exception; a surroundingtry/catchclears it. - The stack is still at depth N-1 (near the overflow threshold) when the catch block runs.
- The catch block calls
Bun.deepEquals(Uint8Array, Bun), which iteratesBun's enumerable properties viagetIfPropertyExists(bindings.cpp:1008). - Uninitialized lazy properties (
sql,postgres,$,SQL) trigger their callbacks:defaultBunSQLObjectcallsrequireId(module loading),constructBunShellcalls into JS viaJSC::call. - At depth N-1 these re-entrant calls overflow the stack again; the callbacks return
jsUndefined()(the new behavior). - JSC calls
putDirect(vm, "$", jsUndefined(), DontDelete|...)— permanently storingundefinedand clearingPropertyCallback. RETURN_IF_EXCEPTIONat bindings.cpp:1009 fires afterputDirecthas already committed the bad value.- The second exception is caught; execution resumes, but
Bun.$,Bun.sql,Bun.SQL,Bun.postgresare now permanentlyundefined.
Why DontDelete makes this unrecoverable
The properties are registered with DontDelete | PropertyCallback. After step 6, the slot holds jsUndefined() and PropertyCallback has been stripped by attributesForStructure. DontDelete prevents delete Bun.$ from succeeding in userland, and the now-resolved slot will never re-invoke the lazy callback. There is no recovery path short of restarting the process.
Why the test does not detect this corruption
The added regression test limits recursion with if (depth++ > 100) throw new Error("too deep"), capping the call stack at ~100 frames — far below JSC's actual stack-overflow threshold. No real VM pending-exception is ever set, so all lazy callbacks execute normally and return valid JSValues. The !result guard and jsUndefined() fallback paths added by this PR are never reached. The test passes on both fixed and unfixed code, providing false confidence.
How to fix
The correct fix must prevent putDirect from being called with a poisoned value when a VM exception is pending. Viable approaches: (1) At the JSC PropertyCallback call site, add an exception check after the callback returns and before calling putDirect — skip putDirect if an exception is pending, leaving the slot unreified for a future access. (2) Inside each callback, check scope.exception() before the operation that can overflow and avoid returning at all if already in an exception state, propagating the existing exception instead of masking it with jsUndefined().
Lazy property callbacks on the Bun namespace object can return an empty
JSValue when they encounter an exception (e.g. after a caught stack
overflow). The empty JSValue was passed directly to JSC's putDirect,
which called isGetterSetter() on a null cell pointer, causing a crash.
Fix the Zig callback wrapper to return jsUndefined() instead of an
empty JSValue on failure, and fix the C++ callbacks (constructBunShell,
defaultBunSQLObject, constructBunSQLObject) to return jsUndefined()
instead of {} on exception.
…idden stderr check
robobun
force-pushed
the
farm/d8266e41/fix-null-deref-lazy-property
branch
from
bbe638c to
60a0594
Compare
1 participant
After a caught stack overflow, calling
Bun.deepEquals()with theBunnamespace object as an argument could crash with a null pointer dereference inisGetterSetter().Root cause
When
deepEqualsiterates the properties of theBunobject, it triggers lazy property reification viagetIfPropertyExists→setUpStaticFunctionSlot→reifyStaticProperty. The lazy property callbacks (both Zig-backed and C++) can return an emptyJSValue(.zero/{}) when they encounter an exception (like a stack overflow propagating through). This empty value was passed directly toputDirect(), which calledisGetterSetter()on a null cell pointer.Fix
BunObject+exports.h): ReturnjsUndefined()instead of an empty JSValue when the callback fails. The pending exception is still on the VM and will be caught byRETURN_IF_EXCEPTIONin the caller.BunObject.cpp): ChangeconstructBunShell,defaultBunSQLObject, andconstructBunSQLObjectto returnjsUndefined()instead of{}on exception.Repro
Crash:
JSCJSValueCellInlines.h:67:34: member call on null pointer of type 'JSC::JSCell'