Skip to content

fix(plugin): handle exception from Bun.plugin target string coercion#32062

Closed
robobun wants to merge 1 commit into
mainfrom
farm/1659c342/fix-plugin-target-exception-check
Closed

fix(plugin): handle exception from Bun.plugin target string coercion#32062
robobun wants to merge 1 commit into
mainfrom
farm/1659c342/fix-plugin-target-exception-check

Conversation

@robobun

@robobun robobun commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a debug assertion failure (ASSERTION FAILED: Unexpected exception observed, ExceptionScope.h(61) assertNoException) found by fuzzing, fingerprint 890c68dc4cf1789a.

In setupBunPlugin (src/jsc/bindings/BunPlugin.cpp), the target option is coerced with targetValue.toStringOrNull(globalObject). That coercion can run user JS (toString/valueOf). When it throws, for example when toString returns a non-primitive so OrdinaryToPrimitive raises "No default value", toStringOrNull returns null and the old code simply skipped the validation block and kept going with the exception still pending. The subsequent call() into the plugin's setup function then trips assertNoException and aborts debug builds.

The fix adds RETURN_IF_EXCEPTION after toStringOrNull and after targetJSString->value() (rope resolution can also throw), so the coercion error propagates to the caller as a normal catchable TypeError.

Repro that aborted before this change:

Bun.plugin({
  setup: () => {},
  target: { toString: () => ({}) },
});

How did you verify your code works?

  • The repro now throws TypeError: No default value cleanly on a debug build (also under BUN_JSC_validateExceptionChecks=1) instead of asserting.
  • Added a regression test in test/js/bun/plugin/plugins.test.ts ("handles 'target' whose string coercion throws"). It aborts the unfixed debug build with the exact assertion from the fuzzer (SIGABRT, exit 134) and passes with the fix. Release builds already surfaced the error correctly because JSC bails out at VM entry with a pending exception, so the bug was only observable on debug/ASAN builds.
  • Full test/js/bun/plugin/plugins.test.ts suite passes with the debug build (30 pass, 1 pre-existing todo).

@robobun

robobun commented Jun 10, 2026

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

@robobun, your commit 58e33ca has 1 failures in Build #61767 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32062

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

bun-32062 --bun

@coderabbitai

coderabbitai Bot commented Jun 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: da2b8f7b-0aac-43d0-8363-805d2c98c427

📥 Commits

Reviewing files that changed from the base of the PR and between 2ad4199 and 58e33ca.

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

Walkthrough

The PR refactors target property parsing in the Bun plugin system to explicitly handle exceptions when string coercion fails. The C++ implementation separates the toStringOrNull call from the condition check and adds exception verification, while a corresponding test validates that invalid string coercion throws "No default value" without invoking the setup callback.

Changes

Plugin target coercion error handling

Layer / File(s) Summary
Target coercion exception handling
src/jsc/bindings/BunPlugin.cpp
targetValue.toStringOrNull is now stored in targetJSString with an explicit exception check before conditionally extracting targetString; target validation logic is unchanged.
Target coercion error validation
test/js/bun/plugin/plugins.test.ts
Test imports jest and adds a case validating plugin() throws "No default value" when target string coercion results in an object instead of a string, confirming the setup function is not called.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main fix: handling an exception that occurs during target string coercion in Bun.plugin, which is the core change across both modified files.
Description check ✅ Passed The description comprehensively covers all required template sections (What does this PR do, How did you verify your code works) with detailed technical context, repro steps, and verification methods.
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.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. fix(plugin): propagate exception from Bun.plugin target string coercion #31286 - Also adds missing RETURN_IF_EXCEPTION guards after target string coercion in setupBunPlugin (BunPlugin.cpp), with a regression test in plugins.test.ts

🤖 Generated with Claude Code

@robobun

robobun commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #31286, which fixes the same missing exception check in setupBunPlugin. That PR is further along: Jarred's review suggestion (toString instead of toStringOrNull) is already applied and its regression test also covers the release-build behavior. This one came from a different fuzzer input hitting the same code path.

@robobun robobun closed this Jun 10, 2026
@robobun robobun deleted the farm/1659c342/fix-plugin-target-exception-check branch June 10, 2026 13:01
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