Make ArrayBuffer::from_bytes unsafe and add owning constructors

JA
6mo ago
+171-112

What

ArrayBuffer::from_bytes(bytes: &mut [u8], ..) (src/jsc/array_buffer.rs) stores bytes.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_bytes had the identical problem (and additionally mi_frees the pointer in destroy()).

This PR makes the borrowing constructor unsafe fn with a # Safety contract 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 existing from_owned_bytes(Box<[u8]>)): adopts a Vec's allocation without reallocating — the deallocator installed by to_js* frees the whole block from its data pointer, so the capacity is discarded rather than shrunk. Used by:
    • streams.rs Start::to_js / StreamResult::to_js (owned chunk delivery, was ManuallyDrop + slice_mut())
    • ReadableStream::drain_from_js
    • TextEncoder__encode8/16 heap paths (was Vec::leak())
    • JSZlib gzip/gunzip/deflate/inflate ×4 (was Vec::leak())
    • node.rs to_array_buffer / MaybeToJs for Vec<u8> (was Vec::leak())
  • MarkedArrayBuffer::from_bytes(&mut [u8]) → from_owned_bytes(Box<[u8]>): callers in Terminal.rs, SubprocessPipeReader.rs, shell/subproc.rs, node_fs.rs (×3), and MarkedArrayBuffer::from_string no longer Box::leak/into_raw + reborrow by hand.
  • Blob Lifetime::Temporary: reclaims the leaked Box<[u8]> with heap::take and hands it to from_owned_bytes.
  • ArrayBufferSink::end_from_js done-path: empty boxed slice instead of &mut [].

Sites left unsafe (cannot transfer ownership)

These pass memory that is not a default-allocator Box/Vec and is released by a custom deallocator; they now call unsafe { ArrayBuffer::from_bytes(..) } with a // SAFETY: comment naming the owner:

  • FFIObject::to_array_buffer — caller-owned FFI memory with an optional user finalizer.
  • Blob Lifetime::Share / Lifetime::Transfer — store-owned bytes released by blob_store_array_buffer_deallocator.
  • Image.rs Deliver::Uint8Array — codec-owned allocation released by the codec's free.

Cost

Zero. from_owned_vec keeps the exact pointer/length the old code passed (no into_boxed_slice/shrink_to_fit), and from_owned_bytes callers already had a Box<[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_bytes itself becomes unsafe fn (+1) and the two callers that keep it gain an unsafe {} block (+2), offset by the removed reborrow blocks in MarkedArrayBuffer::from_string and node_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.rs plus callers of ArrayBuffer::from_bytes. MarkedArrayBuffer::from_bytes is 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

  • New: text-encoder.test.js round-trips >2048-byte latin1 and UTF-16 encode results across a forced GC (heap path → from_owned_vec).
  • New: streams.test.js streams a 512 KB file, forces GC, and verifies the collected chunks round-trip (owned chunk delivery → from_owned_vec).
  • Existing suites run locally against the debug build: 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).
  • The JSC crate calls C++ at runtime, so Miri cannot cover it.