Reject instead of aborting when arrayBuffer() or bytes() reads a body above 4 GiB - #37243
Conversation
|
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughNative ArrayBuffer creation checks the size limit and handles byte cleanup and exceptions. The Rust wrapper propagates FFI exceptions. Fetch tests cover ChangesArrayBuffer size-limit handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Oversized fetched bodies are handled as errors rather than process aborts in the reviewed paths; no issue identified here needs resolution before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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 adds error-path guards inside native JSC/Blob body-consumption code (with WTF ref and InternalBlob byte cleanup ordering on the new early returns), a human look would still be worthwhile.
What was reviewed
constructFromEncodingguard: gate math (span.size() > MAX_ARRAY_BUFFER_SIZE / 3) and why only the UTF-16/utf8 arm needs it — Latin-1→utf8 and ucs2 both cap at 2·INT_MAX < 2^32.Any::to_array_buffer_view: on both new error returns the owned bytes / +1 WTF ref are released (*self = Any::Blob(...)drops theInternalBlob;OwnedStringadopts before thetoo_largeearly return).- New tests follow the adjacent 2 GiB subprocess pattern in the same files (memory gate, pipe drain, exit-code-last).
Extended reasoning...
Overview
Converts a process abort (JSC RELEASE_ASSERT m_sizeInBytes <= MAX_ARRAY_BUFFER_SIZE) into a catchable RangeError on three paths that hand an already-encoded buffer to ArrayBuffer::createFromBytes: Buffer.from(string, "utf8") in JSBuffer.cpp, and the WTFStringImpl/InternalBlob arms of Any::to_array_buffer_view in Blob.rs. Adds a mirrored MAX_ARRAY_BUFFER_SIZE constant in array_buffer.rs and two subprocess tests gated on totalmem() >= 10 GiB.
Security risks
None introduced. This turns an unrecoverable abort on adversarial-size input into a catchable error, which is strictly a hardening. No new unsafe blocks, no new FFI surface, no change to successful-path ownership.
Level of scrutiny
Medium-high. The logic is small and additive, but it lives in native memory-handling paths (JSBuffer.cpp, Blob.rs) where the repo's review history flags error-path resource release as the most-blocked category. I traced each new early return: the InternalBlob arm assigns *self = Any::Blob(Blob::default()) before returning, which drops the owned Vec<u8>; the WTFStringImpl arm computes too_large while the raw pointer is still live, then adopts it into OwnedString (whose Drop releases the ref) before the early return — so neither path leaks. The gate arithmetic checks out: WTF string length ≤ INT_MAX, so 8-bit → utf8 caps at 2·INT_MAX = 4294967294 < 2^32 (hence the !is_8bit() skip), and > MAX/3 correctly bounds the only expansion factor (3× for BMP U+0800..U+FFFF) that can cross 2^32.
Other factors
The tests are direct copies of the 2 GiB pattern immediately above them in the same files (subprocess, Promise.all pipe drain, JSON-encoded results, exit code asserted last, 90s timeout with a comment). The PR description references #37215 as the sibling byte→string fix and demonstrates the mechanism was traced end-to-end. Given the native error-path cleanup ordering is the kind of thing a maintainer will want to eyeball, I'm deferring rather than approving.
![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. The size guards look correct (only UTF-16→UTF-8 can cross 2^32; the / 3 fast-path gate is exact; the error paths release the WTF ref / drop the InternalBlob), and the tests are properly subprocess-isolated and mem-gated. Because the fix places the cap check at three separate call sites rather than in the shared createFromBytes wrappers, and adds ~4.5 min of 10 GiB-gated subprocess tests to CI, a human look at the layering and CI-cost tradeoff would still be worthwhile.
What was reviewed
- Confirmed no other encoding arm can exceed 2^32 bytes: Latin1→UTF-8 caps at 2×INT_MAX, UTF-16→UTF-16LE at 2×INT_MAX, base64/hex shrink.
WTFStringImplerror path:OwnedStringstill adopts the +1 ref before the early return, so it's released on drop;InternalBloberror path drops the bytes via the enum reassignment.Bun__encoding__byteLengthUTF16AsUTF8returnssize_tandMAX_ARRAY_BUFFER_SIZEis already in scope in JSBuffer.cpp — no truncation on the compare.
Extended reasoning...
Overview
Converts a process abort (JSC RELEASE_ASSERT(m_sizeInBytes <= MAX_ARRAY_BUFFER_SIZE)) into a catchable RangeError on three paths where a string's UTF-8 encoding, or an accumulated body, can exceed 2^32 bytes: Buffer.from(s, "utf8") in JSBuffer.cpp, and the WTFStringImpl / InternalBlob arms of Any::to_array_buffer_view in Blob.rs. Adds a MAX_ARRAY_BUFFER_SIZE constant to array_buffer.rs mirroring JSC's, plus subprocess-based regression tests gated on totalmem() >= 10 GiB with 90s timeouts.
Security risks
None new. The change fails closed (throws instead of continuing), and the size scan is gated so ordinary strings never hit it. This actually removes a user-reachable DoS-via-abort.
Level of scrutiny
Medium-high. This is native C++/Rust touching ArrayBuffer construction and WTF string lifetime — the sort of code where a misplaced early-return leaks a ref. I traced both new error paths: in the WTFStringImpl arm the too_large bool is computed against a borrow, then the impl is adopted into OwnedString (which releases on Drop), then the early return fires — so the +1 ref is balanced. In the InternalBlob arm *self = Any::Blob(...) drops the old InternalBlob and its Vec. No leaks introduced.
Other factors
Two things push me to defer rather than approve:
-
Layer placement. REVIEW.md's "fix bugs at the layer that owns the violated invariant" would point at putting the size check inside
from_default_allocator/JSBuffer__bufferFromPointerAndLengthAndDeinit(the last stop beforecreateFromBytes) so every caller is covered once. The PR instead checks at three call sites, with a stated rationale (avoid materializing the >4 GiB encode buffer, ~4 GB RSS savings). That's a reasonable tradeoff, but it is a design choice, and it means any future caller offrom_default_allocatorwith an unchecked size hits the same abort. A maintainer should confirm this is the intended shape (or ask for a belt-and-suspenders check in the shared helper too). -
CI cost. The three new 90 s subprocess tests plus two existing tests bumped to 90 s add meaningful wall-clock to a hot test file, and they only run on ≥10 GiB runners. This matches the neighboring 2 GiB / 4 GiB tests, but per the repo's own guidance a new file segment over ~10 s gets scrutinized — a human should sign off on the cost.
The comment-cop bot flagged the new comments three times; all threads are now resolved and the final two-line comments look like legitimate invariant documentation, not workaround justification.
|
On the two deferred points: Layer placement: the checks run before encoding so the error path never materializes the oversized buffer (the encode for the repro is a 4.3 GB allocation). A check inside the shared adopt-bytes helpers would only fire after that allocation exists, and those helpers would also need to free the adopted bytes on the throw path, which is a larger ownership change than this fix needs. Callers that only learn the size after producing bytes (mmap, gunzip) have separate open PRs: #34119, #35856. Test cost: 90s is a ceiling, not a runtime. The 4 GiB case measures about 2.6s in release and 13s under debug ASAN, and the block skips on runners with less than 10 GiB. The two existing 2 GiB tests got the same ceiling because one of them measured 5.001s against the default 5s limit in a release run. |
|
CI status: the diff is green on every lane that ran it, including the new 4 GiB tests on the 10 GiB+ runners. The two remaining red jobs in build 90881 are unrelated and also failing on main:
Ready for review. |
…39564) ### Problem - More than 2^32 bytes of captured child output kill the process instead of throwing. ``await Bun.$`head -c 4294967297 /dev/zero`.quiet()`` dies with `panic(main thread): abort() called` (SIGABRT). `Bun.spawnSync({ cmd: ["head", "-c", "4294967297", "/dev/zero"] })` dies with `panic: int cast: TryFromIntError(PosOverflow)`. So does an output of exactly 2^32 bytes, which is a valid Buffer length. - `bun:ffi` `toBuffer(ptr, 0, 2 ** 32 + 1)` reaches the same abort with no memory at all. - Cause 1: `JSBuffer__bufferFromPointerAndLengthAndDeinit` (`src/jsc/bindings/JSBuffer.cpp:383` on main) and `JSBuffer__fromMmap` (`JSBuffer.cpp:2588`) hand their bytes to `ArrayBuffer::createFromBytes` without a length check. JSC RELEASE_ASSERTs there above `MAX_ARRAY_BUFFER_SIZE` (`vendor/WebKit/Source/JavaScriptCore/runtime/ArrayBuffer.cpp:150`). - Cause 2: the spawnSync output arms (`subprocess/Readable.rs:294`, `SubprocessPipeReader.rs:333`) went through `ArrayBuffer::from_owned_bytes` / `from_bytes`, which convert the length through `u32` with `expect`. That panic fires at 2^32 and above, before the bytes reach JSC. - Cause 3: once the hand-off throws, two error paths that nothing had reached before misbehave. `spawn_maybe_sync` (`js_bun_spawn_bindings.rs:2041`) returns without `finalize`, which leaks the subprocess. The shell (`interpreter.rs:1227`, added in #37275) rejects with the `JSC::Exception` cell instead of the thrown value, and the JS `reject` callback in `builtins/shell.ts` expects `(code, stdout, stderr)`, so it throws a TypeError and the promise never settles. In a debug build the child dies on an assertion in `JSCell::toStringSlowCase`. ### Fix - Both entry points in `JSBuffer.cpp` release the bytes (the deallocator, or `munmap`) and throw `RangeError: Out of memory`, the error `new ArrayBuffer(2 ** 32 + 1)` throws, for a length above `MAX_ARRAY_BUFFER_SIZE`. The `JSBuffer.cpp` and `JSBuffer.h` hunks are byte for byte the ones in #39558 (same blobs), which adds the check to the ArrayBuffer and typed array entry points at the same time. The two PRs merge in either order. The rest of this PR is what that check makes reachable. - The check belongs in the entry points because they are the last step before the assert, and every producer of a Buffer from native bytes uses them: spawnSync, `Bun.$`, `bun:ffi`, `bun:sqlite` serialize, zlib, `Bun.Archive`, `Bun.Image`. A length of exactly 2^32 still passes. JSC accepts it, and `toBuffer(ptr, 0, 2 ** 32)` already works today. - The bytes are released before the throw because the caller gave them up when it called. The `length == 0` branch of the same function already does this. The repro's RSS drops to about 340 MiB after the catch. - The two spawnSync output arms call `JSValue::create_buffer_from_box`. It makes the same C++ call as before with the same deallocator, without the `u32` hop. The `u32` conversions themselves stay as they are: #39558 removes them for the `Bun.file()` path, and this PR does not depend on that. - `spawn_maybe_sync` builds stdout, stderr and the resource usage object first, runs `finalize`, and propagates an error after that. The pre-existing return for an exception that is already pending after the wait (a termination) runs `finalize` too. `finalize` releases the other stream's buffer and the abort signal reference, and it does not run JS, so a pending exception does not affect it. - `to_js_buffer_from_memfd` and `to_js_buffer_from_fd` return `Err` when they throw. Before, they returned `Ok(JSValue::ZERO)` with the exception pending, and spawnSync would have stored that empty value in the result object. Nothing reaches this path from JS since #33832 (spawnSync no longer uses a memfd for stdout), so it has no test of its own. - The shell interpreter takes the error with `take_error`, which unwraps the `JSC::Exception`, and the JS `reject` callback forwards the value to the promise. `reject` has exactly one caller, this path. Exit codes still go through `resolve`, which builds the `ShellError`. - Verified with `test/js/bun/ffi/ffi.test.js` ("toBuffer at the Buffer length limit"): on main the child aborts, with the fix it throws a RangeError and a view of exactly 2^32 bytes is still created. This case costs no memory and runs on every platform. It only checks the error class, so it holds with #33353 too. - Verified with `test/js/bun/spawn/spawnSync.test.ts` ("spawnSync output at the Buffer length limit"): 2^32 + 1 bytes throw the RangeError, 2^32 bytes come back as a Buffer. On main both cases die with the `int cast` panic. About 11 s per case in a debug build, 8.3 GiB peak RSS in the child, so the block skips below 16 GiB of RAM and is POSIX only (`head -c`). The glibc CI test machines have 8 GB and skip it, the ASAN machines have 64 GB and run it. - Verified with `test/js/bun/shell/shelloutput.test.ts` ("stdout at the Buffer length limit"): the promise rejects with the RangeError. On main the child aborts. With the entry point check alone the child dies on the `toStringSlowCase` assertion. Same size gate. - `bun bd test` passes on `spawnSync.test.ts`, `spawn.test.ts`, `spawn-maxbuf.test.ts`, `spawnsync-isolated-event-loop.test.ts`, `spawnsync-no-microtask-drain.test.ts`, `ffi.test.js`, `bunshell.test.ts`, `shelloutput.test.ts`, `throw.test.ts` and `test/internal/source-lints`. The repros are clean under `BUN_JSC_validateExceptionChecks=1`. `cargo check` for `x86_64-pc-windows-msvc` and clippy are clean. - Related open PRs: #39558 (see above), #33353 (a check in `bun:ffi` in front of the entry point), #37243 (a check in `Buffer.from(string)` in front of it). They apply on top of this change. ### Background A Buffer is a Uint8Array over a JSC `ArrayBuffer`. JSC stores the byte length as `size_t` but caps it at `MAX_ARRAY_BUFFER_SIZE` (2^32 on 64-bit, `PageCount.h`). Bun exposes the cap as `require("buffer").kMaxLength`. JSC's allocating constructors return null above the cap, and the callers turn that into `RangeError: Out of memory`. The adopting constructor, `createFromBytes`, takes bytes that already exist and asserts instead, so the check has to happen before the hand-off. Adopting means JSC takes ownership of a byte range that native code allocated, and frees it through a deallocator passed along with the bytes when the object is collected. spawnSync and the shell use this to return their captured output without a copy. `bun:ffi` `toBuffer` uses it with a deallocator that frees nothing, which is why it can describe any length without owning that much memory. In Rust, `JsResult` is `Result<JSValue, JsError>`, and `Err(Thrown)` means an exception is pending on the VM. `from_js_host_call` turns a C++ call that returns an empty value with an exception pending into that `Err`. `take_exception` removes the pending exception and returns JSC's `Exception` cell, the wrapper that carries the thrown value and its stack. `take_error` returns the thrown value itself, which is what a JS callback has to receive. The promise helpers unwrap the cell themselves, which is why the other `take_exception` callers are fine. <details> <summary>Earlier version of this PR</summary> The first push threw `RangeError` with code `ERR_BUFFER_TOO_LARGE` from the two entry points, with its own helper. #39558 added its check to the same two functions in the meantime, with `RangeError: Out of memory` for all four entry points. This PR now carries that PR's hunks unchanged instead, and the tests expect that error. </details>
…rayBuffer limit (#39558) ### Problem - `await Bun.file(path).arrayBuffer()` on a file of exactly 2^32 bytes reads the whole file and then crashes with `panic: int cast: TryFromIntError(PosOverflow)`. The same happens through `new Response(Bun.file(path)).arrayBuffer()`. Bun 1.3.14 returns the 4294967296-byte ArrayBuffer, so this is a regression of the Rust port. - Cause: `ArrayBuffer::from_bytes` and `from_owned_bytes` (`src/jsc/array_buffer.rs:392`) convert the length through `u32` with `expect` before they store it in a `usize` field. The read path reaches them from `Blob.rs:3074` with the bytes it read. - Behind that panic sits a second failure. JSC's `ArrayBuffer::createFromBytes` RELEASE_ASSERTs when it is given more than `MAX_ARRAY_BUFFER_SIZE` (2^32) bytes. `Bun.mmap()` of a file larger than 4 GiB aborts there today, and a file of 2^32 + 1 bytes would abort there once the panic is gone. ### Fix - `from_bytes` and `from_owned_bytes` store the length as is. A file of exactly 2^32 bytes is returned whole again. - `Bun::rejectBytesNoCopyAboveArrayBufferLimit` (`JSBuffer.cpp`) rejects a length above `MAX_ARRAY_BUFFER_SIZE` with the `RangeError: Out of memory` that `new ArrayBuffer(2 ** 32 + 1)` throws. The four functions that adopt bytes from Rust call it before `createFromBytes`: `Bun__makeArrayBufferWithBytesNoCopy` and `Bun__makeTypedArrayWithBytesNoCopy` (every `ArrayBuffer::to_js*` call, and `Bun.mmap`), `JSBuffer__bufferFromPointerAndLengthAndDeinit` (`create_buffer*`, `to_node_buffer`) and `JSBuffer__fromMmap` (`to_js_buffer_from_memfd`). - The caller has already handed the bytes over, so the helper runs the deallocator before it throws. That frees the read buffer, unmaps the mapping, or drops the Blob store reference, exactly as a collection would. The typed array function already ran the deallocator when creation failed after `createFromBytes`, and the Buffer function already runs it for an empty length, so no caller releases the bytes twice. The doc comments on the Rust wrappers state this. - `ArrayBuffer__fromSharedMemfd` returns an empty value for such a length. Its only caller (`Blob.rs`, the `Clone` arm) then falls back to the copying allocation, which already throws the same RangeError. - Verified with `test/js/web/fetch/blob-oom.test.ts` ("at the 4 GiB ArrayBuffer limit"): a sparse file of 2^32 bytes comes back whole, and a file of 2^32 + 1 bytes rejects with the RangeError. On main both cases die with the panic above after reading 4 GiB. The block skips below 10 GiB of RAM and on Windows, where the file reader itself rejects files of 2^32 bytes or more with ENOMEM (`read_file.rs`, `ReadFileUV`). Each case has a 120 s timeout, like the 2 GiB cases in the same file (about 6 s each in a debug build here). The two existing 2 GiB cases get the 90 s timeout they already needed on loaded debug builds. - Verified with `test/js/bun/util/mmap.test.js`: a sparse file of 2^32 + 4096 bytes throws a RangeError from `Bun.mmap(file)` and from `{ size: 2 ** 32 + 1 }`, while `{ size: 2 ** 32 }` still maps. On main the first call aborts with SIGABRT. This case costs address space only. - The two Buffer entry points have no test of their own. The only way to reach them with such a length is more than 4 GiB of piped subprocess output (`to_js_buffer_from_memfd` is not reachable from JS today: a buffer or blob is not accepted as stdout). They call the same helper the two tests above exercise. - `bun bd test` on `blob.test.ts`, `blob-cow.test.ts`, `body.test.ts`, `zstd.test.ts`, `ffi.test.js`, `spawn.test.ts` and `buffer.test.js`: all pass. - Related open PRs. #33353 removes the same `u32` conversion for `bun:ffi` and adds a check in the FFI layer. #34119 adds a `Bun.mmap` message with the byte count in `BunObject.rs`. #37243 covers in-memory string and `InternalBlob` bodies, which take the separate `fromDefaultAllocator` path. All three still apply on top of this change. The mmap test here only checks the error name, so it holds with or without #34119. ### Background A Blob that is backed by a file is read on a thread into a Rust buffer. To return it as an `ArrayBuffer` without a copy, Bun builds an `ArrayBuffer` descriptor (`ptr`, `len`) and hands it to JSC through `Bun__makeArrayBufferWithBytesNoCopy`. JSC then owns the bytes and calls the deallocator that was passed along when the object is collected. JSC stores the size of an ArrayBuffer as a `size_t`, but caps it at `MAX_ARRAY_BUFFER_SIZE` (`PageCount.h`, 2^32 on 64-bit). Its allocating constructors return null above the cap, and Bun turns that into `RangeError: Out of memory`. The adopting constructor used for zero-copy hand-offs asserts instead, so the check has to happen in Bun before the hand-off. The `u32` conversion is the port of a `@as(u32, @intcast(len))` in the Zig version, which had the same `usize` fields. In a release build that cast was unchecked, and in practice the value passed through, which is why 1.3.14 returned the buffer. The port made it a checked conversion, so the same input now panics. <!-- 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/bun/util/mmap.test.js test/js/web/fetch/blob-oom.test.ts <!-- robobun:evidence:end -->
ee14b97 to
0a2dfee
Compare
robobun
changed the title
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. The fix is well-scoped and the logic checks out, but since it changes ownership-transfer and error-path behavior in the JSC ArrayBuffer adopt-bytes bindings, a human sign-off would be worthwhile.
What was reviewed:
rejectBytesNoCopyAboveArrayBufferLimitfrees the adopted bytes before throwing, so the new guards inJS{Uint8Array,ArrayBuffer}__fromDefaultAllocatordon't leak; the pattern matches the existing call sites in ZigGlobalObject.cpp / JSBuffer.cpp.from_default_allocator's only two callers (both in Blob.rs) are updated for the newJsResultreturn; thetoo_largeearly-throw drops theOwnedStringand releases the WTF ref.- The
!is_8bit() && len > MAX/3gate is sound: Latin-1→UTF-8 caps at 2×(2^31-1) < 2^32, and 3 bytes/code-unit is the worst case for UTF-16→UTF-8. JSUint8Array::from_bytes's other callers (TextEncoderStreamEncoder) already propagateJSValue::ZERO-with-exception per the existing__encodeForStreamcontract.
The four open comment-cop notes flag 2–3 line doc comments; the author already addressed the same complaint on earlier commits and the comments read as invariant documentation, not workaround justification.
Extended reasoning...
Overview
The PR converts a process abort (RELEASE_ASSERT in JSC::ArrayBufferContents) into a catchable RangeError: Out of memory when a string body's UTF-8 encoding exceeds MAX_ARRAY_BUFFER_SIZE (2^32). It touches 4 native source files: Uint8Array.cpp (adds throw scopes + rejectBytesNoCopyAboveArrayBufferLimit guards to both fromDefaultAllocator FFI entrypoints), array_buffer.rs (from_default_allocator now returns JsResult<JSValue> via from_js_host_call; adds MAX_ARRAY_BUFFER_SIZE constant), JSBuffer.cpp (constructFromEncoding pre-computes UTF-8 length for oversized 16-bit strings under the utf8 encoding), and Blob.rs (the WTFStringImpl arm of to_array_buffer_view pre-checks the encoded length; both from_default_allocator call sites propagate the new JsResult). Two subprocess-based tests are added, gated on ≥10 GiB RAM, following the exact pattern of neighboring 2 GiB / 4 GiB cases in the same files.
Security risks
None identified. The change tightens a bound (rejects earlier) rather than loosening one. Inputs at or below the cap are unchanged, and the new MAX_ARRAY_BUFFER_SIZE constant matches JSC's own definition (referenced from PageCount.h). No user-controlled data reaches new parsing or pointer arithmetic.
Level of scrutiny
High — this is exactly the "memory safety (the most-blocked category)" area of REVIEW.md: ownership transfer of an allocation across FFI, with a new error path where the callee must free the caller's bytes. The change also introduces DECLARE_THROW_SCOPE into two functions that previously had none, which under validateExceptionChecks needs the check/return sequencing to be exact. I traced the ownership on every path (below-cap success, above-cap throw, and the pre-encode throw in Blob.rs where the OwnedString drop releases the WTF ref) and the from_js_host_call wrapping that turns the C++ throw into an Err. It all looks correct, and the PR description says BUN_JSC_validateExceptionChecks=1 was verified with the pre-check disabled so the central guard was actually exercised. But this is precisely the kind of change where a second pair of eyes on the ownership hand-off is worth having.
Other factors
- All Rust-side callers of the changed signature are inside the diff;
JSUint8Array::from_bytes's remaining direct callers inTextEncoderStreamEncoder.rsalready returnJSValue::ZEROwith a pending exception, so the new failure mode fits the existing contract. - The tests follow harness conventions (subprocess isolation,
Promise.allon stdout/stderr/exited, memory-gatedskipIf, exit-code asserted last, 90 s ceiling explained as a bound not a target). CI on the diff was green except for two lanes the author documented as also failing on main. - Four unresolved comment-cop bot comments landed today; they flag short doc comments as "paragraph-long workaround justification". The author already resolved identical complaints on earlier commits with the same rationale — the comments document why only the 16-bit/utf8 arm can exceed the cap and that ownership transfers even on error. I don't think they need further trimming, but a human can make that call.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This adds an extra bytelength call. Do not add more passes over the inputs.
|
Removed. The pre-encode length checks in constructFromEncoding and the WTFStringImpl body arm are gone, along with the Buffer test that only exercised them. What is left is the check at the two adopting functions in Uint8Array.cpp, where the length is already known, plus the JsResult plumbing for it. Buffer.from is covered by the existing guard from #39564. |
0a2dfee to
3d5b513
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The guard follows the exact rejectBytesNoCopyAboveArrayBufferLimit pattern already landed in #39558/#39564, and the JsResult plumbing is the standard from_js_host_call shape. Because it changes memory ownership on the C++/Rust FFI boundary (bytes freed on the throw path before createFromBytes), a human look is still worthwhile.
What was reviewed:
- Ownership on the reject path:
rejectBytesNoCopyAboveArrayBufferLimitfrees viafreeDefaultAllocatorBytes→defaultAllocatorFree, matching the allocator the Rust side handed off from; no double-free or leak. - Callers:
from_default_allocatorhas exactly the two Blob.rs call sites, both updated;JSUint8Array::from_bytes's zero-on-throw return flows throughTextEncoderStreamEncoder__encodeForStream, which already documents that contract. - Exception scopes:
JSArrayBuffer__fromDefaultAllocatoromits a trailingRETURN_IF_EXCEPTIONbecauseJSArrayBuffer::create(vm, ...)takes a VM, not a global, so it cannot throw — consistent with thevalidateExceptionCheckspass noted in the description.
Extended reasoning...
Overview
The PR guards two C++ byte-adopting functions (JSUint8Array__fromDefaultAllocator, JSArrayBuffer__fromDefaultAllocator) with the existing Bun::rejectBytesNoCopyAboveArrayBufferLimit helper so that string bodies whose UTF-8 encoding exceeds MAX_ARRAY_BUFFER_SIZE (2^32) throw RangeError: Out of memory instead of hitting a RELEASE_ASSERT in JSC's ArrayBuffer::createFromBytes. On the Rust side, ArrayBuffer::from_default_allocator becomes JsResult<JSValue> via from_js_host_call, and its two callers in Blob.rs propagate the result. JSUint8Array::from_bytes keeps its raw JSValue return with the zero-on-throw contract now documented. Two subprocess tests are added mirroring the adjacent 4 GiB file tests.
Security risks
None. The change turns a user-reachable process abort into a catchable RangeError, which is a strict improvement (user input reaching a RELEASE_ASSERT is a DoS). No new input parsing, no auth/crypto surface.
Level of scrutiny
High — this is native FFI code that changes who frees a buffer on an error path, which is REVIEW.md's most-blocked category. That said, the change is mechanically identical to four existing call sites of the same helper in ZigGlobalObject.cpp and JSBuffer.cpp, and the PR description states #39558 explicitly deferred these two functions to this PR. I traced the ownership: on reject, the helper calls freeDefaultAllocatorBytes(ptr, nullptr) → Bun::defaultAllocatorFree(ptr), which matches the mimalloc allocation the Rust side gave up via heap::into_raw. On success nothing changes. The exception-scope shape differs between the two functions because JSUint8Array::create takes a JSGlobalObject* (can throw, hence RETURN_IF_EXCEPTION) while JSArrayBuffer::create takes a VM& (cannot).
Other factors
- All
from_default_allocatorcallers (only the two inBlob.rs) were updated for the signature change. - The
TextEncoderStreamcallers ofJSUint8Array::from_bytes(encode_latin1/encode_utf16) return throughTextEncoderStreamEncoder__encodeForStream, whose doc comment already states theJSValue::zero-with-pending-exception contract, so the new throw path is handled there without changes. - The two new tests each encode a real ~4 GiB string in a subprocess (~30 s debug) and are gated on
os.totalmem() >= 10 GiB, exactly matching the existing 4 GiB file tests above them in the same file. - All comment-cop threads on the PR are resolved; the earlier CI failures were unrelated (napi.test.ts on darwin x64, and a runner death) per the author's note.
Deferring rather than approving solely because this is FFI memory-ownership code; the change itself looks correct and follows established patterns without ambiguity.
|
CI on 3d5b513 (build 100934): every lane that ran this diff is green, including the ASAN lanes that run the two new blob-oom cases. The one red job is test/cli/install/bun-install.test.ts on ubuntu 25.04 aarch64, where api.github.com answered 504 to the GitHub-dependency tests. The same tests hit the same 504 on debian x64 and passed on retry. Not related to this change, reported to triage. Ready for review. |
3d5b513 to
9575340
Compare
There was a problem hiding this comment.
LGTM — applies the existing rejectBytesNoCopyAboveArrayBufferLimit guard to the two remaining byte-adopting functions, matching the pattern already on main for the sibling adopters.
What was reviewed:
- Ownership on the reject path:
freeDefaultAllocatorBytes→defaultAllocatorFreematches the allocator of the leaked&mut [u8]from Blob.rs; bytes freed exactly once. - Exception plumbing:
call_zero_is_throwon the ArrayBuffer arm mirrors the Uint8Array arm (viaJSUint8Array::from_bytes);RETURN_IF_EXCEPTIONafter the globalObject-basedJSUint8Array::create, none needed after the vm-basedJSArrayBuffer::create. - Tests follow the sibling 4 GiB subprocess pattern in the same file (concurrent pipe drain, memory gate, per-function case).
Extended reasoning...
Overview
Three files: Uint8Array.cpp adds rejectBytesNoCopyAboveArrayBufferLimit before ArrayBuffer::createFromBytes in JSUint8Array__fromDefaultAllocator and JSArrayBuffer__fromDefaultAllocator, plus the throw-scope machinery to carry the throw. array_buffer.rs wraps the ArrayBuffer arm of from_default_allocator in call_zero_is_throw so the C++ throw surfaces as Err (the Uint8Array arm already goes through JSUint8Array::from_bytes, which returns JsResult since #40410). blob-oom.test.ts gains a two-case describe block exercising both functions via Response(string).arrayBuffer() / .bytes().
Security risks
None introduced. The change converts a user-reachable RELEASE_ASSERT abort (a DoS vector) into a catchable RangeError, which is strictly a hardening. No new input parsing, no trust boundary crossed.
Level of scrutiny
Moderate — this is native memory-ownership code, but the change is a direct application of an existing shared helper already used identically at four other adopting sites (Bun__makeArrayBufferWithBytesNoCopy, Bun__makeTypedArrayWithBytesNoCopy, JSBuffer__fromMmap, and the JSBuffer no-copy constructor). I confirmed rejectBytesNoCopyAboveArrayBufferLimit (JSBuffer.cpp:419) frees via the passed deallocator before throwing, and freeDefaultAllocatorBytes routes to defaultAllocatorFree — the same allocator that owned the bytes leaked from .into_vec().leak() / .to_owned_slice().leak() in Blob.rs. One owner, one free, correct allocator. On the Rust side, both call sites in Blob.rs already return from_default_allocator's result directly, so the new Err propagates without further changes.
Other factors
The exception-scope shape checks out: JSUint8Array::create(globalObject, ...) can throw, so it gets a trailing RETURN_IF_EXCEPTION; JSArrayBuffer::create(vm, ...) is the vm-based non-throwing wrap, so it does not — and the PR states it verified under BUN_JSC_validateExceptionChecks=1, on top of #40410's exception-lint. The tests reuse the sibling subprocess/memory-gate/timeout pattern already in the file, drain stdout/stderr/exited concurrently, and assert exact error name+message. All prior inline threads (comment-cop bot noise about comment length) are resolved. No CODEOWNERS on these paths and no outstanding human review comments.
robobun
changed the title
9575340 to
bff8d29
Compare
|
Rebased onto main (the conflict was in blob-oom.test.ts only, #40962 rewrote its helpers) and widened what this PR pins down. The source change is the same as before. The same two functions are also the hand-off for fetch response bodies. A 4 MiB gzip response that inflates past 2^32 bytes aborts The two tests now use that fetch case, one per guarded function. It costs 16 s per case under ASAN against 28 s for the string body case, and it is the door a remote server can reach. The string body case still passes by hand. Title and description are updated to match. |
… above 4 GiB JSUint8Array__fromDefaultAllocator and JSArrayBuffer__fromDefaultAllocator passed the bytes to ArrayBuffer::createFromBytes, which RELEASE_ASSERTs above JSC's 2^32 byte cap. A fetch response that inflates past 4 GiB, a string body with more than 2^32 bytes of utf8, or any other natively buffered body read with arrayBuffer() or bytes() aborted the process. Both functions now call rejectBytesNoCopyAboveArrayBufferLimit, the helper the other byte-adopting functions already use. It frees the bytes and throws the RangeError that new ArrayBuffer() throws for that size. from_default_allocator reports the throw as an Err.
bff8d29 to
abb61e9
Compare
There was a problem hiding this comment.
I re-reviewed the latest push and found no bugs; the string-body case is now covered in the suite, which closes the coverage gap I raised earlier. A maintainer's changes-requested review is still outstanding, so a human sign-off is the remaining step.
What was reviewed:
- Ownership on the over-limit path: both Rust callers in Blob.rs leak the Vec before the FFI call and never touch it again, and the C++ helper frees through defaultAllocatorFree (libc free under ASAN, mi_free otherwise), so no double free or allocator mismatch.
- Exception plumbing: the ArrayBuffer arm now goes through call_zero_is_throw like the Uint8Array arm already did; the ThrowScope and RETURN_IF_EXCEPTION after JSUint8Array::create are in place, and the empty-length path never dereferences the pointer.
- Tests: children drain both pipes concurrently, assert exitCode 0 and signalCode null, and the new block is memory-gated and serial like the neighbouring 4 GiB cases.
Extended reasoning...
The change adds a length guard to the two zero-copy adopting entry points in src/jsc/bindings/Uint8Array.cpp, wraps the ArrayBuffer arm of from_default_allocator in src/jsc/array_buffer.rs with the throw-aware wrapper, and adds three child-process tests in test/js/web/fetch/blob-oom.test.ts. It touches no auth, crypto or injection surface; the security-relevant part is untrusted-length handling, which now rejects instead of hitting a JSC assertion. The code is small and reuses the existing helper the sibling adopters already use, and the latest push added the string-body test that a prior run asked for. Approval is withheld because a CHANGES_REQUESTED review from Jarred-Sumner has no later re-review, and the at-limit fetch boundary remains untested by the author's stated choice.
2 participants
Problem
await (await fetch(url)).arrayBuffer()or.bytes()aborts the process when the body exceeds 2^32 bytes. A 4 MiB gzip response is enough. Debug builds printASSERTION FAILED: m_sizeInBytes <= (1ull << 32)inJSC::ArrayBufferContents::ArrayBufferContents.text()rejects.Responsestring body with more than 2^32 bytes of utf8 aborts the same way.JSArrayBuffer__fromDefaultAllocatorandJSUint8Array__fromDefaultAllocator(src/jsc/bindings/Uint8Array.cpp) callArrayBuffer::createFromBytes, which asserts aboveMAX_ARRAY_BUFFER_SIZE.Fix
Bun::rejectBytesNoCopyAboveArrayBufferLimit. It frees the bytes and throws theRangeError: Out of memorythatnew ArrayBuffer(2 ** 32 + 1)throws. Bun.file().arrayBuffer(): stop panicking at 4 GiB, throw above the ArrayBuffer limit #39558 and Throw instead of aborting when child output does not fit in a Buffer #39564 added it to the other four adopting functions.ArrayBuffer::from_default_allocatorwraps itsArrayBufferarm incall_zero_is_throw, so callers get anErr.test/js/web/fetch/blob-oom.test.ts(three new cases). Without the fix, every child dies with SIGABRT. The whole file passes.Background
ArrayBufferat 2^32 bytes. Constructors that allocate throw above it.createFromBytesadopts bytes the caller owns, and asserts.Any::to_array_buffer_view(Blob.rs) andfrom_default_allocator, without a copy.Downsides
bloaty, method in Notes).Notes
Who reaches the two functions. Every caller of
ArrayBuffer::from_default_allocatorandJSUint8Array::from_bytes:Any::to_array_buffer_view,InternalBlobarm: a fetch response (read while it streams, or after it is buffered), aBun.serverequest body whenmaxRequestBodySizeallows more than 4 GiB, a native stream read throughnew Response(stream)orBun.readableStreamToArrayBuffer/ToBytes, HTMLRewriter output.WTFStringImplarm: a string body whose utf8 encoding is owned (not ASCII).TextEncoderStreamchunks.For fetch the path is
FetchTasklet::on_body_received->Body::Value::resolve->AnyBlob::to_array_buffer_transfer/to_uint8_array_transfer->Any::to_array_buffer_view->from_default_allocator.Repro for the fetch case (about 7 GiB of free memory). On bun 1.4.3 and on main it exits 134. With this PR it prints the RangeError.
text()rejects withERR_STRING_TOO_LONGandblob()resolves with 4362076160 bytes, before and after.Repro for the string case.
await new Response("\u20ac".repeat(1431655766)).arrayBuffer()(3 utf8 bytes per character, 2^32 + 2 bytes). It aborts on main and on 1.4.3, and rejects with the RangeError here, forarrayBuffer()andbytes(). It is a test, for release builds only: one call takes 28 s in a debug ASAN build (65 s for the case) and about 2 s in a release build (7 s for the case).The tests. One fetch case per guarded function, at exactly 2^32 + 1 bytes. The body is 64 gzip members of 64 MiB of zeros and one member of a single byte, so the child builds it in 0.3 s (
Bun.gzipSynconce per member size) where a streamed level 9 gzip of 4 GiB takes 70 s in a debug build. If fetch ever stops at the first gzip member, the call resolves and the test fails onunexpectedByteLength. Measured here under ASAN: 15 s to 18 s per case, 6.6 GiB to 8.8 GiB of peak RSS, so the cases stay out of the concurrent batch and use the same 10 GiB gate as the 4 GiB file cases. In a release build a case takes about 11 s and 4.5 GiB to 6.1 GiB.The boundary. The comparison is in
rejectBytesNoCopyAboveArrayBufferLimit, and the two functions only call it. The file cases already pin both sides of that comparison (exactly 2^32 bytes is returned whole, 2^32 + 1 rejects). Through fetch, by hand with this build: a body of exactly 2^32 bytes resolves fromarrayBuffer()and frombytes()withbyteLength4294967296, and 2^32 + 1 rejects from both. The at-limit side is not a test here. Each case is one more 4 GiB transfer (11 s in release, 16 s under ASAN, up to 9 GiB of RSS) on a shared agent.Size number.
ninja -C build/release obj/unified/UnifiedSource-src_jsc_bindings-4.cpp.owith this PR's and main'sUint8Array.cpp, each lowered withclang++ -O3 -c -x ir: text 162942 against 162798. The two functions grow from 387 to 480 bytes and from 442 to 488 bytes.The other callers of
createFromBytes/createAdopted.Bun__makeArrayBufferWithBytesNoCopy,Bun__makeTypedArrayWithBytesNoCopy,JSBuffer__bufferFromPointerAndLengthAndDeinit,JSBuffer__fromMmap.ArrayBuffer__fromSharedMemfdhas its own length check.BunObject.cpp(heap snapshot asarraybuffer) andSerializedScriptValue::toArrayBufferpass aWTF::Vector<uint8_t>, whose capacity is 31 bits.DirectByteBuffer::tryGrowTorefuses to grow aboveMAX_ARRAY_BUFFER_SIZE.JSX509Certificate.cpppasses the DER bytes of one certificate.napi.cpp(external arraybuffer and buffer, the length comes from the addon) andNodeSqlite.cpp(adoptSqliteBuffer, aserialize()above 4 GiB). The helper frees the bytes before it throws, which is wrong for napi, where the caller keeps ownership on failure. Those two need their own error contract and are tracked separately.Earlier shapes of this PR. The first version checked the size at three Rust and C++ call sites, before the shared helper existed. A later one also computed the utf8 length of a string ahead of encoding, which was removed at review because it is an extra pass over the input. Rebases over #39558, #39564, #40410 and #40962 moved the Rust plumbing and the test helpers to main, so the PR is now the guard in
Uint8Array.cpp, one wrapped arm inarray_buffer.rs, and the tests. The last rebase conflict was inblob-oom.test.tsonly (#40962 rewrote its helpers). The new cases use those helpers.no test proof · iteration 5 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/blob-oom.test.ts