Conversation
|
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 (4)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughChangesNative error fallback handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: rebuilt on current main (732b669), waiting on CI. Main consolidated the error constructors into Reproduced on main with
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix that guarantees the four get*ErrorInstance constructors always return an object, replacing an over-limit message with a fallback string.
What was reviewed:
errorMessageOrFallbackonly affects the null-return edge case; normal-size messages pass through unchanged, so the hot path is byte-identical.- Verified
create_error_instance's other path (JSC__createErrorviaBunString::to_error_instance) already always returns an object, so the deletedis_empty()guards inthrow/throw_sys_error/throw_todoare dead;JSC__VM__throwErrorstillASSERT(!value.isEmpty())if that ever regresses. - Test covers the boundary (at-limit kept intact vs one-past → fallback), the crash case (
Bun.listenputs properties on the result), and the sibling constructor (SyntaxErrorfromBun.JSONC.parse), each in its own subprocess since the limit is process-wide.
Extended reasoning...
Overview
Two production files. src/jsc/bindings/helpers.h gains errorMessageOrFallback(message, source), which returns "The error message exceeds the maximum string length" when toString/toStringCopy returned null for a non-empty source, and passes message through otherwise. All four get*ErrorInstance functions now route their message through it, so they always call JSC::create*Error with a valid WTF::String and always return an object. src/jsc/JSGlobalObject.rs deletes the three if err.is_empty() { debug_assert!(has_exception()); return JsError::Thrown } branches from throw, throw_sys_error, and throw_todo, which were the only Rust-side callers that special-cased the empty return (and whose debug_assert was wrong — nothing on that path threw). One new test file with three subprocess-isolated cases.
Security risks
None. This is error-construction plumbing; the only externally observable change is that an error whose message would exceed the string length limit now carries a stand-in message instead of coming back as an empty JSValue (crash / silent pass) or with no message property. No new user input is parsed and no security check is loosened.
Level of scrutiny
Medium. helpers.h is included widely and the four constructors have dozens of callers, but the change is additive on the [[unlikely]] null-string path only — for any message under the limit errorMessageOrFallback is an identity function, so normal error creation is behaviorally identical. The Rust deletions are dead code once the C++ invariant holds; I verified the other path into create_error_instance (BunString::static_str(...).to_error_instance → JSC__createError) unconditionally calls JSC::createError and never returned empty, and that JSC__VM__throwError retains ASSERT(!value.isEmpty()) as a debug backstop.
Other factors
The PR description enumerates the sibling constructors it intentionally left alone (JSC__createError & co., systemErrorToErrorInstance, Bun__createErrorWithCode, AggregateError, DOMException) with a reason for each, which matches what I found. The test file follows harness conventions (tempDir, bunEnv, Buffer.alloc(n, fill).toString(), describe.concurrent for independent subprocesses, drains stdout/stderr/exited concurrently, asserts stderr/stdout before exit code) and exercises the exact boundary. The author verified the test fails on main (release: silent pass / segfault 139; debug: UBSan + panic) and re-ran the affected suites (expect.test.js, socketaddress.spec.ts, jsonc.test.ts, etc.). No prior reviews on the timeline beyond a CodeRabbit rate-limit notice.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the shared error-construction path in the JSC bindings (used by dozens of callers) and picks a specific fallback strategy, a human look would still be worthwhile.
What was reviewed:
- Confirmed
JSC::createError(JSGlobalObject*, ErrorType, const String&)is an existing overload already used atbindings.cpp:2586andBunAnalyzeTranspiledModule.cpp:221, so the four consolidated helpers dispatch to the same constructors as before. - Traced
create_error_instance's two paths (JSC__createErrorforBunString,ZigString__toErrorInstance→getErrorInstance) — both now always return an object, so the removedis_empty()guards inthrow/throw_sys_error/throw_todoare dead. - Checked the
source.len == 0case:errorMessageOrFallbackpasses the null string through unchanged, matching the old behavior for empty messages. - Verified the "always return an object" contract matches the reasoning already documented in
ErrorCodeCache::createError(ErrorCode.cpp:210-213).
Extended reasoning...
Overview
The PR fixes a crash / silent-failure when a natively formatted error message exceeds the string length limit (WTF::String::MaxLength or the lowered Bun__stringSyntheticAllocationLimit). Zig::toString/toStringCopy return a null WTF::String in that case; the old getErrorInstance turned that into an empty JSValue, which callers then threw (nothing pending → lost failure), put() on (null deref → segfault), or wrapped in an AggregateError with no message. The fix introduces errorMessageOrFallback, which substitutes a fixed message when a non-empty source produced a null conversion, and folds the four get*ErrorInstance helpers into one createErrorInstance(globalObject, ErrorType, message, source). createAggregateError gets the same fallback. The now-unreachable is_empty() guards in JSGlobalObject.rs's throw, throw_sys_error, and throw_todo are removed.
Security risks
None. This is error-message construction; no untrusted input is parsed differently, no allocation size is derived from external data, and the change strictly narrows the set of states callers can observe (from "object or empty JSValue" to "object").
Level of scrutiny
Moderate-to-high. The four get*ErrorInstance helpers back ZigString__to{,Type,Syntax,Range}ErrorInstance, which are called from create_*_error_instance and dozens of sites across the runtime. The refactor is a pure dedup — JSC::createError(JSGlobalObject*, ErrorType, const String&) is an existing WebKit overload already used in bindings.cpp:2586 and BunAnalyzeTranspiledModule.cpp:221 — and each wrapper keeps its original conversion (toString vs toStringCopy), so the only semantic change is in the previously-broken null-message case. The removed Rust guards were checking the wrong invariant (they debug_assert!(has_exception()) when nothing on the path throws). Still, this is the shared error path in the JSC FFI layer, and the choice of fallback strategy (fixed message vs throwing vs bounded truncation) is a design call a maintainer should ratify.
Other factors
The test file is thorough: one subprocess per case (limit is process-wide; unfixed Bun.listen crashes), covers at-limit / one-past-limit boundaries for both expect() and AggregateError, exercises Error, Error+put, SyntaxError, and AggregateError paths, and asserts bun test reports (fail) with exit 1 for the uncaught case. The PR description enumerates what was intentionally left unchanged (BunString constructors, DOMException) with reasons. The comment-cop feedback about the long code comment was addressed in c43e894. The description also lists a broad set of existing tests that were re-run.
c43e894 to
a079c43
Compare
There was a problem hiding this comment.
LGTM — targeted fix for a real crash / silent-pass bug, with the fallback applied at the shared constructor so every caller is covered.
What was reviewed:
errorMessageOrFallbackgates onsource.len > 0, so a legitimately emptyZigStringstill yields an empty-message error (unchanged from before); only the null-from-oversize case gets the fallback.- The four
get*ErrorInstancewrappers keep their original conversions (toStringvstoStringCopy), andEnsureStillAliveScopeis preserved in the sharedcreateErrorInstance. - The removed
is_empty()branches inthrow/throw_sys_error/throw_todoare dead now that bothcreate_error_instancepaths (JSC__createErrorandZigString__toErrorInstance→getErrorInstance) always return an object. - Tests hit the exact-limit / one-past boundary and each constructor family (Error, SyntaxError, AggregateError, plus the
put-on-result crash viaBun.listen), each in its own subprocess.
Extended reasoning...
Overview
The PR fixes native error construction when the formatted message exceeds Bun__stringSyntheticAllocationLimit / WTF::String::MaxLength. Previously Zig::getErrorInstance returned an empty JSValue in that case (comment claimed a pending exception, but nothing threw), and the sibling constructors passed a null WTF::String through, producing errors with no message. Downstream, JSGlobalObject::throw turned the empty value into JsError::Thrown with nothing pending (silent (pass) in release, debug panic), and callers that decorate the result (Bun.listen setting syscall/errno/address) segfaulted on the null cell.
The fix adds errorMessageOrFallback (substitutes a fixed message when a non-empty ZigString converted to a null WTF::String) and a shared createErrorInstance(globalObject, ErrorType, message, source) that the four get*ErrorInstance helpers now delegate to. JSC__JSGlobalObject__createAggregateError applies the same fallback to its message. The now-dead is_empty() / debug_assert!(has_exception()) branches in throw, throw_sys_error, and throw_todo are removed.
Security risks
None. This is error-message construction; the only user-controlled input is the length of a message that was already being formatted, and the change makes the oversize case more robust (an object with a short constant message instead of a null deref or a lost throw).
Level of scrutiny
Medium. helpers.h is included across the C++ bindings and these constructors sit under every Rust-side create_*_error_instance call, so the contract change ("always returns an object") is widely relied on. But the change is small and strictly widens the set of inputs that produce a valid error: the common path (message under the limit) is byte-for-byte unchanged, the empty-ZigString path is unchanged (source.len > 0 guard), and only the previously-broken oversize path gains behaviour. The consolidation into createErrorInstance preserves each wrapper's original conversion (toString for Error, toStringCopy for the typed errors) and keeps the EnsureStillAliveScope. JSC::createError(JSGlobalObject*, ErrorType, const String&) is the standard JSC dispatcher to the same per-type factories the old bodies called; CI (debug+ASAN and release) confirms it compiles and behaves.
Other factors
- Tests are thorough: four subprocess cases (process-wide limit, and the unfixed
Bun.listencase crashes) covering Error viaexpect()(at-limit kept intact, one-past gets fallback, uncaught reported as(fail)with exit 1), theput-on-result crash viaBun.listen, SyntaxError viaBun.JSONC.parse(plus a normal-sized message unchanged), and AggregateError via adata:import with two build errors (at-limit intact, one-past fallback,errorspreserved either way). Each drains stdout/stderr/exited concurrently and asserts exit code last. - The
describe.concurrentkeeps the file under ~4.5s despite four subprocess spawns. - The github-actions comment-cop feedback about the long comment on
errorMessageOrFallbackwas addressed (removed in c43e894); the fallback text itself states the condition. - CI is green on the head commit (Buildkite build 100497), and the PR body shows the test failing on main (release:
message: ""for JSONC/AggregateError, segfault forBun.listen/expect; debug: UBSan null member call / panic) and passing with the fix.
…g length limit BunString__toErrorInstance returned an empty JSValue, without throwing, when the message could not be converted to a WTF::String (longer than the synthetic allocation limit or WTF::String::MaxLength). JSGlobalObject::throw then returned JsError::Thrown with nothing pending, so a failing bun:test assertion whose message was over the limit reported as passing, and callers that set properties on the result before throwing it (Bun.listen) crashed. The error is now always created; when the message cannot be materialized it is replaced with "The error message exceeds the maximum string length". The now-unreachable empty-instance checks in throw, throw_sys_error and throw_todo are removed.
a079c43 to
732b669
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the central native error-creation path (BunString__toErrorInstance) and removes the empty-value guards in throw/throw_sys_error/throw_todo based on the new invariant, a human look would still be worthwhile.
What was reviewed:
- Traced every Rust caller whose guard is removed (
EncodedSlice::to_error_instance,JSGlobalObject::error_instance) back toBunString__toErrorInstance, which now always returns a non-null object. - Checked the
Dead/Emptytag handling in the new fallback condition —Emptystill yields an error with no message (unchanged),Deadnow gets the fallback instead of a null message. - Note: the PR description references
helpers.h/bindings.cppand an AggregateError test; the current diff (post-rebase onto theBunString__toErrorInstanceconsolidation) has 3 files / 3 tests, andJSC__JSGlobalObject__createAggregateErroris unchanged. Worth confirming the AggregateError drop was intentional.
Extended reasoning...
Overview
This PR fixes a crash/silent-pass bug where a natively formatted error message exceeding the string-length limit caused BunString__toErrorInstance to return an empty JSValue. Callers that then put properties on it segfaulted (release) or hit UBSan (debug), and JSGlobalObject::throw returned JsError::Thrown with no exception pending, so a failing expect() reported as (pass). The fix substitutes a fallback message ("The error message exceeds the maximum string length") instead of returning {}, and removes the now-dead is_empty() guards in throw, throw_sys_error, and throw_todo. A new subprocess-based test exercises expect(), Bun.listen, and Bun.JSONC.parse at and past the limit.
Security risks
None identified. This is error-message construction; no untrusted-input parsing, auth, or crypto is touched. The change closes a null-deref crash reachable from user input (a very long string flowing into an error message), which is a robustness improvement.
Level of scrutiny
Medium-high. The source change is small (~6 lines in BunString.cpp, ~15 lines removed in JSGlobalObject.rs), but BunString__toErrorInstance is the single funnel for all four Rust create_*_error_instance helpers and EncodedSlice::to_*_error_instance, so every native throw goes through it. Removing defensive guards based on a newly-established invariant is exactly the kind of change REVIEW.md flags for careful auditing ("delete defensive code only when you can show the condition cannot occur"). I traced the callers and the invariant holds — all four JSC::create*Error overloads return non-null — but a maintainer should confirm.
Other factors
- The evidence gate shows the new tests fail on main (segfault / UBSan /
message: "") and pass with the fix, in both debug+ASAN and release. - The PR description is stale relative to the current diff: it describes changes to
helpers.h(errorMessageOrFallback,createErrorInstance), a one-linebindings.cppchange forcreateAggregateError, and a fourth AggregateError test. After rebase onto main (where theget*ErrorInstancehelpers were consolidated intoBunString__toErrorInstance), the diff now only touchesBunString.cppand the test file has three tests.JSC__JSGlobalObject__createAggregateErrorstill callsarg3->toWTFString()directly and would produce an empty-messageAggregateErroron overflow — not a crash, but the description claims it was fixed. A human should confirm whether dropping that piece was intentional. - The comment-cop bot's feedback about a paragraph-long comment in
helpers.his moot sincehelpers.his no longer in the diff.
|
On the review note about the description: the body was rewritten for the rebuilt branch at the same time that review ran, so it now describes the current diff ( Dropping the AggregateError case was intentional. On current main |
…ng length limit (#42202) ### Problem - An error message that embeds a string from JS has a user-controlled length. Building one past the string length limit aborts the process, also inside `try`/`catch`: `panic(main thread): abort() called`, exit code 134. Example: `bun -e 'try { Buffer.from("x", "q".repeat(2**31-10)) } catch {}'`. - The cause: `WTF::makeString` and a default-constructed `WTF::StringBuilder` call `CRASH()` when the result passes `String::MaxLength`. Every message builder in `src/jsc/bindings/ErrorCode.cpp` is one of the two. ### Fix - `Bun::MessageBuilder` (`ErrorCode.h`) records the overflow. Every builder in `ErrorCode.cpp` uses it, and `determineSpecificType` and `JSValueToStringSafe` accept only that type. `createError` and `throwError` report a message that did not fit as `RangeError: Out of memory`. - Five sinks outside `ErrorCode.cpp` build with `tryMakeString`: the `performance.measure` mark, the `ReadableStream` source `type`, three `MessageEvent` constructor messages. - Correct because JSC reports `RangeError: Out of memory` for a string it cannot create. Node v26.3.0 reports the same inputs as `RangeError: Invalid string length`. A message under the limit is unchanged. - Verified: `test/js/bun/util/error-message-string-length-limit.test.ts`, 8 cases in one child, which aborts on main. Also the buffer, streams, webcrypto and MessageEvent suites. Self-reviewed: 7 concerns raised, 6 addressed, 1 is a question for a maintainer (Notes). ### Background - `String::MaxLength` is 2^31 - 1 code units. A JS string can be that long. - `StringBuilder` takes an `OverflowPolicy`. The default crashes. `RecordOverflow` sets a flag that `hasOverflowed()` reports, and later appends do nothing. - `determineSpecificType` renders the `Received ...` part of `ERR_INVALID_ARG_TYPE`. It appends `constructor.name` with no bound. - JSC treats its own messages this way in `ExceptionHelpers.cpp`. <details><summary>Notes</summary> **The type guards its own reads.** `StringBuilder::toString()` and `length()` assert that the builder did not overflow, so on `MessageBuilder` they are private. `tryToString()` returns a null string on overflow, and `finish(globalObject, scope)` throws. No `hasOverflowed()` check is written by hand in `ErrorCode.cpp`. **Question for a maintainer.** This PR reports an over-long message as `RangeError: Out of memory` with no `.code`, like JSC and like Node. #39481 (open) takes the other policy for the Rust-side error constructor: it keeps the error type and properties and uses a stand-in message. If that policy is preferred here too, the change is in two functions: `createError(ErrorCode, MessageBuilder&)` and `MessageBuilder::finish`. **Moved out of this PR.** The first revision also changed `process.execve` and `Error.stack`. Neither uses `MessageBuilder`, and each has its own question, so each has its own PR: - `process.execve`: #42308 - `Error.stack` and the default `Error.prepareStackTrace`: #42314 **Cases in the test.** All abort on main and on 1.4.2 with exit code 134. With this branch each one throws `RangeError: Out of memory`. ```js const long = "q".repeat(2 ** 31 - 10); class C {} Object.defineProperty(C, "name", { value: long }); Buffer.from("x", long); // UNKNOWN_ENCODING Buffer.from(new C()); // determineSpecificType Buffer.alloc(1).indexOf(new C()); // JSBuffer.cpp message new SocketAddress({ address: "1.2.3.4", port: new C() }); // Bun__ErrorCode__determineSpecificType, the entry Rust validators call performance.measure("m", long); // PerformanceUserTiming.cpp crypto.subtle.importKey(long, new Uint8Array(8), "AES-GCM", false, ["encrypt"]); // JSSubtleCrypto.cpp new ReadableStream({ type: long }); // JSReadableStream.cpp ReadableStream.from(Symbol(long)); // throwNotIterable ``` **Verified by hand, not in the test.** - The three `MessageEvent` messages: `new MessageEvent("m", { ports: long })`, `{ ports: [long] }`, `{ source: long }`. Each aborts on main and throws with this branch. They render the value through the inspector first, which takes about 20 s in a release build and about 9 minutes in a debug ASAN build for a 2 GiB string, so they cannot be a test. - The quoted `ERR_INVALID_ARG_VALUE` path (`Buffer.alloc(4).fill(long, "hex")`), `ERR_OUT_OF_RANGE` with a long name, and `NodeError.prototype.toString` with a long message throw with this branch. The quoted path appends one character at a time, about 8 minutes in a debug build. - The doors reported in the comments below (`Buffer.byteLength`, `fs.mkdirSync` mode, `process.kill` signal name, `child_process.spawn`) all go through `MessageBuilder`. - The whole fixture also passes under `BUN_JSC_validateExceptionChecks=1`. **Scope.** This does not cover every `makeString` call that can reach an error sink. A census at `4ff91937` counted 276 `makeString(` calls and 110 default-constructed `StringBuilder` locals under `src/jsc`. The `makeString` calls left in `ErrorCode.cpp` take an `ASCIILiteral` or an internal string, so the binary bounds their length. A different mechanism is also out of scope: `String::utf8()` aborts for a Latin-1 string of more than about 2^30 characters, at many call sites. **Cost of the test.** The length is what is under test, so the input is about 2 GiB. One child runs all 8 cases, so the string is allocated once. Peak RSS is 4.4 GB. The file takes 2 to 4 s in a debug ASAN build. It skips below 10 GiB of total memory, the gate `test/js/bun/transpiler/source-too-large.test.ts` uses. The one test keeps a 30 s ceiling, because the default 5 s is too close on a loaded machine. **Pre-existing failures** seen in the surrounding suites, which also fail with `src/` at `origin/main`: `error-gc-test.test.js` (timeouts in a debug build), `socket.test.ts > should not call drain before handshake`, and `process.test.js > process` (this container has no `USER` in the environment). </details>
…ng length limit (oven-sh#42202) ### Problem - An error message that embeds a string from JS has a user-controlled length. Building one past the string length limit aborts the process, also inside `try`/`catch`: `panic(main thread): abort() called`, exit code 134. Example: `bun -e 'try { Buffer.from("x", "q".repeat(2**31-10)) } catch {}'`. - The cause: `WTF::makeString` and a default-constructed `WTF::StringBuilder` call `CRASH()` when the result passes `String::MaxLength`. Every message builder in `src/jsc/bindings/ErrorCode.cpp` is one of the two. ### Fix - `Bun::MessageBuilder` (`ErrorCode.h`) records the overflow. Every builder in `ErrorCode.cpp` uses it, and `determineSpecificType` and `JSValueToStringSafe` accept only that type. `createError` and `throwError` report a message that did not fit as `RangeError: Out of memory`. - Five sinks outside `ErrorCode.cpp` build with `tryMakeString`: the `performance.measure` mark, the `ReadableStream` source `type`, three `MessageEvent` constructor messages. - Correct because JSC reports `RangeError: Out of memory` for a string it cannot create. Node v26.3.0 reports the same inputs as `RangeError: Invalid string length`. A message under the limit is unchanged. - Verified: `test/js/bun/util/error-message-string-length-limit.test.ts`, 8 cases in one child, which aborts on main. Also the buffer, streams, webcrypto and MessageEvent suites. Self-reviewed: 7 concerns raised, 6 addressed, 1 is a question for a maintainer (Notes). ### Background - `String::MaxLength` is 2^31 - 1 code units. A JS string can be that long. - `StringBuilder` takes an `OverflowPolicy`. The default crashes. `RecordOverflow` sets a flag that `hasOverflowed()` reports, and later appends do nothing. - `determineSpecificType` renders the `Received ...` part of `ERR_INVALID_ARG_TYPE`. It appends `constructor.name` with no bound. - JSC treats its own messages this way in `ExceptionHelpers.cpp`. <details><summary>Notes</summary> **The type guards its own reads.** `StringBuilder::toString()` and `length()` assert that the builder did not overflow, so on `MessageBuilder` they are private. `tryToString()` returns a null string on overflow, and `finish(globalObject, scope)` throws. No `hasOverflowed()` check is written by hand in `ErrorCode.cpp`. **Question for a maintainer.** This PR reports an over-long message as `RangeError: Out of memory` with no `.code`, like JSC and like Node. oven-sh#39481 (open) takes the other policy for the Rust-side error constructor: it keeps the error type and properties and uses a stand-in message. If that policy is preferred here too, the change is in two functions: `createError(ErrorCode, MessageBuilder&)` and `MessageBuilder::finish`. **Moved out of this PR.** The first revision also changed `process.execve` and `Error.stack`. Neither uses `MessageBuilder`, and each has its own question, so each has its own PR: - `process.execve`: oven-sh#42308 - `Error.stack` and the default `Error.prepareStackTrace`: oven-sh#42314 **Cases in the test.** All abort on main and on 1.4.2 with exit code 134. With this branch each one throws `RangeError: Out of memory`. ```js const long = "q".repeat(2 ** 31 - 10); class C {} Object.defineProperty(C, "name", { value: long }); Buffer.from("x", long); // UNKNOWN_ENCODING Buffer.from(new C()); // determineSpecificType Buffer.alloc(1).indexOf(new C()); // JSBuffer.cpp message new SocketAddress({ address: "1.2.3.4", port: new C() }); // Bun__ErrorCode__determineSpecificType, the entry Rust validators call performance.measure("m", long); // PerformanceUserTiming.cpp crypto.subtle.importKey(long, new Uint8Array(8), "AES-GCM", false, ["encrypt"]); // JSSubtleCrypto.cpp new ReadableStream({ type: long }); // JSReadableStream.cpp ReadableStream.from(Symbol(long)); // throwNotIterable ``` **Verified by hand, not in the test.** - The three `MessageEvent` messages: `new MessageEvent("m", { ports: long })`, `{ ports: [long] }`, `{ source: long }`. Each aborts on main and throws with this branch. They render the value through the inspector first, which takes about 20 s in a release build and about 9 minutes in a debug ASAN build for a 2 GiB string, so they cannot be a test. - The quoted `ERR_INVALID_ARG_VALUE` path (`Buffer.alloc(4).fill(long, "hex")`), `ERR_OUT_OF_RANGE` with a long name, and `NodeError.prototype.toString` with a long message throw with this branch. The quoted path appends one character at a time, about 8 minutes in a debug build. - The doors reported in the comments below (`Buffer.byteLength`, `fs.mkdirSync` mode, `process.kill` signal name, `child_process.spawn`) all go through `MessageBuilder`. - The whole fixture also passes under `BUN_JSC_validateExceptionChecks=1`. **Scope.** This does not cover every `makeString` call that can reach an error sink. A census at `4ff91937` counted 276 `makeString(` calls and 110 default-constructed `StringBuilder` locals under `src/jsc`. The `makeString` calls left in `ErrorCode.cpp` take an `ASCIILiteral` or an internal string, so the binary bounds their length. A different mechanism is also out of scope: `String::utf8()` aborts for a Latin-1 string of more than about 2^30 characters, at many call sites. **Cost of the test.** The length is what is under test, so the input is about 2 GiB. One child runs all 8 cases, so the string is allocated once. Peak RSS is 4.4 GB. The file takes 2 to 4 s in a debug ASAN build. It skips below 10 GiB of total memory, the gate `test/js/bun/transpiler/source-too-large.test.ts` uses. The one test keeps a 30 s ceiling, because the default 5 s is too close on a loaded machine. **Pre-existing failures** seen in the surrounding suites, which also fail with `src/` at `origin/main`: `error-gc-test.test.js` (timeouts in a debug build), `socket.test.ts > should not call drain before handshake`, and `process.test.js > process` (this container has no `USER` in the environment). </details>
This branch has not been deployed
1 participant
Problem
expect()whose failure message is over the limit reports as(pass); a debug build panics withassertion failed: self.has_exception()inJSGlobalObject::throw(src/jsc/JSGlobalObject.rs:815on main), reached fromExpect::throw_rendered.Bun.listen({ unix })with a path whose "Failed to listen at ..." message is over the limit segfaults in release (Segmentation fault at address 0x5) and trips UBSan in debug (bindings.cpp: member call on null pointer of type 'JSC::JSCell'), becausesrc/runtime/socket/Listener.rs:504creates the error and thenputssyscall/errno/addresson it.Bun.JSONC.parseof a ~1 MiB token throws the empty value itself (debug aborts onASSERT(!value.isEmpty())inJSC__VM__throwError).BunString__toErrorInstance(src/jsc/bindings/BunString.cpp:144on main), the one C++ entry behindcreate_error_instance,EncodedSlice::to_*_error_instanceandString::to_*_error_instancefor all four error kinds, converts the message withZig::toStringCopy/toWTFString, which return a nullWTF::Stringwhen the text is longer thanBun__stringSyntheticAllocationLimit/WTF::String::MaxLength(or the copy fails to allocate). It then returned an emptyJSValue. Nothing on that path throws, soJSGlobalObject::throwturned it intoJsError::Thrownwith no exception pending, and native code unwound as if it had thrown while JS observed nothing. The other callers (dozens of sites: promise rejections, callbacks,puton the result) assume they got an object.setSyntheticAllocationLimitForTesting(tests using it could silently pass) and, without it, through any message over 2^31 characters (expect(<happy-dom Element>).toBeNull() takes ~40s+ on a large tree and then SILENTLY PASSES (throw is lost) #37310 hit this with a rendered happy-dom tree).Fix
BunString__toErrorInstancealways creates the error. When the message conversion came back null for a non-empty (orDead, i.e. already failed to allocate) source, the error carries"The error message exceeds the maximum string length"instead. Name, type and the properties callers add afterwards (code,errno,syscall, ...) are unaffected; a message under the limit is byte-for-byte unchanged.ErrorCodeCache::createErroralready documents for theErrorCodepath). Truncating here would mean materializing a message of up to 2 GiB; bounding rendered values belongs to the producer (test runner: stop printing shared references without bound in assertion diffs and snapshots #34179 and Cap expect() failure message rendering so huge values cannot swallow the assertion error #37311 do that forexpect()and remain useful independently; with this change thethrow()-site fallback in Cap expect() failure message rendering so huge values cannot swallow the assertion error #37311 becomes unreachable). For comparison, Node surfaces the analogous case (anassertmessage that overflows V8's string limit) asRangeError: Invalid string length, so the failure is still reported there as well; here the original error type and properties are kept.JSGlobalObject.rs: theis_empty()/debug_assert!(has_exception())branches inthrow,throw_sys_errorandthrow_todoare deleted; the constructor can no longer return an empty value.JSC__VM__throwErrorstill asserts a non-empty value in debug builds if that ever regresses.systemErrorToErrorInstance,Bun__createErrorWithCodeand the twocreateAggregateErrorhelpers already always return an object, and their messages areWTF-backed strings built byBunString::create_format, so a missing message there needs aDeadstring, i.e. a message over 2^31 characters, which the testing limit cannot reach.EncodedSlice__toDOMExceptionInstanceis also left alone:createDOMExceptiongives an empty message its own meaning per exception code (default texts;InvalidURLErrorstores the message as.input), so a stand-in string would be wrong there.test/js/bun/util/native-error-message-too-long.test.ts, one subprocess per case since the limit is process-wide and the unfixed cases crash:expect(): a failure message of exactly the limit is kept intact, one character past it gets the fallback, a rendered received value past the limit gets the fallback, and an uncaught one is reported bybun testas(fail)witherror: The error message exceeds the maximum string length(exit code 1).Bun.listenthrows the Error instead of crashing.Bun.JSONC.parsethrows aSyntaxErrorwith the fallback; a normal-sized parse error message is unchanged.src/changes (UBSan null member call, abort, panic) and on the last release canary, and all pass with them. Also ranexpect-label,expect-unreaachable,plugins.test.ts,socketaddress.spec.ts(coversthrow_sys_error),jsonc,error-name-preservation: all pass.Background
BunString/bun_core::String: the tagged string shared between Rust and C++. AWTFStringImpltag shares an existing JSC string,EncodedSliceborrows Rust bytes (with encoding bits in the pointer) and is copied on the way in,Deadmeans a creation already failed.create_error_instanceand friends format a Rust message into a buffer and hand it over as a UTF-8EncodedSlice.WTF::String::MaxLengthis 2^31 - 1 code units.Bun__stringSyntheticAllocationLimitis a process-wide lower cap thatbun:internal-for-testing'ssetSyntheticAllocationLimitForTestingcan set down to 1 MiB so tests can exercise the too-long paths without allocating gigabytes;Zig::toStringCopyhonours both forEncodedSliceinput.JsError::Thrown: the Rust-side marker meaning "a JS exception is now pending in the VM"; host functions return it and the thunk hands control back to JSC expectingvm.exception()to be set. Returning it with nothing pending is how the throw was lost.JSValue::empty()(RustJSValue::ZERO): JSC's "no value" sentinel, not a JS value; using it as one (put, throwing it,awaiting it) dereferences null.Notes
The first revision of this PR predates the consolidation of the error constructors into
BunString__toErrorInstance(theZigStringrename and the removal ofZig::get*ErrorInstance). It fixed the same defect inhelpers.hfor the four per-type helpers pluscreateAggregateError, whose message then arrived as aZigStringand was reachable through the testing limit; on current main that message is aWTF-backed string and the site is covered by the exclusion above. The branch was rebuilt on top of main rather than rebased, so the diff is the one described here.[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file