ffi: toArrayBuffer/toBuffer throw RangeError instead of aborting on a huge byteLength - #33353
ffi: toArrayBuffer/toBuffer throw RangeError instead of aborting on a huge byteLength#33353robobun wants to merge 2 commits into
Conversation
WalkthroughThis PR reworks ChangesFFI pointer decoding and ArrayBuffer size limit
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:42 PM PT - Jul 9th, 2026
❌ @robobun, your commit 75bc1fc has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33353That installs a local version of the PR into your bun-33353 --bun |
f8da7ab to
127dc7b
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — both earlier comments are addressed (the byteOffset polarity fix + test, and the at-the-limit test no longer trips ASan). I'm not auto-approving only because this changes user-visible error semantics (returned Error → thrown) and adjusts the shared ArrayBuffer::MAX_SIZE constant, which is worth a maintainer glance.
Extended reasoning...
Overview
Three files: src/jsc/array_buffer.rs (drops the u32::try_from narrowing in from_bytes/from_owned_bytes; corrects MAX_SIZE from u32::MAX to 1 << 32), src/runtime/ffi/FFIObject.rs (reworks get_ptr_slice to return JsResult<(ptr, len)> and throw on validation failure instead of returning an Error object; adds a max_byte_length bound that surfaces ERR_OUT_OF_RANGE; fixes the inverted byteOffset arms), and three new tests in test/js/bun/ffi/ffi.test.js.
Both of my earlier inline comments have been addressed in commit 5169768: the byteOffset else-if polarity is corrected (with a dedicated test covering undefined/null/garbage and CString#arrayBuffer), and the subprocess test now holds backing in a named local, drops the toBuffer half of the at-the-limit check so no foreign-memory deallocator is installed, and drains stderr without asserting it empty.
Security risks
None new. bun:ffi is inherently a trust-the-caller API operating on raw addresses; this PR tightens validation (bounds byteLength before it can trip a JSC RELEASE_ASSERT) rather than loosening anything. No auth/crypto/permissions surface.
Level of scrutiny
Medium-high. The mechanical parts (dropping dead u32::try_from(..) as usize casts, threading a max through get_ptr_slice) are straightforward, but there are two cross-cutting decisions a maintainer should ratify:
- Return → throw: every validation failure in
get_ptr_slice/to_array_buffer/to_buffer/new_cstringnow throws instead of returning anErrorobject. The description calls out thereturns: "cstring"sentinel-address consequence; that's almost certainly a bug fix, but it is a user-visible semantics change on a public API. ArrayBuffer::MAX_SIZEbump (u32::MAX→1 << 32): the description audits the other consumer (node:crypto'sMAX_POSSIBLE_LENGTH = min(MAX_SIZE, i32::MAX), unchanged either way), but it's a shared constant so worth a second pair of eyes.
Other factors
Tests cover both boundaries (2**32 succeeds, 2**32+1 throws), the throw-vs-return distinction, and the byteOffset polarity fix. The at-the-limit toArrayBuffer(address, 0, 2**32) still forms a Rust slice larger than its backing allocation — technically UB per from_raw_parts_mut's contract — but that's inherent to exercising this API without a real 4 GiB mapping, only .byteLength is read, and backing.length at the end keeps the source alive; I don't think there's a better option in-suite. The PR also flags a likely merge conflict with #32260 on the same byteOffset lines. CI on the latest commit (build #68472) is still building at the time of this review.
|
Since the review flagged two things as wanting a maintainer glance, here is the evidence for both so they are cheap to check.
The constant was already documented as Returned
The only callers of these three entry points inside the repo are in
|
![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/bun/ffi/ffi.test.js`:
- Around line 723-767: This FFI subprocess test is self-contained and can run
alongside the other cases. Update the existing `it("toArrayBuffer and toBuffer
reject a byteLength past the max ArrayBuffer size", ...)` test to use
`it.concurrent(...)` so it overlaps with other FFI tests without changing the
assertions or subprocess logic.
🪄 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: e4ed0eab-bbf4-475c-835e-4762598ff4b7
📒 Files selected for processing (3)
src/jsc/array_buffer.rssrc/runtime/ffi/FFIObject.rstest/js/bun/ffi/ffi.test.js
CI status: diff is green, red lanes are unrelatedRebased at Neither failure touches The recurring red:
Build 70679 finished 284 / 2, the second being Build 68514 / 68472 (original base) both finished 285 / 1, the one red being Locally the three new tests pass under I have spent my one retrigger and will not push more empty commits. This is ready for review. |
07d88c1 to
17aa758
Compare
There was a problem hiding this comment.
No issues found on the current revision — my earlier feedback is addressed. Deferring to a human since this restructures the FFI pointer-decoding path, changes user-visible error semantics (returned Error → thrown), and adjusts the shared ArrayBuffer::MAX_SIZE constant.
Extended reasoning...
Overview
This PR touches three files: src/jsc/array_buffer.rs (drops u32::try_from narrowing in from_bytes/from_owned_bytes, changes MAX_SIZE from u32::MAX to 1 << 32), src/runtime/ffi/FFIObject.rs (rewrites get_ptr_slice to return JsResult<(ptr, len)> with a max_byte_length bound, converts to_invalid_arguments returns into throw_invalid_arguments, fixes the inverted byteOffset arms), and test/js/bun/ffi/ffi.test.js (three new tests covering thrown-vs-returned errors, omitted-byteOffset, and the 2**32 boundary).
Security risks
None new. bun:ffi is an inherently trusting API — the code already dereferences user-supplied raw addresses. The change tightens validation by bounding byteLength before it reaches JSC's RELEASE_ASSERT, converting an abort into a catchable RangeError. No new attack surface is introduced.
Level of scrutiny
High. This is FFI/unsafe code operating on raw pointers, and it makes several non-mechanical decisions a maintainer should sign off on:
- Error semantics change:
toArrayBuffer/toBuffer/CStringargument errors now throw instead of being returned as the result value. The author audited callers (src/js/bun/ffi.ts,ffi.d.ts) and none inspect forinstanceof Error, but this is a user-visible behavior change on a public API. - Shared constant:
ArrayBuffer::MAX_SIZEmoves fromu32::MAX(4294967295) to1 << 32(4294967296). The author verified the one other consumer (node_crypto_binding.rs'smin(MAX_SIZE, i32::MAX)) is unaffected, but a human should confirm. - Widened
from_bytes: dropping the narrowing cast affects ~15 other callers (per the PR description). The author's analysis is that they previously panicked and now reach JSC's assert instead — abort either way — but this reaches beyond the FFI module.
Other factors
I left two inline comments on an earlier revision (inverted byteOffset arms; ASan bad-free in the at-the-limit toBuffer test) — both were addressed with fixes and detailed responses, and both threads are resolved. The current bug-hunting pass found nothing. Tests are thorough, follow harness conventions, and were verified to fail on USE_SYSTEM_BUN=1. CI is green except for a fleet-wide darwin artifact-download timeout unrelated to this diff. The PR also flags a likely conflict with #32260 on the same byteOffset lines.
… huge byteLength
toArrayBuffer(ptr, 0, byteLength) aborted the process for any byteLength at
or above 2^32: ArrayBuffer::from_bytes narrowed the length through
u32::try_from(..).expect("int cast"), a leftover from when the descriptor's
len/byte_len were u32. They are usize now, and the C ABI they mirror
(Bun__ArrayBuffer) has always used size_t, so the cast only served to panic.
toBuffer aborted the same way past 2^32, there by tripping JSC's
RELEASE_ASSERT(m_sizeInBytes <= MAX_ARRAY_BUFFER_SIZE).
Drop the narrowing casts and bound the byteLength in the FFI layer instead.
The limit is MAX_ARRAY_BUFFER_SIZE (2^32 inclusive, what new ArrayBuffer(2**32)
and require("buffer").kMaxLength already accept), so a caller with a mapping
larger than that now gets a RangeError it can handle by windowing. The bound
covers the NUL-scan path too, not just an explicit byteLength.
ArrayBuffer::MAX_SIZE is documented as kMaxLength but held u32::MAX; correct it
and use it. Its one other consumer, node:crypto's MAX_POSSIBLE_LENGTH =
min(MAX_SIZE, i32::MAX), is unchanged.
CString keeps the address bound: its result is capped by WTF::String::MaxLength
in code units, which a UTF-8 byte count does not map onto.
get_ptr_slice handed its validation failures back as Error objects in the
return slot rather than throwing them, which would have left the new RangeError
uncatchable. Make them throw, along with the finalizer-argument checks in
toArrayBuffer/toBuffer. A "cstring" symbol whose C function returns one of the
sentinel addresses now throws from the call rather than yielding a CString
whose text is the error message.
The two else-if arms were inverted, so `toArrayBuffer(ptr, undefined, len)` errored while `toArrayBuffer(ptr, "garbage", len)` silently ignored the bad offset. CString#arrayBuffer passes a byteOffset that defaults to undefined, so it cached a TypeError as its ArrayBuffer. `ptr()` already has the right polarity; match it. The byteLength test held a toBuffer view over a pointer bun does not own. toBuffer without a finalizer installs MarkedArrayBuffer_deallocator, which is libc free under cfg(bun_asan), and BUN_DESTRUCT_VM_ON_EXIT=1 on the ASan lane finalizes it at teardown: a bad-free on JSC memory. toArrayBuffer installs no deallocator, so it alone holds the at-the-limit view.
17aa758 to
75bc1fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ffi/FFIObject.rs`:
- Around line 648-656: Align the empty-value handling in the twin functions to
avoid inconsistent behavior. In to_array_buffer, change the ctx validation guard
to use is_empty_or_undefined_or_null(), matching to_buffer, so empty JSValue
inputs are treated as absent rather than rejected.
- Around line 536-539: Move the byteOffset finiteness validation before the
to_int64() conversion and saturating_add/sub pointer arithmetic in the relevant
FFI pointer construction logic. Reuse the existing
byte_off.as_number().is_finite() check and return the invalid-arguments error
immediately, then perform address calculation and retain the later pointer
validation.
- Around line 555-600: In the explicit length branch of the byteLength handling,
reject non-finite and fractional numeric values before calling to_int64(),
matching the validation used for byteOffset. Update the value_length checks in
the length_i computation to require a finite integer, then retain the existing
positive-value validation and cast.
🪄 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: c004c153-ad99-46dc-9e5a-691e681a4cef
📒 Files selected for processing (2)
src/jsc/array_buffer.rssrc/runtime/ffi/FFIObject.rs
There was a problem hiding this comment.
Both earlier findings are addressed and I found nothing new, but the returned→thrown error semantics change and the ArrayBuffer::MAX_SIZE bump are user-visible API decisions worth a maintainer's sign-off.
Extended reasoning...
Overview
Fixes a process abort in bun:ffi's toArrayBuffer/toBuffer when byteLength exceeds JSC's MAX_ARRAY_BUFFER_SIZE. Touches three files: src/jsc/array_buffer.rs (drops u32::try_from narrowing in from_bytes/from_owned_bytes, corrects MAX_SIZE from u32::MAX to 1 << 32), src/runtime/ffi/FFIObject.rs (rewrites get_ptr_slice to return JsResult<(*mut u8, usize)> with a max_byte_length cap, converts all validation failures from returned Error objects to thrown errors, fixes the inverted byteOffset arms), and test/js/bun/ffi/ffi.test.js (three new tests).
Security risks
None introduced. bun:ffi is an inherently trusting API where the user supplies raw pointers; this change tightens validation (adds a RangeError bound where there was previously an abort) rather than loosening it. No new unsafe blocks beyond what already existed; the from_raw_parts_mut calls are unchanged in shape.
Level of scrutiny
Moderate-to-high. This is not a mechanical fix — it bundles four distinct behavior changes: (1) the headline abort→RangeError fix, (2) converting get_ptr_slice validation failures from returned-in-result-slot to thrown (affects toArrayBuffer, toBuffer, and CString — the PR description notes a returns: "cstring" symbol hitting a sentinel address now throws instead of yielding a CString whose text is the error message), (3) swapping the inverted byteOffset arms so undefined/null are accepted and non-numbers rejected, and (4) bumping the shared ArrayBuffer::MAX_SIZE constant by 1 and widening it to usize. The author has audited each (the MAX_SIZE consumer audit and the returned→thrown behavior matrix are both in the thread), and the .d.ts types already declare these functions as returning ArrayBuffer/Buffer without | Error, so (2) aligns runtime with published types. But these are still API-shape decisions on a public module that a maintainer should confirm.
Other factors
Both of my earlier inline findings (inverted byteOffset arms turning a valid CString#arrayBuffer call into a throw; the ASan-lane toBuffer finalizer freeing foreign memory at VM teardown) were fixed and the threads resolved. The from_bytes cast removal widens slightly beyond FFI (~15 other callers per the PR description), which the author characterizes as abort→abort with no new correctness hole. CI on the latest commit is reported green on all ASan shards with two unrelated infra failures. Test coverage is thorough (both boundary sides, both entry points, the byteOffset fix, and the returned-vs-thrown distinction spelled out explicitly). The PR also flags an expected merge conflict with #32260 on the same byteOffset lines.
1 participant
Repro
An uncatchable
SIGABRT.toBufferaborts the same way past2 ** 32, silently, by trippingRELEASE_ASSERT(m_sizeInBytes <= MAX_ARRAY_BUFFER_SIZE)inside JSC. Both are reachable from an entirely in-contract FFI call:mmapa region bigger than 4 GiB throughdlopen'd libc and ask for a view of it.Cause
ArrayBuffer::from_bytesnarrowed the length throughu32::try_from(bytes.len()).expect("int cast"), a leftover from when the descriptor'slen/byte_lenwereu32. They areusizenow, and the C ABI they mirror (Bun__ArrayBuffer,size_t len; size_t byte_len;) has always been 64-bit, so the cast did nothing except panic. Past that,bun:ffinever bounded the user-suppliedbyteLengthagainst the largest buffer JSC can back, so a length in(2^32, 2^56)sailed into JSC and hit the release assert.JSC's
MAX_ARRAY_BUFFER_SIZEis1ull << 32inclusive on 64-bit (PageCount.h), which is alsoBun::Buffer::kMaxLength, alsorequire("buffer").kMaxLength, and also exactly whatnew ArrayBuffer(2 ** 32)accepts today.Fix
ArrayBuffer::from_bytes/from_owned_bytes.RangeError [ERR_OUT_OF_RANGE]pastMAX_ARRAY_BUFFER_SIZE, matching what every other Buffer entry point already does. The bound covers the NUL-scan path (no explicitbyteLength) as well. A caller with a larger mapping can now catch the error and window the view.ArrayBuffer::MAX_SIZEis documented askMaxLengthbut heldu32::MAX, one below the real limit. Correct it and use it. Its one other consumer isnode:crypto'sMAX_POSSIBLE_LENGTH = min(MAX_SIZE, i32::MAX), which isi32::MAXeither way.CStringkeeps the address bound: its result is capped byWTF::String::MaxLengthin code units, which a UTF-8 byte count does not map onto, soMAX_ARRAY_BUFFER_SIZEwould be the wrong number there.get_ptr_slicehanded its validation failures back asErrorobjects in the return slot rather than throwing them, so a newRangeErrorwould have been uncatchable (toArrayBuffer(0)returned aTypeErrorobject where anArrayBufferwas expected). They throw now, as do the finalizer-argument checks intoArrayBuffer/toBuffer. One user-visible consequence: areturns: "cstring"symbol whose C function hands back one of the sentinel addresses (0xDEADBEEF,0xAAAAAAAA) now throws from the call instead of yielding aCStringwhose text is the error message.Removing the cast widens slightly beyond FFI: the ~15 other callers of
ArrayBuffer::from_bytes(Blob.arrayBuffer()on a >4 GiB file being the only realistic one) previously panicked on such a length and now reach JSC's own release assert. Abort either way, no new correctness hole, and every one of them hands the descriptor straight to ato_js*sink rather than reading the length back.Also: an omitted byteOffset was the error case
While restructuring
get_ptr_sliceit turned out its twobyteOffsetarms are inverted, whichptr()a few lines up gets right:Nullish meant "error" and a non-number meant "ignore it".
CString.prototype.arrayBufferpasses abyteOffsetthat defaults toundefined, so it has been handing back (and caching) aTypeErrorwhere anArrayBufferbelongs. Swapping the arms fixes both, and it has to happen here: converting these errors from returned to thrown would otherwise turn that into a throw on a valid call.Verification
2 ** 32keeps working,2 ** 32 + 1throws, against a real 6 GiB anonymous mapping:dlopen + mmap
Three tests in
test/js/bun/ffi/ffi.test.js, all of which fail on the released binary (USE_SYSTEM_BUN=1 bun test, the byteLength one by aborting the child) and pass onbun bd test. The byteLength test pins both ends of the boundary:2 ** 32succeeds,2 ** 32 + 1throughNumber.MAX_SAFE_INTEGERthrow for both entry points.Note for reviewers: #32260 touches the
byteOffsetlines ofget_ptr_slicefor an unrelated negative-offset panic, so expect a small conflict if both land.no test proof · iteration 8 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi.test.js