Handle exceptions from toStringOrNull in Bun.plugin target parsing#28369
Handle exceptions from toStringOrNull in Bun.plugin target parsing#28369robobun wants to merge 1 commit into
Conversation
|
Updated 11:34 PM PT - Mar 25th, 2026
❌ @robobun, your commit 6751096 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 28369That installs a local version of the PR into your bun-28369 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded explicit exception checks in the Bun.plugin native setup to return early if a pending JS exception exists while extracting/processing the plugin target. Added a spawned-process regression test for a target whose Symbol.toPrimitive returns a non-primitive. Also swapped import order in an unrelated s3 test file. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
No related open issues found. This PR addresses a specific and previously unreported crash scenario involving 🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the two RETURN_IF_EXCEPTION additions correctly fix the crash by propagating the pending JSC exception rather than continuing into constructEmptyObject.
Extended reasoning...
Overview
This PR touches two files: src/bun.js/bindings/BunPlugin.cpp (2-line logic change) and a new test file. The C++ change adds RETURN_IF_EXCEPTION after JSString::value(globalObject) (which can throw) and after the entire toStringOrNull conditional block (to catch the case where toStringOrNull returned null because it threw, but the inner if body was skipped).
Security risks
None. This is a crash fix that converts an assertion failure into a proper JS TypeError. No auth, crypto, or permission code is involved.
Level of scrutiny
Low-to-medium. The pattern of adding RETURN_IF_EXCEPTION after JSC operations that can throw is mechanical and well-established throughout this file and the broader codebase. Both placement sites are correct: the first guards value() (which resolves a JSString and can trigger getters/toPrimitive), and the second guards the toStringOrNull path where the returned null means an exception is already pending.
Other factors
The bug hunter flagged two nits on the test: a missing expect(stderr).toBe("") (CLAUDE.md convention for spawned-process tests) and a hardcoded JSC-internal error string that could break on WebKit upgrades. These are valid test-quality observations already posted as inline comments, but they don't affect the correctness of the C++ fix. The fix itself is minimal, targeted, and clearly correct.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/plugin/plugin-toprimitive-crash.test.ts`:
- Line 35: Replace the brittle negative stderr check
(expect(stderr).not.toContain("error:")) with a positive assertion that verifies
the specific expected failure behavior: capture the process result used in this
test (stdout/stderr or the thrown exception from the test harness) and assert
the exact error type or message you expect (e.g., match the expected error
name/message from stdout or assert the thrown exception via the test runner).
Update the assertion around the variables stdout/stderr or the function that
runs the plugin to check for the precise expected output/exception instead of
asserting absence of "error:".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68795341-bfeb-47f2-8979-77d808b95c75
📥 Commits
Reviewing files that changed from the base of the PR and between 56c07c0870b8d4fa9c9742c54a952d7940d754ae and 64c33e97e51b32be26d981e0d273860e9e54ee55.
📒 Files selected for processing (1)
test/js/bun/plugin/plugin-toprimitive-crash.test.ts
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed.
Extended reasoning...
Overview
This PR touches two files: src/bun.js/bindings/BunPlugin.cpp (2 lines added) and a new regression test test/js/bun/plugin/plugin-toprimitive-crash.test.ts (34 lines). The C++ change adds two RETURN_IF_EXCEPTION(throwScope, {}) guards in setupBunPlugin after the toStringOrNull call and after the value() call, matching the identical pattern already used at lines 290/293/300 in the same function.
Security risks
No security risks. The change only adds early-exit guards for an existing exception path; it does not alter any auth, crypto, permissions, or data-exposure logic.
Level of scrutiny
Low. The fix is mechanical: two one-liner macro invocations following an established pattern already present in the same function block. The regression test is subprocess-based (survives assertion crashes), uses bunEnv, wraps Bun.plugin() in try/catch, and now asserts expect(stderr).toBe("") per test/CLAUDE.md convention. Both the positive stdout assertions ("TypeError", "toPrimitive") and the empty-stderr invariant are present in the final state.
Other factors
The pre-existing bug noted in the bug report (validated target string is never actually used to select per-target plugin functions) predates this PR and is not introduced or worsened by it. All reviewer feedback across multiple iterations has been resolved in commit 380e035.
| if (targetValue) { | ||
| if (auto* targetJSString = targetValue.toStringOrNull(globalObject)) { | ||
| String targetString = targetJSString->value(globalObject); | ||
| RETURN_IF_EXCEPTION(throwScope, {}); | ||
| if (!(targetString == "node"_s || targetString == "bun"_s || targetString == "browser"_s)) { | ||
| JSC::throwTypeError(globalObject, throwScope, "plugin target must be one of 'node', 'bun' or 'browser'"_s); | ||
| return {}; | ||
| } | ||
| } | ||
| RETURN_IF_EXCEPTION(throwScope, {}); | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing issue: Bun.plugin({ target: "node", ... }) silently registers as a bun-target plugin because the validated target string is never used. After the validation block, the builder object is hardcoded with target: "bun" and always wires jsFunctionAppendOnLoadPluginBun / jsFunctionAppendOnResolvePluginBun, regardless of what the caller specified. This is unrelated to the PR's exception-propagation fix, but the PR directly touches this code block.
Extended reasoning...
What the bug is
In setupBunPlugin (BunPlugin.cpp lines 301–311), the function reads and validates the JS-level target property against "node", "bun", and "browser", throwing a TypeError for any other value. However, the validated targetString variable is never used after the validation check. The function always constructs the builder object with target: "bun" hardcoded and always wires jsFunctionAppendOnLoadPluginBun / jsFunctionAppendOnResolvePluginBun as the onLoad/onResolve handlers.
The specific code path
After the validation block (line ~311), the code does:
builderObject->putDirect(vm, Identifier::fromString(vm, "target"_s), jsString(vm, String("bun"_s)), 0);
// ...
builderObject->putDirectNativeFunction(vm, globalObject, ..., jsFunctionAppendOnLoadPluginBun, ...);
builderObject->putDirectNativeFunction(vm, globalObject, ..., jsFunctionAppendOnResolvePluginBun, ...);This is unconditional. The BunPluginTarget target parameter accepted by setupBunPlugin is never referenced inside the function body, and the only call site (jsFunctionBunPlugin) always passes BunPluginTargetBun anyway.
Why existing code does not prevent it
The validation block correctly rejects invalid target strings, giving the impression that per-target semantics are implemented. The infrastructure for per-target registration does exist — jsFunctionAppendOnLoadPluginNode, jsFunctionAppendOnLoadPluginBrowser, etc. are defined — but setupBunPlugin never selects between them based on the validated target.
Step-by-step proof
- User calls
Bun.plugin({ target: "node", setup(b) { b.onLoad({ filter: /\.txt$/ }, handler) } }). setupBunPluginreads and validates thetargetproperty — "node" passes validation.targetStringholds "node" but is never consulted again.- The builder object is constructed with
target: "bun"andonLoadwired tojsFunctionAppendOnLoadPluginBun. - When
setup(b)callsb.onLoad(...), the handler is registered inglobalObject->onLoadPluginsas a bun-target plugin, not a node-target plugin. - The user receives silent misbehavior — no error, but wrong semantics.
Impact
Any code that depends on target-specific plugin registration (e.g., plugins that should only apply during bundling for a specific target) will silently apply as a bun-runtime plugin instead. This is a correctness issue but pre-dates this PR.
How to fix
After validation, capture the resolved BunPluginTarget enum value and use it to select the correct onLoad/onResolve native functions. A switch on the validated targetString ("node" → BunPluginTargetNode, "browser" → BunPluginTargetBrowser, else BunPluginTargetBun) would allow the existing per-target functions to be wired correctly.
Pre-existing status
This bug predates the PR. The PR only adds RETURN_IF_EXCEPTION(throwScope, {}) guards at lines 304 and 310 to handle the crash when Symbol.toPrimitive returns a non-primitive. It does not introduce or worsen the target-ignored behavior, but it directly touches the affected block, making this an appropriate finding.
There was a problem hiding this comment.
Yes, worth fixing, but as a separate PR. The pre-existing bug is real: BunPluginTarget target is accepted as a parameter by setupBunPlugin and jsFunctionBunPlugin always passes BunPluginTargetBun, while the validated targetString is never used to select jsFunctionAppendOnLoadPluginNode/jsFunctionAppendOnLoadPluginBrowser (which do exist). This means Bun.plugin({ target: "node" }) silently registers as a bun-target plugin.
That said, this PR is specifically about the exception-propagation crash fix and the target-selection logic is orthogonal. I would recommend merging this PR as-is and opening a follow-up to wire the target enum through setupBunPlugin properly.
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
3 times, most recently
from
193d9ba to
6764546
Compare
There was a problem hiding this comment.
LGTM — all prior feedback has been addressed in the final commit.
Extended reasoning...
Overview
This PR adds two RETURN_IF_EXCEPTION(throwScope, {}) guards in setupBunPlugin (BunPlugin.cpp) after toStringOrNull() and after the if (targetValue) block. It adds a subprocess-based regression test and an import-order tweak in an unrelated test file.
Security risks
None. The change purely adds exception propagation guards in JS exception handling paths; no auth, crypto, or permissions code is touched.
Level of scrutiny
Low. The two added lines follow the identical pattern already present at lines 290, 293, and 300 in the same function. The fix is mechanical and the intent is clear: propagate a pending JS exception rather than continue into constructEmptyObject which asserts no exception is pending.
Other factors
All inline feedback from prior review rounds has been incorporated in the final diff: stderr: "pipe" is present in spawn options, proc.stderr.text() is included in the Promise.all destructuring, expect(stderr).toBe("") is asserted, and the stdout assertions use .toContain rather than a hardcoded JSC-internal message string. No bugs were found by the automated bug hunting system.
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
from
ec3cfe3 to
491e1c7
Compare
There was a problem hiding this comment.
LGTM — the C++ fix is minimal and correct, and the regression test now properly captures stderr with toBe("") as required.
Extended reasoning...
Overview
This PR touches two files: src/bun.js/bindings/BunPlugin.cpp (2 lines added) and a new regression test test/js/bun/plugin/plugin-toprimitive-crash.test.ts. The C++ change adds two RETURN_IF_EXCEPTION(throwScope, {}) guards in setupBunPlugin — one after targetJSString->value(globalObject) and one after the entire if (targetValue) block — to prevent execution from continuing with a pending JS exception into constructEmptyObject, which would hit assertNoException().
Security risks
None. The change only adds early-return guards on exception paths; it does not alter any auth, permissions, crypto, or data-exposure logic.
Level of scrutiny
Low. The fix is mechanical and follows the identical pattern of the existing guards at lines 290, 293, and 300 in the same function. The pattern (RETURN_IF_EXCEPTION(throwScope, {})) is well-established JSC idiom throughout the codebase. The regression test is subprocess-based (survives assertion crashes), asserts exitCode === 0, stdout contains TypeError and toPrimitive, and stderr is empty — covering the full crash scenario.
Prior review concerns
My earlier comments flagged (1) missing stderr: "pipe", (2) missing proc.stderr.text() in destructuring, and (3) missing expect(stderr).toBe(""). The final diff addresses all three: stderr: "pipe" is present in spawn options, all three values are destructured from Promise.all, and expect(stderr).toBe("") appears before expect(exitCode).toBe(0). All prior inline threads are marked resolved.
Other factors
The pre-existing issue I noted (target string being validated but then hardcoded as "bun") predates this PR and is out of scope. The unrelated bundler_compile.test.ts CI failures appear to be infrastructure flakiness not caused by this PR.
There was a problem hiding this comment.
LGTM — all previously flagged test issues have been addressed.
Extended reasoning...
Overview
This PR touches src/bun.js/bindings/BunPlugin.cpp (2 lines added), adds a new regression test test/js/bun/plugin/plugin-toprimitive-crash.test.ts, and makes a trivial import-order tweak in test/js/bun/s3/s3-fd-validation.test.ts.
Security risks
No security-sensitive code is touched. The change adds exception propagation guards in a plugin registration path; there is no auth, crypto, or permission-sensitive logic involved.
Level of scrutiny
Low. The C++ change is two lines of RETURN_IF_EXCEPTION(throwScope, {}), following the identical macro pattern already present at lines 290, 293, and 300 of the same function. The fix is mechanical and follows established JSC exception-handling conventions. The regression test spawns a subprocess and now correctly captures and asserts on stderr (expect(stderr).toBe("")) — the pattern required by test/CLAUDE.md.
Other factors
All prior inline comment threads from previous review runs are marked resolved. The s3-fd-validation.test.ts change is a no-op import reorder (autofix.ci output). No bugs were found by the automated bug hunting system. The fix is self-contained and the scope matches the PR description exactly.
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
from
5c5c413 to
bc43f78
Compare
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
from
95d7100 to
cbd0792
Compare
There was a problem hiding this comment.
LGTM — the two RETURN_IF_EXCEPTION guards are correct and match the existing pattern in the same function.
Extended reasoning...
Overview
This PR touches src/bun.js/bindings/BunPlugin.cpp (two-line guard addition), a new regression test, and a trivial import-order tweak in an unrelated test. Scope is minimal.
Security risks
None. The change only adds early-return guards on an existing exception scope; it does not affect auth, crypto, permissions, or any sensitive code path.
Level of scrutiny
Low. The fix is mechanical: RETURN_IF_EXCEPTION(throwScope, {}) is a well-understood JSC macro used many times elsewhere in the same file. The two new calls are placed exactly where toStringOrNull() and value() can leave a pending exception, which is precisely the documented root cause of the crash.
Other factors
The void stderr test style nit has been raised and resolved through prior inline comment rounds. Robobun verification explicitly calls it a non-blocking style gap. The regression test's exitCode + stdout assertions are sufficient to detect the original crash. CI failures in the timeline are in bundler_compile.test.ts across multiple platforms, unrelated to this diff.
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
from
cbd0792 to
3356005
Compare
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
from
04b84b2 to
b729322
Compare
|
Verification (robobun): C++ fix adds two standard |
There was a problem hiding this comment.
LGTM — the two RETURN_IF_EXCEPTION guards are correct and follow the established pattern in the same function.
Extended reasoning...
Overview
The PR touches three files: BunPlugin.cpp (C++ fix), a new regression test plugin-toprimitive-crash.test.ts, and a cosmetic import-order change in s3-fd-validation.test.ts.
Security risks
No security-sensitive code paths are affected. The change adds exception propagation guards in plugin target parsing — no auth, crypto, or permissions code is involved.
Level of scrutiny
Low: the C++ change is two lines of RETURN_IF_EXCEPTION(throwScope, {}) matching the identical pattern already used at lines 290/293/300 in the same function. This is a mechanical safety fix for a crash path, not a logic change. The regression test is a clean in-process test that directly exercises the crash path and confirms it now throws instead of asserting.
Other factors
The test was heavily iterated based on prior review feedback (subprocess approach → simpler in-process approach). The final 15-line version is clean and correct. The one remaining nit — using .toThrow("toPrimitive") instead of .toThrow(TypeError) — is filed as an inline comment but does not block approval: the test still catches the regression (crash vs. throw) and the ECMAScript spec mandates TypeError for this path, so a different error type from JSC is not a realistic concern.
| }; | ||
| }, | ||
| }); | ||
| expect(() => Bun.plugin(obj)).toThrow("toPrimitive"); |
There was a problem hiding this comment.
🟡 The test uses .toThrow("toPrimitive") which only verifies the error message contains the substring, but does not assert the thrown error is a TypeError. The PR explicitly states the fix causes "throws TypeError" and the C++ code calls JSC::throwTypeError(...); a future regression throwing a different error type (e.g. RangeError) with "toPrimitive" in its message would silently pass this check. Fix: use .toThrow(TypeError) to also assert the error type.
Extended reasoning...
What the bug is and how it manifests
Line 14 of the new test uses expect(() => Bun.plugin(obj)).toThrow("toPrimitive"). Jest/Bun’s .toThrow(string) overload only checks that the thrown error’s message contains the given substring. It does not check the error class at all. This means the test only validates two things: (1) the function throws something, and (2) whatever is thrown has a message containing "toPrimitive".
The specific code path that triggers it
The C++ fix in BunPlugin.cpp inserts RETURN_IF_EXCEPTION(throwScope, {}) guards after toStringOrNull() and after value(). When Symbol.toPrimitive returns a non-primitive, JSC internally calls JSC::throwTypeError(...) (from within toPrimitive()), setting a pending TypeError in the throw scope. The RETURN_IF_EXCEPTION guard then surfaces that pending exception to JavaScript. The PR description explicitly documents this: "was: assertion failure / crash; now: throws TypeError".
Why existing code does not prevent it
The .toThrow("toPrimitive") matcher is satisfied by any thrown value whose .message contains "toPrimitive", regardless of the error’s constructor or prototype chain. If a future regression changed the error type—say, a refactor accidentally used throwRangeError or a generic throwException—while preserving a message containing "toPrimitive", the test would continue to pass, providing false confidence that the TypeError contract is maintained.
Impact assessment
This is a test-precision gap, not a bug in the production fix itself. The C++ fix is correct and the primary regression goal (crash → throw) is properly tested. However, the stated contract of the fix (specifically TypeError) is not enforced by the test. The PR description and the inline C++ semantics create an expectation that a TypeError is thrown; the test does not verify this.
Addressing the refutation
The refutation correctly notes that the ECMAScript spec (sec-toprimitive) mandates a TypeError when Symbol.toPrimitive returns a non-primitive, and JSC follows the spec. There is no plausible scenario within spec-compliant JSC where a different error type with "toPrimitive" in its message would be thrown from this path. This analysis is accepted—the gap is theoretical rather than a realistic regression risk—which is why this is filed at nit severity, not normal.
Step-by-step proof
- The test at line 14 calls
expect(() => Bun.plugin(obj)).toThrow("toPrimitive"). - Internally, Bun’s test runner resolves
.toThrow(string)as a substring message check. - Suppose a future engineer refactors
setupBunPluginto call a custom error constructor that extendsError(notTypeError) but uses the same JSC message string. - The thrown error’s message still contains "toPrimitive";
.toThrow("toPrimitive")passes. - But the thrown error is not a
TypeError;.toThrow(TypeError)would have failed, catching the regression.
How to fix
Replace line 14 with:
expect(() => Bun.plugin(obj)).toThrow(TypeError);Or, to assert both type and message:
expect(() => Bun.plugin(obj)).toThrow(new TypeError());
// and separately:
try { Bun.plugin(obj); } catch (e) {
expect(e).toBeInstanceOf(TypeError);
expect((e as TypeError).message).toContain("toPrimitive");
}The single .toThrow(TypeError) form is idiomatic and sufficient.
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
2 times, most recently
from
8f21762 to
543c4fb
Compare
robobun
left a comment
robobun
left a comment
There was a problem hiding this comment.
The target-ignored behavior predates this PR - the validated target string has never been wired to select between the per-target onLoad/onResolve functions (jsFunctionAppendOnLoadPluginNode, etc.). It's always hardcoded to bun. Happy to fix it in a follow-up if you'd like, but keeping this PR scoped to the crash fix.
There was a problem hiding this comment.
LGTM — the two RETURN_IF_EXCEPTION guards are correct and minimal, matching the existing pattern in the same function, and the regression test directly exercises the crash path.
Extended reasoning...
Overview
The PR touches src/bun.js/bindings/BunPlugin.cpp (two RETURN_IF_EXCEPTION(throwScope, {}) guards added at lines 304 and 310 of setupBunPlugin), a new in-process regression test test/js/bun/plugin/plugin-toprimitive-crash.test.ts, and a trivial import-order swap in an unrelated S3 test.
Security risks
None. The change only adds early-return exception propagation guards; it does not alter any auth, permission, crypto, or sandbox boundary.
Level of scrutiny
Low. This is a minimal, surgical bug fix following a well-established JSC pattern already used at three other sites in the same function (lines 290, 293, 300). The crash path (pending JS exception reaching assertNoException() in constructEmptyObject) is straightforward and the fix is the canonical remedy.
Other factors
The regression test constructs the exact repro from the PR description in-process — it would SIGABRT on unpatched code and now throws a TypeError. The only nit (.toThrow("toPrimitive") vs .toThrow(TypeError)) is already documented in an inline comment; the ECMAScript spec mandates TypeError for this case so the assertion is functionally sound. No bugs were found by the automated hunting system. CI is pending but unrelated failures (bundler timeouts, worker_threads) have been identified as pre-existing.
There was a problem hiding this comment.
LGTM — the two RETURN_IF_EXCEPTION guards are minimal, correct, and match the established pattern; the in-process regression test covers the exact crash path.
Extended reasoning...
Overview
This PR touches src/bun.js/bindings/BunPlugin.cpp (two one-line exception guards), a new 15-line in-process regression test, and a cosmetic import-order swap in an unrelated test file.
Security risks
None. The change is purely defensive exception handling — it propagates an already-pending JSC exception rather than allowing execution to continue with a dirty throw scope into assertNoException().
Level of scrutiny
Low. Both RETURN_IF_EXCEPTION(throwScope, {}) calls follow the identical macro pattern used at multiple other sites in the same function (lines 290, 293, 300 of the context). The fix does not introduce new logic, change semantics for valid inputs, or affect any code paths other than the crash path.
Other factors
The PR went through many review iterations focused on spawned-subprocess test patterns. The author resolved all those concerns by switching to a clean in-process test, which is simpler and more reliable. The remaining open nit (.toThrow("toPrimitive") vs .toThrow(TypeError)) is theoretical — ECMAScript sec-toprimitive mandates TypeError for this case and JSC follows the spec. The bug hunting system found no bugs. CI passed on 6 platforms with the remaining failures attributed to infra issues unrelated to this change.
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
from
0457145 to
be0c025
Compare
There was a problem hiding this comment.
LGTM — the two RETURN_IF_EXCEPTION guards are correct, follow the established pattern, and the regression test covers the crash path.
Extended reasoning...
Overview
Two files changed: BunPlugin.cpp gets two RETURN_IF_EXCEPTION(throwScope, {}) guards in setupBunPlugin, and a new 15-line in-process regression test is added at test/js/bun/plugin/plugin-toprimitive-crash.test.ts.
C++ fix
The guards are inserted at exactly the right points: after targetJSString->value(globalObject) (which can throw if the string value conversion fails) and after the entire if (targetValue) block (to catch the case where toStringOrNull returned nullptr due to a pending exception from Symbol.toPrimitive). Both placements follow the identical pattern used throughout the same function and file. The fix is minimal and correct.
Regression test
The test directly exercises the crash path: a Symbol.toPrimitive returning a non-primitive object. The expect(() => Bun.plugin(obj)).toThrow("toPrimitive") assertion confirms the crash is converted to a proper thrown exception. A prior inline comment I filed noted .toThrow(TypeError) would be marginally stricter, but .toThrow("toPrimitive") is sufficient to confirm the crash → throw conversion, and the ECMAScript spec mandates TypeError here so there is no realistic false-negative risk.
Security risks
None. This only affects Bun.plugin() target validation and adds exception propagation — no auth, crypto, or permission code paths involved.
Level of scrutiny
Low. This is a two-line targeted exception-propagation fix in a well-understood function, with a direct regression test. The pattern is identical to existing guards in the same function (lines 290/293/300). CI passed on all reported platforms.
There was a problem hiding this comment.
LGTM — the two RETURN_IF_EXCEPTION guards are correct and the in-process regression test cleanly covers the crash path.
Extended reasoning...
Overview
Two files changed: BunPlugin.cpp receives two RETURN_IF_EXCEPTION(throwScope, {}) guards inside setupBunPlugin, and a new 15-line in-process test exercises the exact crash path (Symbol.toPrimitive returning a non-primitive object).
Security risks
None. This is pure exception-propagation hygiene — converting a native SIGABRT into a proper JS TypeError. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low. The diff is minimal and mechanical: the RETURN_IF_EXCEPTION macro is the standard JSC pattern used dozens of times in the same file and throughout the codebase. The test is synchronous and self-contained. CI passed on 6 platforms.
Other factors
The PR went through many iterations focused on the regression test format (spawned subprocess vs in-process). The final in-process approach is simpler and makes all the previous stderr/Promise.all concerns moot. My outstanding inline comment about .toThrow(TypeError) vs .toThrow("toPrimitive") is a theoretical nit — ECMAScript mandates TypeError for this case — and is already noted on the PR.
There was a problem hiding this comment.
LGTM — the C++ fix is minimal and correct, and the in-process regression test covers the exact crash path.
Extended reasoning...
Overview
Two files changed: BunPlugin.cpp adds two RETURN_IF_EXCEPTION(throwScope, {}) guards in setupBunPlugin (lines 304 and 309), and a new 15-line in-process test exercises the exact crash path (Symbol.toPrimitive returning a non-primitive {}).
Security risks
No security-sensitive code touched. The change is purely exception-propagation hygiene in the plugin setup path.
Level of scrutiny
Low — the fix pattern is identical to the existing guards at lines 290, 293, and 300 of the same function. The root cause (pending exception surviving into constructEmptyObject → assertNoException() → SIGABRT) is well-understood and the two guards directly close both exit paths from toStringOrNull and value() that can throw.
Other factors
The CI failure (webview.test.ts timeout on macOS x64) is unrelated to this PR. The in-process test in the final revision resolves all previous subprocess/stderr concerns. The pre-existing BunPluginTarget-ignored bug I noted is orthogonal and tracked for a separate PR. The outstanding nit about .toThrow(TypeError) vs .toThrow("toPrimitive") is minor — the test still exercises the crash path correctly.
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
from
cbbbd50 to
aef07b1
Compare
|
Verification (robobun): CI Lint JS pass, buildkite build 41268 in progress (not red). Diff is 2 lines in BunPlugin.cpp adding standard RETURN_IF_EXCEPTION guards matching existing pattern at lines 290/293/300. 15-line in-process regression test exercises the exact crash path (Symbol.toPrimitive returning non-primitive - would SIGABRT on main, now throws TypeError). No TODO/FIXME/HACK. All reviews LGTM. |
robobun
force-pushed
the
farm/8f0ba90e/fix-plugin-toprimitive-exception
branch
from
aef07b1 to
6751096
Compare
There was a problem hiding this comment.
LGTM — the C++ fix is correct and the final in-process test is clean.
Extended reasoning...
Overview
This PR touches two files: src/bun.js/bindings/BunPlugin.cpp (two RETURN_IF_EXCEPTION guard insertions) and test/js/bun/plugin/plugin-toprimitive-crash.test.ts (a new in-process regression test). The scope is narrow and self-contained.
Security risks
None. The change adds early-return exception guards to existing validation logic; it does not affect authentication, permissions, or data handling.
Level of scrutiny
This is a minimal crash-fix: two guard lines inserted at the exact points where a pending JSC exception could be left active, following the identical RETURN_IF_EXCEPTION(throwScope, {}) pattern already present at lines 290, 293, and 300 of the same function. The fix is mechanical and correct.
Other factors
The long review thread was driven by iterating on a spawned-subprocess version of the test, all of which is no longer relevant — the final committed test is a simple in-process 15-line test that directly exercises the crash path. The .toThrow("toPrimitive") assertion is sufficient to catch any regression (the ECMAScript spec mandates a TypeError when Symbol.toPrimitive returns a non-primitive, so the message substring check is reliable). CI lint passed per robobun verification.
|
Superseded by #31286, which applies the same |
2 participants
When
Bun.plugin()receives an object whosetargetproperty has aSymbol.toPrimitivethat returns a non-primitive value,toStringOrNullthrows a JS exception and returnsnullptr. The code correctly skipped the innerifblock, but then continued execution with the pending exception still active, eventually hittingassertNoException()inconstructEmptyObject.This adds
RETURN_IF_EXCEPTIONchecks after bothtoStringOrNull()(which can throw whenSymbol.toPrimitivereturns a non-primitive) andvalue()(which can also throw) to properly propagate the exception back to JavaScript.Repro:
robobun verification (iter 40): Diff is exactly two
RETURN_IF_EXCEPTION(throwScope, {})guards in BunPlugin.cpp (lines 304 and 310) matching the established pattern at lines 290/293/300 of the same function. On main, execution falls through toconstructEmptyObjectwith a pending exception, hittingassertNoException(). Regression test (15 lines, in-process) exercises the exact crash path viaSymbol.toPrimitivereturning non-primitive{}; asserts.toThrow("toPrimitive")— would crash/SIGABRT on main, not pass. Lint JS passed on both a960e7d and cbbbd50. No TODO/FIXME/HACK. No CHANGES_REQUESTED reviews. No unrelated changes in final diff. Buildkite #41233 pending at time of approval.