jsc: replace MarkedArrayBuffer::from_bytes with an owning constructor#31986
jsc: replace MarkedArrayBuffer::from_bytes with an owning constructor#31986robobun wants to merge 3 commits into
Conversation
MarkedArrayBuffer::from_bytes(&mut [u8]) was a safe public constructor that stored a borrowed slice's pointer with owns_buffer: true, so safe code could mark a stack or short-lived buffer as allocator-owned and destroy() would later free it with the default allocator. Replace it with from_owned_bytes(Box<[u8]>, JSType) so the ownership transfer is enforced by the type system, route from_string through it, and migrate the callers (Terminal.rs, SubprocessPipeReader.rs, shell/subproc.rs, node_fs.rs x3) off their manual Box::leak / heap::into_raw dances. destroy() now skips the free for empty buffers: an empty Box<[u8]> has no backing allocation, so freeing its dangling pointer with the default allocator was itself an invalid free. test/regression/marked-array-buffer-ownership-soundness.test.ts runs cargo check over a fixture crate containing the unsound pattern and asserts it no longer resolves. Fixes #31969
|
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 (5)
WalkthroughReplaces the unsafe borrowed-slice ChangesArrayBuffer Ownership Model
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Known overlap, noted in the PR description. The relationship:
If #31174 gets rebased and lands first, this one can be closed. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/array_buffer.rs (1)
953-963:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftEnforce ownership invalidation after JS handoff to prevent double-free.
from_owned_bytesmarksowns_buffer = true, but afterto_js/to_node_buffertransfer ownership to JSC, this flag is still live. A safe sequence can still calldestroy()and free the same allocation a second time later when JSC runs the installed deallocator.Suggested direction
-pub fn to_node_buffer(&self, global: &JSGlobalObject) -> JSValue { +pub fn to_node_buffer(&mut self, global: &JSGlobalObject) -> JSValue { let mut buf = self.buffer; - JSValue::create_buffer(global, buf.byte_slice_mut()) + let out = JSValue::create_buffer(global, buf.byte_slice_mut()); + self.owns_buffer = false; + out } -pub fn to_js(&self, global: &JSGlobalObject) -> JsResult<JSValue> { +pub fn to_js(&mut self, global: &JSGlobalObject) -> JsResult<JSValue> { ... - make_typed_array_with_bytes_no_copy(...) + let out = make_typed_array_with_bytes_no_copy(...)?; + self.owns_buffer = false; + self.buffer.value = out; + Ok(out) }As per coding guidelines, every allocation must have exactly one named owner and be released exactly once.
Also applies to: 981-995
🤖 Prompt for 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. In `@src/jsc/array_buffer.rs` around lines 953 - 963, from_owned_bytes currently sets owns_buffer = true but never clears it after handing the buffer to JSC, allowing a double-free via destroy(); fix by ensuring ownership is invalidated when the buffer is transferred to JS: update the transfer paths (the methods to_js and to_node_buffer) to consume or mutate the MarkedArrayBuffer so they set owns_buffer = false (or take self by value) after installing the JSC deallocator, and ensure destroy() checks owns_buffer before freeing; reference the MarkedArrayBuffer type and the methods from_owned_bytes, to_js, to_node_buffer, and destroy so the owner flag is cleared exactly once on handoff.Source: Coding guidelines
🤖 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/regression/marked-array-buffer-ownership-soundness.test.ts`:
- Line 38: Remove the explicit timeout override in the test invocation—delete
the "{ timeout: 10 * 60 * 1000 }" options object passed to the test (the call in
the marked-array-buffer-ownership-soundness test) so the test relies on Bun's
default timeout behavior; search for the test declaration/name in the file and
remove that options argument from the test call (leaving other options/arguments
intact).
---
Outside diff comments:
In `@src/jsc/array_buffer.rs`:
- Around line 953-963: from_owned_bytes currently sets owns_buffer = true but
never clears it after handing the buffer to JSC, allowing a double-free via
destroy(); fix by ensuring ownership is invalidated when the buffer is
transferred to JS: update the transfer paths (the methods to_js and
to_node_buffer) to consume or mutate the MarkedArrayBuffer so they set
owns_buffer = false (or take self by value) after installing the JSC
deallocator, and ensure destroy() checks owns_buffer before freeing; reference
the MarkedArrayBuffer type and the methods from_owned_bytes, to_js,
to_node_buffer, and destroy so the owner flag is cleared exactly once on
handoff.
🪄 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: a94c84f8-ec51-4784-b15f-69a307f207c6
⛔ Files ignored due to path filters (1)
test/regression/marked-array-buffer-ownership-soundness-fixture/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
src/jsc/array_buffer.rssrc/runtime/api/bun/Terminal.rssrc/runtime/api/bun/subprocess/SubprocessPipeReader.rssrc/runtime/node/node_fs.rssrc/runtime/shell/subproc.rstest/regression/marked-array-buffer-ownership-soundness-fixture/.gitignoretest/regression/marked-array-buffer-ownership-soundness-fixture/Cargo.tomltest/regression/marked-array-buffer-ownership-soundness-fixture/src/lib.rstest/regression/marked-array-buffer-ownership-soundness.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review follow-ups: - to_js/to_node_buffer now take &mut self and clear owns_buffer at the handoff, so a later destroy() cannot free the allocation JSC now owns. - Migrate the Readable.rs struct-literal construction to MarkedArrayBuffer::from_owned_bytes so the destroy() SAFETY comment holds for every owns_buffer producer. - Make the soundness test self-diagnosing when the fixture Cargo.lock goes stale, and document the regeneration command.
|
Addressed CodeRabbit's out-of-band critical finding in d156042: |
There was a problem hiding this comment.
Thanks — both earlier nits are addressed (Readable.rs now goes through from_owned_bytes, and the test surfaces a stale-lockfile hint). The added owns_buffer clearing in to_js/to_node_buffer looks right too, but since this is allocator-ownership code at the Rust↔JSC FFI boundary with a public &self→&mut self signature change, I'd like a human to give it a final look.
Extended reasoning...
Overview
This PR replaces the unsound MarkedArrayBuffer::from_bytes(&mut [u8]) constructor with from_owned_bytes(Box<[u8]>), migrates six call sites (Terminal, subprocess pipe readers, shell subproc, three node_fs paths), guards destroy() against freeing an empty box's dangling pointer, and adds a compile-fail regression fixture + test. Since my previous review, a new commit also changed to_js/to_node_buffer to take &mut self and clear owns_buffer before handing the buffer to JSC, and migrated the Readable.rs struct-literal site I'd flagged.
Prior feedback
Both of my earlier inline comments were addressed:
Readable.rsnow callsMarkedArrayBuffer::from_owned_bytes(own, …)instead of constructing the struct literal withowns_buffer: true, so the SAFETY comment indestroy()is now accurate.- The soundness test now detects
--locked was passedin cargo's output and throws with the regeneration command plus full stderr, so a stale fixture lockfile is self-diagnosing.
Security risks
No injection/auth/data-exposure surface. The risk class here is memory safety: this code governs which allocator frees buffers that cross the Rust↔JSC boundary, and the new owns_buffer = false clearing in to_js/to_node_buffer changes when destroy() becomes a no-op. A mistake here is a double-free or invalid-free in production. The reasoning in the diff (clear before the fallible FFI call; leak-on-throw is the safe direction) looks correct, and the empty-box guard in destroy() matches the existing pattern in JSValue::create_buffer_from_box.
Level of scrutiny
High. This is core JSC FFI ownership logic, not config or test-only code. The &self → &mut self signature change on to_js/to_node_buffer is a public API change that required updating every caller (e.g. Readdir::Buffers now iterates with iter_mut()). The migrated call sites are mechanically simpler than before (no more heap::into_raw + unsafe { &mut *raw }), which is good, but the overall change deserves a maintainer's eyes.
Other factors
- There is a known overlapping PR (#31174) covering the same constructor; the description and a robobun comment explain the relationship, but a maintainer should decide which lands.
- CI shows
test/cli/install/bunx.test.tsfailing across all platforms on the latest build; this looks unrelated to the change (it's abunxinstall test) but is worth confirming before merge. - ASAN verification was run per the description over the migrated paths (terminal, spawnSync, fs readFile/readlink, empty-buffer cases).
|
CI analysis for build 61379 (final state): every failure is pre-existing or external, none touch this diff.
The suites covering the paths this PR changes (terminal, spawnSync, fs readFile/readdir, shell, and the compile-fail soundness test) pass on every lane. |
|
Superseded by the adopted/rebased copy — see replacement PR. |
2 participants
What
MarkedArrayBuffer::from_bytes(&mut [u8], JSType)(src/jsc/array_buffer.rs) was a safe, public constructor that stored a borrowed slice's pointer in a lifetime-less struct withowns_buffer: true. The safedestroy()then freed that pointer with the default allocator, so safe code could free a stack buffer:to_js/to_node_bufferhad the same problem deferred: they installMarkedArrayBuffer_deallocatorover the stored pointer, so GC frees it later.All in-tree callers upheld the contract manually (
Box::leak/heap::into_rawa default-allocatorBox<[u8]>, then reborrow), so there is no user-reachable corruption today; this closes the API-boundary soundness hole.Fixes #31969
Fix
MarkedArrayBuffer::from_bytes(&mut [u8])withMarkedArrayBuffer::from_owned_bytes(Box<[u8]>, JSType), built on the existingArrayBuffer::from_owned_bytes, so the ownership transfer is enforced by the type system. This is the same shape Make ArrayBuffer::from_bytes unsafe and add owning constructors #31174 proposed for this type (that PR also covers the separateArrayBuffer::from_byteshole, [Unsoundness] ArrayBuffer no-copy constructors trust raw backing pointers #31970, which this PR deliberately does not touch).MarkedArrayBuffer::from_stringthrough it.Terminal.rs,SubprocessPipeReader.rs,shell/subproc.rs, andnode_fs.rs(x3, aliased asBuffer::from_bytes).to_js/to_node_buffernow take&mut selfand clearowns_bufferat the handoff, so adestroy()after the buffer has been given to JSC cannot free the allocation a second time (flagged by review). TheReadable.rsstruct-literal construction was migrated tofrom_owned_bytesfor the same reason.destroy()now skips the free whenbyte_len == 0: an emptyBox<[u8]>has no backing allocation (its pointer is dangling), so freeing it with the default allocator was itself an invalid free. Same reasoning as the existing empty-case guard inJSValue::create_buffer_from_box.No behavior change: the pointers handed to JSC and the installed deallocators are identical before and after (callers already passed
into_boxed_slice()allocations).Test
test/regression/marked-array-buffer-ownership-soundness.test.ts+ companion fixture crate, following the compile-fail pattern from #31093: the fixture contains exactly the unsound snippet above, and the test assertscargo check --lockedrejects it (error[E0599]: no associated function or constant named 'from_bytes' found for struct 'MarkedArrayBuffer', exit 101). On the unfixed tree the fixture compiles cleanly and the test fails; if a safe borrowed-slice constructor is ever reintroduced, it fails again. Skips whencargoor the cppbind codegen output is unavailable (prebuilt-only CI runners). The fixture'sCargo.lockis committed and checked with--lockedso resolution never floats.Verification
src/(fixture compiles), passes with the fix (E0599).test/js/bun/terminal/terminal.test.ts(87 pass),test/js/bun/spawn/spawnSync.test.ts(6 pass),test/js/node/fs/fs.test.ts -t readFile(20 pass) and-t readlink(4 pass).readFileSyncBuffer path verified with a compiled executable reading an embedded file.readFileSyncof an empty file, multipart FormData with an empty file blob) verified under ASAN.cargo clippy -p bun_jsc -p bun_runtime --no-depsclean.