Conversation
…rting
JSC ArrayBuffers are hard-capped at MAX_ARRAY_BUFFER_SIZE (1<<32 bytes,
JavaScriptCore/runtime/PageCount.h), enforced by RELEASE_ASSERT in the
ArrayBufferContents constructor. mmap_file passed the fstat()'d size
straight into make_typed_array_with_bytes_no_copy with no bound check,
so mapping any file one byte past 4 GiB aborted the whole process with
no catchable error.
Check map.len() against the limit after the mmap succeeds; on overflow,
munmap and throw a RangeError that points at the {size} option.
|
Updated 1:27 AM PT - Jul 14th, 2026
❌ @robobun, your commit b3282fc has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34119That installs a local version of the PR into your bun-34119 --bun |
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesmmap safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/runtime/api/BunObject.rs`:
- Around line 1844-1852: Update the oversized-mapping cleanup in the error path
of the surrounding mmap function to handle the result of sys::munmap instead of
discarding it. Preserve the RangeError return, but log any unmap failure with
scoped_log! and include the failure details for diagnostics.
In `@test/js/bun/util/mmap.test.js`:
- Around line 89-125: Remove the generic stderr content assertion from the mmap
subprocess test after the existing stdout, signalCode, and exitCode
expectations. Keep awaiting proc.stderr.text() so stderr remains drained, and
rely on the process outcome fields for abort detection.
🪄 Autofix (Beta)
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: f1caaf99-c3da-498d-843a-cb4f6c6e0420
📒 Files selected for processing (2)
src/runtime/api/BunObject.rstest/js/bun/util/mmap.test.js
signalCode: null and exitCode: 0 in the toEqual already detect any abort; the stderr check is redundant and can false-positive on benign debug output.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the stderr-marker nit from my earlier pass is addressed in b3282fc, and the remaining change is a straightforward bounds check that turns a JSC RELEASE_ASSERT abort into a catchable RangeError.
What was reviewed:
- Boundary is
> (1 << 32), matching JSC'sm_sizeInBytes <= (1ull << 32)assert — exactly 4 GiB still succeeds, and the test'sat-limitcase covers it. - Error path munmaps before throwing;
let _ =matches the existingmunmap_deallocconvention directly below. - Test spawns a subprocess so a SIGABRT regression fails the assertion (
signalCode: null/exitCode: 0) rather than the runner; stderr is drained concurrently but not asserted on.
Extended reasoning...
Overview
Two files: a 13-line addition to mmap_file in src/runtime/api/BunObject.rs that checks map.len() against JSC's MAX_ARRAY_BUFFER_SIZE (1 << 32) after the mmap succeeds, munmaps and throws a RangeError on overflow; and a new subprocess test in test/js/bun/util/mmap.test.js covering >4 GiB (throws), exactly 4 GiB (succeeds), and a capped { size: 4096 } (succeeds).
Security risks
None. This adds a defensive size check to a local-file mmap path that previously aborted the process. It strictly narrows the reachable behavior (abort → catchable error) and does not expose any new surface, change permissions, or handle untrusted network input.
Level of scrutiny
Low-to-moderate. The Rust change is a single guarded early-return with cleanup, following the exact pattern already used a few lines below (munmap_dealloc also discards the munmap result with let _ =). I verified: the constant matches JSC's MAX_ARRAY_BUFFER_SIZE (also referenced in src/jsc/bindings/JSBuffer.{h,cpp}); the comparison operator (>) matches the JSC assert's inclusive bound (<=); sys::munmap's signature (*mut u8, usize) matches the call; and sys / create_range_error_instance are already in scope. On 64-bit targets (all Bun targets) 1usize << 32 is well-defined.
Other factors
All prior review threads are resolved: my earlier nit about the .not.toContain("ASSERTION FAILED") assertion was addressed in b3282fc (stderr is now drained via Promise.all but not asserted on), and the CodeRabbit munmap-logging suggestion was withdrawn after the author pointed out it matches the existing munmap_dealloc convention and cannot realistically fail on a just-returned mapping. The test uses a sparse file (no disk cost), spawns a child so a regressed SIGABRT surfaces as signalCode: "SIGABRT" in the toEqual rather than killing the runner, and matches the file's existing tmpdirSync/bunExe/bunEnv conventions. The bug hunting system found no issues.
|
CI status on b3282fc (build #72722, finished: 284 passed, 2 failed):
Ready for review. |
…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 -->
|
The abort is fixed on main by #39558 (92fa5e4). That change rejects every zero-copy hand-off above the 4 GiB ArrayBuffer limit with a RangeError. Verified on a debug build of main (32e8703) with the reproduction from this PR (a sparse file of 2^32 + 1 bytes):
The only remaining difference in this PR is the more specific error message. Closing as superseded. |
1 participant
Reproduction
Release build: silent
SIGABRT, exit 134, empty stderr. Debug build:The boundary is exact:
2 ** 32maps fine,2 ** 32 + 1kills the process. mmap is the API you reach for on huge files, so the first >4 GiB log, VM image, or dataset takes the whole process down with no catchable error.Cause
mmap_fileinsrc/runtime/api/BunObject.rspasses thefstat()'d size unchecked intomake_typed_array_with_bytes_no_copy. JSC ArrayBuffers are hard-capped atMAX_ARRAY_BUFFER_SIZE(1ull << 32,JavaScriptCore/runtime/PageCount.h), enforced byRELEASE_ASSERTin theArrayBufferContentsconstructor.Fix
After
bun_sys::mmap_filereturns, checkmap.len()against the limit. On overflow,munmapthe region and throw aRangeErrorthat points the user at the existing{ size }option:The check is placed after the mmap (rather than before) because
bun_sys::mmap_filedoes thefstatinternally; checking the returnedmap.len()also covers an explicit{ size }that exceeds the cap after being clamped to the file length.Related: #33353 applies the same bound to
bun:ffi'stoArrayBuffer/toBuffer, a different entry point to the same JSC assertion.Verification
New test in
test/js/bun/util/mmap.test.jscreates a sparse2 ** 32 + 1byte file and spawns a child that:Bun.mmap(f)with no options: must throwRangeErrormentioning4294967297Bun.mmap(f, { size: 2 ** 32 }): exactly at the limit, must succeed with.length === 4294967296Bun.mmap(f, { size: 4096 }): capped, must succeedFails on the released binary with
signalCode: "SIGABRT"/exitCode: 134:Passes on the debug build (all 9 mmap tests green).
no test proof · iteration 0 · 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