Skip to content

Fix crash in Bun.plugin when target coercion throws#31162

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

Fix crash in Bun.plugin when target coercion throws#31162
robobun wants to merge 1 commit into
mainfrom
farm/87f8d06e/bun-plugin-target-exception

Conversation

@robobun

@robobun robobun commented May 21, 2026

Copy link
Copy Markdown
Collaborator

When the target option passed to Bun.plugin() has a Symbol.toPrimitive or toString that throws (or returns a non-primitive), toStringOrNull() sets a pending exception and returns nullptr. The code then fell through to constructEmptyObject() and call() with the exception still pending, triggering an assertion failure in debug builds:

ASSERTION FAILED: Unexpected exception observed
!exception()
ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()

Repro:

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

Fix: add RETURN_IF_EXCEPTION after coercing target to a string so the exception is propagated to the caller as a normal JS error.

Fuzzer fingerprint: e19793ee445baafd

When the `target` option passed to Bun.plugin() had a Symbol.toPrimitive
or toString that threw (or returned a non-primitive), toStringOrNull()
would set a pending exception and return null. The code then fell
through to constructEmptyObject() and call() with the exception still
pending, triggering an assertion failure in debug builds.

Add RETURN_IF_EXCEPTION checks after the string coercion so the
exception is properly propagated to the caller.
@robobun

robobun commented May 21, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 9:03 PM PT - May 20th, 2026

@robobun, your commit 6ddac7b has 5 failures in Build #56542 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31162

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

bun-31162 --bun

@coderabbitai

coderabbitai Bot commented May 21, 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: 7b75218c-930e-41ea-b5f7-bca7ed9cd47a

📥 Commits

Reviewing files that changed from the base of the PR and between 753f1c4 and 6ddac7b.

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

Walkthrough

The PR improves exception handling for the Bun plugin's target field. The C++ implementation adds early-return exception checks during string coercion, preventing silent failures. Two new test cases verify that coercion errors (TypeError from Symbol.toPrimitive returning non-primitives and exceptions from Symbol.toPrimitive or toString) are properly propagated.

Changes

Plugin target validation exception handling

Layer / File(s) Summary
Exception handling in target string coercion
src/jsc/bindings/BunPlugin.cpp
setupBunPlugin adds RETURN_IF_EXCEPTION checks around the targetValue to targetString conversion and validation, surfacing coercion exceptions via early returns.
Tests for target coercion error cases
test/js/bun/plugin/plugins.test.ts
Two error test cases verify plugin() throws TypeError when Symbol.toPrimitive() returns non-primitive objects, and propagates exceptions from coercion failures.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: adding exception handling when target coercion throws in Bun.plugin.
Description check ✅ Passed The description provides clear problem statement, reproduction case, and solution, but lacks explicit verification details and partially addresses the template structure.
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() for target coercion exception handling
  2. Handle exceptions from toStringOrNull in Bun.plugin target parsing #28369 - Adds the same two RETURN_IF_EXCEPTION guards in setupBunPlugin() at identical locations for the same crash

🤖 Generated with Claude Code

@robobun

robobun commented May 21, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #31017 (same fix, already green).

@robobun robobun closed this May 21, 2026
@robobun robobun deleted the farm/87f8d06e/bun-plugin-target-exception branch May 21, 2026 04:03

@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 following the established RETURN_IF_EXCEPTION pattern, with tests.

Extended reasoning...

Overview

This PR adds two RETURN_IF_EXCEPTION checks in setupBunPlugin() (src/jsc/bindings/BunPlugin.cpp) after coercing the target option to a string. When toStringOrNull() throws (e.g., Symbol.toPrimitive throws or returns a non-primitive) it returns nullptr with a pending exception; the old code fell through to constructEmptyObject() / call() and tripped a debug assertion. The inner check after ->value(globalObject) covers rope-resolution / OOM throws, and the outer check covers the nullptr-with-pending-exception path. Two regression tests are added to plugins.test.ts.

Security risks

None. This only adds early-return guards on an already-existing throw scope in input-validation code. No new attack surface, no auth/crypto/permissions involved.

Level of scrutiny

Low. The change is two lines of boilerplate that match the idiom used dozens of times elsewhere in this same file (and throughout the JSC bindings). It is a fuzzer-found debug-assert fix with a clear repro and a precise, minimal patch.

Other factors

  • Bug hunting system found no issues.
  • New tests directly exercise both the throwing-toPrimitive and throwing-toString paths, plus the "toPrimitive returns object" TypeError case.
  • No CODEOWNERS entry covers this path.
  • 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