Skip to content

Fix missing exception check when coercing Bun.plugin target to a string#32106

Merged
Jarred-Sumner merged 2 commits into
mainfrom
farm/4a031a47/fix-plugin-target-exception
Jun 12, 2026
Merged

Fix missing exception check when coercing Bun.plugin target to a string#32106
Jarred-Sumner merged 2 commits into
mainfrom
farm/4a031a47/fix-plugin-target-exception

Conversation

@robobun

@robobun robobun commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a debug assertion failure (ASSERTION FAILED: Unexpected exception observed / assertNoException()) found by fuzzing, fingerprint 99e6ccd6698f2a64.

Repro:

Bun.plugin({
  setup() {},
  target: { [Symbol.toPrimitive]: () => ({}) },
});

On a debug build this aborts with SIGABRT instead of throwing.

Root cause: in setupBunPlugin (src/jsc/bindings/BunPlugin.cpp), the target property is coerced with JSValue::toStringOrNull, which can run user JS (Symbol.toPrimitive, toString). Per the JSC header, toStringOrNull returns null exactly when an exception was thrown, so the old if (auto* targetJSString = ...) silently skipped validation and continued into builder-object construction and the setup() call with the exception still pending on the VM. Debug builds trip assertNoException() at the next VM entry.

The fix adds RETURN_IF_EXCEPTION after the coercion and after JSString::value (rope resolution can also throw), matching the pattern the rest of the file already uses. The thrown error now propagates to the Bun.plugin() caller and setup() is not invoked.

How did you verify your code works?

  • Added a test in test/js/bun/plugin/plugins.test.ts next to the existing invalid-target test. It asserts the coercion error propagates and that setup is not called. On an unfixed debug build the test aborts with the assertion (exit 134); with the fix it passes. Release builds do not compile the assertion, so the crash is only observable on debug/ASAN builds.
  • Ran the fuzzer repro against the fixed debug build: clean TypeError: Symbol.toPrimitive returned an object (exit 1), also clean under BUN_JSC_validateExceptionChecks=1.
  • Full plugins.test.ts suite passes with the fix (30 pass, 1 todo, 0 fail), including the existing valid and invalid target cases.

When the target property's toString coercion throws (for example a
Symbol.toPrimitive that returns an object), toStringOrNull returns null
with the exception left pending. setupBunPlugin ignored the null and
continued into object construction and the setup() call, tripping
assertNoException() in debug builds.

Propagate the exception with RETURN_IF_EXCEPTION after both the
coercion and the rope resolution, matching the rest of the file.
@robobun

robobun commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 3:13 AM PT - Jun 11th, 2026

@robobun, your commit 0722034da4334925ae7c6b6a3dfc22ff3ec8cfdd passed in Build #61896! 🎉


🧪   To try this PR locally:

bunx bun-pr 32106

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

bun-32106 --bun

@coderabbitai

coderabbitai Bot commented Jun 11, 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: 41aaab53-eccd-4ec3-91bd-19d72eae8efe

📥 Commits

Reviewing files that changed from the base of the PR and between 4c45ff9 and 0722034.

📒 Files selected for processing (1)
  • test/js/bun/plugin/plugins.test.ts

Walkthrough

Refactors plugin.target coercion in setupBunPlugin to use intermediate string variables before validation, and adds tests asserting plugin() throws when opts.target coercion fails via Symbol.toPrimitive returning a non-primitive or toString() throwing; setup is not called in those cases.

Changes

Plugin target option validation

Layer / File(s) Summary
Target option parsing and coercion error handling
src/jsc/bindings/BunPlugin.cpp
plugin.target coercion is refactored to extract intermediate variables (targetJSString, targetString) and then validate allowed values ("node", "bun", "browser"), throwing a TypeError for invalid strings.
Coercion failure tests
test/js/bun/plugin/plugins.test.ts
Adds tests: when target[Symbol.toPrimitive] returns a non-primitive, plugin() throws "Symbol.toPrimitive returned an object"; when target.toString() throws, plugin() propagates that error. Both assert 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 summarizes the main change: fixing a missing exception check during target string coercion in Bun.plugin, which is the core issue addressed by the PR.
Description check ✅ Passed The description includes both required sections: 'What does this PR do?' with detailed root cause analysis and 'How did you verify your code works?' with comprehensive testing methodology.
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.


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 fixes missing exception propagation after toStringOrNull in setupBunPlugin (BunPlugin.cpp) when coercing Bun.plugin target to a string

🤖 Generated with Claude Code

@robobun

robobun commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed: #31286 was an earlier PR for the same bug (as were #28369 and #31017, both closed unmerged). None of them landed, which is why the fuzzer keeps rediscovering this crash. I closed #31286 since its base was months stale with a fully red build, and folded its test case (a target whose toString throws) into this PR alongside the Symbol.toPrimitive repro, so this PR supersedes it with no lost coverage.

@Jarred-Sumner Jarred-Sumner merged commit fd99f8b into main Jun 12, 2026
76 checks passed
@Jarred-Sumner Jarred-Sumner deleted the farm/4a031a47/fix-plugin-target-exception branch June 12, 2026 02:26
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.

2 participants