Make ArrayBuffer::from_bytes unsafe and add owning constructors - #31174
Make ArrayBuffer::from_bytes unsafe and add owning constructors#31174Jarred-Sumner wants to merge 3 commits into
Conversation
ArrayBuffer::from_bytes(&mut [u8]) stored the borrowed slice's pointer in a lifetime-less struct that becomes a JS-visible ArrayBuffer, so safe code could hand JSC a dangling pointer. Mark it unsafe fn with a # Safety contract and migrate every caller that actually owns its buffer to an ownership-taking constructor: - ArrayBuffer::from_owned_vec(Vec<u8>): new safe constructor that adopts a Vec's allocation without reallocating (the installed deallocator frees from the data pointer, so capacity is not needed). - MarkedArrayBuffer::from_bytes(&mut [u8]) -> from_owned_bytes(Box<[u8]>): same fix for the owning wrapper; callers no longer Box::leak by hand. Callers that genuinely cannot transfer ownership (FFI.toArrayBuffer over caller-owned memory, Blob share/transfer over store-owned bytes, image codec output with a custom free) keep an explicit unsafe call with a SAFETY comment. No behavior change; the pointers handed to JSC and the installed deallocators are identical before and after.
|
Updated 3:51 AM PT - May 21st, 2026
❌ @Jarred-Sumner, your commit dc39a8f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31174That installs a local version of the PR into your bun-31174 --bun |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds owned-allocation constructors for ArrayBuffer/MarkedArrayBuffer and replaces leak/manual-pointer patterns with ownership-transfer APIs across compression, I/O, file-system, text-encoding, blob/streaming, and FFI code; includes tests verifying large buffers survive explicit GC. ArrayBuffer Owned-Allocation Refactoring
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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/web/encoding/text-encoder.test.js`:
- Around line 491-497: Replace the use of String.prototype.repeat for the test
input strings: instead of building latin1 with "héllo wörld å ".repeat(200) and
utf16 with "❤️ Red Heart ✨ Sparkles 🔥 Fire ".repeat(100), construct equivalent
repetitive strings using Buffer.alloc(count, fill).toString() so the tests that
reference variables latin1 and utf16 (used with encoder.encode, decoder.decode,
and getByteLength) follow the repo guideline and avoid debug-build performance
issues; ensure the fill argument matches the original substring and preserve the
same repetition counts (200 and 100).
🪄 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: 6bb9b651-6fa3-489e-8d47-52928de7cd07
📒 Files selected for processing (16)
src/jsc/array_buffer.rssrc/runtime/api/BunObject.rssrc/runtime/api/bun/Terminal.rssrc/runtime/api/bun/subprocess/SubprocessPipeReader.rssrc/runtime/ffi/FFIObject.rssrc/runtime/image/Image.rssrc/runtime/node.rssrc/runtime/node/node_fs.rssrc/runtime/shell/subproc.rssrc/runtime/webcore/ArrayBufferSink.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/TextEncoder.rssrc/runtime/webcore/streams.rstest/js/web/encoding/text-encoder.test.jstest/js/web/streams/streams.test.js
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this refactor touches memory-ownership semantics at the JSC FFI boundary across 14 source files (Blob lifetimes, streams, fs, subprocess, zlib, FFI), so it warrants a human pass — also note the CI unnecessary unsafe block failure at src/bun_core/lib.rs:3352 which may be a missed call site.
Extended reasoning...
Overview
This PR hardens ArrayBuffer::from_bytes by marking it unsafe (it stores a raw pointer with no lifetime into a JS-visible object) and introduces ArrayBuffer::from_owned_vec / MarkedArrayBuffer::from_owned_bytes as safe, ownership-taking alternatives. It then migrates ~16 call sites across src/jsc/array_buffer.rs, zlib (BunObject.rs), Terminal.rs, SubprocessPipeReader.rs, FFIObject.rs, Image.rs, node.rs, node_fs.rs, shell/subproc.rs, ArrayBufferSink.rs, Blob.rs, ReadableStream.rs, TextEncoder.rs, and streams.rs, plus two new GC round-trip tests.
Security risks
The change sits squarely on the Rust↔JSC ownership boundary where mistakes manifest as use-after-free or double-free. The new from_owned_vec discards Vec capacity and relies on the mimalloc-backed deallocator freeing the whole block from its data pointer — a correct but subtle invariant. The Blob Lifetime::Temporary arm now reclaims a leaked Box<[u8]> via heap::take before handing it to from_owned_bytes, which changes the ownership-transfer shape. These look correct on read-through, but they are exactly the class of change that benefits from a second pair of eyes.
Level of scrutiny
High. This is core runtime memory management touching GC finalizers, FFI deallocators, and buffer hand-off across a dozen subsystems. While each individual call-site edit is mostly mechanical (replace leak() + from_bytes with from_owned_vec/from_owned_bytes), the aggregate surface area and the criticality of getting allocator/deallocator pairing right put this well outside the auto-approve envelope.
Other factors
- CI (Build #56592) reports
unnecessary unsafe blockatsrc/bun_core/lib.rs:3352on four targets — that file is not in the diff, which suggests either a missedfrom_bytescaller that now needs adjustment, or pre-existing noise. Worth confirming before merge. - The bug-hunting system found no issues, and the PR description is thorough with a clear cost/unsafe-count accounting.
- New tests exercise the heap-path GC round-trip for TextEncoder and file streams; existing suites pass locally per the description.
- The one CodeRabbit nit (test string construction) was already addressed in dc39a8f.
|
CI notes — two jobs needed retries, both for failures unrelated to this change:
|
Fixes #31970 ### Problem `make_array_buffer_with_bytes_no_copy` and `make_typed_array_with_bytes_no_copy` in `src/jsc/array_buffer.rs` were safe `pub fn`s accepting an arbitrary raw pointer, length, deallocator fn pointer, and deallocator context. They forward those values to JSC, which adopts the memory as the backing store of a JS-visible ArrayBuffer/TypedArray: every JS read/write dereferences the pointer, and GC later invokes `deallocator(ptr, ctx)`. Safe Rust could therefore mint a JS object backed by a dangling pointer, and get an arbitrary fn-pointer/context pair invoked at GC time, without writing `unsafe`: ```rust let backing = core::ptr::NonNull::<c_void>::dangling().as_ptr(); let _ = make_array_buffer_with_bytes_no_copy(global, backing, 1, None, ptr::null_mut()); ``` The FFI imports were declared `safe fn` with a comment deferring the validity obligation to "the wrapper layer", but the wrappers were themselves safe and public, so the obligation was never discharged at an `unsafe` boundary. ### Fix - Mark both free functions `pub unsafe fn` with a documented `# Safety` contract (`ptr` valid for reads and writes of `len` bytes until the deallocator runs; deallocator/ctx a pair sound to invoke exactly once on the JS thread at GC; `ptr` null only when `len == 0`). - Mark `ArrayBuffer::to_js_with_context` `pub unsafe fn` as well. It forwards a caller-supplied deallocator/ctx pair straight to the functions above (same class), and clippy's deny-level `not_unsafe_ptr_arg_deref` flags it once the callees are unsafe. `to_js` and `to_js_unchecked` take no raw arguments and stay safe; the soundness of the `ArrayBuffer` descriptor struct itself (`from_bytes` accepting arbitrary `&mut [u8]`) is tracked separately per the issue and is being fixed in #31174. - Drop the `safe` marker from the `Bun__makeArrayBufferWithBytesNoCopy` / `Bun__makeTypedArrayWithBytesNoCopy` externs so the obligation lives in the wrappers' contracts instead of being asserted away in the extern block. - Add `unsafe {}` blocks with per-site SAFETY justifications at every caller: `ArrayBuffer::to_js_unchecked` / `to_js` / `to_js_with_context`, `MarkedArrayBuffer::to_js`, `Bun.mmapFile` (BunObject.rs), the zlib/libdeflate sync paths (BunObject.rs), Blob `Share`/`Transfer` lifetimes (Blob.rs), `bun:ffi` `toArrayBuffer` (FFIObject.rs), and the image codec delivery path (Image.rs). All in-tree callers already passed valid pointers; this change is API contract only, no behavior change. ### Test No test file: the change is contract-only with zero behavior delta, and the enforcement is the compiler itself. Reverting any signature to safe trips `unused_unsafe` (denied workspace-wide) at every call site, and clippy's deny-level `not_unsafe_ptr_arg_deref` re-fires on `to_js_with_context`. A source-lint test under `test/internal/` was originally included and removed at maintainer request. ### Verification - `cargo clippy -p bun_jsc -p bun_runtime` clean (was failing on `to_js_with_context` mid-fix, which is what pulled it into scope) - `bun run rust:check-all`: 10 ok, 0 failed - `bun bd test` on suites covering every touched call site: `test/js/bun/util/mmap.test.js` (8 pass), `test/regression/issue/18413-all-compressions.test.ts` + `17793.test.ts` (zlib paths), `test/js/web/fetch/blob.test.ts` + `blob-cow.test.ts` (27 pass), `test/js/bun/ffi/ffi.test.js`, `test/js/bun/image/image.test.ts` (93 pass)
2 participants
What
ArrayBuffer::from_bytes(bytes: &mut [u8], ..)(src/jsc/array_buffer.rs) storesbytes.as_mut_ptr()in a lifetime-less struct that becomes a JS-visible ArrayBuffer. The signature borrows, but the semantics transfer ownership to JSC — safe code could pass a stack or short-lived buffer and produce a dangling JS object.MarkedArrayBuffer::from_byteshad the identical problem (and additionallymi_frees the pointer indestroy()).This PR makes the borrowing constructor
unsafe fnwith a# Safetycontract and migrates every caller that actually owns its buffer to an ownership-taking constructor, so the transfer is visible in the type system.Sites converted to safe owning constructors
ArrayBuffer::from_owned_vec(Vec<u8>)(new, next to the existingfrom_owned_bytes(Box<[u8]>)): adopts aVec's allocation without reallocating — the deallocator installed byto_js*frees the whole block from its data pointer, so the capacity is discarded rather than shrunk. Used by:streams.rsStart::to_js/StreamResult::to_js(owned chunk delivery, wasManuallyDrop+slice_mut())ReadableStream::drain_from_jsTextEncoder__encode8/16heap paths (wasVec::leak())JSZlibgzip/gunzip/deflate/inflate ×4 (wasVec::leak())node.rsto_array_buffer/MaybeToJs for Vec<u8>(wasVec::leak())MarkedArrayBuffer::from_bytes(&mut [u8])→from_owned_bytes(Box<[u8]>): callers inTerminal.rs,SubprocessPipeReader.rs,shell/subproc.rs,node_fs.rs(×3), andMarkedArrayBuffer::from_stringno longerBox::leak/into_raw+ reborrow by hand.BlobLifetime::Temporary: reclaims the leakedBox<[u8]>withheap::takeand hands it tofrom_owned_bytes.ArrayBufferSink::end_from_jsdone-path: empty boxed slice instead of&mut [].Sites left unsafe (cannot transfer ownership)
These pass memory that is not a default-allocator
Box/Vecand is released by a custom deallocator; they now callunsafe { ArrayBuffer::from_bytes(..) }with a// SAFETY:comment naming the owner:FFIObject::to_array_buffer— caller-owned FFI memory with an optional user finalizer.BlobLifetime::Share/Lifetime::Transfer— store-owned bytes released byblob_store_array_buffer_deallocator.Image.rsDeliver::Uint8Array— codec-owned allocation released by the codec'sfree.Cost
Zero.
from_owned_veckeeps the exact pointer/length the old code passed (nointo_boxed_slice/shrink_to_fit), andfrom_owned_bytescallers already had aBox<[u8]>in hand. The generated FFI calls and installed deallocators are identical before and after.Unsafe count
rg -o -e 'unsafe \{' -e 'unsafe fn' ...over the touched files: 624 → 623. (from_bytesitself becomesunsafe fn(+1) and the two callers that keep it gain anunsafe {}block (+2), offset by the removed reborrow blocks inMarkedArrayBuffer::from_stringandnode_fs.rs(−4).) The point of the change is not the count but that the remaining unsafe is at sites whose preconditions genuinely live outside Rust.Scope note
The brief scoped this to
array_buffer.rsplus callers ofArrayBuffer::from_bytes.MarkedArrayBuffer::from_bytesis one of those callers and is itself the same bug, so its own callers (Terminal.rs,SubprocessPipeReader.rs,shell/subproc.rs,node_fs.rs) had to change as part of the same ripple.Tests
text-encoder.test.jsround-trips >2048-byte latin1 and UTF-16 encode results across a forced GC (heap path →from_owned_vec).streams.test.jsstreams a 512 KB file, forces GC, and verifies the collected chunks round-trip (owned chunk delivery →from_owned_vec).text-encoder.test.js(24 pass),streams.test.js(70 tests, 3 only fail under the 5 s default timeout on the ASAN debug build and pass with a longer timeout),arraybuffersink.test.ts(6 pass),zlib.test.js(374 pass; the 2 failures are the brotli/zstd streaming timing tests, which don't touch this code and exceed their 15 s timeout under ASAN),blob.test.ts(16 pass).