Fix missing exception check on Bun.plugin target coercion#31956
Conversation
setupBunPlugin converted the target option to a string without checking for a pending exception. A target object whose toString throws left the exception pending while setup() was still invoked, tripping ExceptionScope::assertNoException in debug builds.
|
Updated 2:03 AM PT - Jun 7th, 2026
❌ @robobun, your commit 0984a55 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31956That installs a local version of the PR into your bun-31956 --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)
WalkthroughThis PR improves error handling in BunPlugin's target property parsing by adding explicit exception checks when converting the target value to a string, and adds a test case to verify that conversion errors are properly propagated. ChangesTarget toString() 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 |
|
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}
There was a problem hiding this comment.
LGTM — straightforward addition of missing RETURN_IF_EXCEPTION checks following the established JSC pattern, with a regression test.
Extended reasoning...
Overview
This PR adds two RETURN_IF_EXCEPTION checks in setupBunPlugin (src/jsc/bindings/BunPlugin.cpp) after toStringOrNull() and JSString::value(), plus a regression test in test/js/bun/plugin/plugins.test.ts. It fixes a fuzzer-found assertion failure where a throwing target coercion left a pending exception while execution continued into setup().
Security risks
None. This is a hardening change that makes exception propagation correct; it adds no new code paths, parsing, or external input handling.
Level of scrutiny
Low. The change is mechanical and matches the ubiquitous JSC exception-checking idiom already used throughout this file (and the rest of the bindings). The diff is tiny and the intent is unambiguous.
Other factors
- The added test directly exercises the failing case (throwing
toStringontarget) and asserts both that the error propagates and thatsetupis not invoked. - The one finding from review is a nit: the
if (targetJSString)guard is now dead (always true) after the newRETURN_IF_EXCEPTION. This has zero runtime impact and the belt-and-suspenders shape already appears elsewhere in the codebase, so it doesn't block approval. - No prior reviewer comments to address.
| auto* targetJSString = targetValue.toStringOrNull(globalObject); | ||
| RETURN_IF_EXCEPTION(throwScope, {}); | ||
| if (targetJSString) { |
There was a problem hiding this comment.
🟡 nit: now that RETURN_IF_EXCEPTION follows toStringOrNull(), targetJSString can never be null on line 311 — toStringOrNull returns null iff an exception is pending (see the [[ZIG_EXPORT(null_is_throw)]] annotation at bindings.cpp:4697). Per CLAUDE.md ("guards a new validator makes redundant — a leftover null-check misleads readers into thinking the value can be absent"), drop the if (targetJSString) wrapper and unindent the body.
Extended reasoning...
What & why
JSC's JSValue::toStringOrNull(globalObject) is implemented as toString() followed by an internal RETURN_IF_EXCEPTION(scope, nullptr). toString() itself never returns null (it returns an empty JSString on failure), so the only way toStringOrNull yields nullptr is when an exception is pending. Bun's own FFI export documents this contract explicitly:
// src/jsc/bindings/bindings.cpp:4697
[[ZIG_EXPORT(null_is_throw)]] JSC::JSString* JSC__JSValue__toStringOrNull(...)This PR adds RETURN_IF_EXCEPTION(throwScope, {}) immediately after the toStringOrNull call (line 310). Once that macro returns on a pending exception, control reaching line 311 implies no exception is pending, which in turn implies targetJSString != nullptr. The if (targetJSString) guard is therefore unreachable-false — dead code introduced by this very change.
Step-by-step proof
Consider the two possible outcomes of line 309:
toString/@@toPrimitivethrows (the fuzzer case this PR fixes).toStringOrNullreturnsnullptrand sets a pending exception. Line 310'sRETURN_IF_EXCEPTIONobserves the exception and returns{}. Line 311 is never reached.- Coercion succeeds.
toString()returns a non-nullJSString*; the internal scope sees no exception, sotoStringOrNullforwards that non-null pointer. Line 310 sees no exception and falls through. Line 311 evaluatesif (non-null)— always true.
There is no third path where line 311 is reached with targetJSString == nullptr.
Why existing code doesn't justify keeping it
One reviewer might note that NodeModuleModule.cpp:220-224 has the same belt-and-suspenders shape (toStringOrNull → RETURN_IF_EXCEPTION → if (!code)). However, the codebase is not uniform here — JSBuffer.cpp:279-281 and ErrorCode.cpp:376-378 both dereference the result directly after RETURN_IF_EXCEPTION with no null guard:
auto arg_ = arg.toStringOrNull(lexicalGlobalObject);
RETURN_IF_EXCEPTION(scope, {});
const auto& view = arg_->view(lexicalGlobalObject); // no null checkSo "prevailing convention" cuts both ways. The tiebreaker is the project's own contributor guidance — CLAUDE.md:323 is unambiguous and calls out this exact scenario:
Delete dead code in the same PR that makes it dead (these deletions are required scope, not drive-bys) … guards a new validator makes redundant (a leftover null-check misleads readers into thinking the value can be absent)
This PR added the validator (RETURN_IF_EXCEPTION); per the documented rule, removing the now-redundant guard is in-scope for the same PR rather than a drive-by style preference.
Impact
None at runtime — the branch is always taken. The cost is purely readability: a future reader seeing if (targetJSString) after the exception check will reasonably (but wrongly) infer there's a non-exception path where toStringOrNull returns null, and may cargo-cult that defensive shape elsewhere.
Fix
auto* targetJSString = targetValue.toStringOrNull(globalObject);
RETURN_IF_EXCEPTION(throwScope, {});
String targetString = targetJSString->value(globalObject);
RETURN_IF_EXCEPTION(throwScope, {});
if (!(targetString == "node"_s || targetString == "bun"_s || targetString == "browser"_s)) {
JSC::throwTypeError(globalObject, throwScope, "plugin target must be one of 'node', 'bun' or 'browser'"_s);
return {};
}(Equivalently, switch to targetValue.toString(globalObject) — which never returns null — and keep the RETURN_IF_EXCEPTION; either form removes the dead branch.)
|
Closing as a duplicate of #31286, which fixes the same missing exception check in |
ZhengSJCode
left a comment
ZhengSJCode
left a comment
There was a problem hiding this comment.
good
Commented in CodeRabbit Change Stack
2 participants
Fuzzing found a debug assertion failure (
ExceptionScope::assertNoException, fingerprint351df89ce3773a75) inBun.plugin():Root cause
setupBunPlugininsrc/jsc/bindings/BunPlugin.cppcoerces thetargetoption withtoStringOrNull(), which can run user JS (toString/valueOf). When that coercion throws (here,OrdinaryToPrimitivefails with "No default value"),toStringOrNull()returns null, the validation block was skipped, and execution continued with the exception still pending: the builder object was constructed andsetup()was invoked. Debug builds abort on the assertion when re-entering the VM; release builds incorrectly callsetup()before the error finally surfaces.Fix
Add the two missing
RETURN_IF_EXCEPTIONchecks aftertoStringOrNull()andJSString::value()so the exception propagates to the caller as a normal catchable error. The other string coercions in this file already check exceptions correctly.Test
Added
handles a 'target' whose toString throwstotest/js/bun/plugin/plugins.test.ts, next to the existing invalid-target test. It asserts the thrown error propagates and thatsetupis never invoked. On the unfixed build it fails withsetupcalled once (release) or the assertion abort (debug); it passes with this fix.