Skip to content

Propagate exception from Bun.plugin target string conversion#31935

Closed
robobun wants to merge 1 commit into
mainfrom
farm/57e6a654/fix-plugin-target-exception
Closed

Propagate exception from Bun.plugin target string conversion#31935
robobun wants to merge 1 commit into
mainfrom
farm/57e6a654/fix-plugin-target-exception

Conversation

@robobun

@robobun robobun commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Fuzzing found a deterministic debug assertion crash in Bun.plugin() (fingerprint 93b5581fa4549ef5):

Bun.plugin({
  setup() {},
  target: { toString() { return {}; } },
});
ASSERTION FAILED: Unexpected exception observed on thread ...
Error Exception: No default value
!exception()
JavaScriptCore/ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()

Root cause

setupBunPlugin in src/jsc/bindings/BunPlugin.cpp converted the target option with targetValue.toStringOrNull(globalObject). That conversion can run user JS (toString/valueOf via ToPrimitive) and throw, for example the "No default value" TypeError when ToPrimitive yields no primitive. On failure toStringOrNull() returns null with the exception left pending, the null case was silently skipped, and execution continued into constructEmptyObject and the setup call with a live exception, which trips assertNoException in debug builds.

Fix

Convert with toWTFString() followed by RETURN_IF_EXCEPTION (the added RETURN_IF_EXCEPTION(throwScope, {}); line is the fix), matching how the other option reads in this file handle conversions. The conversion error now propagates to the caller as a normal TypeError before setup runs. Non-throwing inputs behave exactly as before, including the existing invalid-target error path.

Test

Added errors > handles 'target' that fails string conversion to test/js/bun/plugin/plugins.test.ts. On the unfixed debug build the test run aborts with the assertion (SIGABRT) at exactly this test; with the fix the whole file passes and the test asserts plugin() throws "No default value" without calling setup. The assertion is compiled out of release builds, so the unfixed release binary already reports the error; the debug assert is the failure mode this guards.

@github-actions github-actions Bot added the claude label Jun 6, 2026
@robobun

robobun commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:09 AM PT - Jun 6th, 2026

@robobun, your commit bb5ca8d has 1 failures in Build #61120 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31935

That installs a local version of the PR into your bun-31935 executable, so you can run:

bun-31935 --bun

@coderabbitai

coderabbitai Bot commented Jun 6, 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: 68cd08d8-4a08-48fc-bbf3-2b2a3cb20dd1

📥 Commits

Reviewing files that changed from the base of the PR and between 73ab3dd and bb5ca8d.

📒 Files selected for processing (2)
  • src/jsc/bindings/BunPlugin.cpp
  • test/js/bun/plugin/plugins.test.ts

Walkthrough

The PR enhances plugin target property validation by unconditionally converting values to strings before validation, replacing the prior conditional check that only validated string values. A test case is added to verify that non-string target values that cannot coerce to valid values throw errors without invoking the setup callback.

Changes

Plugin target validation

Layer / File(s) Summary
Plugin target validation enforcement
src/jsc/bindings/BunPlugin.cpp
Plugin target property is converted to a string via toWTFString() and must match one of "node", "bun", or "browser"; previously non-string values bypassed validation.
Test coverage for target validation
test/js/bun/plugin/plugins.test.ts
Jest is added to imports, and a new error test case verifies that plugin() throws when target is an object whose toString() returns an invalid value, and that the setup callback is not invoked.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing exception propagation when converting the Bun.plugin target option to a string.
Description check ✅ Passed The description includes all required sections (What does this PR do, How did you verify your code works) with comprehensive explanations of the bug, root cause, fix implementation, and test verification.
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.

✏️ 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.cpp

