Skip to content

fix(Bun.plugin): propagate exception thrown while converting 'target' to a string#31390

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

fix(Bun.plugin): propagate exception thrown while converting 'target' to a string#31390
robobun wants to merge 1 commit into
mainfrom
farm/be6c7681/bun-plugin-target-exception

Conversation

@robobun

@robobun robobun commented May 25, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a debug assertion failure (ASSERTION FAILED: Unexpected exception observed ... !exception() in ExceptionScope::assertNoException) found by fuzzing, fingerprint 34a19a3501a83cae.

Bun.plugin() converted the target option to a string with toStringOrNull() and silently ignored the case where the conversion throws — for example when target is an object whose Symbol.toPrimitive returns an object. The pending TypeError: Symbol.toPrimitive returned an object was then carried into the subsequent object construction and the setup() call, tripping assertNoException in debug builds (and invoking setup() with a pending exception in release builds).

The fix converts target with toWTFString() and returns early if the conversion throws, matching how the rest of BunPlugin.cpp handles string conversions.

Repro that crashed before:

const v = {};
v[Symbol.toPrimitive] = () => [1, 2, 3];
v.target = v;
Object.defineProperty(v, "setup", { value: ArrayBuffer });
Bun.plugin(v);

Now it throws TypeError: Symbol.toPrimitive returned an object instead.

How did you verify your code works?

  • Reproduced the assertion failure with the fuzzilli debug build before the change; after the change the script exits with a proper TypeError.
  • Added a regression test in test/js/bun/plugin/plugins.test.ts (errors > handles a 'target' that throws while being converted to a string); ran the full file with bun bd test test/js/bun/plugin/plugins.test.ts — 30 pass, 0 fail.
  • Verified with BUN_JSC_validateExceptionChecks=1 that no unchecked-exception warnings are reported on this path.

… to a string

Bun.plugin() converted the 'target' option to a string with toStringOrNull()
and ignored the case where the conversion throws (e.g. a Symbol.toPrimitive
that returns an object). The pending exception was then carried into
subsequent JSC calls, failing the assertNoException assertion in debug
builds. Use toWTFString() and return early if the conversion throws.
@coderabbitai

coderabbitai Bot commented May 25, 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 includes 5 reviews of capacity. Refill in 16 minutes and 39 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: 2653b11a-0fb8-4133-9139-e2591c94e605

📥 Commits

Reviewing files that changed from the base of the PR and between a49b308 and ab9fd78.

📒 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 25, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:47 AM PT - May 25th, 2026

@robobun, your commit ab9fd78 has 1 failures in Build #57914 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31390

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

bun-31390 --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-propagation fix matching the existing toWTFString + RETURN_IF_EXCEPTION pattern used elsewhere in this file, with a regression test.

Extended reasoning...

Overview

This PR changes 6 lines in setupBunPlugin() (src/jsc/bindings/BunPlugin.cpp) to convert the target option via toWTFString() + RETURN_IF_EXCEPTION instead of toStringOrNull() with no exception check. It adds a 15-line regression test in test/js/bun/plugin/plugins.test.ts.

The previous code called toStringOrNull(), which returns nullptr when ToString throws (e.g., a Symbol.toPrimitive that returns an object), and then silently fell through with a pending exception into constructEmptyObject / call(setup, ...), tripping assertNoException in debug builds. The new code returns early on the exception, which is the correct JSC idiom and exactly mirrors how namespaceValue.toWTFString(globalObject) and moduleIdValue.toWTFString(globalObject) are handled elsewhere in the same file.

Security risks

None. This is purely an exception-handling correctness fix on a validation-only code path (the converted targetString is only compared against "node"/"bun"/"browser" and never stored or used afterward). No new attack surface, no auth/crypto/permissions involvement.

Level of scrutiny

Low. The diff is tiny, mechanical, and follows an established pattern repeated throughout BunPlugin.cpp. The only behavioral change is that an exception thrown during target stringification now surfaces immediately instead of being carried into later calls — strictly a correctness improvement. The success path (valid string target) is semantically unchanged since both toStringOrNull and toWTFString invoke the same ToString abstract operation.

Other factors

  • The bug-hunting system found no issues.
  • A focused regression test was added next to the existing "handles invalid 'target'" test and the author reports the full test file passes (30/0) plus a clean BUN_JSC_validateExceptionChecks=1 run.
  • No CODEOWNERS entry covers these paths.
  • No prior reviewer comments are outstanding.

@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 Bun.plugin() when converting target to a string, replacing toStringOrNull with toWTFString + RETURN_IF_EXCEPTION in BunPlugin.cpp

🤖 Generated with Claude Code

@robobun

robobun commented May 25, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #31286, which fixes the same unchecked exception in setupBunPlugin's target string conversion and already includes a regression test.

@robobun robobun closed this May 25, 2026
@robobun robobun deleted the farm/be6c7681/bun-plugin-target-exception branch May 25, 2026 11:47
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