Skip to content

Bun.sql getters: do not report a still-pending exception as uncaught in debug builds - #37329

Closed
robobun wants to merge 2 commits into
mainfrom
farm/c03a3846/bun-sql-lazy-getter-pending-exception
Closed

Bun.sql getters: do not report a still-pending exception as uncaught in debug builds#37329
robobun wants to merge 2 commits into
mainfrom
farm/c03a3846/bun-sql-lazy-getter-pending-exception

Conversation

@robobun

@robobun robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a debug-build abort found by fuzzing. The first access of Bun.sql, Bun.postgres or Bun.SQL runs a lazy property builder (defaultBunSQLObject / constructBunSQLObject in BunObject.cpp) that loads the internal bun:sql module. If that load throws (the fuzzer case: the property is first touched from the bottom of a runaway recursion, so entering the module body throws RangeError: Maximum call stack size exceeded), the builder had a #if BUN_DEBUG block that called reportUncaughtExceptionAtEventLoop while the exception was still pending on the VM, and only then fell through to RETURN_IF_EXCEPTION.

Every other caller of that function clears the exception first. Running the uncaught exception machinery with one still set does a process._fatalException lookup and error printing on a VM that has an exception pending, which trips JSC's exception scope assertions. Depending on what state process is in at that point, the abort surfaces as Structure::storedPrototype (object->structure() == this, since setUpStaticFunctionSlot sees the pre-existing exception right after reifying _fatalException), as the EXCEPTION_ASSERT in JSObject::get, or as assertNoExceptionExceptTermination further down, which is why the fuzzer saw it as flaky. It also counted an error the user went on to catch as unhandled (exit code 1, or exit code 7 when it happened inside an uncaughtException handler).

The fix is removing the two debug-only blocks. The exception from requireId already propagates to the property access: reifyStaticProperty skips the putDirect when the builder returns empty and setUpStaticFunctionSlot reports the slot as missing, so the access throws the RangeError, the property stays lazy, and the next access loads the module normally. Release builds never had the extra report, so their behavior is unchanged.

How did you verify your code works?

The fuzzer script (a constructor that recurses into itself and calls Bun.postgres(this) in a try/catch on the way back up) aborts on an unfixed debug build with the storedPrototype assertion, and exits 0 with this change.

Added three tests to test/js/sql/sql.test.ts that touch Bun.sql, Bun.postgres and Bun.SQL from an exhausted stack in a subprocess and check that the access throws the RangeError, that a later access returns the function, and that the process exits 0 with nothing on stderr. On an unfixed debug build the subprocess aborts and all three fail; with this change they pass. Since the removed code was debug-only, the tests also pass on a release build. Also ran the repro with BUN_JSC_validateExceptionChecks=1, which is clean.

…bug builds

defaultBunSQLObject and constructBunSQLObject called
reportUncaughtExceptionAtEventLoop while the exception thrown by
requireId was still pending on the VM. That runs the uncaught exception
machinery (process._fatalException lookup, error printing) with an
exception set, which trips JSC's exception scope assertions and aborts
the debug build, for example when Bun.sql is first touched with the
stack nearly exhausted. It also marked a caught error as unhandled.

The exception already propagates to the property access through
reifyStaticProperty, so just let RETURN_IF_EXCEPTION handle it.
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: fe330909-610d-41a3-9c2b-7172e899d18b

📥 Commits

Reviewing files that changed from the base of the PR and between ecfd4e7 and b51acc1.

📒 Files selected for processing (1)
  • test/js/sql/sql.test.ts

Walkthrough

The SQL constructors no longer report initialization exceptions through debug-only event-loop handling. A subprocess regression test covers stack exhaustion in lazy Bun.sql, Bun.postgres, and Bun.SQL property getters.

Changes

SQL exception handling

Layer / File(s) Summary
Constructor handling and regression coverage
src/jsc/bindings/BunObject.cpp, test/js/sql/sql.test.ts
The SQL constructors remove debug-only uncaught-exception reporting while preserving RETURN_IF_EXCEPTION. The regression test verifies catchable RangeError results, continued property access, clean stderr, and successful subprocess termination.

Possibly related PRs

  • oven-sh/bun#37281: Modifies the same constructors and adds equivalent stack-exhaustion regression coverage.
  • oven-sh/bun#37309: Removes debug-only exception reporting and tests catchable lazy SQL getter errors.
  • oven-sh/bun#37234: Removes the same debug exception reporting and adds related lazy SQL getter coverage.

