Skip to content

Fix pending exception when Bun.plugin target coercion throws#31662

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

Fix pending exception when Bun.plugin target coercion throws#31662
robobun wants to merge 1 commit into
mainfrom
farm/0ae67d90/fix-plugin-target-exception-check

Conversation

@robobun

@robobun robobun commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a missing exception check in setupBunPlugin (Bun.plugin()), found by fuzzing.

When the plugin options object has a target property whose string coercion throws — e.g. an object whose Symbol.toPrimitive returns an object — toStringOrNull() returns null with a pending exception. The code treated the null as "no valid string, skip validation" and continued on to build the builder object and invoke the plugin's setup() function with the exception still pending. In debug builds this trips ExceptionScope::assertNoException() and aborts:

ASSERTION FAILED: Unexpected exception observed on thread ...
Error Exception: Symbol.toPrimitive returned an object
!exception()
JavaScriptCore/ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()

Repro:

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

The fix adds RETURN_IF_EXCEPTION after the toStringOrNull() and JSString::value() calls so the exception propagates to the caller as a normal catchable TypeError, matching the pattern used elsewhere in the bindings (e.g. JSBuffer.cpp, ErrorCode.cpp).

How did you verify your code works?

  • Reproduced the assertion failure with a debug (ASAN) build before the fix; after the fix the same script throws a catchable TypeError and exits cleanly.
  • Added a regression test in test/js/bun/plugin/plugins.test.ts (errors > handles 'target' that throws while being converted to a string). It aborts with the assertion on an unfixed debug build and passes with the fix.
  • bun bd test test/js/bun/plugin/plugins.test.ts, test/js/bun/plugin/plugin-namespace-drive-letter.test.ts, and test/bundler/bundler_plugin.test.ts all pass.

setupBunPlugin called toStringOrNull on the plugin's target property but
did not check for an exception when the conversion failed (e.g. when
Symbol.toPrimitive returns an object). The pending exception was carried
into the subsequent call to the plugin's setup() function, which trips
ExceptionScope::assertNoException in debug builds.

Propagate the exception immediately, matching the pattern used elsewhere
in the bindings.
@github-actions github-actions Bot added the claude label Jun 1, 2026
@robobun

robobun commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:10 AM PT - Jun 1st, 2026

@robobun, your commit 243c0c8 has 1 failures in Build #59551 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31662

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

bun-31662 --bun

@coderabbitai

coderabbitai Bot commented Jun 1, 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: ae44dd0f-1119-49e7-b79c-6fee961a5fe9

📥 Commits

Reviewing files that changed from the base of the PR and between 5ac120c and 243c0c8.

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

Walkthrough

This PR refactors exception handling in the plugin target property binding. The setupBunPlugin function now converts the target value to a string and immediately checks for exceptions from that conversion before attempting validation. A test verifies the plugin correctly rejects target options whose string conversion throws.

Changes

Plugin target conversion exception handling

Layer / File(s) Summary
Exception handling during target string conversion
src/jsc/bindings/BunPlugin.cpp, test/js/bun/plugin/plugins.test.ts
The toStringOrNull conversion of targetValue is moved outside the null check with RETURN_IF_EXCEPTION added immediately after to catch conversion errors before validation. A new test verifies that plugin() rejects a target option when its string conversion throws via Symbol.toPrimitive.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing a missing exception check when the Bun.plugin target property's string coercion throws an exception.
Description check ✅ Passed The description fully addresses both required template sections with comprehensive technical details, including problem explanation, reproduction case, fix approach, and verification steps.
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

github-actions Bot commented Jun 1, 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 - Both fix the same pending-exception bug in setupBunPlugin's target coercion path in BunPlugin.cpp by adding RETURN_IF_EXCEPTION guards after toStringOrNull()/JSString::value() calls

🤖 Generated with Claude Code

@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 addition of RETURN_IF_EXCEPTION after toStringOrNull()/value(), with a regression test.

Extended reasoning...

Overview

This PR touches two files: src/jsc/bindings/BunPlugin.cpp (4 lines added in setupBunPlugin) and test/js/bun/plugin/plugins.test.ts (one new regression test). The change pulls targetValue.toStringOrNull(globalObject) out of the if condition and adds RETURN_IF_EXCEPTION(throwScope, {}) after it and after targetJSString->value(globalObject). This is the standard JSC idiom for handling pending exceptions after operations that may invoke user code (here, Symbol.toPrimitive / toString), and matches dozens of identical patterns elsewhere in the bindings.

Security risks

None. The change strictly tightens error handling on an edge-case input — it adds early returns where an exception is already pending, preventing a debug assertion / undefined behavior from continuing with a pending exception. There is no new attack surface, no auth/crypto/permission logic, and no data exposure.

Level of scrutiny

Low. This is a mechanical, well-understood JSC binding pattern (RETURN_IF_EXCEPTION after a fallible JS operation). The diff is tiny and self-contained, the surrounding control flow is unchanged for the non-throwing path, and the fix is verified by a targeted regression test that asserts both that the exception surfaces as a catchable TypeError and that setup() is not invoked.

Other factors

  • The bug-hunting system found no issues.
  • No CODEOWNERS cover the affected paths.
  • No outstanding reviewer comments.
  • The added RETURN_IF_EXCEPTION after JSString::value() is technically defensive (rope resolution OOM), but harmless and consistent with the codebase's conventions.

@robobun

robobun commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #31286, which fixes the same missing exception check in setupBunPlugin's target coercion path and is already reviewed with green CI (plugin test suite passing on all platforms). That PR's toString() + RETURN_IF_EXCEPTION change covers this crash case (Symbol.toPrimitive returning an object) as well — verified the same code path is guarded. Keeping a single PR for this fix, consistent with #28369 which was previously closed in favor of #31286.

@robobun robobun closed this Jun 1, 2026
@robobun robobun deleted the farm/0ae67d90/fix-plugin-target-exception-check branch June 1, 2026 18:09
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