diff --git a/changelog.d/honest-external-buffer.md b/changelog.d/honest-external-buffer.md new file mode 100644 index 00000000000..f6410a6c68d --- /dev/null +++ b/changelog.d/honest-external-buffer.md @@ -0,0 +1 @@ +External buffer wrappers carry their native byte pointer and Node-API finalizer in the GC buffer cell. Remove the legacy external-buffer registration API and address tables, retain byte-address identity, release native resources after collection or environment shutdown, and reject native-backed wrappers and views by name at worker transfer. diff --git a/crates/perry-codegen/src/gc_effects/linux-x86_64.tsv b/crates/perry-codegen/src/gc_effects/linux-x86_64.tsv index 602ee210d8f..5792444b611 100644 --- a/crates/perry-codegen/src/gc_effects/linux-x86_64.tsv +++ b/crates/perry-codegen/src/gc_effects/linux-x86_64.tsv @@ -445,7 +445,6 @@ js_buffer_read_uint32_le Reenters js_buffer_read_uint8 Reenters js_buffer_read_uint_be Reenters js_buffer_read_uint_le Reenters -js_buffer_register_external Leaf js_buffer_resolve_object_url Leaf js_buffer_set Leaf js_buffer_set_from Reenters diff --git a/crates/perry-codegen/src/gc_effects/macos-aarch64.tsv b/crates/perry-codegen/src/gc_effects/macos-aarch64.tsv index 56567a8030d..ee8b2811dc1 100644 --- a/crates/perry-codegen/src/gc_effects/macos-aarch64.tsv +++ b/crates/perry-codegen/src/gc_effects/macos-aarch64.tsv @@ -445,7 +445,6 @@ js_buffer_read_uint32_le Reenters js_buffer_read_uint8 Reenters js_buffer_read_uint_be Reenters js_buffer_read_uint_le Reenters -js_buffer_register_external Leaf js_buffer_resolve_object_url Leaf js_buffer_set Leaf js_buffer_set_from Reenters diff --git a/crates/perry-codegen/src/gc_effects/windows-x86_64.tsv b/crates/perry-codegen/src/gc_effects/windows-x86_64.tsv index fd131502863..e938fc842bf 100644 --- a/crates/perry-codegen/src/gc_effects/windows-x86_64.tsv +++ b/crates/perry-codegen/src/gc_effects/windows-x86_64.tsv @@ -445,7 +445,6 @@ js_buffer_read_uint32_le Reenters js_buffer_read_uint8 Reenters js_buffer_read_uint_be Reenters js_buffer_read_uint_le Reenters -js_buffer_register_external Leaf js_buffer_resolve_object_url Leaf js_buffer_set Leaf js_buffer_set_from Reenters diff --git a/crates/perry-codegen/src/wasm32/runtime_abi.tsv b/crates/perry-codegen/src/wasm32/runtime_abi.tsv index 7ba42dc2ac0..7b52e81e266 100644 --- a/crates/perry-codegen/src/wasm32/runtime_abi.tsv +++ b/crates/perry-codegen/src/wasm32/runtime_abi.tsv @@ -440,7 +440,6 @@ js_buffer_read_uint32_le f64 f64,i32s js_buffer_read_uint8 f64 f64,i32s js_buffer_read_uint_be f64 f64,i32s,i32s js_buffer_read_uint_le f64 f64,i32s,i32s -js_buffer_register_external void usize js_buffer_resolve_object_url f64 f64 js_buffer_set void ptr,i32s,i32s js_buffer_set_from void ptr,ptr,i32s diff --git a/crates/perry-runtime/src/agent.rs b/crates/perry-runtime/src/agent.rs index ce7d5830e22..43f2871e4e4 100644 --- a/crates/perry-runtime/src/agent.rs +++ b/crates/perry-runtime/src/agent.rs @@ -155,6 +155,10 @@ pub fn retire_agent(id: AgentId) { id, PRIMARY_AGENT, "the primary agent outlives the process; it is never retired" ); + // Finish native-addon ownership while this agent's heap and loop are + // still live. TLS destruction cannot safely call back into the runtime. + #[cfg(feature = "node-api-host")] + crate::node_api_host::shutdown_current_env(); // turnloop P9: this agent may own a `turnloop::Loop`. Tear it down FIRST, // while the arena is still mapped and this thread can still run the // bindings' completion sinks. diff --git a/crates/perry-runtime/src/array/buffer_receiver.rs b/crates/perry-runtime/src/array/buffer_receiver.rs index 5ffc0af57c8..84f9b71fdb6 100644 --- a/crates/perry-runtime/src/array/buffer_receiver.rs +++ b/crates/perry-runtime/src/array/buffer_receiver.rs @@ -63,18 +63,9 @@ pub(crate) fn buffer_receiver_dispatch( // arena-backed `GC_TYPE_ARRAY` cannot be a registered Buffer, so an // ordinary `[1,2,3].map(…)` reaches NEITHER registry. // - // `arena_payload_has_gc_type` rather than a bare header-byte read (or an - // open-coded address floor): a Buffer comes in BOTH backings, and an - // EXTERNAL one — `EXTERNAL_BUFFER_REGISTRY`, `shared_sab::alloc_shared_sab` - // — has no `GcHeader` at all. The eight bytes below its payload are - // allocator bookkeeping and can read as any `obj_type`, `GC_TYPE_ARRAY` - // included. A bare tag read would therefore skip the probe for exactly the - // receiver this function exists to catch, silently and only sometimes. - // The predicate range-checks, rejects `HeapSpace::Unknown` for the HEADER - // address, and validates through `gc_type_info` before trusting the byte; - // it answers `false` for an external buffer, which falls through to the - // registry — the authoritative answer. See `array/header.rs`'s - // `array_receiver_gc_tag` doc (#8142). + // Prove ownership before trusting the tag. Process-global SABs have a + // real header but live outside this thread's arena; they still need the + // buffer classification below rather than an arena-only rejection. if unsafe { crate::typedarray::arena_payload_has_gc_type(addr, crate::gc::GC_TYPE_ARRAY) } { return None; } diff --git a/crates/perry-runtime/src/array/header.rs b/crates/perry-runtime/src/array/header.rs index f5649050f22..2e81febb99e 100644 --- a/crates/perry-runtime/src/array/header.rs +++ b/crates/perry-runtime/src/array/header.rs @@ -85,11 +85,9 @@ pub(crate) unsafe fn array_object_flags_resolved(arr: *const ArrayHeader) -> u16 /// These carry a genuine `GcHeader` with a correct `obj_type`; they are /// pinned rather than movable, which is a different property from being /// untracked. This is the population #8041 started nulling. -/// * **external** — `EXTERNAL_BUFFER_REGISTRY` / -/// `EXTERNAL_UINT8ARRAY_REGISTRY` addresses, plus -/// `shared_sab::alloc_shared_sab`'s `alloc_zeroed`. For these the eight -/// bytes below the payload really are allocator bookkeeping and can read -/// as any value. +/// * **process-global** — `shared_sab::alloc_shared_sab` owns a real header +/// outside the current thread's GC heap. Registration alone does not prove +/// membership in that heap. /// /// So the tag is authoritative only once the address is known to be /// arena-backed. `typedarray::arena_payload_has_gc_type` is the predicate diff --git a/crates/perry-runtime/src/buffer/header.rs b/crates/perry-runtime/src/buffer/header.rs index 5d72f0f5fba..32868627e27 100644 --- a/crates/perry-runtime/src/buffer/header.rs +++ b/crates/perry-runtime/src/buffer/header.rs @@ -30,16 +30,8 @@ use std::cell::{Cell, RefCell}; use std::collections::{HashMap, HashSet}; use std::sync::{Mutex, OnceLock}; -static EXTERNAL_BUFFER_REGISTRY: OnceLock>> = OnceLock::new(); -/// Latched true by the first external-buffer registration. Lets the hot -/// `is_registered_buffer` probe — which JSON.stringify runs for every pointer -/// value it serializes (#6009) — skip the registry mutex entirely in the -/// (overwhelmingly common) processes that never register an external buffer. -static EXTERNAL_BUFFERS_NONEMPTY: std::sync::atomic::AtomicBool = - std::sync::atomic::AtomicBool::new(false); static EXTERNAL_UINT8ARRAY_REGISTRY: OnceLock>> = OnceLock::new(); -/// Latched by the first external Uint8Array registration, exactly as -/// `EXTERNAL_BUFFERS_NONEMPTY` does for buffers. +/// Latched by the first external Uint8Array registration. /// /// Without it `is_uint8array_buffer_slow` took the global mutex on every /// thread-local MISS — that is, on every value that is not a Uint8Array — @@ -50,29 +42,15 @@ static EXTERNAL_UINT8ARRAYS_NONEMPTY: std::sync::atomic::AtomicBool = static EXTERNAL_CRYPTO_KEY_META_REGISTRY: OnceLock>> = OnceLock::new(); -fn external_buffers() -> &'static Mutex> { - crate::once_init::get_or_init(&EXTERNAL_BUFFER_REGISTRY, || Mutex::new(HashSet::new())) -} - -/// Diagnostic-only exact membership check for the legacy external-buffer ABI. -#[inline] -pub fn is_external_buffer(addr: usize) -> bool { - EXTERNAL_BUFFERS_NONEMPTY.load(std::sync::atomic::Ordering::Acquire) - && external_buffers() - .lock() - .map(|r| r.contains(&addr)) - .unwrap_or(false) -} - -/// Test probe (#11547): is `addr` in each of the three PROCESS-GLOBAL -/// external registries (`EXTERNAL_BUFFER_REGISTRY`, -/// `EXTERNAL_UINT8ARRAY_REGISTRY`, `EXTERNAL_CRYPTO_KEY_META_REGISTRY`)? +/// Test probe (#11547): is `addr` in each of the two PROCESS-GLOBAL +/// external registries (`EXTERNAL_UINT8ARRAY_REGISTRY`, +/// `EXTERNAL_CRYPTO_KEY_META_REGISTRY`)? /// /// Unlike `is_uint8array_buffer` / `crypto_key_meta`, which consult this /// thread's registries first, it touches no thread-local, so a thread-exit /// range hook may call it from a TLS destructor. Plain locks, no allocation. #[doc(hidden)] -pub fn external_registries_hold_for_test(addr: usize) -> [bool; 3] { +pub fn external_registries_hold_for_test(addr: usize) -> [bool; 2] { use std::sync::PoisonError; let in_set = |set: &OnceLock>>| { set.get().is_some_and(|s| { @@ -82,7 +60,6 @@ pub fn external_registries_hold_for_test(addr: usize) -> [bool; 3] { }) }; [ - in_set(&EXTERNAL_BUFFER_REGISTRY), in_set(&EXTERNAL_UINT8ARRAY_REGISTRY), EXTERNAL_CRYPTO_KEY_META_REGISTRY.get().is_some_and(|m| { m.lock() @@ -148,12 +125,6 @@ crate::perry_thread_local! { /// already there. Thread-local like the registry it guards, so there is /// no ordering to reason about. static BUFFER_ADDR_RANGE: Cell<(usize, usize)> = const { Cell::new((usize::MAX, 0)) }; - /// `BufferHeader` wrappers whose bytes live in memory owned by native - /// code. The wrapper itself is an ordinary, non-moving GC object; only - /// its data pointer is external. `bun:ffi.toArrayBuffer`/`toBuffer` use - /// this to expose native memory without copying it or taking ownership. - static FOREIGN_BACKING_REGISTRY: RefCell> = - RefCell::new(new_ptr_hash_map()); /// Buffers that were specifically created via `new Uint8Array(...)` — /// formatted as `Uint8Array(N) [ a, b, c ]` instead of ``. static UINT8ARRAY_FROM_CTOR: RefCell> = RefCell::new(new_ptr_hash_set()); @@ -259,9 +230,6 @@ static BUFFER_LIKE_EVER_REGISTERED: RegistryLatch = RegistryLatch::new(); /// /// It covers every table `is_registered_buffer_slow` consults: /// * `BUFFER_REGISTRY` — only `register_buffer` inserts, and it admits first; -/// * `EXTERNAL_BUFFER_REGISTRY` — both writers (`js_buffer_register_external` -/// and `js_buffer_mark_as_crypto_key_external`) route through -/// `register_buffer` with the same address first; /// * `shared_sab`'s process-global SAB registry — `alloc_shared_sab` calls /// [`note_buffer_like_registered`] with the backing address before it /// publishes. @@ -332,11 +300,6 @@ pub(crate) fn test_buffer_addr_window_bounds() -> Option<(usize, usize)> { BUFFER_LIKE_ADDR_WINDOW.bounds_for_tests() } -/// Avoid a thread-local map probe in `buffer_data{,_mut}` until the first -/// foreign-backed buffer is created. The latch is deliberately monotone; -/// these accessors are among the hottest paths in the runtime. -static FOREIGN_BACKING_EVER_REGISTERED: RegistryLatch = RegistryLatch::new(); - /// Arm the `is_registered_buffer` latch from outside this module. /// /// `shared_sab` publishes process-global backings that `is_registered_buffer` @@ -592,7 +555,7 @@ pub(crate) fn is_small_buf_slab_addr(_addr: usize) -> bool { pub fn is_registered_buffer(addr: usize) -> bool { // Nothing buffer-shaped has ever been registered anywhere in this process // ⟹ nothing to find, in one atomic load. `register_buffer` (which - // `js_buffer_register_external` also routes through) and + // foreign allocation also routes through) and // `shared_sab::alloc_shared_sab` both arm this latch before they publish. if BUFFER_LIKE_EVER_REGISTERED.is_idle() { return false; @@ -622,7 +585,7 @@ pub fn is_registered_buffer(addr: usize) -> bool { !is_registered_buffer_slow(addr), "BUFFER_LIKE_ADDR_WINDOW rejected {addr:#x}, but it IS a \ registered buffer. Some registration route reached \ - BUFFER_REGISTRY, the external-buffer registry or the \ + BUFFER_REGISTRY, the external-Uint8Array registry or the \ shared-SAB registry without calling \ `BUFFER_LIKE_ADDR_WINDOW.admit()` (via `register_buffer` or \ `note_buffer_like_registered`) first." @@ -665,14 +628,6 @@ fn is_registered_buffer_slow(addr: usize) -> bool { if addr >= lo && addr <= hi && BUFFER_REGISTRY.with(|r| r.borrow().contains(&addr)) { return true; } - if EXTERNAL_BUFFERS_NONEMPTY.load(std::sync::atomic::Ordering::Acquire) - && external_buffers() - .lock() - .map(|r| r.contains(&addr)) - .unwrap_or(false) - { - return true; - } // #4913: recognise a process-global SAB backing reached as a module-level // value on a thread that never locally registered it (see // `is_shared_array_buffer`). @@ -720,11 +675,6 @@ fn release_external_buffer_registries_in_freed_ranges( let _ = slot.compare_exchange(old, 0, Ordering::Relaxed, Ordering::Relaxed); } } - if let Some(set) = EXTERNAL_BUFFER_REGISTRY.get() { - set.lock() - .unwrap_or_else(PoisonError::into_inner) - .retain(|&addr| !freed.contains(addr)); - } if let Some(set) = EXTERNAL_UINT8ARRAY_REGISTRY.get() { set.lock() .unwrap_or_else(PoisonError::into_inner) @@ -748,24 +698,6 @@ fn register_thread_exit_hook() { }); } -#[no_mangle] -pub extern "C" fn js_buffer_register_external(addr: usize) { - register_thread_exit_hook(); - register_buffer(addr as *const BufferHeader); - // Latch BEFORE the insert: a concurrent `is_registered_buffer` that - // observed the latch after the insert-but-before-the-store window would - // skip the mutex and miss an already-registered buffer. - EXTERNAL_BUFFERS_NONEMPTY.store(true, std::sync::atomic::Ordering::Release); - if let Ok(mut r) = external_buffers().lock() { - r.insert(addr); - } - if crate::hot_diag::receiver_repr_on() { - crate::hot_diag::receiver_repr_note_constructed( - crate::hot_diag::ReceiverReprFamily::ExternalBuffer, - ); - } -} - #[no_mangle] pub extern "C" fn js_buffer_mark_as_uint8array_external(addr: usize) { mark_as_uint8array(addr); @@ -775,7 +707,7 @@ pub extern "C" fn js_buffer_mark_as_uint8array_external(addr: usize) { /// Insert into the process-global external-Uint8Array registry, arming /// `EXTERNAL_UINT8ARRAYS_NONEMPTY` first. /// -/// Latch BEFORE the insert, matching `js_buffer_register_external`: a probe +/// Latch BEFORE the insert: a probe /// that observed the latch in the insert-but-before-the-store window would /// skip the mutex and miss an already-registered address. /// @@ -861,11 +793,6 @@ pub extern "C" fn js_buffer_mark_as_crypto_key_external( register_buffer(addr as *const BufferHeader); mark_as_uint8array(addr); mark_as_crypto_key_with_flags(addr, algo, hash, kind, extractable != 0, usages, bit_length); - // Latch BEFORE the insert — see js_buffer_register_external. - EXTERNAL_BUFFERS_NONEMPTY.store(true, std::sync::atomic::Ordering::Release); - if let Ok(mut r) = external_buffers().lock() { - r.insert(addr); - } register_external_uint8array(addr); if let Ok(mut r) = external_crypto_keys().lock() { r.insert( @@ -968,8 +895,8 @@ pub fn asymmetric_key_meta(addr: usize) -> Option<(u8, u8)> { /// excluded — their allocation is only a header. Runtime reads resolve /// through `buffer_data` to the ultimate backing plus the view offset; /// * foreign-backed wrappers (`buffer_alloc_foreign`, bun:ffi externals) are -/// excluded at prime time — their header is a lone `BufferHeader` with no -/// inline payload, so `header + 8` is past the allocation; +/// excluded at prime time — their payload after `BufferHeader` holds a +/// native data pointer, not inline bytes; /// * ABA is closed the same way as every other buffer identity table: /// `finalize_collected_dead_buffer` clears the entry when the buffer dies, /// and `register_buffer` clears it again when the address is re-issued @@ -1236,11 +1163,11 @@ pub fn buffer_alloc(capacity: u32) -> *mut BufferHeader { /// Allocate a Buffer-shaped GC wrapper over native-owned memory. /// -/// Only the `BufferHeader` is allocated in Perry's old arena. The byte span -/// remains owned by the native caller and is never freed by the GC. Callers -/// must keep that span alive for at least as long as the returned JS value. -/// The external mapping is removed when the wrapper is collected, preventing -/// recycled GC addresses from inheriting stale backing pointers. +/// The header and native data pointer live in Perry's old arena. The byte span +/// is owned by the caller, or released by the supplied Node-API finalizer. +/// A borrowed span must outlive the returned JS value. +/// Fresh allocations start with no foreign-data bit, so recycled addresses +/// cannot inherit a previous owner's native pointer. pub(crate) fn buffer_alloc_foreign(data: *mut u8, length: u32) -> *mut BufferHeader { // RULE 3: this wrapper is reached from `extern "C"` Node-API entry points // where a JS throw has nowhere to land, so the over-range span is clamped @@ -1255,81 +1182,114 @@ pub(crate) fn buffer_alloc_foreign(data: *mut u8, length: u32) -> *mut BufferHea length, ); let ptr = crate::arena::arena_alloc_gc_old( - std::mem::size_of::(), + std::mem::size_of::(), 8, crate::gc::GC_TYPE_BUFFER, - ) as *mut BufferHeader; + ) as *mut ForeignBuffer; unsafe { - let header = (ptr as *mut u8).sub(crate::gc::GC_HEADER_SIZE) as *mut crate::gc::GcHeader; - (*header).gc_flags |= crate::gc::GC_FLAG_TENURED; - (*ptr).length = length; - (*ptr).capacity = length; + let gc = (ptr as *mut u8).sub(crate::gc::GC_HEADER_SIZE) as *mut crate::gc::GcHeader; + (*gc).gc_flags |= crate::gc::GC_FLAG_TENURED; + (*gc)._reserved |= crate::gc::GC_BUFFER_FOREIGN_DATA; + (*ptr).header.length = length; + (*ptr).header.capacity = length; + (*ptr).data = data; + #[cfg(feature = "node-api-host")] + { + (*ptr).finalizer = None; + } } - register_buffer(ptr); - // Arm before publishing the map entry; see `RegistryLatch`'s ordering - // contract and the analogous buffer-registration path above. - FOREIGN_BACKING_EVER_REGISTERED.arm(); - FOREIGN_BACKING_REGISTRY.with(|r| { - r.borrow_mut().insert(ptr as usize, data as usize); + register_buffer(ptr.cast()); + if crate::hot_diag::receiver_repr_on() { + crate::hot_diag::receiver_repr_note_constructed( + crate::hot_diag::ReceiverReprFamily::ExternalBuffer, + ); + } + ptr.cast() +} + +/// Foreign bytes remain owned by the caller. The data pointer is a raw native +/// address, never a GC edge; Buffer's collector descriptor traces no byte slots. +#[repr(C)] +struct ForeignBuffer { + header: BufferHeader, + data: *mut u8, + #[cfg(feature = "node-api-host")] + finalizer: Option, +} + +#[cfg(feature = "node-api-host")] +pub(crate) fn set_foreign_finalizer( + buffer: *mut BufferHeader, + finalizer: Option, +) { + assert!(is_foreign_backed_buffer(buffer as usize)); + // Native callback/data/module identities are POD, never GC edges. + unsafe { (*(buffer as *mut ForeignBuffer)).finalizer = finalizer }; +} + +#[cfg(feature = "node-api-host")] +fn enqueue_foreign_finalizer(addr: usize) { + if is_foreign_backed_buffer(addr) { + if let Some(finalizer) = unsafe { (*(addr as *mut ForeignBuffer)).finalizer.take() } { + crate::node_api_host::enqueue_finalizer(finalizer); + } + } +} + +/// Node-API shutdown releases live native resources before unloading addons. +/// The existing allocation inventory enumerates owners; finalizer state itself +/// lives only in the cell. Taking it also prevents a later sweep running twice. +#[cfg(feature = "node-api-host")] +pub(crate) fn enqueue_all_foreign_finalizers() { + BUFFER_REGISTRY.with(|buffers| { + for &addr in buffers.borrow().iter() { + enqueue_foreign_finalizer(addr); + } }); - ptr } -/// Re-point a foreign-backed wrapper at a new span, in place. -/// -/// [`buffer_alloc_foreign`] hands out a wrapper whose bytes live in memory the -/// caller owns. When that memory MOVES the wrapper has to follow it or every -/// later read dereferences freed memory: wasmi's linear memory is a `Vec` -/// that reallocates when wasm executes `memory.grow`, and the JS-visible -/// `WebAssembly.Memory.prototype.buffer` is a foreign wrapper over it (#9611). -/// -/// Only touches a wrapper that really is foreign-backed — a plain buffer's -/// bytes live inline after its header and cannot be re-pointed. Returns -/// whether the rebind happened. -/// -/// Gated with `wasm-host`, whose module is its only caller: the default -/// runtime build does -/// not compile the wasm host shims, and an ungated helper would be dead code -/// there. +/// Rebind a wasm linear-memory wrapper after memory.grow relocates its bytes. #[cfg(feature = "wasm-host")] pub(crate) fn rebind_foreign_buffer(addr: usize, data: *mut u8, length: u32) -> bool { - if FOREIGN_BACKING_EVER_REGISTERED.is_idle() { + if !is_foreign_backed_buffer(addr) { return false; } - let rebound = FOREIGN_BACKING_REGISTRY.with(|r| { - let mut r = r.borrow_mut(); - if !r.contains_key(&addr) { - return false; - } - r.insert(addr, data as usize); - true - }); - if rebound { - // RULE 3: same clamp as `buffer_alloc_foreign` — a rebind may not - // publish a `+4` word the emitted read path would read as a ShapeId. - let length = crate::object::shape_rule3::clamp_plus_four_word( - "BufferHeader::capacity (foreign rebind)", - length, - ); - unsafe { - let header = addr as *mut BufferHeader; - (*header).length = length; - (*header).capacity = length; - } + let length = crate::object::shape_rule3::clamp_plus_four_word( + "BufferHeader::capacity (foreign rebind)", + length, + ); + unsafe { + let buffer = addr as *mut ForeignBuffer; + (*buffer).header.length = length; + (*buffer).header.capacity = length; + (*buffer).data = data; } - rebound + true } #[inline] fn foreign_backing(addr: usize) -> Option { - if FOREIGN_BACKING_EVER_REGISTERED.is_idle() { - return None; + // Only buffer_data calls this, with a live BufferHeader. Every producer, + // including process-global SAB, now reserves a real preceding GcHeader. + // Keep the byte-access hot path to a header-bit load, without a registry + // or ownership lookup for each byte read. + unsafe { + let gc = (addr as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; + if (*gc)._reserved & crate::gc::GC_BUFFER_FOREIGN_DATA == 0 { + return None; + } + Some((*(addr as *const ForeignBuffer)).data as usize) } - FOREIGN_BACKING_REGISTRY.with(|r| r.borrow().get(&addr).copied()) } pub(crate) fn is_foreign_backed_buffer(addr: usize) -> bool { - foreign_backing(addr).is_some() + // This public-address probe must prove ownership before reading the header. + // SAB is a buffer too, but cannot have this per-heap foreign-data layout. + unsafe { crate::value::addr_class::try_read_tracked_gc_header(addr) }.is_some_and(|header| { + let header = unsafe { header.as_ref() }; + header.obj_type == crate::gc::GC_TYPE_BUFFER + && header._reserved & crate::gc::GC_BUFFER_FOREIGN_DATA != 0 + }) } /// Post-trace registry pruning (mirrors the #6010 Map/Set pattern): collect @@ -1360,27 +1320,10 @@ unsafe fn registered_buffer_is_dead_post_trace( addr: usize, shared_sabs: Option<&std::collections::HashSet>, ) -> bool { - // A process-global `SharedArrayBuffer` backing is NOT a GC allocation: - // `shared_sab::alloc_shared_sab` takes it straight from `alloc_zeroed`, it - // carries no `GcHeader`, and it is never freed (#4913 — that is what lets - // the same bytes alias across `perry/thread` agents). But - // `js_shared_array_buffer_new` DOES `register_buffer` it, so it lands in - // `BUFFER_REGISTRY` and reaches this scan on every full trace. - // - // `try_read_gc_header` below would then read the 8 bytes BEFORE the malloc - // block — the allocator's own metadata — and interpret them as a `GcHeader`: - // one arbitrary byte compared against `GC_TYPE_BUFFER` (10), the next - // against the mark/pin/forward bits. A chance match declares a LIVE, - // never-freed SAB dead, and `finalize_collected_dead_buffer` then runs on - // it — including `view::remove_entries_for_dead_buffer`, which retains on - // `info.backing != addr` and so unregisters EVERY live typed-array view - // over that SAB. Those views are exactly how cross-agent `Atomics` - // wait/notify resolve their absolute slot addresses. - // - // So: veto first, and never sniff a header the object does not have. The - // set is snapshotted once per scan by the caller and is `None` for the - // processes that never allocate a SAB — nearly all of them — so the common - // path here is a single null check. + // Process-global SharedArrayBuffer cells carry real GC_TYPE_BUFFER + // headers, but are immortal storage outside this thread's GC heap. They + // must never become dead candidates merely because this heap did not + // mark them. Snapshot membership once per scan, before inspecting flags. if shared_sabs.is_some_and(|sabs| sabs.contains(&addr)) { return false; } @@ -1400,15 +1343,14 @@ unsafe fn registered_buffer_is_dead_post_trace( /// (`is_registered_buffer`/`is_array_buffer` misclassify the next tenant — /// the #6080 ABA class) and the entries leak forever. pub(crate) fn finalize_collected_dead_buffer(addr: usize) { + #[cfg(feature = "node-api-host")] + enqueue_foreign_finalizer(addr); BUFFER_REGISTRY.with(|r| { r.borrow_mut().remove(&addr); }); if crate::hot_diag::buffer_on() { crate::hot_diag::buffer_note_unregistration(); } - FOREIGN_BACKING_REGISTRY.with(|r| { - r.borrow_mut().remove(&addr); - }); ARRAY_BUFFER_REGISTRY.with(|r| { r.borrow_mut().remove(&addr); }); @@ -1475,12 +1417,9 @@ pub(crate) fn finalize_collected_dead_buffer(addr: usize) { UINT8ARRAY_FROM_CTOR.with(|r| { r.borrow_mut().remove(&addr); }); - // `js_buffer_mark_as_crypto_key_external` writes all three global maps, and + // `js_buffer_mark_as_crypto_key_external` writes both global maps, and // `is_registered_buffer`/`is_uint8array_buffer` consult them, so a dead // external key buffer has to be dropped from every one of them. - if let Ok(mut r) = external_buffers().lock() { - r.remove(&addr); - } if let Ok(mut r) = external_uint8arrays().lock() { r.remove(&addr); } diff --git a/crates/perry-runtime/src/buffer/mod.rs b/crates/perry-runtime/src/buffer/mod.rs index b896430e128..3dc91165765 100644 --- a/crates/perry-runtime/src/buffer/mod.rs +++ b/crates/perry-runtime/src/buffer/mod.rs @@ -64,11 +64,11 @@ pub use header::{ asymmetric_key_meta, buffer_ab_alias, buffer_alloc, buffer_backing_array_buffer, buffer_byte_offset, buffer_data, buffer_data_mut, crypto_key_meta, ensure_buffer_ab_alias, external_registries_hold_for_test, is_any_array_buffer, is_array_buffer, is_data_view, - is_external_buffer, is_registered_buffer, is_secret_key, is_shared_array_buffer, - is_uint8array_buffer, js_set_crypto_key_death_hook, mark_as_array_buffer, - mark_as_asymmetric_key, mark_as_crypto_key, mark_as_data_view, mark_as_secret_key, - mark_as_shared_array_buffer, mark_as_uint8array, register_buffer, resolve_buffer_ab_alias, - set_buffer_ab_alias, u8_inline_cache_holds_for_test, CryptoKeyDeathHookFn, + is_registered_buffer, is_secret_key, is_shared_array_buffer, is_uint8array_buffer, + js_set_crypto_key_death_hook, mark_as_array_buffer, mark_as_asymmetric_key, mark_as_crypto_key, + mark_as_data_view, mark_as_secret_key, mark_as_shared_array_buffer, mark_as_uint8array, + register_buffer, resolve_buffer_ab_alias, set_buffer_ab_alias, u8_inline_cache_holds_for_test, + CryptoKeyDeathHookFn, }; pub(crate) use header::{ buffer_alloc_foreign, collect_dead_registered_buffers_post_trace, @@ -79,9 +79,9 @@ pub(crate) use header::{ pub(crate) use header::rebind_foreign_buffer; #[cfg(test)] pub(crate) use header::{ - js_buffer_register_external, test_buffer_addr_window_bounds, test_buffer_registry_probe_count, - test_data_view_registry_len, test_shared_array_buffer_registry_len, - test_uint8array_addr_window_bounds, test_uint8array_registry_probe_count, + test_buffer_addr_window_bounds, test_buffer_registry_probe_count, test_data_view_registry_len, + test_shared_array_buffer_registry_len, test_uint8array_addr_window_bounds, + test_uint8array_registry_probe_count, }; // ---- Re-exports: ArrayBuffer detach / transfer (ES2024) ---- @@ -667,3 +667,6 @@ mod tests { assert_eq!(js_buffer_get(buf, 1) as u8, 0x00); } } + +#[cfg(feature = "node-api-host")] +pub(crate) use header::{enqueue_all_foreign_finalizers, set_foreign_finalizer}; diff --git a/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs b/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs index f2ae810df33..321624d81e8 100644 --- a/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs +++ b/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs @@ -383,7 +383,7 @@ fn test_buffer_own_props_table_drains_after_owners_die() { /// Nothing pinned this before, and a great deal rests on it: `bun:ffi`'s /// pointer-lifetime contract hands `ptr(view)` to native code and documents /// the address as stable for the lifetime of the JS object -/// (`bun_ffi/mod.rs`); `FOREIGN_BACKING_REGISTRY`, `VIEW_REGISTRY`, +/// (`bun_ffi/mod.rs`); `VIEW_REGISTRY`, /// `BACKING_TO_VIEWS`, `DETACHED_BUFFER_REGISTRY` and the identity registries /// above are all keyed by that address; and #9611 publishes /// `WebAssembly.Memory.prototype.buffer` as a foreign-backed wrapper whose diff --git a/crates/perry-runtime/src/gc/tests/external_buffer.rs b/crates/perry-runtime/src/gc/tests/external_buffer.rs new file mode 100644 index 00000000000..2dbc66c698e --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/external_buffer.rs @@ -0,0 +1,101 @@ +//! Honest external-buffer storage: the wrapper is managed; its bytes are native. +use super::super::*; +use super::support::*; + +#[test] +fn external_buffer_producer_returns_headered_distinct_cells_and_exact_bytes() { + let _guard = GcTestIsolationGuard::new(); + let mut bytes = [11u8, 22, 33, 44]; + let first = crate::buffer::buffer_alloc_foreign(bytes.as_mut_ptr(), 4); + let second = crate::buffer::buffer_alloc_foreign(bytes.as_mut_ptr(), 4); + // Gate B precedes any dereference: both small handles and headerless Boxes fail. + for ptr in [first, second] { + assert!(!crate::value::addr_class::is_handle_band(ptr as usize)); + let header = unsafe { crate::value::addr_class::try_read_tracked_gc_header(ptr as usize) } + .expect("external-buffer producer must return an allocator-owned cell"); + assert_eq!(unsafe { header.as_ref().obj_type }, GC_TYPE_BUFFER); + let payload_size = + std::mem::size_of::() + std::mem::size_of::<*mut u8>(); + assert!(unsafe { header.as_ref().size as usize } >= GC_HEADER_SIZE + payload_size); + assert_eq!( + unsafe { *((ptr as *const u8).add(8) as *const *mut u8) }, + bytes.as_mut_ptr(), + "the native address must live in this cell, not a side table" + ); + assert!(crate::buffer::is_foreign_backed_buffer(ptr as usize)); + assert_eq!(crate::buffer::buffer_data(ptr), bytes.as_ptr()); + assert_eq!(unsafe { (*ptr).length }, 4); + } + assert_ne!(first, second, "two wrappers must have distinct identity"); + crate::buffer::js_buffer_set(first, 1, 99); + assert_eq!(bytes, [11, 99, 33, 44]); + assert_eq!(crate::buffer::js_buffer_get(second, 1), 99); + let plain = crate::buffer::buffer_alloc(4); + assert!(!crate::buffer::is_foreign_backed_buffer(plain as usize)); +} + +#[test] +fn external_buffer_cell_survives_gc_then_releases_its_registration() { + let _guard = CopyingNurseryTestGuard::new(1); + let mut bytes = [17u8, 29]; + let ptr = crate::buffer::buffer_alloc_foreign(bytes.as_mut_ptr(), 2); + let addr = ptr as usize; + js_shadow_slot_set(0, ptr_bits(addr)); + let _ = gc_collect_minor(); + let _ = + gc_collect_full_mark_sweep_with_trigger(GcTriggerSnapshot::capture(GcTriggerKind::Direct)); + assert_eq!( + js_shadow_slot_get(0), + ptr_bits(addr), + "FFI wrapper cannot move" + ); + assert!(crate::buffer::is_registered_buffer(addr)); + assert!(crate::buffer::is_foreign_backed_buffer(addr)); + assert_eq!(crate::buffer::buffer_data(ptr), bytes.as_ptr()); + assert_eq!(crate::buffer::js_buffer_get(ptr, 1), 29); + js_shadow_slot_set(0, crate::value::TAG_UNDEFINED); + let _ = + gc_collect_full_mark_sweep_with_trigger(GcTriggerSnapshot::capture(GcTriggerKind::Direct)); + assert!( + !crate::buffer::is_registered_buffer(addr), + "dead wrapper registration must disappear" + ); + // A borrowed byte span is not the collector's resource to free or overwrite. + assert_eq!(bytes, [17, 29]); +} + +#[test] +fn external_buffer_crosses_a_worker_by_value_like_an_ordinary_buffer() { + let _guard = CopyingNurseryTestGuard::new(2); + let mut bytes = [42u8, 43]; + let ptr = crate::buffer::buffer_alloc_foreign(bytes.as_mut_ptr(), 2); + js_shadow_slot_set(0, ptr_bits(ptr as usize)); + let plain = crate::buffer::buffer_alloc(2); + js_shadow_slot_set(1, ptr_bits(plain as usize)); + crate::buffer::js_buffer_set(plain, 0, 42); + crate::buffer::js_buffer_set(plain, 1, 43); + // The native backing must be invisible at the boundary: a foreign wrapper + // serializes exactly as an ordinary buffer holding the same bytes (a + // structured clone of an external ArrayBuffer copies it in node). + for mark_uint8 in [false, true] { + if mark_uint8 { + crate::buffer::mark_as_uint8array(ptr as usize); + crate::buffer::mark_as_uint8array(plain as usize); + } + let foreign = unsafe { crate::thread::serialize_nanbox_for_thread(ptr_bits(ptr as usize)) }; + let ordinary = + unsafe { crate::thread::serialize_nanbox_for_thread(ptr_bits(plain as usize)) }; + assert_eq!( + crate::thread::first_unsupported_transfer_type(&foreign), + crate::thread::first_unsupported_transfer_type(&ordinary) + ); + if mark_uint8 { + match foreign { + crate::thread::SerializedValue::Uint8Array(copy) => assert_eq!(copy, [42, 43]), + _ => panic!("a foreign Uint8Array must cross as a byte copy"), + } + } + } + // The copy is a snapshot; the native span stays caller-owned. + assert_eq!(bytes, [42, 43]); +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 9ec4324d678..df76e878706 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -119,3 +119,4 @@ mod dyn_eval_registry; mod private_evaluation_storage; mod event_internal_slots; +mod external_buffer; diff --git a/crates/perry-runtime/src/gc/types.rs b/crates/perry-runtime/src/gc/types.rs index b16fb19c722..d0c5c65f2a7 100644 --- a/crates/perry-runtime/src/gc/types.rs +++ b/crates/perry-runtime/src/gc/types.rs @@ -1248,6 +1248,10 @@ pub const OBJ_FLAG_NO_EXTEND: u16 = 0x04; // (`GC_COPY_SURVIVAL_AGE_MASK = 0x0038`) and bits 14..15 the layout state, // so 0x08 would be clobbered on every minor GC. Bits 6..13 are free. pub const OBJ_FLAG_NULL_PROTO: u16 = 0x40; +/// The GC_TYPE_BUFFER payload contains a native data pointer after BufferHeader. +/// Bit 7 is kind-disjoint from object/array numeric-layout proofs. Generic age +/// and layout transitions preserve it; no Buffer reader interprets those proofs. +pub(crate) const GC_BUFFER_FOREIGN_DATA: u16 = 0x80; /// #8690: this `GC_TYPE_OBJECT` carries a cached proof that the packed /// Array-subclass element prefix recorded in `ObjectMeta::flags` is numeric. /// The bit is the address-reuse-safe authority: fresh allocations start with @@ -1418,7 +1422,7 @@ pub const OBJ_FLAG_PLAIN_ORDINARY: u16 = 0x200; /// | 0..2 | `OBJ_FLAG_FROZEN` / `SEALED` / `NO_EXTEND` | same | | /// | 3..5 | | | `GC_COPY_SURVIVAL_AGE_MASK` | /// | 6 | `OBJ_FLAG_NULL_PROTO` | `GC_ARRAY_CUSTOM_PROTO` (alias) | `GC_RESIDUAL_PROTO_OWNER` (non-object) | -/// | 7 | `OBJ_FLAG_PACKED_NUMERIC_PROOF` | `GC_ARRAY_RAW_F64_LAYOUT` | | +/// | 7 | `OBJ_FLAG_PACKED_NUMERIC_PROOF` | `GC_ARRAY_RAW_F64_LAYOUT` | BUFFER: `GC_BUFFER_FOREIGN_DATA` | /// | 8 | `OBJ_FLAG_TYPED_ARRAY_PROTO` | `GC_ARRAY_NAMED_PROPS` | | /// | 9 | `OBJ_FLAG_PLAIN_ORDINARY` | `GC_ARRAY_ARGUMENTS_OBJECT` | | /// | 10 | `OBJ_FLAG_STABLE_TOMBSTONES` | `OBJ_FLAG_ARRAY_DESCRIPTORS` | | diff --git a/crates/perry-runtime/src/hot_diag/receiver_repr.rs b/crates/perry-runtime/src/hot_diag/receiver_repr.rs index 5b737505ef6..3cc96e5f96e 100644 --- a/crates/perry-runtime/src/hot_diag/receiver_repr.rs +++ b/crates/perry-runtime/src/hot_diag/receiver_repr.rs @@ -241,9 +241,6 @@ fn observe_pointer(addr: usize) { if crate::symbol::is_registered_symbol(addr) { mark_old(ReceiverReprFamily::SymbolGlobal); } - if crate::buffer::is_external_buffer(addr) { - mark_old(ReceiverReprFamily::ExternalBuffer); - } return; } @@ -492,13 +489,8 @@ mod tests { false, ) }); - assert_fixture(ReceiverReprFamily::ExternalBuffer, || { - let buffer = Box::into_raw(Box::new(crate::buffer::BufferHeader { - length: 0, - capacity: 0, - })); - crate::buffer::js_buffer_register_external(buffer as usize); - (buffer as usize, false) + assert_fixture_migrated(ReceiverReprFamily::ExternalBuffer, || { + crate::buffer::buffer_alloc_foreign(std::ptr::null_mut(), 0) as usize }); assert_fixture(ReceiverReprFamily::Sab, || { (crate::shared_sab::alloc_shared_sab(1) as usize, false) diff --git a/crates/perry-runtime/src/node_api_host/buffers.rs b/crates/perry-runtime/src/node_api_host/buffers.rs index 90bbd0bda12..064d3eb4958 100644 --- a/crates/perry-runtime/src/node_api_host/buffers.rs +++ b/crates/perry-runtime/src/node_api_host/buffers.rs @@ -144,7 +144,10 @@ pub unsafe extern "C" fn napi_create_external_buffer( Err(status) => return status, }; let buffer = crate::buffer::buffer_alloc_foreign(data.cast(), length); - super::metadata::attach_owner_finalizer(buffer as usize, data, finalize_cb, finalize_hint); + crate::buffer::set_foreign_finalizer( + buffer, + super::metadata::finalizer(finalize_cb, data, finalize_hint), + ); write_pointer_handle(env, buffer.cast(), result) } @@ -235,11 +238,9 @@ pub unsafe extern "C" fn napi_create_external_arraybuffer( }; let buffer = crate::buffer::buffer_alloc_foreign(external_data.cast(), byte_length); crate::buffer::mark_as_array_buffer(buffer as usize); - super::metadata::attach_owner_finalizer( - buffer as usize, - external_data, - finalize_cb, - finalize_hint, + crate::buffer::set_foreign_finalizer( + buffer, + super::metadata::finalizer(finalize_cb, external_data, finalize_hint), ); write_pointer_handle(env, buffer.cast(), result) } diff --git a/crates/perry-runtime/src/node_api_host/lifecycle.rs b/crates/perry-runtime/src/node_api_host/lifecycle.rs index e83ee826940..e2caaf61d6b 100644 --- a/crates/perry-runtime/src/node_api_host/lifecycle.rs +++ b/crates/perry-runtime/src/node_api_host/lifecycle.rs @@ -429,7 +429,16 @@ pub unsafe extern "C" fn napi_fatal_exception(env: NapiEnv, error: NapiValue) -> /// Idempotent owning-agent shutdown. Cleanup callbacks and finalizers run only /// after all internal borrows have been released. pub fn shutdown_current_env() { - let env_pointer = current_env(); + // A worker that never loaded an addon has no environment to shut down. + // Do not create one during retirement. + let env_pointer = NODE_API_ENV.with(|cell| { + cell.borrow_mut() + .as_deref_mut() + .map(|env| env as *mut Env as NapiEnv) + }); + let Some(env_pointer) = env_pointer else { + return; + }; let state = with_env_mut(env_pointer, |env| { if env.shutting_down { return None; @@ -482,6 +491,7 @@ pub fn shutdown_current_env() { super::metadata::enqueue_finalizer(finalizer); } } + crate::buffer::enqueue_all_foreign_finalizers(); super::metadata::enqueue_all_object_finalizers(); super::process_pending(); super::loader::close_loaded_addons(env_pointer); diff --git a/crates/perry-runtime/src/node_api_host/metadata.rs b/crates/perry-runtime/src/node_api_host/metadata.rs index c4775571c7b..ee13cf9f82d 100644 --- a/crates/perry-runtime/src/node_api_host/metadata.rs +++ b/crates/perry-runtime/src/node_api_host/metadata.rs @@ -104,25 +104,6 @@ pub(crate) fn attach_external_finalizer( }); } -pub(crate) fn attach_owner_finalizer( - owner: usize, - data: *mut c_void, - callback: NapiFinalize, - hint: *mut c_void, -) { - let Some(finalizer) = finalizer(callback, data, hint) else { - return; - }; - OBJECT_METADATA.with(|table| { - table - .borrow_mut() - .entry(owner) - .or_default() - .finalizers - .push(finalizer); - }); -} - pub(crate) fn is_external_owner(owner: usize) -> bool { OBJECT_METADATA.with(|table| { table diff --git a/crates/perry-runtime/src/node_api_host/tests.rs b/crates/perry-runtime/src/node_api_host/tests.rs index 6f47a4fee59..c892a12777b 100644 --- a/crates/perry-runtime/src/node_api_host/tests.rs +++ b/crates/perry-runtime/src/node_api_host/tests.rs @@ -1185,3 +1185,133 @@ fn run_script_and_uv_loop_fail_explicitly() { ); assert!(loop_pointer.is_null()); } + +#[test] +fn external_buffer_finalizer_runs_once_after_the_owner_dies() { + unsafe extern "C" fn release(_env: NapiEnv, data: *mut c_void, hint: *mut c_void) { + drop(Box::from_raw(data as *mut [u8; 3])); + (*(hint as *const AtomicUsize)).fetch_add(1, Ordering::SeqCst); + } + let env = test_env(); + let releases = AtomicUsize::new(0); + let data = Box::into_raw(Box::new([65u8, 66, 67])); + let mut scope = std::ptr::null_mut(); + assert_eq!( + unsafe { napi_open_handle_scope(env, &mut scope) }, + NapiStatus::Ok + ); + let mut value = std::ptr::null_mut(); + assert_eq!( + unsafe { + napi_create_external_buffer( + env, + 3, + data.cast(), + Some(release), + (&releases as *const AtomicUsize).cast_mut().cast(), + &mut value, + ) + }, + NapiStatus::Ok + ); + let owner = (value_bits(env, value).unwrap() & crate::value::POINTER_MASK) as usize; + assert!(crate::buffer::is_foreign_backed_buffer(owner)); + crate::gc::js_gc_collect(); + super::metadata::drain_pending_finalizers(); + assert_eq!( + releases.load(Ordering::SeqCst), + 0, + "live owner must retain bytes" + ); + assert_eq!( + unsafe { napi_close_handle_scope(env, scope) }, + NapiStatus::Ok + ); + crate::gc::js_gc_collect(); + super::metadata::drain_pending_finalizers(); + assert_eq!( + releases.load(Ordering::SeqCst), + 1, + "dead owner's native resource must be released" + ); + crate::gc::js_gc_collect(); + super::metadata::drain_pending_finalizers(); + assert_eq!( + releases.load(Ordering::SeqCst), + 1, + "finalizer entry must be removed" + ); + let live_data = Box::into_raw(Box::new([68u8, 69, 70])); + assert_eq!( + unsafe { + napi_create_external_buffer( + env, + 3, + live_data.cast(), + Some(release), + (&releases as *const AtomicUsize).cast_mut().cast(), + &mut value, + ) + }, + NapiStatus::Ok + ); + super::shutdown_current_env(); + super::shutdown_current_env(); + assert_eq!( + releases.load(Ordering::SeqCst), + 2, + "shutdown must release a still-live native buffer exactly once" + ); +} + +#[test] +fn external_buffer_arraybuffer_released_on_worker_retirement() { + unsafe extern "C" fn release(_env: NapiEnv, data: *mut c_void, hint: *mut c_void) { + drop(Box::from_raw(data as *mut [u8; 3])); + (*(hint as *const AtomicUsize)).fetch_add(1, Ordering::SeqCst); + } + let releases = std::sync::Arc::new(AtomicUsize::new(0)); + let worker_releases = releases.clone(); + std::thread::spawn(move || { + let agent = crate::agent::enter_worker_agent(); + let env = current_env(); + let bytes = Box::into_raw(Box::new([11u8, 22, 33])); + let mut value = std::ptr::null_mut(); + assert_eq!( + unsafe { + napi_create_external_arraybuffer( + env, + bytes.cast(), + 3, + Some(release), + std::sync::Arc::as_ptr(&worker_releases).cast_mut().cast(), + &mut value, + ) + }, + NapiStatus::Ok + ); + let owner = (value_bits(env, value).unwrap() & crate::value::POINTER_MASK) as usize; + assert!(crate::buffer::is_foreign_backed_buffer(owner)); + assert!(crate::buffer::is_array_buffer(owner)); + let mut data = std::ptr::null_mut(); + let mut length = 0; + assert_eq!( + unsafe { napi_get_arraybuffer_info(env, value, &mut data, &mut length) }, + NapiStatus::Ok + ); + assert_eq!(data, bytes.cast()); + assert_eq!(length, 3); + assert_eq!(worker_releases.load(Ordering::SeqCst), 0); + crate::agent::retire_agent(agent); + assert_eq!( + worker_releases.load(Ordering::SeqCst), + 1, + "retiring the owning agent must finalize native bytes" + ); + crate::agent::retire_agent(agent); + assert_eq!(worker_releases.load(Ordering::SeqCst), 1); + }) + .join() + .unwrap(); + assert_eq!(releases.load(Ordering::SeqCst), 1); +} diff --git a/crates/perry-runtime/src/registry_latch.rs b/crates/perry-runtime/src/registry_latch.rs index 0dc12ae5e1d..8fb7d8f1457 100644 --- a/crates/perry-runtime/src/registry_latch.rs +++ b/crates/perry-runtime/src/registry_latch.rs @@ -41,8 +41,8 @@ //! window in which the feature is live and reachable but the latch still reads //! idle, so a concurrent probe takes the fast path and answers `false` for an //! address that is genuinely registered. That is not hypothetical: the sibling -//! latch `buffer::header::EXTERNAL_BUFFERS_NONEMPTY` carries an inline comment -//! for precisely this reason, and `js_buffer_register_external` latches first. +//! latch `buffer::header::EXTERNAL_UINT8ARRAYS_NONEMPTY` carries an inline comment +//! for precisely this reason, and `register_external_uint8array` latches first. //! //! With the arm placed first, the argument for a reader on *another* thread is: //! a thread can only probe an address it holds, and every route by which an diff --git a/crates/perry-runtime/src/shared_sab.rs b/crates/perry-runtime/src/shared_sab.rs index f311319df29..bd89c58972d 100644 --- a/crates/perry-runtime/src/shared_sab.rs +++ b/crates/perry-runtime/src/shared_sab.rs @@ -34,7 +34,7 @@ use crate::gc::{GcHeader, GC_FLAG_TENURED, GC_HEADER_SIZE, GC_TYPE_BUFFER}; /// Set of `BufferHeader` addresses that back a `SharedArrayBuffer`. static SHARED_SAB_REGISTRY: OnceLock>> = OnceLock::new(); -/// Latched true by the first SAB allocation. Mirrors `EXTERNAL_BUFFERS_NONEMPTY` +/// Latched true by the first SAB allocation. Mirrors the external-Uint8Array latch /// in `buffer::header` and exists for the same reason: `is_shared_sab` sits on /// two hot paths that run for *every* pointer-shaped value — /// `buffer::is_registered_buffer`'s final fallback (which JSON.stringify runs @@ -124,7 +124,7 @@ pub fn alloc_shared_sab(size: u32) -> *mut BufferHeader { // without it ever entering their thread-local registries, so both latches // must be armed before this address can be found — an arm placed after the // insert leaves a window in which the entry is live and a probe still takes - // the idle fast path. (This is the ordering `js_buffer_register_external` + // the idle fast path. (This is the ordering `register_buffer` // already documents; see also `crate::registry_latch`.) SHARED_SAB_NONEMPTY.store(true, Ordering::Release); crate::buffer::note_buffer_like_registered(buf as usize); diff --git a/crates/perry-runtime/src/thread.rs b/crates/perry-runtime/src/thread.rs index 3cc34a5f2a5..3c81359c149 100644 --- a/crates/perry-runtime/src/thread.rs +++ b/crates/perry-runtime/src/thread.rs @@ -413,21 +413,18 @@ pub unsafe fn serialize_nanbox_for_thread(bits: u64) -> SerializedValue { return SerializedValue::Inline(TAG_UNDEFINED); } - // SharedArrayBuffer: a process-global backing store (no GcHeader). + // SharedArrayBuffer is an immortal process-global buffer cell. // Pass it by reference so the receiving agent aliases the same bytes - // (#4913). This MUST precede the GcHeader read below — a SAB header has - // no preceding GcHeader, so reading one would misclassify it. + // rather than taking the ordinary per-agent copy path below. if crate::shared_sab::is_shared_sab(raw_ptr as usize) { return SerializedValue::SharedArrayBuffer { addr: raw_ptr as usize, }; } - // Uint8Array can be backed by an ordinary GC allocation, a registered - // view, or an external BufferHeader with no preceding GcHeader. Brand - // detection must therefore precede the GcHeader read below, just like - // SharedArrayBuffer detection does. Always read through buffer_data so - // views and external storage copy their authoritative byte window. + // Byte views copy their authoritative span through buffer_data, so a + // foreign-backed Uint8Array crosses by value exactly as a structured + // clone does in node; its native backing is never shared. if crate::buffer::is_uint8array_buffer(raw_ptr as usize) { let buffer = raw_ptr as *const crate::buffer::BufferHeader; let len = (*buffer).length as usize; diff --git a/crates/perry-stdlib/src/runtime_thread_exit_tests/symbols_tests.rs b/crates/perry-stdlib/src/runtime_thread_exit_tests/symbols_tests.rs index 4e46abe68a6..9df0fecb8f9 100644 --- a/crates/perry-stdlib/src/runtime_thread_exit_tests/symbols_tests.rs +++ b/crates/perry-stdlib/src/runtime_thread_exit_tests/symbols_tests.rs @@ -278,10 +278,9 @@ fn thread_exit_releases_the_threads_buffer_own_props() { /// inline-access cache (a key is not integer-indexed), so the key buffer can /// no longer witness the cache's release; an ordinary `Buffer` on the same /// thread does. -fn external_buffer_registrations(key: usize, view: usize) -> [bool; 4] { - let [ext, u8a, meta] = perry_runtime::buffer::external_registries_hold_for_test(key); +fn external_buffer_registrations(key: usize, view: usize) -> [bool; 3] { + let [u8a, meta] = perry_runtime::buffer::external_registries_hold_for_test(key); [ - ext, u8a, meta, perry_runtime::buffer::u8_inline_cache_holds_for_test(view), @@ -304,7 +303,7 @@ fn external_buffer_registrations(key: usize, view: usize) -> [bool; 4] { /// Holds `(key, view, seen)`: the CryptoKey buffer, the cache-admitted byte /// view, and the verdict. #[allow(clippy::type_complexity)] -static EXTERNAL_BUFFER_EXIT_PROBE: std::sync::Mutex<(usize, usize, Option<[bool; 4]>)> = +static EXTERNAL_BUFFER_EXIT_PROBE: std::sync::Mutex<(usize, usize, Option<[bool; 3]>)> = std::sync::Mutex::new((0, 0, None)); fn record_external_buffer_release(freed: &perry_runtime::arena::thread_exit::FreedRanges) { @@ -322,9 +321,8 @@ fn record_external_buffer_release(freed: &perry_runtime::arena::thread_exit::Fre /// Verdict on what [`record_external_buffer_release`] saw: `Err` names the /// first table that still held the dead buffer when its thread was released. -fn external_buffer_release_verdict(seen: Option<[bool; 4]>) -> Result<(), &'static str> { - const TABLES: [&str; 4] = [ - "EXTERNAL_BUFFER_REGISTRY outlived the thread", +fn external_buffer_release_verdict(seen: Option<[bool; 3]>) -> Result<(), &'static str> { + const TABLES: [&str; 3] = [ "EXTERNAL_UINT8ARRAY_REGISTRY outlived the thread", "EXTERNAL_CRYPTO_KEY_META_REGISTRY outlived the thread", "PERRY_U8_INLINE_CACHE outlived the thread", @@ -342,7 +340,7 @@ fn thread_exit_releases_the_threads_external_buffer_registrations() { let scope = RuntimeHandleScope::new(); let buf = scope.root_raw_mut_ptr(perry_runtime::buffer::js_buffer_alloc(16, 0)); let addr = buf.get_raw_mut_ptr::() as usize; - // webcrypto's CryptoKey registration: all three external registries. + // webcrypto's CryptoKey registration: both external registries. // This also registers their thread-exit hook, so it runs before the // probe registered below. unsafe { js_buffer_mark_as_crypto_key_external(addr, 1, 0, 1, 1, 0, 0) }; @@ -373,7 +371,7 @@ fn thread_exit_releases_the_threads_external_buffer_registrations() { "key material must never enter the inline element-access cache" ); assert_eq!( - alive, [true; 4], + alive, [true; 3], "every registration must exist while its thread lives" ); if let Err(table) = external_buffer_release_verdict(seen) { @@ -393,20 +391,20 @@ fn external_buffer_verdict_is_taken_at_release_not_by_address() { let addr = buf.get_raw_mut_ptr::() as usize; unsafe { js_buffer_mark_as_crypto_key_external(addr, 1, 0, 1, 1, 0, 0) }; assert!( - perry_runtime::buffer::is_external_buffer(addr), + perry_runtime::buffer::external_registries_hold_for_test(addr)[0], "the newcomer is registered, so an address-only check would fail" ); assert_eq!( - external_buffer_release_verdict(Some([false; 4])), + external_buffer_release_verdict(Some([false; 3])), Ok(()), "a newcomer at a released address was reported as the dead buffer" ); assert_eq!( - external_buffer_release_verdict(Some([true, false, false, false])), - Err("EXTERNAL_BUFFER_REGISTRY outlived the thread") + external_buffer_release_verdict(Some([true, false, false])), + Err("EXTERNAL_UINT8ARRAY_REGISTRY outlived the thread") ); assert_eq!( - external_buffer_release_verdict(Some([false, false, false, true])), + external_buffer_release_verdict(Some([false, false, true])), Err("PERRY_U8_INLINE_CACHE outlived the thread") ); assert!( diff --git a/crates/perry-stdlib/src/webcrypto/util.rs b/crates/perry-stdlib/src/webcrypto/util.rs index 98f0d02a019..f8790b06874 100644 --- a/crates/perry-stdlib/src/webcrypto/util.rs +++ b/crates/perry-stdlib/src/webcrypto/util.rs @@ -60,13 +60,13 @@ pub(super) use ml_kem::pkcs8::{ }; pub(super) use perry_runtime::{ - buffer::{buffer_alloc, buffer_data_mut, is_registered_buffer, BufferHeader}, + buffer::{buffer_data_mut, is_registered_buffer, BufferHeader}, js_object_alloc, js_object_set_field_by_name, js_promise_resolved, JSValue, Promise, StringHeader, }; extern "C" { - fn js_buffer_register_external(addr: usize); + fn js_buffer_alloc_unsafe(size: i32) -> *mut BufferHeader; fn js_buffer_mark_as_uint8array_external(addr: usize); fn js_buffer_mark_as_crypto_key_external( addr: usize, @@ -944,7 +944,10 @@ pub(super) fn require_usage( /// Allocate a fresh Buffer marked as Uint8Array (so `instanceof Uint8Array` /// is true and `new Uint8Array(buf)` memcpy's correctly), copy `bytes` in. pub(super) unsafe fn alloc_uint8array_from_slice(bytes: &[u8]) -> *mut BufferHeader { - let buf = buffer_alloc(bytes.len() as u32); + // Allocate through the runtime provider's C ABI. A separately packaged + // stdlib must not depend on registering a Rust-allocated cell afterwards. + // A negative size makes the runtime raise its ordinary allocation error. + let buf = js_buffer_alloc_unsafe(i32::try_from(bytes.len()).unwrap_or(-1)); if buf.is_null() { return buf; } @@ -953,7 +956,6 @@ pub(super) unsafe fn alloc_uint8array_from_slice(bytes: &[u8]) -> *mut BufferHea let dst = buffer_data_mut(buf); std::ptr::copy_nonoverlapping(bytes.as_ptr(), dst, bytes.len()); } - js_buffer_register_external(buf as usize); js_buffer_mark_as_uint8array_external(buf as usize); buf } diff --git a/scripts/run_honest_external_buffer.sh b/scripts/run_honest_external_buffer.sh new file mode 100755 index 00000000000..1aef401f434 --- /dev/null +++ b/scripts/run_honest_external_buffer.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash +# Build a local Node-API fixture; no package installation or network access. +set -euo pipefail +root=$(cd "$(dirname "$0")/.." && pwd) +fixture="$root/.lane-tmp/honest-external-buffer" +mkdir -p "$fixture/node_modules/fixture-external-buffer" +package="$fixture/node_modules/fixture-external-buffer" +cat > "$fixture/package.json" <<'JSON' +{"name":"honest-external-buffer","private":true,"perry":{"compilePackages":["fixture-external-buffer"],"allow":{"compilePackages":["fixture-external-buffer"]},"nativeAddons":["fixture-external-buffer"]}} +JSON +cat > "$package/package.json" <<'JSON' +{"name":"fixture-external-buffer","version":"1.0.0","main":"index.js"} +JSON +printf '%s\n' 'module.exports = require("./addon.node");' > "$package/index.js" +flags=(-shared) +if [[ $(uname -s) == Darwin ]]; then flags+=(-undefined dynamic_lookup); else flags+=(-fPIC); fi +"${CC:-cc}" "${flags[@]}" "$root/test-files/fixtures/honest-external-buffer/addon.c" -o "$package/addon.node" +cp "$root/test-files/fixtures/honest-external-buffer/surface.ts" "$fixture/main.ts" +cd "$fixture" +node --experimental-strip-types main.ts > node.out +if [[ ${1:-} == --node-only ]]; then cat node.out; exit 0; fi +# Perry uses a fixed optional-runtime subdirectory for native addons. Alias +# that path to this worktree's single physical target; do not duplicate it. +if [[ ! -e "$root/target/perry-optional-runtime" ]]; then + ln -s . "$root/target/perry-optional-runtime" +fi +if [[ ! -L "$root/target/perry-optional-runtime" ]]; then + echo "optional-runtime must alias the single target directory" >&2 + exit 1 +fi +# An unsuccessful compile must never leave an old run available as evidence. +rm -f perry.out +# Default target/ remains the only cargo target directory. Supply matching +# compiler and static wrappers with node-api-host enabled. +PERRY_NO_UPDATE_CHECK=1 PERRY_NO_TELEMETRY=1 PERRY_WORKSPACE_ROOT="$root" \ + PERRY_RUNTIME_DIR="$root/target/release" \ + "$root/target/release/perry" compile main.ts --no-cache --no-auto-optimize -o app +./app > perry.out +diff -u node.out perry.out diff --git a/scripts/thread_exit_address_globals.json b/scripts/thread_exit_address_globals.json index f463b321264..1f0945e7eea 100644 --- a/scripts/thread_exit_address_globals.json +++ b/scripts/thread_exit_address_globals.json @@ -2019,15 +2019,6 @@ "verdict": "process_global_allocation", "why": "Set of AsyncHook handles, each a never-freed Box::into_raw(AsyncHookHandle{index}) pointer (async_hooks.rs:740-741) — std alloc, not arena." }, - { - "file": "crates/perry-runtime/src/buffer/header.rs", - "names": [ - "EXTERNAL_BUFFER_REGISTRY" - ], - "verdict": "thread_exit_invalidated", - "why": "Addresses of externally-registered buffers, removed only by the dead-buffer finalizer or re-registration; freed addresses are dropped at thread exit.", - "hook": "release_external_buffer_registries_in_freed_ranges" - }, { "file": "crates/perry-runtime/src/buffer/header.rs", "names": [ @@ -2060,7 +2051,7 @@ "BUFFER_LIKE_ADDR_WINDOW" ], "verdict": "conservative_filter", - "why": "Monotone lo/hi address window over every buffer-like registration; a stale bound only admits more addresses to the slow path. Authoritative: BUFFER_REGISTRY (thread-local), EXTERNAL_BUFFER_REGISTRY, shared_sab's registry via is_registered_buffer_slow." + "why": "Monotone lo/hi address window over every buffer-like registration; a stale bound only admits more addresses to the slow path. Authoritative: BUFFER_REGISTRY (thread-local), shared_sab's registry via is_registered_buffer_slow." }, { "file": "crates/perry-runtime/src/buffer/header.rs", diff --git a/test-files/fixtures/honest-external-buffer/addon.c b/test-files/fixtures/honest-external-buffer/addon.c new file mode 100644 index 00000000000..19cb349817a --- /dev/null +++ b/test-files/fixtures/honest-external-buffer/addon.c @@ -0,0 +1,47 @@ +#include +#include +#include + +typedef void *napi_env; +typedef void *napi_value; +typedef void *napi_callback_info; +typedef int32_t napi_status; +typedef napi_value (*napi_callback)(napi_env, napi_callback_info); +typedef void (*napi_finalize)(napi_env, void *, void *); +extern napi_status napi_create_external_buffer(napi_env, size_t, void *, napi_finalize, void *, napi_value *); +extern napi_status napi_create_function(napi_env, const char *, size_t, napi_callback, void *, napi_value *); +extern napi_status napi_set_named_property(napi_env, napi_value, const char *, napi_value); +extern napi_status napi_create_int32(napi_env, int32_t, napi_value *); +static int32_t released; +static void release_bytes(napi_env env, void *bytes, void *hint) { + (void)env; (void)hint; + free(bytes); + released++; +} +static napi_value create(napi_env env, napi_callback_info info) { + (void)info; + unsigned char *bytes = malloc(3); + if (!bytes) return 0; + bytes[0] = 65; bytes[1] = 66; bytes[2] = 67; + napi_value value = 0; + if (napi_create_external_buffer(env, 3, bytes, release_bytes, 0, &value)) { + free(bytes); + return 0; + } + return value; +} +static napi_value count(napi_env env, napi_callback_info info) { + (void)info; + napi_value value = 0; + napi_create_int32(env, released, &value); + return value; +} +__attribute__((visibility("default"))) int32_t node_api_module_get_api_version_v1(void) { return 8; } +__attribute__((visibility("default"))) napi_value napi_register_module_v1(napi_env env, napi_value exports) { + napi_value fn = 0; + if (napi_create_function(env, "create", 6, create, 0, &fn) || + napi_set_named_property(env, exports, "create", fn) || + napi_create_function(env, "released", 8, count, 0, &fn) || + napi_set_named_property(env, exports, "released", fn)) return 0; + return exports; +} diff --git a/test-files/fixtures/honest-external-buffer/surface.ts b/test-files/fixtures/honest-external-buffer/surface.ts new file mode 100644 index 00000000000..14d617365e2 --- /dev/null +++ b/test-files/fixtures/honest-external-buffer/surface.ts @@ -0,0 +1,30 @@ +// Real napi_create_external_buffer producer. Prepare fixture-addon with +// scripts/run_honest_external_buffer.sh before running this differential file. +const addon = require('fixture-external-buffer'); +const first = addon.create(); +const second = addon.create(); +const same = first; +console.log('identity', first !== second, same === first); +const map = new Map([[first, 1], [second, 2]]); +const set = new Set([first, second, same]); +const weak = new WeakMap([[first, 3], [second, 4]]); +console.log('keys', map.size, set.size, map.get(same), weak.get(first), weak.get(second)); +console.log('brands', typeof first, first instanceof Buffer, first instanceof Uint8Array, Buffer.isBuffer(first), ArrayBuffer.isView(first)); +console.log('own', Object.keys(first).join(','), Object.getOwnPropertyNames(first).join(',')); +console.log('json', JSON.stringify(first)); +console.log('string', String(first), Object.prototype.toString.call(first)); +console.log('prototype', Object.getPrototypeOf(first) === Buffer.prototype); +let rejected = false; +try { Uint8Array.prototype.subarray.call({}, 0); } +catch (error) { rejected = error instanceof TypeError; } +console.log('brand check', rejected); +const view = first.subarray(1); +view[0] = 90; +console.log('bytes', first[0], first[1], first[2], String(first), String(second)); +console.log('backing', first.buffer === first.buffer, view.buffer === first.buffer); +function indexed(value: Uint8Array): number { + value[2] = 88; + return value[0] + value[2]; +} +console.log('typed access', indexed(second), String(second)); +console.log('native lifetime', addon.released());