Fix debug assertion when a BunString error message is empty - #40556
Conversation
…assertion
`BunString__toErrorInstance` called `JSC::createError` and its siblings.
Those helpers assert that the message is not empty. An empty `BunString`
converts to a null `WTF::String`, so `expect.unreachable("")` hit the
assertion in debug builds.
Construct the `ErrorInstance` directly with the structure for the error
type, as the napi bindings already do. An empty message now produces the
same error object that release builds already produce.
|
Updated 1:16 AM PT - Sep 22nd, 2026
✅ @robobun, your commit c9e280c3ead8c2a7b3fad7986c851665b03528f7 passed in 🧪 To try this PR locally: bunx bun-pr 40556That installs a local version of the PR into your bun-40556 --bun |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. Walkthrough
ChangesBun error construction
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The fix preserves error types and empty messages without the debug assertion, and the regression test confirms successful behavior. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Addressed the review note on the test. The empty-message case now runs as a spawned fixture in 4986cb8: the test spawns Checked both directions with the debug build:
The release build has no assertions, so the test passes there before and after the fix. |
There was a problem hiding this comment.
LGTM — thanks for moving the repro into a spawned subprocess.
What was reviewed:
BunString__toErrorInstancenow mapsBunErrorKind→JSC::ErrorTypeand callsErrorInstance::createdirectly; matches the existingcreateErrorWithCodepattern innapi.cppfor the same reason.- Test spawns
bunExe() -e, drains stdout/stderr/exited concurrently, and asserts output before exit code — a regressed debug assertion aborts the child, not the runner. - No CODEOWNERS entries cover the changed paths.
Extended reasoning...
Overview
The PR changes one function in src/jsc/bindings/BunString.cpp: BunString__toErrorInstance previously dispatched to JSC::createError / createTypeError / createSyntaxError / createRangeError, all of which ASSERT(!message.isEmpty()) in debug builds. It now maps the BunErrorKind enum to a JSC::ErrorType and constructs the error via JSC::ErrorInstance::create(vm, globalObject->errorStructure(type), message, ...). A regression test was appended to test/js/bun/test/expect-unreaachable.test.ts.
Security risks
None. This is a debug-assertion fix in the error-instance construction path. No untrusted input parsing, no auth/crypto, no allocation-size arithmetic changed. Release-build behavior is byte-identical (the removed assertion compiles out in release).
Level of scrutiny
Low-to-moderate. The C++ change is ~10 lines, mechanical, and mirrors an existing in-tree precedent: createErrorWithCode in src/jsc/bindings/napi.cpp already calls ErrorInstance::create directly with the comment "we don't call JSC::createError() as it asserts the message is not an empty string". All four BunErrorKind cases are handled; the default initializer ErrorType::Error is dead but harmless.
Other factors
My earlier inline comment asked for the crashing input to run in a spawned fixture rather than in-process (per REVIEW.md: "Crash fixes need the crashing input as a spawned fixture"). Commit 4986cb8 addressed this: the test now spawns [bunExe(), "-e", ...] with bunEnv, drains all three pipes via Promise.all, and asserts stdout/stderr before exitCode. If the assertion ever regresses, the child aborts and the test fails on the empty stdout / non-zero exit code without taking down the test runner. The PR author acknowledged that release builds pass this test regardless (no assertions compiled in) — that's inherent to a debug-only crash fix and the test still guards the debug CI lane.
|
Thanks for the review. CI for 4986cb8 is still in progress on #106277: 28 jobs passed, none failed so far. One job shows a retry. |
|
The fuzzer reported this assertion again on a newer main (cf71211). It is the same call, I merged main into the branch (7a0f00c) so that CI runs against the current tree. The merge had no conflicts, and the diff against main is still the same two files. Results on the merged tree with a debug build:
The previous CI run was green. This PR is ready for a maintainer review. |
| break; | ||
| } | ||
| // Not JSC::createError(): it asserts the message is not empty, and `new Error("")` is valid. | ||
| JSC::JSObject* result = JSC::ErrorInstance::create(globalObject->vm(), globalObject->errorStructure(type), message, JSValue(), nullptr, JSC::TypeNothing, type, true); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: V8 addons passing an empty message still abort a debug build, since only this one caller was moved off JSC::createError. src/jsc/bindings/v8/V8Exception.cpp:15 and :25 forward a user-supplied v8::String straight into JSC::createError / JSC::createTypeError, which carry the same ASSERT(!message.isEmpty()). Fix: route every user-string-to-error site through one shared helper that calls JSC::ErrorInstance::create directly (as BunString.cpp:190 now does), which covers the 3 sites listed. Same pattern at 3 sites (src/jsc/bindings/BunString.cpp:190, src/jsc/bindings/v8/V8Exception.cpp:15, src/jsc/bindings/v8/V8Exception.cpp:25).
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A V8-API native addon calls v8::Exception::Error or v8::Exception::TypeError with an empty v8::String (e.g. v8::String::NewFromUtf8(isolate, "")). src/jsc/bindings/v8/V8Exception.cpp:14 converts it to an empty WTF::String and src/jsc/bindings/v8/V8Exception.cpp:15 (and :25 for TypeError) passes it to JSC::createError / JSC::createTypeError, which assert !message.isEmpty() in debug builds. A debug or fuzz build aborts with the same ASSERTION FAILED: !message.isEmpty() the PR fixes for BunString__toErrorInstance; release builds are unaffected. The base branch behaves the same at these sites; this PR fixes only the BunString.cpp:190 caller and leaves the sibling sites that share the identical pattern unfixed, so the bug class remains reachable from user-controlled addon input.
Verification: pre-existing (debug/fuzz builds only; the base branch already aborts by the same route in untouched code). Triggering condition: a V8-API native addon calls v8::Exception::Error or v8::Exception::TypeError with an empty v8::String. Mechanism verified: /home/claude/bun/src/jsc/bindings/v8/V8Exception.cpp:14-15 does `WTF::String wtfMessage = message->localToJSString()->value(globalObject);…
|
On the V8 note: I read I also corrected the verification list in the description. It said that a Worker parse error run checked the |
|
Build #119614 has one failed test: The other annotations in the build passed on a retry. The regression test ran on the x64-asan lane, which builds with assertions on ( Edit: an earlier version of this comment said the test passed on every lane that ran it. I had checked only the failure list at that point, so I replaced that sentence with what I verified. |
2 participants
What does this PR do?
Fixes a debug assertion failure in
BunString__toErrorInstance(src/jsc/bindings/BunString.cpp). The fuzzer found it withexpect.unreachable("").BunString__toErrorInstanceis the shared path forString::to_error_instanceand its TypeError, SyntaxError, and RangeError siblings. It calledJSC::createErrorand friends. Those JSC helpers assert that the message is not empty. An emptyBunStringconverts to a nullWTF::String, so any caller that forwards an empty string aborts a debug build.expect.unreachable(reason)forwards the user's string as is, soexpect.unreachable("")reached the assertion.The fix constructs the
ErrorInstancedirectly, with the error structure for the requested type.JSC::createError(globalObject, message)is exactlyErrorInstance::create(vm, globalObject->errorStructure(type), message, JSValue(), nullptr, TypeNothing, type, true)plus the assertion, so the result is the same object for a non-empty message. The napi bindings already use this pattern for the same reason (createErrorWithCodeinsrc/jsc/bindings/napi.cpp).Release builds do not compile the assertion, so their behavior does not change.
expect.unreachable("")throws anUnreachableErrorwith an emptymessagethere before and after this change.How did you verify your code works?
Bun.jest().expect.unreachable("").UnreachableErrorand exits with code 1. The debug build now matches the release build for an empty, a custom, and an omitted message.test/js/bun/test/expect-unreaachable.test.ts. On the unfixed debug build the test process aborts with the assertion. With the fix it passes (bun bd test). The release build has no assertions, so the test also passes withUSE_SYSTEM_BUN=1. The failure is only visible in a debug build.bun bd test test/js/bun/test/expect.test.js: 415 pass, 0 fail.BUN_JSC_validateExceptionChecks=1: pass.messageisSyntaxError: Unexpected ;anderror instanceof SyntaxErroris false. I did not confirm that this run reachesto_syntax_error_instance, so it is not coverage of the SyntaxError kind. Only theErrorkind is exercised, throughexpect.unreachable.[auto-merge] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file