jsc: replace MarkedArrayBuffer::from_bytes with an owning constructor#32031
jsc: replace MarkedArrayBuffer::from_bytes with an owning constructor#32031alii wants to merge 7 commits into
Conversation
|
Done (current head a1a13d1): the fixture crate and bun:test driver are replaced with a CI: every lane that reports a test failure is unrelated to this diff. |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
can we use rust tests for this rather than an entirely new separate crate that is built from within bun:test? seems wasteful. we have some other rust tests you could likely follow for reference. just make sure that they do still run in ci otherwise not useful. @robobun
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
WalkthroughRefactors ChangesMarkedArrayBuffer ownership safety and migration
Possibly related PRs
Suggested reviewers
🚥 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/jsc/array_buffer.rs`:
- Around line 1036-1045: The method to_node_buffer currently only clears
owns_buffer but leaves self.buffer.ptr/byte_len live — change it to consume the
backing buffer by taking/replacing self.buffer with ArrayBuffer::EMPTY (or
equivalent take/null) before calling JSValue::create_buffer so the original
MarkedArrayBuffer no longer holds the allocation; apply the same take/replace
pattern in the sibling method to_js (and any other handoff sites) so ownership
is transferred exactly once and the source handle is neutralized.
- Around line 962-969: MarkedArrayBuffer::from_string currently uses
Box::from(str) which is infallible and will OOM-abort instead of returning
bun_alloc::AllocError; replace the infallible allocation with a fallible path
that returns Err(bun_alloc::AllocError) on OOM (e.g., allocate into a Vec<u8>
using try_reserve/try_reserve_exact then extend_from_slice and convert to
Box<[u8]> or call the in-tree fallible allocator helper such as allocator.dupe
for u8), then pass the owned bytes to MarkedArrayBuffer::from_owned_bytes so the
function honors its Result<..., bun_alloc::AllocError> contract and keeps
existing semantics with MarkedArrayBuffer_deallocator/default_alloc::free.
🪄 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: 3052f0c9-4b3c-42d4-baa8-ed10e6fb9acd
⛔ Files ignored due to path filters (1)
test/regression/marked-array-buffer-ownership-soundness-fixture/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
src/jsc/array_buffer.rssrc/runtime/api/bun/Terminal.rssrc/runtime/api/bun/subprocess/Readable.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
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)
test/regression/marked-array-buffer-ownership-soundness.test.ts (1)
23-26:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd missing imports from harness.
The test uses
bunExe()(lines 82, 115) andbunEnv(lines 99, 132) but never imports them. This will fail at runtime withReferenceError.🐛 Proposed fix
-import { spawn, which } from "bun"; +import { bunEnv, bunExe } from "harness"; +import { spawn, which } from "bun"; import { expect, test } from "bun:test"; import { existsSync } from "node:fs"; import { join } from "node:path";🤖 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 `@test/regression/marked-array-buffer-ownership-soundness.test.ts` around lines 23 - 26, The test references bunExe() and bunEnv but never imports them; add missing imports for bunExe and bunEnv from the test harness module at the top of test/regression/marked-array-buffer-ownership-soundness.test.ts so the functions are available to the tests (import the named exports bunExe and bunEnv from the harness module used by other tests).
🤖 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`:
- Around line 74-139: Delete the duplicated test block that repeats the test
titled "empty buffers round-trip without freeing the dangling sentinel" so only
one instance remains; locate the two identical test(...) declarations (the
blocks starting with test("empty buffers round-trip without freeing the dangling
sentinel", async () => { ... })) and remove one of them (keep the other intact)
to eliminate the copy-paste duplicate while preserving the assertions and spawn
logic.
---
Outside diff comments:
In `@test/regression/marked-array-buffer-ownership-soundness.test.ts`:
- Around line 23-26: The test references bunExe() and bunEnv but never imports
them; add missing imports for bunExe and bunEnv from the test harness module at
the top of test/regression/marked-array-buffer-ownership-soundness.test.ts so
the functions are available to the tests (import the named exports bunExe and
bunEnv from the harness module used by other tests).
🪄 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: de0c920e-5573-41b0-a2b5-45bff4568055
📒 Files selected for processing (1)
test/regression/marked-array-buffer-ownership-soundness.test.ts
|
@robobun? adopt plz |
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 @.github/workflows/rust-test.yml:
- Line 42: The action metadata for setup-bun uses unsupported `type:` keys on
inputs which breaks workflow parsing; open the action metadata for the setup-bun
local action and remove the `type:` entries from the inputs definitions (for
inputs named `bun-version`, `baseline`, and `download-url`) so the inputs remain
declared without `type` fields and the workflow can parse the action correctly.
🪄 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: 39531e4c-ccd2-4776-a299-8a1f4580c2f5
📒 Files selected for processing (4)
.github/workflows/rust-test.ymlsrc/jsc/JSGlobalObject.rssrc/jsc/array_buffer.rssrc/jsc/host_object.rs
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
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.
An empty Box<[u8]> has no backing allocation - its pointer is NonNull::dangling(), not a heap address - so destroy() and the JSC handoff must skip the free when byte_len == 0. Exercises empty readFileSync round-trips plus GC in a subprocess; on the ASAN debug build, removing the guard makes the free of the dangling sentinel abort, so the test discriminates (verified by mutation).
Mutation-testing the byte_len guard out of destroy() leaves the test green - the empty readFileSync path hands ownership to JSC before any destroy(), so the dangling-pointer free is not reachable from JS here. The guard's claim was verified at the Rust level instead (an empty Box<[u8]> performs no allocation and its pointer is NonNull::dangling).
The MarkedArrayBuffer ownership contract test moves from a detached cargo fixture crate driven by bun:test into a compile_fail,E0599 doctest on from_owned_bytes. rustdoc verifies compile_fail doctests without linking, so this works for bun_jsc even though its non-doc test targets cannot link without the C++ side of the runtime. A new Rust Tests workflow runs cargo test --doc -p bun_jsc on PRs touching src/jsc/**, so the contract is actually checked in CI (the fixture test skipped on every runner without a cargo toolchain and build tree). Two pre-existing indented doc blocks in host_object.rs and JSGlobalObject.rs are fenced as ignore; rustdoc treated them as doctests and they never compiled. to_js/to_node_buffer also take the backing buffer out of self at the JSC handoff (leaving ArrayBuffer::EMPTY), so neither a later destroy() nor a repeated handoff can release the same allocation twice.
robobun
force-pushed
the
ali/marked-array-buffer-owning-ctor
branch
from
20e2596 to
299c27b
Compare
|
Updated 1:22 AM PT - Jun 29th, 2026
❌ @robobun, your commit a1a13d1 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32031That installs a local version of the PR into your bun-32031 --bun |
|
Rebased onto main and force-pushed (299c27b). One conflict, in the Re-verified on the rebased tree: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…lchain pin destroy() now resets self.buffer to ArrayBuffer::EMPTY after releasing the allocation, so the freed pointer cannot be observed through slice() or a later handoff; this matches the take/replace pattern to_js and to_node_buffer already use. The Rust toolchain bump checklist in .github/workflows/CLAUDE.md now lists rust-test.yml alongside clippy.yml and miri.yml, since it pins the same RUSTUP_TOOLCHAIN.
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 take the backing buffer out ofself(leavingArrayBuffer::EMPTY), so neither adestroy()after the handoff nor a repeated handoff on the same value can release the allocation twice (both 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.destroy()also resets the handle toArrayBuffer::EMPTYafterward, so the freed pointer cannot be observed throughslice()or a later handoff (review finding). The dangling free is not reachable from JS (empty reads return the non-owningBuffer::EMPTY, and the JS-reachabledestroy()call sites sit behind that path), so this is defense in depth for the new constructor's contract.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
A
compile_fail,E0599doctest onMarkedArrayBuffer::from_owned_bytespins the contract: the exact unsound snippet above must fail to resolve. rustdoc verifiescompile_faildoctests without linking, which is what makes a Rust-level test possible here at all: bun_jsc's non-doc test targets cannot link without the C++ side of the runtime.A new
Rust Testsworkflow (.github/workflows/rust-test.yml, same setup as clippy.yml: configure +ninja codegen clone-lolhtml) runscargo test --doc -p bun_jscon PRs touchingsrc/jsc/**, so the contract is actually checked in CI. Two pre-existing indented doc blocks inhost_object.rsandJSGlobalObject.rsare now fenced asignore: rustdoc treated them as doctests and they never compiled, which only surfaces once something runs--doc.This replaces the earlier fixture-crate approach (a detached cargo crate with a committed 2.4k-line
Cargo.lock, cargo-checked from insidebun:test), which was heavyweight and silently skipped on every CI runner without a cargo toolchain and build tree.Mutation-tested: re-adding a safe
from_bytes(&mut [u8])makes the doctest fail (the snippet compiles); with the fix in place it passes.Verification
cargo test --doc -p bun_jsc: 1 passed (thecompile_faildoctest), and it fails whenfrom_bytesis reintroduced.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),-t readlink(4 pass), thereaddirSync({encoding: 'buffer', recursive: true})destroy-path test (1 pass), andbunshell.test.ts -t quiet(0 fail).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 --no-depsclean;cargo fmt --allclean.Replaces #31986 (adopted from #31981, which touched the same file).
Rebased onto main (post #32970). One conflict, in
ret::Readdir::to_js'sBuffersarm innode_fs.rs: main independently switched that arm fromto_jstoto_node_buffersoreaddir({ encoding: "buffer" })returnsBuffer[]instead ofUint8Array[], while this PR changes both methods to take&mut self. Resolved by keeping main'sto_node_buffercall overitems.iter_mut(). Main'sreaddir with { encoding: 'buffer' } returns Buffer entriestest passes on the result.