fix(plugin): propagate exception from Bun.plugin target string coercion#31286
fix(plugin): propagate exception from Bun.plugin target string coercion#31286robobun wants to merge 4 commits into
Conversation
When the `target` option passed to Bun.plugin() is an object whose toString() throws, the pending exception was ignored and execution continued into the setup() call, tripping ExceptionScope::assertNoException in debug builds. Check for the exception after coercing `target` to a string and propagate it.
|
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 refactors how the Bun plugin validates the ChangesPlugin target option error handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/jsc/bindings/BunPlugin.cppIn file included from src/jsc/bindings/BunPlugin.cpp:1: ... [truncated 2200 characters] ... "src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52 Comment |
|
Updated 7:05 AM PT - Jun 7th, 2026
❌ @robobun, your commit 6aa3152 has 3 failures in
Add 🧪 To try this PR locally: bunx bun-pr 31286That installs a local version of the PR into your bun-31286 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward addition of RETURN_IF_EXCEPTION checks after toStringOrNull/value, with a regression test.
Extended reasoning...
Overview
This PR adds two RETURN_IF_EXCEPTION(throwScope, {}) checks in setupBunPlugin (src/jsc/bindings/BunPlugin.cpp) after targetValue.toStringOrNull(globalObject) and targetJSString->value(globalObject), so that a JS exception thrown during string coercion of the target option is propagated instead of leaving the VM with a pending exception while continuing to call setup(). A regression test is added in test/js/bun/plugin/plugins.test.ts alongside the existing "handles invalid 'target'" test.
Security risks
None. This is purely defensive exception propagation in argument validation for Bun.plugin(). It does not touch auth, crypto, filesystem, or networking, and does not change any success-path behavior.
Level of scrutiny
Low. The change is a 4-line mechanical application of the standard JSC RETURN_IF_EXCEPTION idiom, identical to the pattern already used on the surrounding lines (e.g., after getIfPropertyExists). The refactor from if (auto* x = ...) to a hoisted assignment + check + if (x) is semantically equivalent when no exception is thrown, and strictly more correct when one is. The added check after ->value(globalObject) is also correct since rope resolution can throw.
Other factors
- The bug-hunting system found no issues.
- A targeted regression test is included and mirrors the existing neighboring test's structure.
- No CODEOWNERS entry covers this path.
- No outstanding human review comments on the PR.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — straightforward RETURN_IF_EXCEPTION fix with a regression test.
Extended reasoning...
Overview
This PR adds two RETURN_IF_EXCEPTION(throwScope, {}) checks in setupBunPlugin (src/jsc/bindings/BunPlugin.cpp) after targetValue.toStringOrNull(globalObject) and targetJSString->value(globalObject), so an exception thrown from a user-supplied target.toString() is propagated instead of leaving a pending exception on the VM. A regression test is added in test/js/bun/plugin/plugins.test.ts that mirrors the existing "handles invalid 'target'" test.
Security risks
None. This is purely defensive exception handling on an edge-case input shape; it does not change auth, permissions, parsing of untrusted data, or any security-sensitive surface.
Level of scrutiny
Low. The change is 4 lines of C++ that hoist a call out of an if condition and insert the standard JSC RETURN_IF_EXCEPTION macro — the exact same pattern already used a few lines above for getIfPropertyExists and throughout the rest of this file (e.g., after toWTFString in jsFunctionAppendOnLoadPluginBody). There are no new control-flow branches beyond the early return, no behavior change for valid inputs, and the test follows the adjacent test's structure.
Other factors
The bug-hunting system found no issues. The duplicate-PR bot flagged two other PRs targeting the same fix, which is a maintainer triage concern rather than a correctness concern for this diff. CI shows failures on an earlier commit (a9c2360) with a retrigger commit on top; given the change is additive exception checks plus a self-contained test, any CI failures are very likely unrelated flakes, but maintainers should confirm green CI before merge as usual.
|
CI status
Every failure across the three builds is infrastructure/flake in files untouched by this PR; the plugin test suite passes on every platform, including with the Update for build #61272 (retriggered on |
| RETURN_IF_EXCEPTION(throwScope, {}); | ||
| if (targetValue) { | ||
| if (auto* targetJSString = targetValue.toStringOrNull(globalObject)) { | ||
| auto* targetJSString = targetValue.toStringOrNull(globalObject); |
There was a problem hiding this comment.
| auto* targetJSString = targetValue.toStringOrNull(globalObject); | |
| auto* targetJSString = targetValue.toString(globalObject); |
There was a problem hiding this comment.
Applied in 993e93a — switched to toString() and also dropped the null check since toString() returns the empty string (never null) on exception; the RETURN_IF_EXCEPTION right after it handles the error path. Repro and test/js/bun/plugin/plugins.test.ts still pass with the debug build.
toString() returns the empty string instead of null on exception, so the null check is unnecessary; keep the exception checks.
There was a problem hiding this comment.
LGTM — straightforward exception-propagation fix with a regression test, and Jarred's review suggestion has been applied.
Extended reasoning...
Overview
This PR fixes a missing exception check in setupBunPlugin (src/jsc/bindings/BunPlugin.cpp): when coercing the target option to a string, the old code used toStringOrNull and silently fell through on a pending exception. The fix replaces it with toString() followed by RETURN_IF_EXCEPTION (and another after ->value()), which is the same pattern already used elsewhere in this file (e.g. in JSMock__jsModuleMock). A regression test is added to test/js/bun/plugin/plugins.test.ts.
Security risks
None. This is purely error-path handling for a user-supplied option object; no auth, crypto, filesystem, or network surface is touched.
Level of scrutiny
Low. The diff is ~7 lines of logic that mechanically applies the canonical JSC exception-check idiom, plus a 15-line test. The only behavioral change is that an exception thrown from target[Symbol.toPrimitive]/toString now surfaces to the caller instead of tripping a debug assertion / leaving a pending exception — strictly a correctness improvement on an edge case found by fuzzing.
Other factors
- A maintainer (Jarred-Sumner) already reviewed and left a concrete suggestion (use
toString()instead oftoStringOrNull()), which was applied in 993e93a and acknowledged. - CI: the plugin test suite passed on every platform where it ran; the only failures were unrelated infra flakes (OOM SIGKILL, agent expiration, IPC timeout).
- No CODEOWNERS entry covers these files.
- The bug-hunting system found no issues.
|
Strengthened the regression test in 6aa3152: it now also asserts |
There was a problem hiding this comment.
LGTM — straightforward exception-propagation fix following the standard JSC pattern, with the maintainer's suggestion already applied and a regression test added.
Extended reasoning...
Overview
This PR fixes a missed exception check in setupBunPlugin (src/jsc/bindings/BunPlugin.cpp). Previously, targetValue.toStringOrNull() could throw and return nullptr, after which the code silently fell through the if and proceeded to call the user's setup() with a pending exception — tripping a debug assertion and leaving the VM in an inconsistent state in release builds. The fix replaces it with toString() followed by RETURN_IF_EXCEPTION, plus a second RETURN_IF_EXCEPTION after ->value(). A regression test is added in test/js/bun/plugin/plugins.test.ts asserting both that the user's error propagates and that setup is never invoked.
Security risks
None. This is pure error-path handling for an obscure input shape (an object with a throwing toString passed as target). No new data flows, no auth/crypto/permissions.
Level of scrutiny
Low. The diff is ~7 lines of mechanical change that exactly matches the idiom used throughout this file and the rest of the JSC bindings (toString → RETURN_IF_EXCEPTION → value → RETURN_IF_EXCEPTION). The only semantic change is that a previously-swallowed pending exception now returns early — which is strictly more correct. The validation logic itself (node/bun/browser check) is unchanged.
Other factors
Jarred-Sumner already reviewed and left a single suggestion (use toString() instead of toStringOrNull()), which was applied in 993e93a and acknowledged. CI is green across platforms for the touched test file; the only failures across three builds were unrelated infra flakes. The bug-hunting system found no issues. The latest commit (6aa3152) only strengthens the test to also fail on an unfixed release build — a test-only improvement.
|
Closing in favor of #32106, which is the same fix rebased on current main. This branch's base is from May and its last build (#61272) failed across every platform, so it would need a full rebase anyway. #32106 carries over this PR's test coverage: it asserts both that the coercion error propagates and that setup() is never invoked, for a throwing toString and for a Symbol.toPrimitive that returns an object (the shape the latest fuzzer report hit). |
2 participants
What does this PR do?
Fixes a crash (debug assertion
ExceptionScope::assertNoException) found by fuzzing inBun.plugin().When the
targetoption is an object whosetoString()throws,setupBunPlugincalledtargetValue.toStringOrNull(globalObject), got backnullptr, and silently fell through with the exception still pending. It then continued building the builder object and invoked the user'ssetup()function while an exception was pending, which tripsExceptionScope::assertNoException()in debug builds and leaves the VM in an inconsistent exception state in release builds.Now the exception from the string coercion of
targetis checked and propagated to the caller as a normal JS error.How did you verify your code works?
ASSERTION FAILED: Unexpected exception observedbefore the fix and throws a catchableError: boomafter the fix (debug/ASAN build).test/js/bun/plugin/plugins.test.ts("handles a 'target' whose toString throws");bun bd test test/js/bun/plugin/plugins.test.tspasses (30 pass, 1 todo, 0 fail).