Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 45 additions & 23 deletions src/jsc/array_buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -907,9 +907,9 @@ pub struct MarkedArrayBuffer {
pub owns_buffer: bool,
}

/// Bytes produced off-thread (`from_bytes`/`from_string`) are owned until they
/// are handed to JSC; a result that is never converted (its VM went away, the
/// conversion path bailed) frees them here.
/// Bytes produced off-thread (`from_owned_bytes`/`from_string`) are owned until
/// they are handed to JSC; a result that is never converted (its VM went away,
/// the conversion path bailed) frees them here.
impl Drop for MarkedArrayBuffer {
fn drop(&mut self) {
self.destroy();
Expand All @@ -932,15 +932,13 @@ impl MarkedArrayBuffer {
}

pub fn from_string(str: &[u8]) -> Result<MarkedArrayBuffer, bun_alloc::AllocError> {
// allocator.dupe(u8, str) → Box::<[u8]>::from(str), but we need a raw
// pointer because the buffer is later freed via the default allocator
// (`MarkedArrayBuffer_deallocator` → `default_alloc::free`).
let buf: Box<[u8]> = Box::from(str);
let len = buf.len();
let ptr = bun_core::heap::into_raw(buf).cast::<u8>();
// SAFETY: ptr/len from heap::alloc; backed by the global allocator.
let bytes = unsafe { bun_core::ffi::slice_mut(ptr, len) };
Ok(MarkedArrayBuffer::from_bytes(bytes, JSType::Uint8Array))
// allocator.dupe(u8, str) → Box::<[u8]>::from(str); the buffer is later
// freed via the default allocator (`destroy` or
// `MarkedArrayBuffer_deallocator` → `default_alloc::free`).
Ok(MarkedArrayBuffer::from_owned_bytes(
Box::from(str),
JSType::Uint8Array,
))
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

pub fn from_js(global: &JSGlobalObject, value: JSValue) -> Option<MarkedArrayBuffer> {
Expand All @@ -951,12 +949,31 @@ impl MarkedArrayBuffer {
})
}

pub fn from_bytes(bytes: &mut [u8], typed_array_type: JSType) -> MarkedArrayBuffer {
/// Take ownership of a default-allocator `Box<[u8]>`. The bytes are freed
/// exactly once: by [`MarkedArrayBuffer::destroy`] (also run by `Drop`) if
/// the value is never converted, or by the deallocator JSC installs when
/// `to_node_buffer` hands them over.
///
/// Requiring `Box<[u8]>` makes the ownership transfer a type-system
/// invariant. There is deliberately no constructor that adopts a borrowed
/// slice as owned storage: the former `from_bytes(&mut [u8], _)` let safe
/// code free a stack buffer (issue #31969). This must not compile
/// (checked by `cargo test --doc -p bun_jsc`):
///
/// ```compile_fail,E0599
/// use bun_jsc::{JSType, MarkedArrayBuffer};
///
/// let mut bytes = [0u8; 1];
/// let buffer = MarkedArrayBuffer::from_bytes(&mut bytes, JSType::Uint8Array);
/// drop(buffer); // would free the stack address
/// ```
pub fn from_owned_bytes(bytes: Box<[u8]>, typed_array_type: JSType) -> MarkedArrayBuffer {
// An empty boxed slice has no backing allocation (dangling ptr):
// nothing to own, so `destroy()` must not free it.
let owns_buffer = !bytes.is_empty();
MarkedArrayBuffer {
buffer: ArrayBuffer::from_bytes(bytes, typed_array_type),
// An empty boxed slice has no backing allocation (dangling ptr):
// nothing to own, so `destroy()` must not free it.
owns_buffer: !bytes.is_empty(),
buffer: ArrayBuffer::from_owned_bytes(bytes, typed_array_type),
owns_buffer,
}
}

Expand All @@ -971,23 +988,28 @@ impl MarkedArrayBuffer {
}

/// Releases the owned byte buffer if this `MarkedArrayBuffer` was created with an
/// allocator (e.g. via `from_string`/`from_bytes`) and never handed to JSC.
/// allocator (e.g. via `from_string`/`from_owned_bytes`) and never handed to JSC.
/// Idempotent; also what `Drop` does.
pub fn destroy(&mut self) {
if self.owns_buffer {
self.owns_buffer = false;
// SAFETY: buffer.ptr was allocated by the global allocator (heap::alloc / allocator.dupe).
// SAFETY: `owns_buffer` is only set by `from_owned_bytes`, which took
// the pointer from a non-empty default-allocator `Box<[u8]>`.
unsafe { bun_alloc::default_alloc::free(self.buffer.ptr.cast()) };
// Neutralize the handle so a later `slice()` or handoff cannot
// observe the freed pointer.
self.buffer = ArrayBuffer::EMPTY;
}
}
Comment thread
robobun marked this conversation as resolved.

/// Ownership of the bytes moves to JSC (freed by the buffer's deallocator).
pub fn to_node_buffer(&mut self, global: &JSGlobalObject) -> JsResult<JSValue> {
// `JSValue::create_buffer` takes `&mut [u8]` (ownership transfers to JSC
// via the deallocator). `ArrayBuffer` is `Copy` over a raw pointer, so
// copy the descriptor and project a mutable slice.
// Take the buffer out of `self` so neither `destroy()`/`Drop` nor a
// repeated handoff can release the allocation a second time.
// `JSValue::create_buffer` takes `&mut [u8]` and installs the
// deallocator over it.
self.owns_buffer = false;
let mut buf = self.buffer;
let mut buf = core::mem::replace(&mut self.buffer, ArrayBuffer::EMPTY);
JSValue::create_buffer(global, buf.byte_slice_mut())
}
}
Expand Down
17 changes: 9 additions & 8 deletions src/runtime/api/bun/Terminal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1903,14 +1903,15 @@ impl Terminal {
return true;
}
v.extend_from_slice(chunk);
// MarkedArrayBuffer::from_bytes takes a `&mut [u8]` it will own (freed
// via mimalloc on the C++ side) — leak the Box and hand over the slice.
let bytes: &'static mut [u8] = Box::leak(v.into_boxed_slice());
// This is the pipe reader's landing frame: a buffer that cannot be
// built (allocation failure, a terminating VM) is folded here and
// reading goes on.
let data = match MarkedArrayBuffer::from_bytes(bytes, jsc::JSType::Uint8Array)
.to_node_buffer(global_this)
// Ownership of the boxed slice transfers to JSC (freed via the
// buffer's deallocator). This is the pipe reader's landing frame: a
// buffer that cannot be built (allocation failure, a terminating VM)
// is folded here and reading goes on.
let data = match MarkedArrayBuffer::from_owned_bytes(
v.into_boxed_slice(),
jsc::JSType::Uint8Array,
)
.to_node_buffer(global_this)
{
Ok(data) => data,
Err(err) => {
Expand Down
32 changes: 11 additions & 21 deletions src/runtime/node/node_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6775,16 +6775,10 @@ impl NodeFS {
if let Some(file) = graph.find_ref(path.as_bytes()) {
let contents: &[u8] = file.utf8_contents();
return if args.encoding == Encoding::Buffer {
// PORTING.md §Forbidden bans `Vec::leak()`; round-trip through
// `into_boxed_slice()` so the allocation layout JSC frees with
// matches what we hand it (capacity == len).
let raw =
bun_core::heap::into_raw(contents.to_vec().into_boxed_slice());
// SAFETY: ownership of the allocation is transferred to JSC; the
// ArrayBuffer finalizer reconstructs the Box and frees it
// (PORTING.md:348 — `heap::alloc`/`from_raw` across FFI).
Ok(ret::ReadFileWithOptions::Buffer(Buffer::from_bytes(
unsafe { &mut *raw },
// Ownership of the boxed slice transfers to the
// `Buffer` (freed by it, or by JSC once converted).
Ok(ret::ReadFileWithOptions::Buffer(Buffer::from_owned_bytes(
contents.to_vec().into_boxed_slice(),
bun_jsc::JSType::Uint8Array,
)))
} else if string_type == ReadFileStringType::Default {
Expand Down Expand Up @@ -6930,15 +6924,12 @@ impl NodeFS {
};
}
}
let raw = bun_core::heap::into_raw(
// Ownership of the boxed slice transfers to the `Buffer`
// (freed by it, or by JSC once converted).
Ok(ret::ReadFileWithOptions::Buffer(Buffer::from_owned_bytes(
temporary_read_buffer_before_stat_call
.to_vec()
.into_boxed_slice(),
);
// SAFETY: ownership transferred to JSC; freed via ArrayBuffer finalizer
// (PORTING.md:348 — `heap::alloc`/`from_raw` across FFI).
Ok(ret::ReadFileWithOptions::Buffer(Buffer::from_bytes(
unsafe { &mut *raw },
bun_jsc::JSType::Uint8Array,
)))
}
Expand Down Expand Up @@ -7097,11 +7088,10 @@ impl NodeFS {
match args.encoding {
Encoding::Buffer => {
buf.truncate(final_len);
let raw = bun_core::heap::into_raw(buf.into_boxed_slice());
// SAFETY: ownership transferred to JSC; freed via ArrayBuffer finalizer
// (PORTING.md:348 — `heap::alloc`/`from_raw` across FFI).
Ok(ret::ReadFileWithOptions::Buffer(Buffer::from_bytes(
unsafe { &mut *raw },
// Ownership of the boxed slice transfers to the `Buffer`
// (freed by it, or by JSC once converted).
Ok(ret::ReadFileWithOptions::Buffer(Buffer::from_owned_bytes(
buf.into_boxed_slice(),
bun_jsc::JSType::Uint8Array,
)))
}
Expand Down
12 changes: 10 additions & 2 deletions test/cli/run/cjs-fixture-leak-small.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 10 additions & 2 deletions test/cli/run/esm-bug-leak-fixture.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 27 additions & 5 deletions test/cli/run/esm-fixture-leak-small.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 10 additions & 2 deletions test/cli/run/require-cache-bug-leak-fixture.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading