Skip to content

Fix unchecked exception in Bun.plugin when target toString throws#31151

Closed
robobun wants to merge 1 commit into
mainfrom
farm/1f0e65e5/plugin-target-tostring-exception
Closed

Fix unchecked exception in Bun.plugin when target toString throws#31151
robobun wants to merge 1 commit into
mainfrom
farm/1f0e65e5/plugin-target-tostring-exception

Conversation

@robobun

@robobun robobun commented May 20, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes an assertNoException() failure in setupBunPlugin found by Fuzzilli (fingerprint e094de1fac1a5ee5).

When Bun.plugin({ target, setup }) is called with a target value whose string coercion throws (e.g. an object with a throwing toString), toStringOrNull returns nullptr and leaves the exception pending. The code then fell through to constructEmptyObject, which asserts that no exception is pending.

Bun.plugin({
  setup: () => {},
  target: { toString() { throw new Error("boom"); } },
});

Before: ASSERTION FAILED: !exception() in debug builds.
After: the thrown error is propagated to the caller.

How did you verify your code works?

  • Reproduced the assertion with the fuzzer output and a minimized repro on the debug+ASAN build.
  • After the fix, both reproduce cleanly as a thrown JS error.
  • Added a regression test to test/js/bun/plugin/plugins.test.ts.

@robobun

robobun commented May 20, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:28 PM PT - May 20th, 2026

@robobun, your commit 9ec2e27 has 9 failures in Build #56479 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31151

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

bun-31151 --bun

@coderabbitai

coderabbitai Bot commented May 20, 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: 05d5232f-038b-4316-97c4-50d2c1fa0d57

📥 Commits

Reviewing files that changed from the base of the PR and between a43a01b and 9ec2e27.

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

Walkthrough

This PR hardens exception handling during Bun plugin target validation. Two RETURN_IF_EXCEPTION guards are added to setupBunPlugin to detect and propagate exceptions during target parsing, and a test verifies that exceptions thrown from invalid target values are correctly forwarded to the caller.

Changes

Plugin Target Exception Handling

Layer / File(s) Summary
Target validation exception guards and test
src/jsc/bindings/BunPlugin.cpp, test/js/bun/plugin/plugins.test.ts
Exception handling is tightened in setupBunPlugin target parsing with RETURN_IF_EXCEPTION guards after targetString extraction and validation. A test verifies that exceptions thrown from target.toString() are propagated correctly when an invalid target is provided to plugin().
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: handling unchecked exceptions in Bun.plugin when target toString throws.
Description check ✅ Passed The description covers both required sections with clear explanation of the problem, code example, and verification method including test addition.
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 unchecked exception when Bun.plugin target coercion throws #31017 - Adds the same two RETURN_IF_EXCEPTION guards in setupBunPlugin in BunPlugin.cpp to fix the unchecked exception when target.toString() throws
  2. Handle exceptions from toStringOrNull in Bun.plugin target parsing #28369 - Adds the identical RETURN_IF_EXCEPTION guards in setupBunPlugin for the same crash path where a throwing Symbol.toPrimitive on target leaves a pending exception before constructEmptyObject

🤖 Generated with Claude Code

@robobun

robobun commented May 20, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #31017 (and #28369). Closing in favor of the existing PR.

@robobun robobun closed this May 20, 2026
@robobun robobun deleted the farm/1f0e65e5/plugin-target-tostring-exception branch May 20, 2026 23:27

@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 RETURN_IF_EXCEPTION guard for a fuzzer-found unchecked exception, with a regression test.

Extended reasoning...

Overview

This PR adds two RETURN_IF_EXCEPTION(throwScope, {}) checks in setupBunPlugin (src/jsc/bindings/BunPlugin.cpp) so that an exception thrown during target string coercion (toStringOrNull / JSString::value) is propagated instead of falling through to constructEmptyObject, which asserts no pending exception. A regression test in test/js/bun/plugin/plugins.test.ts exercises a target whose toString throws and asserts the error surfaces.

Security risks

None. This is purely an exception-propagation correctness fix in argument validation; no auth, crypto, permissions, or untrusted-input parsing changes. If anything it hardens behavior by failing fast on a pending exception rather than continuing in an inconsistent VM state.

Level of scrutiny

Low. The change is two lines of the canonical JSC idiom (RETURN_IF_EXCEPTION after a fallible operation) already used dozens of times in this same file. Placement is correct: the outer check covers toStringOrNull returning nullptr with a pending exception, and the inner check covers value(globalObject) (rope resolution can OOM). No control-flow or semantic changes beyond early-returning the existing exception.

Other factors

  • Fuzzer-reported assertion failure with a clear minimized repro in the PR description.
  • Regression test added next to the existing "handles invalid 'target'" test, following the same pattern.
  • Bug-hunting system found no issues.
  • No CODEOWNERS coverage for this path and no outstanding reviewer comments.

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