In file included from src/jsc/bindings/BunPlugin.cpp:1:
In file included from src/jsc/bindings/BunPlugin.h:3:
src/jsc/bindings/root.h:42:10: fatal error: 'cmakeconfig.h' file not found
42 | #include "cmakeconfig.h"
| ^~~~~~~~~~~~~~~
1 error generated.
Aborting translation of method 'Zig::jsFunctionAppendOnLoadPluginBody' in file 'src/jsc/bindings/BunPlugin.cpp': "Assert_failure src/clang/cAst_utils.ml:249:53"
Uncaught Internal Error: "Assert_failure src/clang/cAst_utils.ml:249:53"
Error backtrace:
Raised at ClangFrontend__CAst_utils.get_decl_from_typ_ptr in file "src/clang/cAst_utils.ml", line 249, characters 53-65
Called from ClangFrontend__CTrans.CTrans_funct.get_destructor_decl_ref in file "src/clang/cTrans.ml", line 658, characters 12-59
Called from ClangFrontend__CTrans.CTrans_funct.destructor_calls.(fun) in file "src/clang/cTrans.ml", line 2048, characters 12-69
Called from Base__List.rev_filter_map.loop in file "src/list.ml", line 944, characters 13-17
Called f

... [truncated 2200 characters] ...

"src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.function_decl in file "src/clang/cFrontend_decl.ml", line 90, characters 12-151
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.translate_one_declaration in file "src/clang/cFrontend_decl.ml", line 453, characters 10-56
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.translate_one_declaration.translate in file "src/clang/cFrontend_decl.ml" (inlined), line 448, characters 24-96
Called from Stdlib__List.iter in file "list.ml", line 110, characters 12-15
Called from Stdlib__List.iter in file "list.ml", line 110, characters 12-15
Called from Stdlib__List.iter in file "list.ml" (inlined), line 110, characters 17-25
Called from Base__List


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

@claude claude 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.

LGTM — straightforward exception-propagation fix matching the existing toWTFString/RETURN_IF_EXCEPTION pattern in this file, with a regression test.

Extended reasoning...

Overview

This PR fixes a debug-build assertion crash in Bun.plugin() when the target option's string conversion throws (e.g., toString() returning an object triggers a "No default value" TypeError). The fix is a 5-line change in src/jsc/bindings/BunPlugin.cpp: replace targetValue.toStringOrNull(globalObject) (whose null-on-exception result was silently skipped, leaving a live exception that later tripped assertNoException) with targetValue.toWTFString(globalObject) followed by RETURN_IF_EXCEPTION(throwScope, {}). A regression test in test/js/bun/plugin/plugins.test.ts exercises the exact fuzzer repro and asserts the TypeError propagates without invoking setup.

Security risks

None. This tightens error handling on an option-parsing path; no new surface, no auth/crypto/permissions involvement. The change makes a previously-swallowed exception propagate sooner, which is strictly safer.

Level of scrutiny

Low. The change is mechanical and mirrors the established idiom used throughout the same file (e.g., namespaceValue.toWTFString(globalObject); RETURN_IF_EXCEPTION(scope, {}); in both onLoad/onResolve handlers). For non-throwing inputs the behavior is identical — the validation against "node"/"bun"/"browser" still runs and the same error message is thrown for invalid strings. The only difference is that a throwing ToPrimitive now returns early with the user's exception instead of continuing with a pending one.

Other factors

The bug hunting system found no issues. The PR includes a targeted regression test that both reproduces the original SIGABRT on unfixed debug builds and verifies correct behavior post-fix (throws "No default value", setup not called). No outstanding reviewer comments. The diff is small, self-contained, and follows existing conventions in the surrounding code.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. fix(plugin): propagate exception from Bun.plugin target string coercion #31286 - Fixes the same exception propagation bug in setupBunPlugin (BunPlugin.cpp) when the target option's toString() throws, using the same RETURN_IF_EXCEPTION approach

🤖 Generated with Claude Code

@robobun

robobun commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #31286, which fixes the same missing exception check in setupBunPlugin and already has Jarred's review suggestion applied. Closing this one in favor of that PR.

@robobun robobun closed this Jun 6, 2026
@robobun robobun deleted the farm/57e6a654/fix-plugin-target-exception branch June 6, 2026 13:08
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