Suggested reviewers: alii, jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix, although it names Bun.sql instead of all three affected getters.
Description check ✅ Passed The description includes both required sections and clearly explains the change, verification steps, and test coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 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 `@test/js/sql/sql.test.ts`:
- Around line 12822-12828: Remove the filteredStderr transformation in the
subprocess test and assert the raw stderr value directly. Update the expectation
associated with the stdout RangeError/function assertion to require stderr to be
empty, preserving the existing stdout check.
🪄 Autofix

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: 7c047ed7-efd7-464f-8587-743a5d9f9f70

📥 Commits

Reviewing files that changed from the base of the PR and between 827475e and ecfd4e7.

📒 Files selected for processing (2)
  • src/jsc/bindings/BunObject.cpp
  • test/js/sql/sql.test.ts
💤 Files with no reviewable changes (1)
  • src/jsc/bindings/BunObject.cpp

Comment thread test/js/sql/sql.test.ts Outdated
@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Applied the stderr suggestion in b51acc1: the new tests now assert the raw stderr is empty instead of filtering the old ASAN banner out. Checked that this holds on the debug ASAN build (the harness passes allow_user_segv_handler=1 through bunEnv, and the current build prints nothing at startup even without it); the three tests still pass repeatedly under bun bd test.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix stale-structure abort when a lazy Bun property builder throws mid-lookup #37001 - Contains this PR's exact 6-line deletion of the #if BUN_DEBUG reportUncaughtExceptionAtEventLoop blocks in defaultBunSQLObject/constructBunSQLObject, for the same fuzzer-found Structure::storedPrototype abort.
  2. Clear pending exception before continuing the property walk in forEachProperty #37256 - Carries the identical deletion in both SQL lazy getters, with only a small forEachProperty exception-clear in bindings.cpp on top.
  3. Clear pending exceptions from lazy property getters during property enumeration #37213 - Carries the identical deletion, bundled with a lazy-property exception-clearing fix in ZigGlobalObject.cpp/bindings.cpp.
  4. Fix crashes when a lazy property or util.inspect fails while a value is being inspected #37160 - Carries the identical deletion, bundled with stale-exception fixes for property enumeration.
  5. util.isError: propagate a throwing getPrototypeOf trap instead of crashing #37202 - Carries the identical deletion, bundled with a broader console.log property-iteration stale-exception fix.
  6. Fix crashes when inspecting objects whose property enumeration throws #37175 - Carries the identical deletion inside a larger fix for crashes when inspecting objects whose property enumeration throws.
  7. Fix segfault when a lazy Bun.* getter throws during reification #33211 - Removes the same two debug-only blocks for the same bug, but with a conflicting remedy: reify undefined and report, instead of letting the throw propagate.
  8. Fix Bun.inspect null deref with Proxy prototypes; stop reporting from inside the sql lazy-property builders #30245 - Removes the same two blocks in the same two getters, replacing them with a helper that clears, reports, and returns jsUndefined() — same code, opposite resolution.

🤖 Generated with Claude Code

@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed, this is a duplicate. The same two-block removal in defaultBunSQLObject / constructBunSQLObject is already in #37001 (opened Aug 5 for the storedPrototype flavor of this abort, together with the getPropertySlot structure reload on the WebKit side), and #37160, #37175, #37202, #37213 and #37256 carry it as well next to their property enumeration fixes. #37281, which reached it through the same stack exhaustion repro as this PR, was already closed for the same reason. Closing this one in favor of the earlier PRs.

Two notes for whichever of them lands, from verifying this fix against the currently pinned WebKit:

  • The removal on its own is enough for the stack exhaustion repros. The storedPrototype assertion, the EXCEPTION_ASSERT in JSObject::get and the assertNoExceptionExceptTermination signature are all the same call running the uncaught exception path with the exception still set; which one fires depends on whether process._fatalException has been reified yet. Without assertions the same call turns an error the script goes on to catch into exit code 1, or exit code 7 when it happens inside an uncaughtException handler.
  • The test here (b51acc1) probes Bun.sql, Bun.postgres and Bun.SQL from an exhausted stack in a subprocess and checks the access throws, a later access works, and the process exits 0 with an empty stderr. It aborts on an unfixed debug build, and the tests in the PRs above do not target that window directly, so it can be cherry-picked if useful.

@robobun robobun closed this Aug 10, 2026
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