Repository navigation
Conversation
When the bun:sql internal module fails to load (e.g. stack overflow), the PropertyCallback returned an empty JSValue. JSC's reifyStaticProperty passes that straight to putDirect without checking for exceptions, which dereferences a null JSCell. Return jsUndefined() on the error paths so the property is reified to a valid value while the exception still propagates. Also ensure the debug-only error reporting re-throws the exception if it was cleared while printing.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughChangesBun SQL initialization no longer reports handled exceptions through debug-only event-loop blocks. Property and prototype inspection clears lookup exceptions and skips unavailable values. Regression tests cover proxy traps and failing lazy properties. Bun exception handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ypes - constructBunShell (Bun.$) had the same empty-JSValue-from-PropertyCallback issue as Bun.sql, crashing in putDirect when stack overflow throws during the shell builtin call. Return jsUndefined() on the error paths. - Coalesce empty returns from the Zig-backed BunObject lazy property wrappers to jsUndefined() for the same reason. - In forEachPropertyImpl, getPrototype on a Proxy can throw (getPrototypeOf trap) and return an empty JSValue; calling getObject() on it dereferences a null JSCell. Clear the exception and stop walking the chain. Also clear exceptions from getPropertySlot before the early continue so a pending exception does not leak into the next getPrototype call. Fixes #19650
robobun
changed the title
|
Updated 9:48 PM PT - Aug 16th, 2026
✅ @robobun, your commit ff102fa80f306e7b35f49190b041724a67c9e40c passed in 🧪 To try this PR locally: bunx bun-pr 30245That installs a local version of the PR into your bun-30245 --bun |
JSC's reifyStaticProperty always stores the PropertyCallback result via putDirect, and JSValue::get then asserts (!scope.exception() || !hasSlot) under EXCEPTION_SCOPE_VERIFICATION. Returning jsUndefined() with a pending exception satisfies putDirect but trips that assertion on the LLInt path. Instead, catch the exception, report it via reportUncaughtExceptionAtEventLoop (so the user sees why initialization failed), clear it, and return jsUndefined(). The property is reified to undefined and subsequent accesses return undefined without crashing. Also covers the case where the second access would previously crash in release builds because an empty JSValue had been cached in the slot.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/BunObject`+exports.h:
- Around line 96-105: The wrapper macro BunObject_lazyPropCb_wrap_##name
currently clears any exception from the Zig lazy getter (called via
BunObject_lazyPropCb_##name) then returns undefined, swallowing the error and
caching undefined; modify the wrapper to report the pending exception to the JS
engine before clearing it (i.e., call the same exception reporting used in the
explicit callbacks in BunObject.cpp while the DECLARE_THROW_SCOPE(vm) scope has
an exception), then call scope.tryClearException() and return undefined only
after reporting; keep the rest of the flow (decode result, return result or
jsUndefined) intact.
In `@test/regression/issue/19650.test.ts`:
- Around line 9-30: Replace the duplicate parameterized matrices by wrapping
both related tests in a single describe.each([...]) over the same key set and
move the existing test.concurrent.each(...) bodies inside that describe block as
individual test.concurrent(...) or test(...) cases; locate the current
test.concurrent.each invocation (the one iterating ["$", "sql", "SQL",
"postgres"]) and the matching block on lines 32-51 and change them to a single
describe.each that iterates the key array, with each iteration defining the two
test cases that previously duplicated the matrix, preserving existing test
names, scopes, and use of Bun.spawn/proc.exited and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f9fb9d2-14aa-4cd7-9c32-695e8cdba72d
📒 Files selected for processing (3)
src/jsc/bindings/BunObject+exports.hsrc/jsc/bindings/BunObject.cpptest/regression/issue/19650.test.ts
…-lazy-init-null-deref
The previous test only checked for a null signal and exit code in {0,1}.
Under the debug/UBSan build, the unfixed code hits a null-pointer member
call, prints a UBSan diagnostic, and exits with code 1 - which the test
accepted. The test now requires the subprocess to write 'OK' to stdout
after the access pattern, which the unfixed build never reaches on any
configuration (segfault on release, UBSan abort on debug).
|
Merged Verified locally:
Previous CI run's only non-expired failures were |
|
CI status: build 56705 at ae3fc29 — all failures unrelated to this PR's code paths. Main at the same merge base (0b20408, build 56565) is green. Error-level:
Warning-level (flaky, passed on retry): This PR only modifies Regression tests ( The fuzzer has reported 120+ variants of this fingerprint against main — all verified fixed by this PR's changes. Ready for review. |
There was a problem hiding this comment.
All prior feedback is addressed and I found no new issues, but this changes exception-handling semantics for several core Bun.* lazy initializers (permanently reifying to undefined on init failure and reporting via the event loop) and overlaps with several other open PRs for #19650 — worth a human look to confirm this is the approach to land.
Extended reasoning...
Overview
This PR fixes two distinct null-JSCell dereference crashes:
BunObject.cpp/BunObject+exports.h—PropertyCallbackentries (Bun.$,Bun.sql,Bun.SQL,Bun.postgres, plus the Zig-backed wrapper macro) previously returned an emptyJSValuewhen initialization threw. JSC'sreifyStaticPropertypasses that straight toputDirect, which null-derefs. The fix catches the exception, reports it viareportUncaughtExceptionAtEventLoop, clears it, and returnsjsUndefined()so the property is reified to a valid value.bindings.cppforEachPropertyImpl— the prototype-walk inBun.inspectcalled.getObject()on the result ofgetPrototype()without checking for a Proxy-trap exception, and didn't clear an exception fromgetPropertySloton the not-found branch. Both are now guarded withCLEAR_IF_EXCEPTIONand an empty-value check.
Tests are added in inspect.test.js (in-process Proxy-trap cases) and a new test/regression/issue/19650.test.ts (subprocess crash repros for all four lazy keys × two trigger scenarios).
Security risks
None identified. This is defensive crash hardening — no new attack surface, no auth/crypto/permissions, no user-controlled data paths beyond what already existed. The change makes hostile inputs (throwing Proxy traps, clobbered globals, near-stack-overflow) less dangerous, not more.
Level of scrutiny
Moderate-to-high. While the diff is not large (~80 lines of C++), it touches JSC exception-scope semantics in core runtime initialization for high-traffic APIs. Specific judgment calls a maintainer should weigh:
- Permanently caching
undefined: after a transient init failure (e.g. stack overflow on first access),Bun.sql/Bun.$etc. becomeundefinedfor the rest of the process. That's clearly better than a segfault, but it's a user-visible behavior choice vs. e.g. leaving the property un-reified for retry. reportUncaughtExceptionAtEventLoopfrom inside a property getter: this fires the global uncaught-exception handler synchronously during a property access, which is unusual.constructBunShellno longer throwsTypeError("Internal error: ...")on the non-object branches — it now silently returnsundefined. Probably fine (those branches were unreachable in practice), but it's a behavior change beyond the crash fix.- Zig wrapper macro intentionally does not report before clearing (discussed in-thread; rationale is those getters don't run JS so only termination exceptions can land there).
Other factors
- The find-duplicate-prs bot lists eight other PRs targeting the same crash class (#29671, #29807, #27308, #27294, #28550, #28413, #27307, #28368). A maintainer should confirm this is the variant they want to merge and close/rebase the others.
- All my prior inline feedback (use
test.concurrent.each, fixconstructBunShelltoo) was applied; thenapi.cppsibling was explicitly deferred to a follow-up, which is reasonable. - CodeRabbit's suggestion to report exceptions in the Zig wrapper macro was declined with a reasoned response that CodeRabbit accepted; that thread is still technically unresolved but settled.
- CI build #53645 was triggered for the latest commit; status not visible in the timeline.
…-lazy-init-null-deref
There was a problem hiding this comment.
No issues found, but this touches JSC exception-scope handling across several core PropertyCallbacks and changes failure semantics (init exceptions are now reported-then-swallowed and the property is permanently reified to undefined), so it's worth a human sign-off on that tradeoff.
Extended reasoning...
Overview
This PR fixes two fuzzer-found null-JSCell dereferences:
BunObject.cpp/BunObject+exports.h— thePropertyCallbacklazy initializers forBun.$,Bun.sql,Bun.SQL,Bun.postgres, and the Zig-backed wrapper macro now catch exceptions thrown during initialization, report them viareportUncaughtExceptionAtEventLoop, clear them, and returnjsUndefined()instead of an emptyJSValue. Previously the empty value flowed intoreifyStaticProperty→putDirect, which dereferenced a null cell.bindings.cppforEachPropertyImpl— the prototype-chain walk now clears exceptions aftergetPropertySlot(on the not-found branch too) and aftergetPrototype, and null-guards the result before calling.getObject(), so a throwing ProxygetPrototypeOf/gettrap no longer crashesBun.inspect.
Tests: two new Bun.inspect Proxy-trap cases in inspect.test.js, and a new test/regression/issue/19650.test.ts that spawns 8 subprocesses exercising the stack-overflow and clobbered-Symbol reproducers against each lazy key and asserts the child writes OK and exits without a signal.
Security risks
None identified. The change strictly hardens error paths (turning crashes into defined behavior) and doesn't touch auth, crypto, permissions, or input parsing. Exceptions are still surfaced to the user via the uncaught-exception reporter.
Level of scrutiny
Medium-high. This is production-critical native code at the JSC↔Bun boundary — ThrowScope discipline is subtle (the PR description itself notes that returning jsUndefined() with a pending exception trips an EXCEPTION_ASSERT). The fix is well-reasoned and consistent across all four callbacks plus the macro, and the regression tests are tight (require positive OK output, verified to fail on main). But there's a real behavioral decision baked in: a failed lazy init now permanently caches undefined for that property rather than throwing on access, and constructBunShell drops its previous throwTypeError("Internal error: ...") paths in favor of silently returning undefined. That's almost certainly the right call given JSC's PropertyCallback contract, but it's the kind of semantic choice a maintainer should ratify.
Other factors
- All prior inline review comments (mine and CodeRabbit's) are resolved; the author applied the
constructBunShellandtest.concurrent.eachsuggestions and gave reasoned pushback on the two declined ones. - The duplicate-PR bot lists 8 other open/closed PRs targeting the same bug class — a human should confirm this is the variant to land.
- CI passed on the ASAN shard for the new tests; remaining failures are documented as unrelated Windows flakes.
- The
napi.cppinstance of the same pattern was intentionally left for a follow-up (acknowledged in-thread).
…-lazy-init-null-deref
- inspect.test.js: the 'throwing getter' test used an accessor on the Proxy target with an empty handler, so getPropertySlot filled the slot as a GetterSetter without invoking the getter and the test passed on pre-PR code. Replace it with a throwing Proxy 'get' trap so performGet itself throws and getPropertySlot returns false with a pending exception, exercising the CLEAR_IF_EXCEPTION reorder. - get_valkey_default_client: createNoJsNoPubsub throws a plain TypeError when REDIS_URL is empty/invalid. The C++ wrapper clears the pending exception and reifies Bun.redis to undefined, so the diagnostic was dropped. Report it via report_active_exception_as_unhandled (mirroring s3_default_client) and return UNDEFINED to satisfy to_js_host_call's 'empty iff pending exception' assertion. Add regression test.
…-lazy-init-null-deref # Conflicts: # src/runtime/api/BunObject.rs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // PropertyCallbacks are reified by JSC::reifyStaticProperty, which hands the | ||
| // result straight to putDirect without checking for an exception, so a getter | ||
| // that threw has its exception folded here: report it, then reify the slot as | ||
| // undefined rather than an empty JSValue. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Trimmed to one line in 6ecdfcd; the reasoning is in the PR description.
| // JSC's reifyStaticProperty passes the PropertyCallback result straight to | ||
| // putDirect without checking for exceptions, and JSValue::get asserts | ||
| // (!scope.exception() || !hasSlot). So these callbacks must not leave a | ||
| // pending exception; if loading the module fails we report it and return | ||
| // jsUndefined() so the property is reified to a valid value. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Trimmed to one line in 6ecdfcd; the reasoning is in the PR description.
…PropertyCallback The pinned WebKit's setUpStaticFunctionSlot skips the put when a PropertyCallback returns empty and reports the slot as absent, so the builders' exceptions propagate to the read and the slot stays unreified. Clearing and reporting them here turned those throws into undefined and broke builtin-esm-lazy-exports.test.ts. What is left of that part is removing the debug-only report inside the sql/SQL/postgres builders: it re-enters the error reporter in the middle of the property lookup with the exception still pending, which aborts a debug build on a Structure assertion.
robobun
changed the title
There was a problem hiding this comment.
Thanks — 5414053 addresses the earlier 🔴: the lazy-init wrappers, BunObject+exports.h/BunObject.rs hunks, and 19650.test.ts are gone, and defaultBunSQLObject/constructBunSQLObject are back to plain RETURN_IF_EXCEPTION(scope, {}) so the read throws catchably via the JSC fix. The remaining forEachPropertyImpl change and inspect tests look right.
One thing still outstanding from that comment: the PR title/description still describe the removed Part 1 approach (report-as-uncaught + reify to undefined) and claim "Fixes #19650" — please update before merge so the squash message matches what actually lands.
Extended reasoning...
Overview
Since my last review (2026-08-14T15:33Z 🔴), commit 5414053 dropped the superseded Part 1 changes. The diff is now 4 files / ~85 lines: bindings.cpp reorders CLEAR_IF_EXCEPTION before the !found continue and null-guards getPrototype()'s result in forEachPropertyImpl; BunObject.cpp removes two debug-only reportUncaughtExceptionAtEventLoop calls that are now wrong (the exception propagates catchably); inspect.test.js adds two Proxy-prototype crash tests; BunObject.test.ts adds a subprocess test locking in the throw-and-stay-unreified behavior.
Security risks
None. Defensive exception clearing / null guard in an inspection path; no auth, crypto, or untrusted-input parsing.
Level of scrutiny
Medium — bindings.cpp is core JSC glue, but the change is a small defensive guard following the same CLEAR_IF_EXCEPTION pattern already used a few lines away, and robobun's 2026-08-13 triage confirms this crash is still live on main.
Other factors
- The code half of my previous 🔴 is fully addressed; the description half (drop the Part-1 prose and "Fixes #19650") is not, so the squash-merge message would be misleading.
- robobun previously noted #30099 and #29642 target the same inspect crash — a maintainer should pick one.
- One nit (undrained piped stderr in the new
BunObject.test.tssubprocess test) is posted inline; not blocking. - I checked that
proto.getObject()is safe whenprotois a non-empty non-object primitive (returnsnullptr, terminating the loop), and that the removed#if BUN_DEBUGblocks were the only remaining Part-1 residue inBunObject.cpp.
|
Title and description were rewritten for the reduced scope (no #19650 claim) shortly before the last review ran, so that part is already done; the stderr nit is fixed in 230e894. Current diff is the forEachPropertyImpl guard plus removing the debug-only report in the two sql builders, with a test for each. |
…n what makes $ throw
There was a problem hiding this comment.
LGTM — both remaining nits from the last pass are addressed in 2a20b8d.
What was reviewed:
forEachPropertyImpl(bindings.cpp): theCLEAR_IF_EXCEPTIONreorder before the!foundcontinue and the empty-proto guard match the fast-path pattern at ~5583; no other unguardedgetPrototype().getObject()sites in that function.BunObject.cpp: pure deletion of the two#if BUN_DEBUGreports;defaultBunSQLObject/constructBunSQLObjectnow propagate identically toconstructBunShell.- Tests: both inspect cases now pin exact output; the lazy-property subprocess test drains stdout/stderr/exited concurrently and asserts the combined object; the comment now attributes
$to the shell builtin correctly.
Extended reasoning...
Overview
Four files. Two native hunks: (1) src/jsc/bindings/bindings.cpp reorders a CLEAR_IF_EXCEPTION in forEachPropertyImpl's slow path so a throwing Proxy get trap is cleared before the early continue, and adds CLEAR_IF_EXCEPTION + an empty check after getPrototype() so a throwing getPrototypeOf trap doesn't null-deref through .getObject(). This mirrors the existing fast-path handling a few lines above (bindings.cpp:5574-5584). (2) src/jsc/bindings/BunObject.cpp deletes two #if BUN_DEBUG blocks that called reportUncaughtExceptionAtEventLoop from inside the sql/SQL/postgres lazy-property builders while an exception was pending — that re-entered JS mid-lookup and tripped a debug-build assertion. The builders now propagate via RETURN_IF_EXCEPTION(scope, {}) exactly as constructBunShell already does, and JSC's fixed reifyStaticProperty handles the empty result. Tests: two new Bun.inspect cases in inspect.test.js (both crash on main) and one subprocess test in BunObject.test.ts covering the debug-only abort.
Security risks
None. This is exception-handling in the inspector's property walk and removal of debug-only diagnostic code. No auth, crypto, permissions, or untrusted-input parsing paths are touched. CLEAR_IF_EXCEPTION in forEachPropertyImpl is the established policy for user-code side effects during formatting (getters, traps) and is already used for the same trap on the fast path.
Level of scrutiny
Medium — native JSC bindings, but the change is mechanically small and pattern-matched against existing code in the same function. The bindings.cpp hunk is a textbook "exception check after every call that can enter JS" fix (REVIEW.md's most-blocked category), and the BunObject.cpp hunk is pure deletion of debug-only code that made the sql builders inconsistent with the shell builder. No new control flow, no allocation/ownership changes, no API surface change.
Other factors
This PR has been through many review rounds since May. The larger "part 1" (lazy-property wrappers in BunObject+exports.h / BunObject.rs) was correctly identified as superseded by oven-sh/WebKit#282 and removed in 5414053; the current diff is the minimal remainder. All prior inline feedback (mine and comment-cop's) is addressed: stderr is now drained (230e894), the inspect tests pin exact output instead of .not.toThrow(), and the test comment correctly distinguishes the $ shell-builtin path from the bun:sql require path (2a20b8d). The bug hunting system found nothing this run. No human reviewer has open concerns on the thread.
There was a problem hiding this comment.
LGTM — the reduced scope addresses everything from the earlier rounds.
What was reviewed:
forEachPropertyImplreordering:CLEAR_IF_EXCEPTIONnow runs before the!foundcontinue, and thegetPrototyperesult is null-checked before.getObject()— matches the fast-path pattern at line ~5584.BunObject.cpp: removing the debug-onlyreportUncaughtExceptionAtEventLoopcalls leavesRETURN_IF_EXCEPTION(scope, {})in place, so the sql builders now propagate the same wayconstructBunShellalready does.- Tests: both Proxy cases pin exact output; both subprocess tests drain stdout/stderr/exited concurrently and assert the combined object; the new
inspect(Bun)walk test exercises thegetPropertySlotreordering via the non-Proxy path.
Extended reasoning...
Overview
Four files touched. Production changes are ~7 lines net: (1) bindings.cpp — reorder getPropertySlot result handling in forEachPropertyImpl so CLEAR_IF_EXCEPTION runs before the early continue, and guard getPrototype().getObject() against an empty result from a throwing Proxy getPrototypeOf trap; (2) BunObject.cpp — delete two #if BUN_DEBUG blocks that called reportUncaughtExceptionAtEventLoop from inside the sql/SQL/postgres lazy-property builders, mid-lookup with the exception still pending. Three new tests in inspect.test.js and one in BunObject.test.ts.
Security risks
None. This is a crash-hardening change in the property-enumeration path of Bun.inspect. No auth, crypto, permissions, or untrusted-input parsing is touched. The added CLEAR_IF_EXCEPTION calls follow the same "ignore trap exceptions during inspection" policy already used a few lines above (line ~5584) and at ~5668-5671, so this does not introduce new exception-swallowing surface.
Level of scrutiny
Low-to-moderate. The bindings.cpp hunk is a mechanical null-guard + exception-clear that mirrors the existing fast-path branch in the same function. The BunObject.cpp hunk deletes debug-only code with no release-build effect; the exception is still propagated by the RETURN_IF_EXCEPTION(scope, {}) immediately after, and the pinned WebKit's reifyStaticProperty now handles the empty return correctly (verified in the 2026-08-14 review round).
Other factors
This PR has been through many review rounds. My 2026-08-14 blocking comment asked for the tryClearException()+jsUndefined() wrappers to be dropped since they regressed main's now-correct catchable-throw behavior — that was done in 5414053. The stderr-drain nit (230e894), the .not.toThrow() → .toBe("{}") strengthening, and the $-vs-bun:sql comment clarification (both 2a20b8d) were all applied. The most recent commit (bafa86f) adds the inspect(Bun) walk test, which exercises the getPropertySlot reordering through the non-Proxy lazy-property path — a good complement to the two Proxy tests. All subprocess tests follow the REVIEW.md convention (Promise.all on stdout/stderr/exited, combined-object assertion). No outstanding reviewer comments remain.
…pect on Windows stays out of the way
There was a problem hiding this comment.
LGTM — the remaining diff is small and mechanical, and every prior review thread has been addressed.
What was reviewed:
forEachPropertyImpl(bindings.cpp): theCLEAR_IF_EXCEPTIONreorder beforecontinueand thegetPrototypeempty-value guard both mirror the fast-path pattern at ~line 5584 in the same function.BunObject.cpp: only debug-gated lines removed; the resultingRETURN_IF_EXCEPTION(scope, {})shape now matchesconstructBunShellexactly, and the WebKit-side fix (reifyStaticProperty skipping empty results) is what handles propagation.- Tests: subprocess tests drain both pipes and assert
{stdout, stderr, exitCode}; the two Proxy tests pin exact output; the third inspect test'srequire("node:util")preload and the reason for it are documented.
Extended reasoning...
Overview
Four files touched. src/jsc/bindings/bindings.cpp reorders one CLEAR_IF_EXCEPTION call so it runs before an early continue when getPropertySlot returns false, and guards the getPrototype(...).getObject() call against an empty JSValue when a Proxy getPrototypeOf trap throws — both changes follow the identical pattern already present in the fast path ~100 lines earlier in the same function. src/jsc/bindings/BunObject.cpp deletes two three-line #if BUN_DEBUG blocks in defaultBunSQLObject / constructBunSQLObject that called reportUncaughtExceptionAtEventLoop from inside a PropertyCallback, mid-lookup, with an exception still pending; the resulting shape is byte-identical to how constructBunShell already handles the same failure. Tests are additive: three cases in inspect.test.js and one in BunObject.test.ts.
Security risks
None. This is object-inspection formatting and debug-only diagnostic removal. No auth, crypto, filesystem, or network paths. The Bun.inspect change makes existing exception-swallowing behavior consistent (the fast path already CLEAR_IF_EXCEPTIONs Proxy trap throws) rather than introducing new swallowing.
Level of scrutiny
Native JSC bindings warrant care, but the actual change here is minimal: 8 lines in bindings.cpp that follow an established in-file pattern, and 6 debug-gated lines deleted from BunObject.cpp with zero release-build effect. This PR has been through many review rounds since May; earlier revisions carried a much larger BunObject+exports.h / BunObject.rs workaround that was correctly identified as regressing main once oven-sh/WebKit#282 landed, and was dropped in 5414053. What remains is the narrow, uncontroversial residue.
Other factors
Every prior review thread is resolved. My last two nits (test-comment accuracy for $ vs bun:sql, and pinning exact output over .not.toThrow()) were addressed in 2a20b8d. The two commits since then (bafa86f, bfb3414) add the third inspect test and preload node:util so Bun.env's Windows custom-inspect doesn't interfere once Symbol is clobbered — the test comment explains why. The subprocess tests follow REVIEW.md conventions (concurrent pipe drain, combined-object assertion, bunEnv, await using). The BunObject.test.ts case pins the WebKit-side contract (throwing builder → catchable TypeError on read, slot stays unreified) so a future re-introduction of the debug-only report or a WebKit regression would be caught. The PR no longer claims #19650 and correctly cross-references #37714/#37258 for the adjacent redis/process.* cases it does not touch.
…-lazy-init-null-deref
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
For whoever picks this up, the overlapping open PRs for this crash family, so only one needs to land:
This PR is the minimal version of the forEachPropertyImpl fix plus the deletion. If #37175 is preferred, this one can be closed once it lands; if the smaller change is preferred, this is it. |
There was a problem hiding this comment.
LGTM — both nits from the last round are addressed and the diff is now the minimal fix.
What was reviewed:
forEachPropertyImplin bindings.cpp: theCLEAR_IF_EXCEPTIONreorder beforecontinueand the empty-protoguard match the fast-path pattern a few lines up; traced the Proxyget-trap andgetPrototypeOf-trap flows through to the test assertions.BunObject.cpp: pure deletion of the two#if BUN_DEBUGmid-builder reports;constructBunShellalready behaves this way.- Tests: subprocess pipes are drained concurrently, assertions are exact-value, and the earlier concerns (comment attribution for
$,.toBe("{}")over.not.toThrow()) are applied.
Extended reasoning...
Overview
Four files touched. src/jsc/bindings/bindings.cpp reorders an existing CLEAR_IF_EXCEPTION in forEachPropertyImpl so it runs before the if (!found) continue, and guards getPrototype()'s result against empty before calling .getObject() (with the same clear). src/jsc/bindings/BunObject.cpp deletes two #if BUN_DEBUG blocks that called reportUncaughtExceptionAtEventLoop from inside defaultBunSQLObject/constructBunSQLObject while an exception was pending mid-lookup. Three new inspect.test.js cases and one new BunObject.test.ts case cover the fixes.
Security risks
None. Bun.inspect already swallows exceptions from getters and traps at multiple points in this function; the change moves one clear earlier and adds one to match the fast path. No new user-controlled data flows, no auth/crypto/permissions.
Level of scrutiny
Medium — native JSC bindings, but the change is mechanical: a reorder of an existing macro, an empty-value guard identical to the pattern already used a few lines above, and removal of debug-only code. The PR has been through six review rounds; the earlier scope (which regressed reifyStaticProperty's new behavior) was correctly cut after the WebKit bump, and the current diff is the residue that still crashes/aborts on main.
Other factors
All prior feedback from this reviewer is resolved: the blocking regression (superseded BunObject wrappers reifying undefined) was removed in 5414053; the stderr-drain and combined-object assertion were applied in 230e894; the $-vs-bun:sql comment attribution and the .toBe("{}") strengthening were applied in 2a20b8d. The bug-hunting system found nothing this run. The one known caveat — BunObject.test.ts only fails on debug builds of main because the removed report was #if BUN_DEBUG-gated — is stated in the PR description and is inherent to a debug-assertion fix; CI runs debug lanes.
…p or getPrototypeOf throws during the property walk (#29642) ### Problem - `Bun.inspect()`, `console.log()` and `expect()` failure output crash with `Segmentation fault at address 0x5` (debug builds: UBSan "member call on null pointer of type 'JSC::JSCell'" in `JSCJSValueCell.h`) when a property lookup throws while an object is being formatted. Release repro: ```js const proto = new Proxy({ a: 1 }, { getPrototypeOf() { throw new Error("boom"); } }); console.log(Object.create(proto)); ``` and likewise with a Proxy whose `get` trap (or a getter reached through a Proxy) throws for one property. - The same happens with a lazily initialized property of the `Bun` object whose initializer throws (fuzzer sample: `globalThis.Symbol` replaced, then `Bun.inspect(Bun)`; the builtin behind `Bun.$` calls `Symbol("cwd")`). Release builds print `Bun` with most of its properties missing (72 of 115 on the shipped build); debug builds abort with `ASSERTION FAILED: Unexpected exception observed` / `Symbol is not a function. (In 'Symbol("cwd")' ...)` when the next lazy property is built. - Plain-JavaScript variant of the same leak: `console.log` of a module namespace during an import cycle, when one export is still in its temporal dead zone, throws `ReferenceError: Cannot access 'x' before initialization` out of `console.log` (the stale exception is picked up while the next export is formatted). `util.inspect` prints such an export as `<uninitialized>`. - Cause, in the slow path of `JSC__JSValue__forEachPropertyImpl` (`src/jsc/bindings/bindings.cpp`): - `object->getPropertySlot()` reports a throwing Proxy trap, a throwing lazy initializer or a TDZ namespace export as "not found" with the exception still pending, and the loop `continue`d before the `CLEAR_IF_EXCEPTION` below it. The following lookups and formatting callbacks then run with that exception pending (the dropped properties, the rethrown `ReferenceError`, the debug assertion). - When the walk moves to the next prototype, `iterating->getPrototype(globalObject).getObject()` runs `getObject()` on the empty `JSValue` that `getPrototype` returns when it threw (either because of the stale exception above or because the `getPrototypeOf` trap itself throws). The empty value passes `isCell()`, so this reads the type byte of a null cell: the fault at address 5. - `napi_get_all_property_names` (`src/jsc/bindings/napi.cpp`, descriptor filter loop) has the same `getPrototype().getObject()` chain after an unchecked `getOwnPropertyDescriptor`, so a Proxy trap throwing there returned `napi_ok` with an exception pending in own-only mode and segfaulted in include-prototypes mode. - `defaultBunSQLObject` / `constructBunSQLObject` (`src/jsc/bindings/BunObject.cpp`) had a debug-only block that handed a sql module load failure to `reportUncaughtExceptionAtEventLoop` while the exception was still pending on the VM, so `globalThis.Symbol = NaN; Bun.sql` (and the fuzzer sample above, once the walk gets past `Bun.$`) aborted debug builds with `ASSERTION FAILED: ... object->structure() == this` in `Structure::storedPrototype` instead of throwing. ### Fix - `bindings.cpp`: clear the exception after `getPropertySlot` regardless of its result, which is what the ordered variant `JSC__JSValue__forEachPropertyOrdered` already does; read the next prototype into a `JSValue`, clear the exception and stop the walk when it is empty. (An earlier revision also held the prototype being walked under an `EnsureStillAliveScope`; dropped, since the raw pointer is used after every call into JS in the loop body and so is live across them anyway.) - Behaviour change to note: a property whose lookup throws is now left out of the output instead of the whole `console.log` / `Bun.inspect` call throwing or crashing. For TDZ namespace exports this differs from `util.inspect`'s `<uninitialized>`; printing that marker would be a formatter feature on top of this fix and is not attempted here. - `napi.cpp`: check for an exception after `getOwnPropertyDescriptor` and after `getPrototype` and return `napi_pending_exception`, which is what Node returns for these cases. - `BunObject.cpp`: drop the debug-only report. The exception is propagated to the reader by the `RETURN_IF_EXCEPTION` right below it, so debug builds now behave like release builds (`Bun.sql` throws). - Why this is the right place: the formatter deliberately swallows errors thrown by individual properties (getters, traps) and prints the rest of the object; these two sites were the only ones in the walk that acted on a "not found" result or a prototype value before clearing the exception that produced it. Skipping just the property (or stopping at just the prototype) whose lookup threw is the existing behaviour for every other throw site in this function. - Verification: - `test/js/bun/util/inspect.test.js`, "Bun.inspect when a property lookup throws" (5 spawned cases): a Proxy `get` trap, a getter behind a Proxy, a `getPrototypeOf` trap, a throwing lazy `Bun` property, and a two-file import cycle with a TDZ export. Without the fix (shipped release build and an unfixed debug build) all five fail: the Proxy children segfault / fail UBSan, the `Bun` child prints `[false,false,...]` on release and aborts on debug, the cycle child exits 1 with the `ReferenceError`; with it each prints everything except the one property whose lookup threw. - `test/js/bun/util/BunObject.test.ts`, "a lazy property whose builtin fails to load throws from the read": `Bun.$` / `sql` / `SQL` / `postgres` with `Symbol` broken throw a `TypeError` on two consecutive reads. Aborts on an unfixed debug build (the `BunObject.cpp` hunk); passes on release either way, as the removed block is debug-only. The fixture builds `process.env` before breaking `Symbol` because the `$` builder reads it, and building it on Windows reifies another `Bun` property mid-lookup, which on a Windows debug build would hit the separate `storedPrototype` assertion that #37001 fixes (verified on Linux only). - `test/napi/napi.test.ts`: `getOwnPropertyDescriptor` trap throwing in own-only and include-prototypes mode (compared against Node), and a `getPrototypeOf` trap that throws on the second call so the check after `getPrototype` is the one that fires. - Repros above and the tests also run clean under `BUN_JSC_validateExceptionChecks=1`. ### Background - `forEachPropertyImpl` is the property walk behind Bun's native formatter. It collects the property names of the object and of up to five prototypes, looks each one up through the original object with `getPropertySlot`, and hands the value to a callback that formats it. Errors thrown by individual properties are swallowed on purpose so that one bad getter does not make `console.log` throw. - A JSC exception is "pending" state on the VM, not C++ unwinding. A function that throws returns a failure value (`false`, or the empty `JSValue`) and leaves the exception on the VM; until something clears or rethrows it, most JSC entry points return early as soon as they are called, and debug builds assert when a function that did not throw is observed returning with an exception pending. `CLEAR_IF_EXCEPTION` drops the pending exception. - The empty `JSValue` (`JSValue()`) is encoded as 0. `isCell()` is true for it, so `getObject()` on it dereferences a null cell pointer rather than returning null; callers have to test the value itself first. - Lazy properties of the `Bun` object are entries in a static property table whose value is produced by a builder the first time the property is read (`PropertyCallback`). Some builders evaluate built-in JavaScript modules (the shell for `Bun.$`, the sql module for `Bun.sql`), so they can throw when that module fails to evaluate, and JSC reports that to the reader as "property not found" plus a pending exception. ### Consolidated duplicates Found repeatedly by the fuzzer (fingerprint `d678cafe50a2ad6e`). Earlier round, folded in here in April: #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325. This round, closed in favour of this PR: #30099 #30245 #37160 #37175 #37213 #37256 #37428 #38700 #38921 #39363 #39365 #39380 #39412 #39413 (and #39411, closed earlier). The `BunObject.cpp` hunk and the `BunObject.test.ts` test come from #30245 / #37428; the same hunk is also part of #37001, which fixes the underlying `storedPrototype` assertion in JSC. Related fixes that are not part of this bug and stay open on their own: #39382 (a custom inspect function when `node:util` fails to load), #37202 (`util.isError` with a throwing `getPrototypeOf` trap), #37331 (`forEachPropertyOrdered` when the callback throws), #37001 (stale structure in `JSObject::getPropertySlot`), #32263 (additional checks in the same napi loop). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js test/napi/napi.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
Two fuzzer findings that both surface as
member call on null pointer of type 'JSC::JSCell'(JSCJSValueCell.h), plus a debug-only abort next to the second one.1.
Bun.inspectwhen a property read throws mid-walkforEachPropertyImpl(bindings.cpp) walks the prototype chain withiterating = iterating->getPrototype(globalObject).getObject();When
iteratingis a Proxy whosegetPrototypeOftrap throws,getPrototypereturns an emptyJSValueand.getObject()dereferences a null cell. ThegetPropertySlot(...) == falsebranch a few lines up has the same problem: itcontinued before clearing the exception, so whatever the read threw stayed pending for the rest of the walk. A Proxygettrap is one way to get there; since the WebKit bump a lazy property whose builder throws is another, and that one needs no Proxy: the Bun object's$builder throwing duringBun.inspect(Bun)leaves its exception pending when the next property's builder is entered (the fuzzer's version of this isnew DecompressionStream(globalThis)near the stack limit, where the argument error message formatsglobalThis).Fix: clear the exception after
getPropertySlotbefore the earlycontinue, and aftergetPrototype(same as the fast path a few lines above), and stop the walk when the result is empty.Tests: three cases in
test/js/bun/util/inspect.test.js. The two Proxy ones hit the null deref on main; the third (globalThis.Symbol = NaN; Bun.inspect(Bun)in a child) aborts a debug build of main in the next builder's exception-scope check and shows the throwing properties being skipped while the rest of the object is still printed. The child loadsnode:utilbefore clobberingSymbol: on WindowsBun.envhas a custom inspect, andutilInspectFunction's lazy initializer returns without setting anything whennode:utilfails to load, which is a separate bug (it reproduces on main on every platform with any custom-inspect object onceSymbolis clobbered) and is being handled separately.2. Bun object lazy properties whose builder throws
This is what the fuzzer was mostly hitting (
Bun.$/Bun.sql/Bun.SQL/Bun.postgresread near the stack limit, or after clobberingSymbol, andBun.rediswith an invalidREDIS_URL): thePropertyCallbackreturned empty with an exception pending andreifyStaticPropertyput the empty value into the slot. That is fixed in the WebKit this branch now pins:reifyStaticPropertyskips the put for an empty result andsetUpStaticFunctionSlotreports the slot as absent with the exception still pending, so the read throws and the slot stays unreified (a later read runs the builder again; #37714 pins therediscase). Earlier revisions of this PR worked around it in the callbacks by reporting and clearing the exception and reifyingundefined. With the engine fix in place that is worse than what main does (the error stops being catchable and the slot is poisoned for the rest of the process), and CI caught it inbuiltin-esm-lazy-exports.test.ts, so those changes are gone.What remains is a debug-only problem in
defaultBunSQLObject/constructBunSQLObject: underBUN_DEBUGthey reported the exception to the uncaught handler from inside the builder, i.e. in the middle of the property lookup with the exception still pending. The reporter re-enters JS, and a debug build aborts onStructure::storedPrototype'sobject->structure() == thisassertion (globalThis.Symbol = NaN; Bun.sql, or anything that formats the whole Bun object near the stack limit). Removing the report makes these builders behave likeconstructBunShellalready does: the exception propagates to the read. #37258 does the same for theprocess.*builders in BunProcess.cpp (by deferring the report instead); the two do not overlap.Test:
test/js/bun/util/BunObject.test.tsreads$,sql,SQLandpostgrestwice each withSymbolclobbered and expects aTypeErrorevery time. On a debug build of main the child aborts; it passes on release builds of main, since the report was debug-only.Issue #19650 was the engine-level bug and is already closed, so this PR does not claim it.
no test proof · iteration 22 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js