diff --git a/src/jsc/array_buffer.rs b/src/jsc/array_buffer.rs index de94e5c9d181..129f4fdee128 100644 --- a/src/jsc/array_buffer.rs +++ b/src/jsc/array_buffer.rs @@ -1,3 +1,4 @@ +use core::cell::Cell; use core::ffi::{c_uint, c_void}; use core::ptr; @@ -135,6 +136,14 @@ unsafe extern "C" { safe fn JSC__ArrayBuffer__deref(self_: &JSCArrayBuffer); // safe: by-value `JSValue`; no-op for non-buffer values. safe fn JSC__JSValue__unpinArrayBuffer(v: JSValue); + // safe: by-value `JSValue`; `&mut *mut u8` / `&mut usize` are + // ABI-identical to non-null out-params the callee fills unconditionally. + safe fn JSC__JSValue__pinAndReadArrayBufferBytes( + v: JSValue, + force_pin: bool, + out_ptr: &mut *mut u8, + out_byte_len: &mut usize, + ) -> bool; } impl JSValue { @@ -815,28 +824,59 @@ impl TypedArrayType { // MarkedArrayBuffer // ────────────────────────────────────────────────────────────────────────── +/// Byte view over a JS `ArrayBuffer`/view or a Rust-owned allocation. For a +/// JS-backed buffer the `(ptr, byte_len)` is taken lazily on the first +/// [`bytes`]/[`slice`] call: pin, read `vector()`/`byteLength()` once, cache. +/// `Drop` releases the pin (JS thread); off-thread users route through +/// [`crate::Unprotect`] which clears the pin first. #[derive(Default)] pub struct MarkedArrayBuffer { - pub buffer: ArrayBuffer, - pub owns_buffer: bool, - pub pinned: bool, + buffer: Cell, + owns_buffer: Cell, + pinned: Cell, + /// [`bytes`] has run; `buffer.ptr`/`byte_len` hold the post-coercion read. + settled: Cell, } impl MarkedArrayBuffer { - pub fn from_typed_array(ctx: &JSGlobalObject, value: JSValue) -> MarkedArrayBuffer { - MarkedArrayBuffer { - owns_buffer: false, - pinned: false, - buffer: ArrayBuffer::from_typed_array(ctx, value), + #[inline] + const fn new(buffer: ArrayBuffer, owns_buffer: bool) -> Self { + Self { + buffer: Cell::new(buffer), + owns_buffer: Cell::new(owns_buffer), + pinned: Cell::new(false), + settled: Cell::new(owns_buffer), } } + pub fn from_typed_array(ctx: &JSGlobalObject, value: JSValue) -> MarkedArrayBuffer { + Self::new(ArrayBuffer::from_typed_array(ctx, value), false) + } + pub fn from_array_buffer(ctx: &JSGlobalObject, value: JSValue) -> MarkedArrayBuffer { - MarkedArrayBuffer { - owns_buffer: false, - pinned: false, - buffer: ArrayBuffer::from_array_buffer(ctx, value), - } + Self::new(ArrayBuffer::from_array_buffer(ctx, value), false) + } + + /// A non-owning view that neither owns the allocation nor the original's + /// pin. [`bytes`] on the borrow takes its own pin (pin count is a + /// counter), released by its own `Drop`. + #[inline] + pub fn borrow(&self) -> MarkedArrayBuffer { + Self::new(self.buffer.get(), false) + } + + /// Adopt a Rust-owned byte descriptor (freed by [`destroy`] / + /// [`to_js`], not by `Drop`). + #[inline] + pub fn from_owned(buffer: ArrayBuffer) -> MarkedArrayBuffer { + Self::new(buffer, true) + } + + /// Wrap a JS-backed descriptor (its `value` must be set). [`bytes`] pins + /// on first access. + #[inline] + pub fn from_unpinned(buffer: ArrayBuffer) -> MarkedArrayBuffer { + Self::new(buffer, false) } pub fn from_string(str: &[u8]) -> Result { @@ -852,50 +892,103 @@ impl MarkedArrayBuffer { } pub fn from_js(global: &JSGlobalObject, value: JSValue) -> Option { - let array_buffer = value.as_array_buffer(global)?; - Some(MarkedArrayBuffer { - buffer: array_buffer, - owns_buffer: false, - pinned: false, - }) + Some(Self::from_unpinned(value.as_array_buffer(global)?)) } - pub fn from_js_pinned(global: &JSGlobalObject, value: JSValue) -> Option { - let buffer = value.as_pinned_arraybuffer(global)?; - Some(MarkedArrayBuffer { - buffer, - owns_buffer: false, - pinned: true, - }) + pub fn from_bytes(bytes: &mut [u8], typed_array_type: JSType) -> MarkedArrayBuffer { + Self::new(ArrayBuffer::from_bytes(bytes, typed_array_type), true) } - pub fn from_bytes(bytes: &mut [u8], typed_array_type: JSType) -> MarkedArrayBuffer { - MarkedArrayBuffer { - buffer: ArrayBuffer::from_bytes(bytes, typed_array_type), - owns_buffer: true, - pinned: false, + pub const EMPTY: MarkedArrayBuffer = Self::new(ArrayBuffer::EMPTY, false); + + /// Copy of the inner descriptor. `ptr`/`byte_len` reflect whatever the + /// most recent [`bytes`] call cached (or the construction-time snapshot + /// if [`bytes`] has not run yet). + #[inline] + pub fn buffer(&self) -> ArrayBuffer { + self.buffer.get() + } + + #[inline] + pub fn value(&self) -> JSValue { + self.buffer.get().value + } + + /// Release the pin taken by [`bytes`]/[`to_thread_safe`] and clear the + /// flag so `Drop` is a no-op. JS-thread only. + #[inline] + pub fn unpin(&self) { + if self.pinned.replace(false) { + self.buffer.get().unpin(); } } - pub const EMPTY: MarkedArrayBuffer = MarkedArrayBuffer { - owns_buffer: false, - pinned: false, - buffer: ArrayBuffer::EMPTY, - }; + /// Pin-and-snapshot then `protect()` the JS value for a threadpool + /// hand-off. A FastTypedArray (which [`bytes`] leaves unpinned) is + /// promoted to a real `ArrayBuffer` here so its storage cannot move under + /// a worker thread. Paired with [`unprotect`]. + #[inline] + pub fn to_thread_safe(&self) { + self.bytes_::(); + self.value().protect(); + } + + /// Undo [`to_thread_safe`]. JS-thread only. + #[inline] + pub fn unprotect(&self) { + self.unpin(); + self.value().unprotect(); + } + + /// Pin the backing `JSC::ArrayBuffer`, read its `vector()`/`byteLength()` + /// once, and cache the result; subsequent calls return the cache. For a + /// Rust-owned or already-pinned buffer this returns the existing + /// snapshot. A FastTypedArray has no backing `ArrayBuffer` to detach so it + /// is not pinned; a detached buffer yields `(null, 0)`. + /// + /// Must run on the JS thread for the first call on a JS-backed buffer; + /// callers that hand the buffer to a threadpool must call this (directly + /// or via [`slice`]) before the hand-off. + pub fn bytes(&self) -> (*mut u8, usize) { + self.bytes_::() + } + + fn bytes_(&self) -> (*mut u8, usize) { + let mut ab = self.buffer.get(); + if (self.settled.get() && (!FORCE_PIN || self.pinned.get())) || ab.value.is_empty() { + return (ab.ptr, ab.byte_len); + } + let mut ptr: *mut u8 = core::ptr::null_mut(); + let mut len: usize = 0; + self.pinned.set(JSC__JSValue__pinAndReadArrayBufferBytes( + ab.value, FORCE_PIN, &mut ptr, &mut len, + )); + self.settled.set(true); + ab.ptr = ptr; + ab.byte_len = len; + self.buffer.set(ab); + (ptr, len) + } #[inline] pub fn slice(&self) -> &[u8] { - self.buffer.byte_slice() + let (ptr, len) = self.bytes(); + if ptr.is_null() { + return &[]; + } + // SAFETY: `ptr`/`len` describe the pinned JSC-owned backing store (or + // the Rust-owned allocation for `owns_buffer`), valid while `self` + // keeps the JSValue rooted / the allocation alive. + unsafe { core::slice::from_raw_parts(ptr, len) } } /// Releases the owned byte buffer if this `MarkedArrayBuffer` was created with an /// allocator (e.g. via `from_string`/`from_bytes`). Does not free the struct itself; /// `MarkedArrayBuffer` is passed and stored by value, so callers own its storage. pub fn destroy(&mut self) { - if self.owns_buffer { - self.owns_buffer = false; + if self.owns_buffer.replace(false) { // SAFETY: buffer.ptr was allocated by the global allocator (heap::alloc / allocator.dupe). - unsafe { bun_alloc::default_alloc::free(self.buffer.ptr.cast()) }; + unsafe { bun_alloc::default_alloc::free(self.buffer.get().ptr.cast()) }; } } @@ -903,21 +996,22 @@ impl MarkedArrayBuffer { // `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. - let mut buf = self.buffer; + let mut buf = self.buffer.get(); JSValue::create_buffer(global, buf.byte_slice_mut()) } pub fn to_js(&self, global: &JSGlobalObject) -> JsResult { - if !self.buffer.value.is_empty_or_undefined_or_null() { - return Ok(self.buffer.value); + let buffer = self.buffer.get(); + if !buffer.value.is_empty_or_undefined_or_null() { + return Ok(buffer.value); } - if self.buffer.byte_len == 0 { + if buffer.byte_len == 0 { // SAFETY: null `ptr` with `len == 0` and no deallocator — every // obligation of the callee's contract holds trivially. return unsafe { make_typed_array_with_bytes_no_copy( global, - self.buffer.typed_array_type.to_typed_array_type(), + buffer.typed_array_type.to_typed_array_type(), ptr::null_mut(), 0, None, @@ -932,16 +1026,23 @@ impl MarkedArrayBuffer { unsafe { make_typed_array_with_bytes_no_copy( global, - self.buffer.typed_array_type.to_typed_array_type(), - self.buffer.ptr.cast(), - self.buffer.byte_len, + buffer.typed_array_type.to_typed_array_type(), + buffer.ptr.cast(), + buffer.byte_len, Some(MarkedArrayBuffer_deallocator), - self.buffer.ptr.cast(), + buffer.ptr.cast(), ) } } } +impl Drop for MarkedArrayBuffer { + #[inline] + fn drop(&mut self) { + self.unpin(); + } +} + // ────────────────────────────────────────────────────────────────────────── // Deallocators // ────────────────────────────────────────────────────────────────────────── diff --git a/src/jsc/bindings/bindings.cpp b/src/jsc/bindings/bindings.cpp index d261ed095ccc..7379833f0a8c 100644 --- a/src/jsc/bindings/bindings.cpp +++ b/src/jsc/bindings/bindings.cpp @@ -3212,6 +3212,47 @@ bool JSC__JSValue__asArrayBuffer( return true; } +// Pin the backing ArrayBuffer (so it cannot be detached) and read its +// vector()/byteLength(). A FastTypedArray has no ArrayBuffer to detach so it +// is left unpinned (avoiding slowDownAndWasteMemory()) unless `force_pin`, +// which promotes it for an off-thread borrow. SharedArrayBuffer is never +// detachable and is left unpinned. Detached or non-buffer -> (nullptr, 0). +// Returns true iff a pin was taken that the caller must release with +// JSC__JSValue__unpinArrayBuffer. +CPP_DECL bool JSC__JSValue__pinAndReadArrayBufferBytes( + JSC::EncodedJSValue encodedValue, bool force_pin, uint8_t** out_ptr, size_t* out_byte_len) +{ + *out_ptr = nullptr; + *out_byte_len = 0; + JSC::JSValue value = JSC::JSValue::decode(encodedValue); + if (!value.isCell()) [[unlikely]] + return false; + if (auto* view = dynamicDowncast(value)) { + bool pinned = false; + if (force_pin || view->hasArrayBuffer()) { + if (auto* buf = view->possiblySharedBuffer(); buf && !buf->isShared()) { + buf->pin(); + pinned = true; + } + } + *out_ptr = static_cast(view->vector()); + *out_byte_len = view->byteLength(); + return pinned; + } + if (auto* jsBuffer = dynamicDowncast(value)) { + auto* buffer = jsBuffer->impl(); + bool pinned = false; + if (!buffer->isShared()) { + buffer->pin(); + pinned = true; + } + *out_ptr = static_cast(buffer->data()); + *out_byte_len = buffer->byteLength(); + return pinned; + } + return false; +} + // Pin/unpin the backing ArrayBuffer of a JSArrayBuffer or JSArrayBufferView so // its storage cannot move or be freed while a native borrower holds a slice // into it. SharedArrayBuffer is never detachable and never moves, so it is left diff --git a/src/jsc/bindings/headers.h b/src/jsc/bindings/headers.h index 6cd8cc0d5b2f..cee102f44d67 100644 --- a/src/jsc/bindings/headers.h +++ b/src/jsc/bindings/headers.h @@ -199,6 +199,7 @@ CPP_DECL uint32_t JSC__JSMap__size(JSC::JSMap* arg0, JSC::JSGlobalObject* arg1); CPP_DECL void JSC__JSValue__then(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1, JSC::EncodedJSValue JSValue2, SYSV_ABI JSC::EncodedJSValue(* ArgFn3)(JSC::JSGlobalObject* arg0, JSC::CallFrame* arg1), SYSV_ABI JSC::EncodedJSValue(* ArgFn4)(JSC::JSGlobalObject* arg0, JSC::CallFrame* arg1)); CPP_DECL bool JSC__JSValue__asArrayBuffer(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1, Bun__ArrayBuffer* arg2); +CPP_DECL bool JSC__JSValue__pinAndReadArrayBufferBytes(JSC::EncodedJSValue JSValue0, bool arg1, uint8_t** arg2, size_t* arg3); CPP_DECL unsigned char JSC__JSValue__asBigIntCompare(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1, JSC::EncodedJSValue JSValue2); CPP_DECL JSC::JSPromise* JSC__JSValue__asInternalPromise(JSC::EncodedJSValue JSValue0); CPP_DECL JSC::JSPromise* JSC__JSValue__asPromise(JSC::EncodedJSValue JSValue0); diff --git a/src/jsc/node_path.rs b/src/jsc/node_path.rs index 84e7d12b6d05..20f42fd0e204 100644 --- a/src/jsc/node_path.rs +++ b/src/jsc/node_path.rs @@ -114,13 +114,9 @@ impl Clone for PathLike { } else { s.borrow() }), - Self::Buffer(b) => Self::Buffer(MarkedArrayBuffer { - buffer: b.buffer, - // The clone borrows the JS-owned backing store; only the - // original (if any) owns the allocation. - owns_buffer: false, - pinned: false, - }), + // The clone borrows the JS-owned backing store; only the + // original (if any) owns the allocation or the pin. + Self::Buffer(b) => Self::Buffer(b.borrow()), Self::SliceWithUnderlyingString(s) => { // `dupe_ref()` alone leaves `utf8` empty (lib.rs:1603) — a // cloned PathLike would then return b"" from `slice()`. Clone @@ -150,12 +146,8 @@ impl Drop for PathLike { // `CowSlice` frees its backing in its own `Drop` iff it owns it; // a borrowed path is a no-op. Self::String(_) => {} - Self::Buffer(b) => { - if b.pinned { - b.pinned = false; - b.buffer.unpin(); - } - } + // `MarkedArrayBuffer::Drop` releases the pin. + Self::Buffer(_) => {} Self::SliceWithUnderlyingString(s) | Self::ThreadsafeString(s) => { core::mem::take(s).deinit(); } @@ -208,7 +200,7 @@ impl PathLike { *self = Self::ThreadsafeString(owned); } Self::Buffer(b) => { - b.buffer.value.protect(); + b.to_thread_safe(); } Self::String(_) | Self::ThreadsafeString(_) | Self::EncodedSlice(_) => {} } @@ -222,7 +214,7 @@ impl Unprotect for PathLike { #[inline] fn unprotect(&mut self) { if let Self::Buffer(b) = self { - b.buffer.value.unprotect(); + b.unprotect(); } } } diff --git a/src/runtime/api/BunObject.rs b/src/runtime/api/BunObject.rs index 1b5c317c1e02..0682bb8f74d7 100644 --- a/src/runtime/api/BunObject.rs +++ b/src/runtime/api/BunObject.rs @@ -170,7 +170,7 @@ mod static_adapters { // re-enters the VM). let _a0_guard = a0.protected(); let _a1_guard = a1.protected(); - let mut output = if a1.is_undefined_or_null() { + let output = if a1.is_undefined_or_null() { None } else { StringOrBuffer::from_js(g, a1)? @@ -180,9 +180,6 @@ mod static_adapters { "expected string, buffer, TypedArray, or Blob", ))); }; - if let Some(StringOrBuffer::Buffer(buffer)) = &mut output { - buffer.buffer = ArrayBuffer::from_typed_array(g, buffer.buffer.value); - } Crypto::SHA512_256::hash_(g, &input, output) } } diff --git a/src/runtime/api/MarkdownObject.rs b/src/runtime/api/MarkdownObject.rs index 525f590bdc70..f68c944356fd 100644 --- a/src/runtime/api/MarkdownObject.rs +++ b/src/runtime/api/MarkdownObject.rs @@ -79,7 +79,7 @@ impl PinnedView { let Some(b) = buffer.buffer() else { return Ok(None); }; - match b.buffer.value.as_pinned_arraybuffer(global) { + match b.value().as_pinned_arraybuffer(global) { Some(pinned) => Ok(Some(Self(pinned))), None => Err(global.throw_out_of_memory()), } diff --git a/src/runtime/api/bun/subprocess/Readable.rs b/src/runtime/api/bun/subprocess/Readable.rs index 61d62db74118..60a4ea3a25ef 100644 --- a/src/runtime/api/bun/subprocess/Readable.rs +++ b/src/runtime/api/bun/subprocess/Readable.rs @@ -309,12 +309,13 @@ impl Readable { // Ownership of the mimalloc-backed buffer transfers to JSC // (freed via `MarkedArrayBuffer_deallocator`). - Ok(jsc::MarkedArrayBuffer { - buffer: jsc::ArrayBuffer::from_owned_bytes(own, jsc::JSType::Uint8Array), - owns_buffer: true, - pinned: false, - } - .to_node_buffer(global)) + Ok( + jsc::MarkedArrayBuffer::from_owned(jsc::ArrayBuffer::from_owned_bytes( + own, + jsc::JSType::Uint8Array, + )) + .to_node_buffer(global), + ) } _ => Ok(JSValue::UNDEFINED), } diff --git a/src/runtime/crypto/CryptoHasher.rs b/src/runtime/crypto/CryptoHasher.rs index debf7bd6f0b6..dbbe1d0be01a 100644 --- a/src/runtime/crypto/CryptoHasher.rs +++ b/src/runtime/crypto/CryptoHasher.rs @@ -12,7 +12,7 @@ use bun_jsc::{ use crate::crypto::evp::{AlgorithmExt as _, EVP}; use crate::crypto::{HMAC, create_crypto_error, evp}; use crate::generated_classes::PropertyName; -use crate::node::{BlobOrStringOrBuffer, Encoding, StringOrBuffer}; +use crate::node::{BlobOrStringOrBuffer, Buffer, Encoding, StringOrBuffer}; use bun_sha_hmac::sha as hashers; // sha3/blake2 algorithms with no BoringSSL streaming context: @@ -271,7 +271,7 @@ impl CryptoHasher { }; // ?Node.StringOrBuffer (static-method arm: only `undefined` → None) - let mut output: Option = match next_eat() { + let output: Option = match next_eat() { Some(arg) => match StringOrBuffer::from_js(global, arg)? { Some(v) => Some(v), None => { @@ -294,9 +294,6 @@ impl CryptoHasher { ); } }; - if let Some(StringOrBuffer::Buffer(buffer)) = &mut output { - buffer.buffer = ArrayBuffer::from_typed_array(global, buffer.buffer.value); - } Self::hash_(global, algorithm, &input, output) } @@ -376,7 +373,7 @@ impl CryptoHasher { global: &JSGlobalObject, evp: &mut EVP, input: &BlobOrStringOrBuffer, - output: Option, + output: Option<&Buffer>, ) -> JsResult { let mut output_digest_buf: Digest = [0u8; EVP_MAX_MD_SIZE_USIZE]; let mut output_digest_slice: &mut [u8] = &mut output_digest_buf; @@ -388,20 +385,19 @@ impl CryptoHasher { ))); } - if let Some(output_buf) = &output { + if let Some(output_buf) = output { let size = evp.size() as usize; - let bytes_len = output_buf.byte_slice().len(); + let (ptr, bytes_len) = output_buf.bytes(); if bytes_len < size { return Err(global.throw_invalid_arguments(format_args!( "TypedArray must be at least {} bytes", size ))); } - // SAFETY: `output_buf.ptr` is the JSC-owned writable backing store + // SAFETY: `ptr` is the pinned JSC-owned writable backing store // (`bytes_len >= size` checked above; not detached since len > 0); - // borrowed for this frame only. Build the `&mut` directly from the - // raw `*mut u8` field — never via `&[u8].as_ptr()` (Stacked-Borrows UB). - output_digest_slice = unsafe { core::slice::from_raw_parts_mut(output_buf.ptr, size) }; + // borrowed for this frame only. + output_digest_slice = unsafe { core::slice::from_raw_parts_mut(ptr, size) }; } let Some(len) = evp.hash(boring_engine(global), input.slice(), output_digest_slice) else { @@ -412,7 +408,7 @@ impl CryptoHasher { }; if let Some(output_buf) = output { - Ok(output_buf.value) + Ok(output_buf.value()) } else { // Clone to GC-managed memory ArrayBuffer::create_buffer(global, &output_digest_slice[0..len as usize]) @@ -441,8 +437,7 @@ impl CryptoHasher { if let Some(string_or_buffer) = output { if let StringOrBuffer::Buffer(buffer) = &string_or_buffer { - let ab = buffer.buffer; - return Self::hash_to_bytes(global, &mut evp, input, Some(ab)); + return Self::hash_to_bytes(global, &mut evp, input, Some(buffer)); } // `inline else => |*str|` — every non-buffer arm yields a string-like // `defer str.deinit()` — handled by Drop. @@ -690,8 +685,7 @@ impl CryptoHasher { ) -> JsResult { if let Some(string_or_buffer) = output { if let StringOrBuffer::Buffer(buffer) = &string_or_buffer { - let ab = buffer.buffer; - return this.digest_to_bytes(global, Some(ab)); + return this.digest_to_bytes(global, Some(buffer)); } // `defer str.deinit()` — handled by Drop. let Some(encoding) = Encoding::from(string_or_buffer.slice()) else { @@ -715,26 +709,22 @@ impl CryptoHasher { fn digest_to_bytes( &self, global: &JSGlobalObject, - output: Option, + output: Option<&Buffer>, ) -> JsResult { let mut output_digest_buf: evp::Digest = [0u8; EVP_MAX_MD_SIZE_USIZE]; let buf_len = output_digest_buf.len(); let output_digest_slice: &mut [u8]; - if let Some(output_buf) = &output { - let bytes_len = output_buf.byte_slice().len(); + if let Some(output_buf) = output { + let (ptr, bytes_len) = output_buf.bytes(); if bytes_len < buf_len { return Err(global.throw_invalid_arguments(format_args!( "TypedArray must be at least {} bytes", boring_ssl::EVP_MAX_MD_SIZE ))); } - // Reshaped for borrowck. - // SAFETY: `bytes_len >= EVP_MAX_MD_SIZE` checked above; `output_buf.ptr` - // is the JSC-owned writable backing store, outliving this frame. Build - // the `&mut` directly from the raw `*mut u8` field — never via - // `&[u8].as_ptr()` (Stacked-Borrows UB). - output_digest_slice = - unsafe { core::slice::from_raw_parts_mut(output_buf.ptr, bytes_len) }; + // SAFETY: `bytes_len >= EVP_MAX_MD_SIZE` checked above; `ptr` is the + // pinned JSC-owned writable backing store, outliving this frame. + output_digest_slice = unsafe { core::slice::from_raw_parts_mut(ptr, bytes_len) }; } else { output_digest_slice = &mut output_digest_buf; } @@ -748,7 +738,7 @@ impl CryptoHasher { } if let Some(output_buf) = output { - Ok(output_buf.value) + Ok(output_buf.value()) } else { // Clone to GC-managed memory ArrayBuffer::create_buffer(global, result) @@ -930,8 +920,7 @@ impl CryptoHasherZig { ) -> JsResult { if let Some(string_or_buffer) = output { if let StringOrBuffer::Buffer(buffer) = &string_or_buffer { - let ab = buffer.buffer; - return Self::hash_by_name_inner_to_bytes::(global, input, Some(ab)); + return Self::hash_by_name_inner_to_bytes::(global, input, Some(buffer)); } let Some(encoding) = Encoding::from(string_or_buffer.slice()) else { return Err(global @@ -983,7 +972,7 @@ impl CryptoHasherZig { fn hash_by_name_inner_to_bytes( global: &JSGlobalObject, input: &BlobOrStringOrBuffer, - output: Option, + output: Option<&Buffer>, ) -> JsResult { // `defer input.deinit()` — handled by Drop. @@ -996,26 +985,21 @@ impl CryptoHasherZig { let mut h = A::init(); let digest_length_comptime = A::DIGEST_LENGTH as usize; - if let Some(output_buf) = &output { - if output_buf.byte_slice().len() < digest_length_comptime { + h.update(input.slice()); + + if let Some(output_buf) = output { + let (ptr, bytes_len) = output_buf.bytes(); + if bytes_len < digest_length_comptime { return Err(global.throw_invalid_arguments(format_args!( "TypedArray must be at least {} bytes", digest_length_comptime ))); } - } - - h.update(input.slice()); - - if let Some(output_buf) = output { - // SAFETY: length checked above; `output_buf.ptr` is the JSC-owned - // writable backing store, outliving this frame. Build the `&mut` - // directly from the raw `*mut u8` field — never via `&[u8].as_ptr()` - // (Stacked-Borrows UB). - let out = - unsafe { core::slice::from_raw_parts_mut(output_buf.ptr, digest_length_comptime) }; + // SAFETY: length checked above; `ptr` is the pinned JSC-owned + // writable backing store, outliving this frame. + let out = unsafe { core::slice::from_raw_parts_mut(ptr, digest_length_comptime) }; h.final_(out); - Ok(output_buf.value) + Ok(output_buf.value()) } else { let mut out = [0u8; EVP_MAX_MD_SIZE_USIZE]; h.final_(&mut out[..digest_length_comptime]); @@ -1260,7 +1244,7 @@ impl StaticCryptoHasher { }; // ?Node.StringOrBuffer (static-method arm: only `undefined` → None) - let mut output: Option = match next_eat() { + let output: Option = match next_eat() { Some(arg) => match StringOrBuffer::from_js(global, arg)? { Some(v) => Some(v), None => { @@ -1283,9 +1267,6 @@ impl StaticCryptoHasher { ); } }; - if let Some(StringOrBuffer::Buffer(buffer)) = &mut output { - buffer.buffer = ArrayBuffer::from_typed_array(global, buffer.buffer.value); - } Self::hash_(global, &input, output) } @@ -1331,12 +1312,12 @@ impl StaticCryptoHasher { fn hash_to_bytes( global: &JSGlobalObject, input: &BlobOrStringOrBuffer, - output: Option, + output: Option<&Buffer>, ) -> JsResult { let mut output_digest_buf: H::Digest = H::new_digest(); let output_digest_slice: &mut H::Digest; - if let Some(output_buf) = &output { - let bytes_len = output_buf.byte_slice().len(); + if let Some(output_buf) = output { + let (ptr, bytes_len) = output_buf.bytes(); if bytes_len < H::DIGEST { return Err(global.throw_invalid_arguments(format_args!( "TypedArray must be at least {} bytes", @@ -1344,10 +1325,8 @@ impl StaticCryptoHasher { ))); } // SAFETY: `bytes_len >= H::DIGEST` checked above; `H::Digest = [u8; H::DIGEST]`; - // `output_buf.ptr` is the JSC-owned writable backing store. Build the - // `&mut` directly from the raw `*mut u8` field — never via - // `&[u8].as_ptr()` (Stacked-Borrows UB). - output_digest_slice = unsafe { &mut *output_buf.ptr.cast::() }; + // `ptr` is the pinned JSC-owned writable backing store. + output_digest_slice = unsafe { &mut *ptr.cast::() }; } else { output_digest_slice = &mut output_digest_buf; } @@ -1363,7 +1342,7 @@ impl StaticCryptoHasher { } if let Some(output_buf) = output { - Ok(output_buf.value) + Ok(output_buf.value()) } else { ArrayBuffer::create_uint8_array(global, output_digest_slice.as_ref()) } @@ -1384,8 +1363,7 @@ impl StaticCryptoHasher { if let Some(string_or_buffer) = output { if let StringOrBuffer::Buffer(buffer) = &string_or_buffer { - let ab = buffer.buffer; - return Self::hash_to_bytes(global, input, Some(ab)); + return Self::hash_to_bytes(global, input, Some(buffer)); } let Some(encoding) = Encoding::from(string_or_buffer.slice()) else { return Err(global @@ -1474,8 +1452,7 @@ impl StaticCryptoHasher { } if let Some(string_or_buffer) = output { if let StringOrBuffer::Buffer(buffer) = &string_or_buffer { - let ab = buffer.buffer; - return this.digest_to_bytes(global, Some(ab)); + return this.digest_to_bytes(global, Some(buffer)); } let Some(encoding) = Encoding::from(string_or_buffer.slice()) else { return Err(global @@ -1498,12 +1475,12 @@ impl StaticCryptoHasher { fn digest_to_bytes( &self, global: &JSGlobalObject, - output: Option, + output: Option<&Buffer>, ) -> JsResult { let mut output_digest_buf: H::Digest = H::new_digest(); let output_digest_slice: &mut H::Digest; - if let Some(output_buf) = &output { - let bytes_len = output_buf.byte_slice().len(); + if let Some(output_buf) = output { + let (ptr, bytes_len) = output_buf.bytes(); if bytes_len < H::DIGEST { return Err(global.throw_invalid_arguments(format_args!( "TypedArray must be at least {} bytes", @@ -1511,10 +1488,8 @@ impl StaticCryptoHasher { ))); } // SAFETY: `bytes_len >= H::DIGEST`; `H::Digest = [u8; H::DIGEST]`; - // `output_buf.ptr` is the JSC-owned writable backing store. Build the - // `&mut` directly from the raw `*mut u8` field — never via - // `&[u8].as_ptr()` (Stacked-Borrows UB). - output_digest_slice = unsafe { &mut *output_buf.ptr.cast::() }; + // `ptr` is the pinned JSC-owned writable backing store. + output_digest_slice = unsafe { &mut *ptr.cast::() }; } else { output_digest_slice = &mut output_digest_buf; } @@ -1523,7 +1498,7 @@ impl StaticCryptoHasher { self.digested.set(true); if let Some(output_buf) = output { - Ok(output_buf.value) + Ok(output_buf.value()) } else { ArrayBuffer::create_uint8_array(global, output_digest_buf.as_ref()) } diff --git a/src/runtime/crypto/PBKDF2.rs b/src/runtime/crypto/PBKDF2.rs index 05030cafd414..ec70824474b1 100644 --- a/src/runtime/crypto/PBKDF2.rs +++ b/src/runtime/crypto/PBKDF2.rs @@ -2,8 +2,7 @@ use core::ffi::c_uint; use bun_boringssl_sys as boringssl; use bun_jsc::{ - AnyTaskJob, AnyTaskJobCtx, ArrayBuffer, CallFrame, JSGlobalObject, JSPromiseStrong, JSValue, - JsResult, + AnyTaskJob, AnyTaskJobCtx, CallFrame, JSGlobalObject, JSPromiseStrong, JSValue, JsResult, }; use crate::node::StringOrBuffer; @@ -216,10 +215,6 @@ impl PBKDF2 { } }; - if guard.salt.slice().len() > i32::MAX as usize { - return Err(global_this.throw_invalid_arguments(format_args!("salt is too long"))); - } - guard.password = match StringOrBuffer::from_js_maybe_async( global_this, arg0, @@ -236,14 +231,12 @@ impl PBKDF2 { } }; - if guard.password.slice().len() > i32::MAX as usize { - return Err(global_this.throw_invalid_arguments(format_args!("password is too long"))); + if guard.salt.slice().len() > i32::MAX as usize { + return Err(global_this.throw_invalid_arguments(format_args!("salt is too long"))); } - if !is_async { - if let StringOrBuffer::Buffer(buffer) = &mut guard.salt { - buffer.buffer = ArrayBuffer::from_typed_array(global_this, buffer.buffer.value); - } + if guard.password.slice().len() > i32::MAX as usize { + return Err(global_this.throw_invalid_arguments(format_args!("password is too long"))); } if is_async { diff --git a/src/runtime/crypto/PasswordObject.rs b/src/runtime/crypto/PasswordObject.rs index 806b682bc967..d299f9b3d4af 100644 --- a/src/runtime/crypto/PasswordObject.rs +++ b/src/runtime/crypto/PasswordObject.rs @@ -5,8 +5,7 @@ use std::io::Write as _; use bun_core::ZigString; use bun_io::KeepAlive; use bun_jsc::{ - self as jsc, ArrayBuffer, CallFrame, JSFunction, JSGlobalObject, JSValue, JsError, JsResult, - WorkPoolTask, + self as jsc, CallFrame, JSFunction, JSGlobalObject, JSValue, JsError, JsResult, WorkPoolTask, }; // `bun_jsc::{AnyTask, ConcurrentTask, EventLoop}` are *modules* (re-exported from // `bun_event_loop`); pull the concrete types out by name. @@ -909,7 +908,7 @@ fn js_password_object_verify_sync( }; } - let Some(mut password) = StringOrBuffer::from_js(global_object, arguments[0])? else { + let Some(password) = StringOrBuffer::from_js(global_object, arguments[0])? else { return Err(global_object.throw_invalid_argument_type( "verify", "password", @@ -926,10 +925,6 @@ fn js_password_object_verify_sync( )); }; - if let StringOrBuffer::Buffer(buffer) = &mut password { - buffer.buffer = ArrayBuffer::from_typed_array(global_object, buffer.buffer.value); - } - // defer password.deinit() / hash_.deinit() — Drop at scope exit. if hash_.slice().is_empty() { diff --git a/src/runtime/node/node_crypto_binding.rs b/src/runtime/node/node_crypto_binding.rs index b52e71deb6c3..98027d5d2e0b 100644 --- a/src/runtime/node/node_crypto_binding.rs +++ b/src/runtime/node/node_crypto_binding.rs @@ -979,18 +979,12 @@ mod _impl { ctx.check_scrypt_params(global)?; - let mut ctx = scopeguard::ScopeGuard::into_inner(ctx); + let ctx = scopeguard::ScopeGuard::into_inner(ctx); if IS_ASYNC { return Ok((ctx, callback)); } - for input in [&mut ctx.password, &mut ctx.salt] { - if let StringOrBuffer::Buffer(buffer) = input { - buffer.buffer = ArrayBuffer::from_typed_array(global, buffer.buffer.value); - } - } - Ok((ctx, JSValue::UNDEFINED)) } diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index 55aa96e4bf4b..eeb9d87da867 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -3550,11 +3550,7 @@ pub mod args { impl Default for MkdirTemp { fn default() -> Self { Self { - prefix: PathLike::Buffer(Buffer { - buffer: bun_jsc::ArrayBuffer::EMPTY, - owns_buffer: false, - pinned: false, - }), + prefix: PathLike::Buffer(Buffer::EMPTY), encoding: Encoding::Utf8, } } @@ -3904,15 +3900,6 @@ pub mod args { } } } - if arguments.will_be_async && matches!(args.buffer, StringOrBuffer::Buffer(_)) { - if let Some(pinned) = bv.as_pinned_arraybuffer(ctx) { - args.buffer = StringOrBuffer::Buffer(Buffer { - buffer: pinned, - owns_buffer: false, - pinned: true, - }); - } - } Ok(args) } } @@ -3923,22 +3910,16 @@ pub mod args { pub offset: u64, pub(crate) length: u64, pub(crate) position: Option, - /// True when `from_js` pinned `buffer` for the async path; balanced in - /// `unprotect()` (the JS-thread release hook). - pub(crate) pinned: bool, } impl Read { pub(crate) fn to_thread_safe(&self) { - self.buffer.buffer.value.protect(); + self.buffer.to_thread_safe(); } } impl Unprotect for Read { #[inline] fn unprotect(&mut self) { - if self.pinned { - self.buffer.buffer.unpin(); - } - self.buffer.buffer.value.unprotect(); + self.buffer.unprotect(); } } impl Read { @@ -3995,7 +3976,6 @@ pub mod args { length: 0, offset: 0, position: None, - pinned: false, }); } @@ -4107,29 +4087,12 @@ pub mod args { None }; - let (buffer, pinned) = if arguments.will_be_async { - match buffer_value.as_pinned_arraybuffer(ctx) { - Some(pinned) => ( - Buffer { - buffer: pinned, - owns_buffer: false, - pinned: true, - }, - true, - ), - None => (buffer, false), - } - } else { - (buffer, false) - }; - Ok(Read { fd, buffer, offset, length, position, - pinned, }) } } @@ -6116,11 +6079,10 @@ impl NodeFS { fn read_inner(&mut self, args: &args::Read) -> Maybe { debug_assert!(args.position.is_none()); - // `ArrayBuffer` is a `Copy` descriptor over JSC-owned heap bytes; copy the - // descriptor locally and use the existing safe `byte_slice_mut` accessor - // instead of rebuilding a `&mut [u8]` from a `&[u8]` borrow by hand. - let mut view = args.buffer.buffer; - let mut buf = view.byte_slice_mut(); + let (ptr, len) = args.buffer.bytes(); + // SAFETY: `ptr`/`len` describe the JSC-owned backing store pinned by + // `bytes()` above (or by `to_thread_safe()` on the async path). + let mut buf = unsafe { bun_core::ffi::slice_mut(ptr, len) }; let off = (args.offset as usize).min(buf.len()); buf = &mut buf[off..]; let l = (args.length as usize).min(buf.len()); @@ -6134,9 +6096,9 @@ impl NodeFS { } fn pread_inner(&mut self, args: &args::Read) -> Maybe { - // See `read_inner` — copy the `ArrayBuffer` descriptor and use its safe accessor. - let mut view = args.buffer.buffer; - let mut buf = view.byte_slice_mut(); + let (ptr, len) = args.buffer.bytes(); + // SAFETY: see `read_inner`. + let mut buf = unsafe { bun_core::ffi::slice_mut(ptr, len) }; let off = (args.offset as usize).min(buf.len()); buf = &mut buf[off..]; let l = (args.length as usize).min(buf.len()); @@ -7289,11 +7251,7 @@ impl NodeFS { array_buffer.ensure_still_alive(); return match array_buffer.as_array_buffer(global) { Some(buffer) => Ok(ret::ReadFileWithOptions::Buffer( - bun_jsc::MarkedArrayBuffer { - buffer, - owns_buffer: false, - pinned: false, - }, + bun_jsc::MarkedArrayBuffer::from_unpinned(buffer), )), // This case shouldn't really happen. None => Err(with_path_like( diff --git a/src/runtime/node/types.rs b/src/runtime/node/types.rs index 3c18a2543297..e1625e34fb89 100644 --- a/src/runtime/node/types.rs +++ b/src/runtime/node/types.rs @@ -299,11 +299,7 @@ impl bun_jsc::Unprotect for StringOrBuffer { #[inline] fn unprotect(&mut self) { if let Self::Buffer(buffer) = self { - if buffer.pinned { - buffer.pinned = false; - buffer.buffer.unpin(); - } - buffer.buffer.value.unprotect(); + buffer.unprotect(); } } } @@ -319,7 +315,7 @@ impl StringOrBuffer { Self::ThreadsafeString(_) => {} Self::EncodedSlice(_) => {} Self::Buffer(buffer) => { - buffer.buffer.value.protect(); + buffer.to_thread_safe(); } } } @@ -353,8 +349,9 @@ impl StringOrBuffer { result } Self::Buffer(buffer) => { - if buffer.buffer.value != JSValue::ZERO { - return Ok(buffer.buffer.value); + let value = buffer.value(); + if value != JSValue::ZERO { + return Ok(value); } Ok(buffer.to_node_buffer(ctx)) } @@ -437,15 +434,10 @@ impl StringOrBuffer { | JSType::BigInt64Array | JSType::BigUint64Array | JSType::DataView => { - let buffer = if is_async { - Buffer::from_js_pinned(global, value) - .unwrap_or_else(|| Buffer::from_array_buffer(global, value)) - } else { - Buffer::from_array_buffer(global, value) - }; + let buffer = Buffer::from_array_buffer(global, value); if is_async { - buffer.buffer.value.protect(); + buffer.to_thread_safe(); } *out = Self::Buffer(buffer); @@ -508,14 +500,9 @@ impl StringOrBuffer { allow_string_object: bool, ) -> JsResult { if value.is_cell() && value.js_type().is_array_buffer_like() { - let buffer = if is_async { - Buffer::from_js_pinned(global, value) - .unwrap_or_else(|| Buffer::from_array_buffer(global, value)) - } else { - Buffer::from_array_buffer(global, value) - }; + let buffer = Buffer::from_array_buffer(global, value); if is_async { - buffer.buffer.value.protect(); + buffer.to_thread_safe(); } *out = Self::Buffer(buffer); return Ok(true); @@ -1142,35 +1129,10 @@ impl PathLikeExt for PathLike { }; use jsc::JSType; match arg.js_type() { - JSType::Uint8Array | JSType::DataView => { - let mut buffer = Buffer::from_js_pinned(ctx, arg) - .unwrap_or_else(|| Buffer::from_typed_array(ctx, arg)); - if let Err(err) = Valid::path_buffer(&buffer, ctx) - .and_then(|_| Valid::path_null_bytes(buffer.slice(), ctx)) - { - if buffer.pinned { - buffer.pinned = false; - buffer.buffer.unpin(); - } - return Err(err); - } - - arguments.protect_eat(); - Ok(Some(Self::Buffer(buffer))) - } - - JSType::ArrayBuffer => { - let mut buffer = Buffer::from_js_pinned(ctx, arg) - .unwrap_or_else(|| Buffer::from_array_buffer(ctx, arg)); - if let Err(err) = Valid::path_buffer(&buffer, ctx) - .and_then(|_| Valid::path_null_bytes(buffer.slice(), ctx)) - { - if buffer.pinned { - buffer.pinned = false; - buffer.buffer.unpin(); - } - return Err(err); - } + JSType::Uint8Array | JSType::DataView | JSType::ArrayBuffer => { + let buffer = Buffer::from_array_buffer(ctx, arg); + Valid::path_buffer(&buffer, ctx) + .and_then(|_| Valid::path_null_bytes(buffer.slice(), ctx))?; arguments.protect_eat(); Ok(Some(Self::Buffer(buffer))) diff --git a/test/js/valkey/valkey-gc.test.ts b/test/js/valkey/valkey-gc.test.ts index 4e57b472cae0..eeb75b3f57b8 100644 --- a/test/js/valkey/valkey-gc.test.ts +++ b/test/js/valkey/valkey-gc.test.ts @@ -618,3 +618,63 @@ test.concurrent("getBuffer replies survive GC with adopted backing stores intact expect(proc.signalCode).toBeNull(); expect(exitCode).toBe(0); }); + +// MarkedArrayBuffer pins lazily: on the sync path StringOrBuffer::from_js +// captures only the JSValue, and the first slice() call pins + reads +// vector()/byteLength(). So when the second argument's toString() transfers +// the first argument's buffer before the command is serialized, the key is +// observed as zero-length rather than serialized from a stale (freed) pointer. +test.concurrent("redis.set reads a Buffer key only after every later argument has been coerced", async () => { + const src = ` + const net = require("node:net"); + let received = Buffer.alloc(0); + const server = net.createServer(sock => { + sock.on("data", chunk => { + received = Buffer.concat([received, chunk]); + // Reply +OK to whatever arrives so the client resolves. + sock.write("+OK\\r\\n"); + }); + }); + await new Promise(r => server.listen(0, r)); + const url = "redis://127.0.0.1:" + server.address().port; + + const client = new Bun.RedisClient(url, { enableAutoPipelining: false }); + const key = Buffer.from(new ArrayBuffer(19)); + try { + await client.connect(); + + key.set(Buffer.from("lazy-pin-test:key-A")); + class DetachKey extends String { + toString() { + structuredClone(key.buffer, { transfer: [key.buffer] }); + Bun.gc(true); + return "the-value"; + } + } + await client.set(key, new DetachKey("x")); + } finally { + client.close(); + server.close(); + } + + // RESP: *3\\r\\n$3\\r\\nSET\\r\\n$\\r\\n\\r\\n$9\\r\\nthe-value\\r\\n + const text = received.toString("latin1"); + const m = /\\$3\\r\\nSET\\r\\n\\$(\\d+)\\r\\n/.exec(text); + if (!m) throw new Error("no SET in wire bytes: " + JSON.stringify(text)); + console.log(JSON.stringify({ detached: key.byteLength === 0, keyLen: Number(m[1]) })); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", src], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(JSON.parse(stdout.trim())).toEqual({ detached: true, keyLen: 0 }); + expect(proc.signalCode).toBeNull(); + expect(exitCode).toBe(0); +});