Skip to content

Fix unchecked exception when Bun.plugin target cannot be converted to a string#31309

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

Fix unchecked exception when Bun.plugin target cannot be converted to a string#31309
robobun wants to merge 1 commit into
mainfrom
farm/dd9dcf61/fix-bun-plugin-target-exception

Conversation

@robobun

@robobun robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes an unchecked JS exception in Bun.plugin() (fuzzer-found, fingerprint e72b98fc73896e0c).

setupBunPlugin converted the target option to a string with toStringOrNull(). When that conversion throws (e.g. target is a Symbol, or an object whose toString returns a Symbol), the code ignored the null result and continued with the exception still pending on the throw scope, eventually invoking the plugin's setup callback. On debug/ASAN builds this trips ExceptionScope::assertNoException() and aborts:

ASSERTION FAILED: Unexpected exception observed on thread ...
Error Exception: Cannot convert a symbol to a string
!exception()
JavaScriptCore/ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()

Repro:

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

The conversion now uses toWTFString() with RETURN_IF_EXCEPTION, matching the rest of the file, so the exception propagates to the caller as a regular TypeError instead of being swallowed while execution continues.

How did you verify your code works?

  • Minimized fuzzer repro now throws TypeError: Cannot convert a symbol to a string instead of aborting (verified on a debug+ASAN build, including with BUN_JSC_validateExceptionChecks=1).
  • Added a regression test in test/js/bun/plugin/plugins.test.ts; bun bd test test/js/bun/plugin/plugins.test.ts passes (30 pass, 1 todo).

@coderabbitai

coderabbitai Bot commented May 24, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, we couldn't start this review because you've used your available PR reviews for now.

Your plan currently allows 1 review/hour. Refill in 58 minutes and 26 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e2510190-c736-464e-8b99-c9c76f17e271

📥 Commits

Reviewing files that changed from the base of the PR and between 295026b and 08b10a8.

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

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

@robobun

robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 10:29 PM PT - May 23rd, 2026

@robobun, your commit 08b10a8 has 4 failures in Build #57462 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31309

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

bun-31309 --bun

@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-check fix matching the existing toWTFString + RETURN_IF_EXCEPTION pattern in this file.

Extended reasoning...

Overview

This PR touches a single code path in src/jsc/bindings/BunPlugin.cpp (setupBunPlugin), replacing toStringOrNull() on the target option with toWTFString() followed by RETURN_IF_EXCEPTION. Previously, when the conversion threw (e.g. target: Symbol()), toStringOrNull returned null, the validation block was skipped, and execution continued with a pending exception on the throw scope — tripping assertNoException() in debug/ASAN builds. The fix is 5 lines of code change plus an 11-line regression test in test/js/bun/plugin/plugins.test.ts.

Security risks

None. This is input validation for a runtime plugin option; the only effect is that an exception now propagates correctly to the caller as a TypeError instead of being left pending. No new data flow, parsing, auth, or filesystem behavior is introduced.

Level of scrutiny

Low. The change is mechanical and mirrors the exact idiom already used multiple times in the same file (e.g. namespaceValue.toWTFString(globalObject); RETURN_IF_EXCEPTION(scope, {}); in both onLoad and onResolve handlers). The non-exception path is semantically unchanged: values that previously stringified successfully and failed/passed the node|bun|browser check still behave identically, since toWTFString and toStringOrNull()->value() both go through ToString semantics.

Other factors

A targeted regression test was added directly adjacent to the existing "handles invalid 'target'" test, and the PR description confirms the full plugins.test.ts suite passes on a debug+ASAN build with BUN_JSC_validateExceptionChecks=1. The bug-hunting system found no issues, no CODEOWNERS apply to this path, and there are no outstanding human reviewer comments.

@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 exception propagation in setupBunPlugin() when Bun.plugin target cannot be converted to a string

🤖 Generated with Claude Code

@robobun

robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #31286, which fixes the same unchecked exception in setupBunPlugin (this crash was a different fuzzer fingerprint for the same root cause) and already has review feedback applied.

@robobun robobun closed this May 24, 2026
@robobun robobun deleted the farm/dd9dcf61/fix-bun-plugin-target-exception branch May 24, 2026 05:28
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