Handle exception from target toString in Bun.plugin#31168
Conversation
|
Updated 1:05 AM PT - May 21st, 2026
❌ @robobun, your commit 0ac1d3d has 5 failures in
🧪 To try this PR locally: bunx bun-pr 31168That installs a local version of the PR into your bun-31168 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds exception handling in the BunPlugin C++ binding when parsing the ChangesPlugin target property exception handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
LGTM — straightforward exception-propagation fix following the standard JSC RETURN_IF_EXCEPTION pattern, 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 is propagated instead of being silently carried into constructEmptyObject, which trips assertNoException() in debug builds. A matching regression test is added to test/js/bun/plugin/plugins.test.ts.
Security risks
None. This is purely defensive exception-handling on a user-supplied options object; it does not touch auth, crypto, filesystem, or network paths. The only behavioral change is that a pending JS exception is now correctly surfaced instead of asserting (debug) or proceeding with a stale exception (release).
Level of scrutiny
Low. The change is two lines of the canonical RETURN_IF_EXCEPTION macro, identical to dozens of other uses in this same function and file (e.g. after getIfPropertyExists, toWTFString, etc.). The first check after targetJSString->value(globalObject) guards rope-resolution failures; the second, placed after the if (auto* targetJSString = ...) block, covers the primary bug where toStringOrNull returned nullptr due to a thrown exception. Both placements are correct and idiomatic.
Other factors
- Fuzzer-found (Fuzzilli) assertion failure with a clear repro in the PR description.
- Test exercises the exact failure path (
target.toString()throws) and asserts the user's error is propagated. - No CODEOWNERS for this path, no outstanding reviewer comments, and no bugs flagged by the bug-hunting system.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #31017. |
When
Bun.plugin()is called with atargetvalue whosetoString()throws (e.g. an object with a throwingtoString, or anything that triggers a stack overflow during string coercion), the pending exception was not checked before proceeding toconstructEmptyObject, tripping theassertNoException()assertion in debug builds.toStringOrNullreturnsnullptron exception, which caused the innerifbody to be skipped without propagating the error.Repro
Before:
ASSERTION FAILED: !exception()inExceptionScope.hAfter:
error: boomis thrown as expected.Found by Fuzzilli (fingerprint
3287a60078202cd7).