Throw ERR_STRING_TOO_LONG instead of aborting for 2 GiB to 4 GiB strings - #37215
Conversation
Blob.text(), Bun.file().text(), fs.readFileSync(path, "utf8") and Blob.json() on 2^31..2^32-1 bytes aborted the process: the Rust-side guards in front of WTF string construction only checked Bun__stringSyntheticAllocationLimit (2^32-1 by default) and missed WTF::StringImpl::MaxLength (2^31-1), which StringImplShape enforces with a RELEASE_ASSERT. Lengths >= 2^32 were already caught. - bun_core::String::max_length() now clamps the synthetic limit to WTF::StringImpl::MaxLength, matching the C++ helpers.h checks - the create_external* guards use > instead of >=, so 2^31-1 (the largest valid WTF length) keeps working - ZigString__toJSONObject checks MaxLength too instead of falling through to JSONParse on a null string - BunString__createUTF8ForJS rejects oversized ASCII input instead of asserting - error messages report the real limit (2147483647, same as the C++ message) instead of 2^32-1
|
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 change aligns string limits with the signed 32-bit WTF maximum, updates oversized-string guards and error messages, and adds subprocess tests for 2 GiB filesystem and web string conversions. ChangesString length limits
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/fs/fs-oom.test.ts`:
- Around line 89-93: Remove the explicit 180_000 per-test timeout arguments from
the boundary tests in test/js/node/fs/fs-oom.test.ts lines 89-93 and 95-99, and
test/js/web/fetch/blob-oom.test.ts lines 159-191 and 193-223. Leave each test’s
assertions and behavior unchanged so the repository-wide timeout policy applies.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28c3c175-3b74-4327-97a6-242a4d3e46ca
📒 Files selected for processing (7)
src/bun_core/string/mod.rssrc/jsc/ZigString.rssrc/jsc/bindings/BunString.cppsrc/jsc/bindings/bindings.cppsrc/jsc/lib.rstest/js/node/fs/fs-oom.test.tstest/js/web/fetch/blob-oom.test.ts
There was a problem hiding this comment.
LGTM — both earlier findings (memory gating on the 2 GiB tests, stale SAFETY bound) are addressed, and the per-test timeouts are gone.
What was reviewed:
max_length()clamp +>=→>guards: verified all Rust callers ofmax_length()use>, and the C++helpers.halready uses the samemin(limit, WTF::String::MaxLength)pattern this now mirrors.- Error-message text matches
ErrorCode.cpp'smakeString(..., WTF::String::MaxLength, ...)(2147483647). ZigString__toJSONObject/BunString__createUTF8ForJSguards follow the existinghelpers.hshape; checked the null-string fallthrough in the JSON path.- Tests: subprocess isolation,
os.totalmem()gate matchesbuffer.test.jsprecedent, both boundary sides (2^31 throws, 2^31-1 succeeds) covered.
Extended reasoning...
Overview
This PR fixes a process abort when converting byte sources in the [2^31, 2^32) range to JS strings. Seven files touched: src/bun_core/string/mod.rs (clamp max_length() to i32::MAX, adjust three create_external* guards from >= to >, add WTF_STRING_MAX_LENGTH constant), src/jsc/bindings/BunString.cpp (guard the all-ASCII fast path in BunString__createUTF8ForJS), src/jsc/bindings/bindings.cpp (add || WTF::String::MaxLength to the JSON null-string check), three error-message string updates in ZigString.rs/lib.rs, and two test files with new subprocess-isolated 2 GiB boundary tests plus updated message assertions.
Security risks
None. The change converts an unrecoverable RELEASE_ASSERT abort (a DoS on user-controllable input size) into a catchable ERR_STRING_TOO_LONG, which is strictly safer. No new untrusted input reaches native code; the guards are tightened, not loosened. The >=→> change admits exactly len == 2^31-1, which StringImplShape's data.size() <= MaxLength assert accepts.
Level of scrutiny
Medium-high — this is core string infrastructure at the Rust/C++ FFI boundary, and the guards front unsafe external-string construction. However, the change is a strict tightening: any input that previously reached WTF now still does (the only newly-admitted length is exactly i32::MAX, which WTF accepts), and inputs in [2^31, 2^32) that previously crashed now return String::DEAD / throw. I verified against helpers.h that the C++ side already uses the identical min(syntheticLimit, MaxLength) pattern this PR brings the Rust side into agreement with, and grepped all max_length() callers to confirm they consistently use >.
Other factors
I left two prior rounds of feedback on this PR (memory-gating the 2 GiB tests; a stale SAFETY comment bound), both addressed in 6f49733 and fb170de. CodeRabbit's per-test-timeout feedback was addressed in ad378c8. The comment-cop doc-comment length warnings were trimmed in edd9c46. Tests cover both sides of the boundary (2^31 throws, 2^31-1 succeeds), run in subprocesses to isolate the 2 GiB peak, and gate on os.totalmem() < 10 GiB matching the buffer.test.js 4 GiB precedent. No outstanding reviewer comments.
|
CI status for build 90684: this PR's own tests pass on every lane that ran them, including the new 2 GiB cases in blob-oom.test.ts and fs-oom.test.ts on the linux, asan, macOS, and Windows lanes that completed. The two failing jobs are unrelated to the change:
Every other annotation entry is a known-flaky test that passed on retry or when run alone. No open review comments; the diff is ready for review. |
…ngs (oven-sh#37215) ### Problem `await new Blob([new Uint8Array(2 ** 31)]).text()` aborts the process. Same for `Bun.file(path).text()` and `fs.readFileSync(path, "utf8")` when the file is between 2 GiB and 4 GiB - 1 bytes (a sparse `truncate -s 2G big.txt` reproduces it). Release builds die with a silent SIGABRT; debug builds fail with: ``` ASSERTION FAILED: data.size() <= MaxLength WTF/wtf/text/StringImpl.h(891) StringImplShape(uint32_t, std::span<const Latin1Character>, unsigned) ``` Sizes of 4 GiB and above already threw catchable errors, and 2^31 - 1 worked, so only the [2^31, 2^32) range crashed. ### Cause The Rust-side guards in front of WTF string construction (`bun_core::String::max_length()`) only consult `Bun__stringSyntheticAllocationLimit`, which defaults to 2^32 - 1. The hard cap enforced by `RELEASE_ASSERT` in the `StringImplShape` constructors is `WTF::StringImpl::MaxLength` = 2^31 - 1. The C++ conversions in `helpers.h` already clamp (`len > Bun__stringSyntheticAllocationLimit || len > WTF::String::MaxLength`); the Rust side missed the second half, so external-string creation for lengths in [2^31, 2^32) passed the guard and tripped the assert. ### Fix - `String::max_length()` clamps the synthetic limit to `WTF::StringImpl::MaxLength` (2^31 - 1). - The `create_external*` guards compare with `>` instead of `>=`, so 2^31 - 1, the largest valid WTF length, still works. - `ZigString__toJSONObject` also checks `WTF::String::MaxLength`, so `Blob.json()` at these sizes reports `ERR_STRING_TOO_LONG` instead of falling through to `JSONParse` on a null string. - `BunString__createUTF8ForJS` rejects oversized all-ASCII input with `ERR_STRING_TOO_LONG` instead of asserting. - Error messages report the real limit (2147483647, matching the C++ `ErrorCode.cpp` text) instead of "2^32-1". `Blob.text()` / `Bun.file().text()` now throw `ERR_STRING_TOO_LONG`; `fs.readFileSync(path, "utf8")` reports `ENOMEM` like the existing >= 4 GiB and `/dev/zero` paths (the fs layer speaks errno). `Buffer.prototype.toString` already threw `ERR_STRING_TOO_LONG` for these sizes via its own `WTF::String::MaxLength` check in `JSBuffer.cpp`. ### Verification - `test/js/web/fetch/blob-oom.test.ts`: subprocess tests for `Blob.text()`, `Blob.json()` and `Bun.file().text()` at 2^31 bytes (abort before, catchable error after), plus the existing synthetic-limit assertions updated to the corrected message. - `test/js/node/fs/fs-oom.test.ts`: `readFileSync(file, "utf8")` at 2^31 bytes throws `ENOMEM`; at 2^31 - 1 bytes still decodes to a 2147483647-length string. The fixture files are sparse so only the in-memory read costs 2 GiB, and each case runs in a subprocess. - Manually verified 4 GiB inputs still produce the same catchable errors as before, and 2^31 - 1 still succeeds on all three faces. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs-oom.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…string Decoding 2^31 .. 2^32-1 bytes of ASCII with TextDecoder.decode() returned the empty string with no error: Zig::toStringCopy maps a failed string creation (length over WTF::StringImpl::MaxLength, or allocation failure) to a null WTF::String, and jsString() turns that into the empty string. Zig::toJSStringGC (and with it ZigString__toValueGC, i.e. ZigString.toJS) now throws ERR_STRING_TOO_LONG when the input is over the length limit and an out-of-memory error when allocation fails, instead of silently returning "". The non-UTF-8 branches of the single-argument Zig::toStringCopy now check the synthetic allocation limit like the other helpers, and JSC__JSValue__fromEntries checks for the exception before putDirect. The sibling guards on the external-string paths (2 GiB to 4 GiB aborts) are fixed separately in #37215.
2 participants
Problem
await new Blob([new Uint8Array(2 ** 31)]).text()aborts the process. Same forBun.file(path).text()andfs.readFileSync(path, "utf8")when the file is between 2 GiB and 4 GiB - 1 bytes (a sparsetruncate -s 2G big.txtreproduces it). Release builds die with a silent SIGABRT; debug builds fail with:Sizes of 4 GiB and above already threw catchable errors, and 2^31 - 1 worked, so only the [2^31, 2^32) range crashed.
Cause
The Rust-side guards in front of WTF string construction (
bun_core::String::max_length()) only consultBun__stringSyntheticAllocationLimit, which defaults to 2^32 - 1. The hard cap enforced byRELEASE_ASSERTin theStringImplShapeconstructors isWTF::StringImpl::MaxLength= 2^31 - 1. The C++ conversions inhelpers.halready clamp (len > Bun__stringSyntheticAllocationLimit || len > WTF::String::MaxLength); the Rust side missed the second half, so external-string creation for lengths in [2^31, 2^32) passed the guard and tripped the assert.Fix
String::max_length()clamps the synthetic limit toWTF::StringImpl::MaxLength(2^31 - 1).create_external*guards compare with>instead of>=, so 2^31 - 1, the largest valid WTF length, still works.ZigString__toJSONObjectalso checksWTF::String::MaxLength, soBlob.json()at these sizes reportsERR_STRING_TOO_LONGinstead of falling through toJSONParseon a null string.BunString__createUTF8ForJSrejects oversized all-ASCII input withERR_STRING_TOO_LONGinstead of asserting.ErrorCode.cpptext) instead of "2^32-1".Blob.text()/Bun.file().text()now throwERR_STRING_TOO_LONG;fs.readFileSync(path, "utf8")reportsENOMEMlike the existing >= 4 GiB and/dev/zeropaths (the fs layer speaks errno).Buffer.prototype.toStringalready threwERR_STRING_TOO_LONGfor these sizes via its ownWTF::String::MaxLengthcheck inJSBuffer.cpp.Verification
test/js/web/fetch/blob-oom.test.ts: subprocess tests forBlob.text(),Blob.json()andBun.file().text()at 2^31 bytes (abort before, catchable error after), plus the existing synthetic-limit assertions updated to the corrected message.test/js/node/fs/fs-oom.test.ts:readFileSync(file, "utf8")at 2^31 bytes throwsENOMEM; at 2^31 - 1 bytes still decodes to a 2147483647-length string. The fixture files are sparse so only the in-memory read costs 2 GiB, and each case runs in a subprocess.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs-oom.test.ts