diff --git a/crates/perry-runtime/src/buffer/header.rs b/crates/perry-runtime/src/buffer/header.rs index 20f620e4a..167f9acd0 100644 --- a/crates/perry-runtime/src/buffer/header.rs +++ b/crates/perry-runtime/src/buffer/header.rs @@ -161,6 +161,19 @@ pub fn is_data_view(addr: usize) -> bool { DATA_VIEW_REGISTRY.with(|r| r.borrow().contains(&addr)) } +/// Live entry counts for the two registries the GC buffer sweep prunes (#6337). +/// Test-only: the leak regression asserts these DRAIN after the owning buffers +/// are collected, which a per-address `is_*` probe cannot show. +#[cfg(test)] +pub(crate) fn test_data_view_registry_len() -> usize { + DATA_VIEW_REGISTRY.with(|r| r.borrow().len()) +} + +#[cfg(test)] +pub(crate) fn test_shared_array_buffer_registry_len() -> usize { + SHARED_ARRAY_BUFFER_REGISTRY.with(|r| r.borrow().len()) +} + /// Register a buffer pointer in the thread-local registry pub fn register_buffer(ptr: *const BufferHeader) { BUFFER_REGISTRY.with(|r| r.borrow_mut().insert(ptr as usize)); @@ -455,16 +468,49 @@ pub(crate) fn collect_dead_registered_buffers_post_trace(full_trace: bool) -> Ve if !full_trace { return Vec::new(); } + // Lock the process-global SAB registry ONCE for the whole scan rather than + // once per registered buffer (see `registered_buffer_is_dead_post_trace`). + // `None` — nearly every process — means no SAB was ever allocated. + let shared_sabs = crate::shared_sab::snapshot_shared_sabs(); BUFFER_REGISTRY.with(|r| { r.borrow() .iter() .copied() - .filter(|&addr| unsafe { registered_buffer_is_dead_post_trace(addr) }) + .filter(|&addr| unsafe { + registered_buffer_is_dead_post_trace(addr, shared_sabs.as_ref()) + }) .collect() }) } -unsafe fn registered_buffer_is_dead_post_trace(addr: usize) -> bool { +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. + if shared_sabs.is_some_and(|sabs| sabs.contains(&addr)) { + return false; + } let Some(header) = crate::value::addr_class::try_read_gc_header(addr) else { return false; }; @@ -487,6 +533,34 @@ pub(crate) fn finalize_collected_dead_buffer(addr: usize) { ARRAY_BUFFER_REGISTRY.with(|r| { r.borrow_mut().remove(&addr); }); + // #6337: the two sibling buffer-identity registries were missing from this + // list — they had no `.remove`/`.retain` site anywhere in the tree. Like + // the three above they are plain address-keyed sets that never rooted the + // `BufferHeader`, so a collected view left its entry behind forever: + // + // * an unbounded leak — one permanent entry per `DataView` (and per + // SAB-flagged buffer) ever created; + // * the #6080 ABA class this function exists to prevent — + // `arena_reset_empty_blocks` resets a fully-empty block's offset to 0 + // while KEEPING its base pointer, so a reset block re-issues the same + // addresses. A recycled address then inherits the dead view's identity: + // `is_data_view`/`is_shared_array_buffer` gate `util.types.isDataView`/ + // `isSharedArrayBuffer`, `ArrayBuffer.isView`, the `[object DataView]` + // tag, and the structuredClone/`.slice()` re-marking above — an + // unrelated fresh Buffer landing there would answer to all of them. + // + // Only GC-heap buffers reach here. A process-global SAB backing is never + // freed and is vetoed as a dead candidate in + // `registered_buffer_is_dead_post_trace`, so the entries pruned from + // SHARED_ARRAY_BUFFER_REGISTRY are the arena-allocated SAB-flagged copies + // (`SharedArrayBuffer.prototype.slice`, structuredClone) — the ones that + // genuinely die and whose addresses genuinely get recycled. + SHARED_ARRAY_BUFFER_REGISTRY.with(|r| { + r.borrow_mut().remove(&addr); + }); + DATA_VIEW_REGISTRY.with(|r| { + r.borrow_mut().remove(&addr); + }); BUFFER_AB_ALIAS.with(|r| { r.borrow_mut().remove(&addr); }); diff --git a/crates/perry-runtime/src/buffer/mod.rs b/crates/perry-runtime/src/buffer/mod.rs index c02255851..bd96b5be0 100644 --- a/crates/perry-runtime/src/buffer/mod.rs +++ b/crates/perry-runtime/src/buffer/mod.rs @@ -43,6 +43,8 @@ pub use header::{ pub(crate) use header::{ collect_dead_registered_buffers_post_trace, finalize_collected_dead_buffer, }; +#[cfg(test)] +pub(crate) use header::{test_data_view_registry_len, test_shared_array_buffer_registry_len}; // ---- Re-exports: ArrayBuffer detach / transfer (ES2024) ---- // `detach_array_buffer` dereferences the raw address it is given, so it stays diff --git a/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs b/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs new file mode 100644 index 000000000..b5a50b611 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs @@ -0,0 +1,236 @@ +//! Death pruning for the buffer-identity side tables (#6337). +//! +//! `finalize_collected_dead_buffer` exists to drop "every registry/side-table +//! entry keyed by a dead buffer's address" — its own doc comment cites +//! preventing the #6080 ABA class. But `DATA_VIEW_REGISTRY` and +//! `SHARED_ARRAY_BUFFER_REGISTRY` were never pruned anywhere in the tree: they +//! had zero `.remove`/`.retain` sites. That leaked one permanent entry per +//! `DataView` / SAB-flagged buffer ever created, and let a recycled address +//! (`arena_reset_empty_blocks` resets a fully-empty block's offset to 0 while +//! KEEPING its base pointer) inherit a dead view's identity. +//! +//! The SharedArrayBuffer side has a wrinkle the DataView side does not: a +//! *process-global* SAB backing (`shared_sab::alloc_shared_sab`) is not a GC +//! allocation at all and is never freed, so it must be vetoed as a dead +//! candidate rather than pruned — see +//! `test_seeded_shared_sab_is_never_a_dead_buffer_candidate`. + +use super::super::*; +use super::support::*; + +fn full_gc() { + let _ = + gc_collect_full_mark_sweep_with_trigger(GcTriggerSnapshot::capture(GcTriggerKind::Direct)); +} + +/// Drops a test-seeded `shared_sab` entry even if the test panics — the +/// registry is process-global and never cleared, so a leaked seed would make +/// every later test in this binary treat that address as a live SAB backing. +struct SeededSharedSabGuard(usize); + +impl Drop for SeededSharedSabGuard { + fn drop(&mut self) { + crate::shared_sab::test_unseed_shared_sab(self.0); + } +} + +/// A dead `DataView`'s `DATA_VIEW_REGISTRY` entry must go. `is_data_view` gates +/// `util.types.isDataView`, `ArrayBuffer.isView`, the `[object DataView]` tag, +/// and the structuredClone / `.slice()` re-marking paths — a fresh Buffer +/// landing on the recycled address would answer to all of them. +#[test] +fn test_dead_data_view_registry_entry_pruned_on_full_gc() { + let _guard = GcTestIsolationGuard::new(); + + let addr = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_data_view(addr); + assert!( + crate::buffer::is_data_view(addr), + "test premise: the view is registered" + ); + + // No roots: dead at the full trace. (Buffers are TENURED old-gen residents, + // so only a FULL trace can prove them dead.) + full_gc(); + + assert!( + !crate::buffer::is_data_view(addr), + "dead buffer must not keep its DataView identity — a recycled address \ + would answer to util.types.isDataView / ArrayBuffer.isView" + ); +} + +/// The SAB-flagged buffers that actually die are the arena-allocated copies: +/// `SharedArrayBuffer.prototype.slice` (`object/buffer_dispatch.rs`) and +/// structuredClone (`builtins/globals.rs`) both `buffer_alloc` a fresh +/// `BufferHeader` and re-`mark_as_shared_array_buffer` it. Those are ordinary +/// GC-heap objects whose addresses get recycled. +#[test] +fn test_dead_shared_array_buffer_flag_pruned_on_full_gc() { + let _guard = GcTestIsolationGuard::new(); + + let addr = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_shared_array_buffer(addr); + assert!( + crate::buffer::is_shared_array_buffer(addr), + "test premise: the SAB flag is registered" + ); + + full_gc(); + + assert!( + !crate::buffer::is_shared_array_buffer(addr), + "dead SAB-flagged buffer must not keep its identity — a recycled \ + address would answer to util.types.isSharedArrayBuffer" + ); +} + +/// The safety inverse: a LIVE (rooted) view keeps its flag. Full mark-sweep is +/// non-moving, so the rooted buffers keep their addresses. +/// +/// `CopyingNurseryTestGuard::new(2)` — not `GcTestIsolationGuard` — because +/// only it pushes the shadow frame that makes `js_shadow_slot_set` an actual +/// root. Under the plain isolation guard the slot writes land in no frame, the +/// buffers stay unreachable, and the test would "pass" for the wrong reason. +#[test] +fn test_live_data_view_and_shared_array_buffer_flags_survive_full_gc() { + let _guard = CopyingNurseryTestGuard::new(2); + + let view = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_data_view(view); + let sab = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_shared_array_buffer(sab); + + js_shadow_slot_set(0, ptr_bits(view)); + js_shadow_slot_set(1, ptr_bits(sab)); + + full_gc(); + + assert!( + crate::buffer::is_data_view(view), + "a live (rooted) DataView must keep its registry entry" + ); + assert!( + crate::buffer::is_shared_array_buffer(sab), + "a live (rooted) SAB-flagged buffer must keep its registry entry" + ); +} + +/// The leak regression: N views, all references dropped, one full collection, +/// and both registries must DRAIN. A per-address `is_*` probe cannot show this +/// — before the fix the tables grew monotonically for the life of the process, +/// one permanent entry per `DataView` / SAB-flagged buffer ever created. +#[test] +fn test_data_view_and_sab_registries_drain_after_owners_die() { + let _guard = GcTestIsolationGuard::new(); + + const N: usize = 4096; + let base_views = crate::buffer::test_data_view_registry_len(); + let base_sabs = crate::buffer::test_shared_array_buffer_registry_len(); + + for _ in 0..N { + let view = crate::buffer::buffer_alloc(16) as usize; + crate::buffer::mark_as_data_view(view); + let sab = crate::buffer::buffer_alloc(16) as usize; + crate::buffer::mark_as_shared_array_buffer(sab); + } + + assert_eq!( + crate::buffer::test_data_view_registry_len(), + base_views + N, + "test premise: every DataView registered" + ); + assert_eq!( + crate::buffer::test_shared_array_buffer_registry_len(), + base_sabs + N, + "test premise: every SAB-flagged buffer registered" + ); + + // Every one of them is unreachable — no shadow slot, no global root. + full_gc(); + + assert_eq!( + crate::buffer::test_data_view_registry_len(), + base_views, + "DATA_VIEW_REGISTRY must drain when its buffers are swept (it had no \ + .remove site at all before #6337)" + ); + assert_eq!( + crate::buffer::test_shared_array_buffer_registry_len(), + base_sabs, + "SHARED_ARRAY_BUFFER_REGISTRY must drain when its buffers are swept" + ); +} + +/// A process-global `SharedArrayBuffer` backing must NEVER be treated as a +/// collectable GC object. +/// +/// `alloc_shared_sab` takes the block straight from `alloc_zeroed`: no +/// `GcHeader`, never freed (that is what lets the bytes alias across +/// `perry/thread` agents, #4913). But `js_shared_array_buffer_new` DOES +/// `register_buffer` it, so it reaches the dead-buffer scan on every full +/// trace, where `try_read_gc_header` would sniff the 8 bytes BEFORE the malloc +/// block and read the allocator's own metadata as a header — one arbitrary byte +/// against `GC_TYPE_BUFFER` (10), the next against the mark/pin/forward bits. +/// +/// A chance match declares a LIVE SAB dead, and `finalize_collected_dead_buffer` +/// then runs `view::remove_entries_for_dead_buffer`, which retains on +/// `info.backing != addr` — silently unregistering EVERY live typed-array view +/// over that SAB, i.e. exactly the views cross-agent `Atomics` wait/notify +/// resolve their absolute slot addresses through. +/// +/// That coincidence cannot be forced from a test without writing outside the +/// allocation, so seed an ordinary GC buffer — whose real header genuinely says +/// "dead" — into the SAB registry. It reproduces the same decision +/// deterministically: without the veto this buffer is reported dead. +#[test] +fn test_seeded_shared_sab_is_never_a_dead_buffer_candidate() { + let _guard = GcTestIsolationGuard::new(); + + let addr = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_shared_array_buffer(addr); + crate::shared_sab::test_seed_shared_sab(addr); + let _seeded = SeededSharedSabGuard(addr); + + // Unrooted, and its real GcHeader is unmarked — the scan would list it. + let dead = crate::buffer::collect_dead_registered_buffers_post_trace(true); + + assert!( + !dead.contains(&addr), + "a process-global SAB backing must never be reported dead: it has no \ + GcHeader and is never freed, and finalizing it would unregister every \ + live typed-array view over it (breaking cross-agent Atomics)" + ); +} + +/// End-to-end companion to the veto test: a real `new SharedArrayBuffer(n)` +/// backing survives a full collection with no roots at all, and stays +/// recognisable through every predicate — the thread-local flag, the buffer +/// registry, and the process-global registry the cross-thread serializer and +/// `Atomics` futex keying depend on. +#[test] +fn test_process_global_sab_backing_survives_full_gc_unrooted() { + let _guard = GcTestIsolationGuard::new(); + + let buf = crate::buffer::js_shared_array_buffer_new(64); + let addr = buf as usize; + assert!(crate::shared_sab::is_shared_sab(addr)); + + // Deliberately unrooted. The backing is never freed, so this must be a + // no-op for it — a SAB is reachable from other agents, not from this + // thread's roots. + full_gc(); + + assert!( + crate::shared_sab::is_shared_sab(addr), + "the process-global SAB registry must still recognise the backing" + ); + assert!( + crate::buffer::is_shared_array_buffer(addr), + "the SAB must still answer util.types.isSharedArrayBuffer" + ); + assert!( + crate::buffer::is_registered_buffer(addr), + "the SAB must still answer as a registered buffer" + ); +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 3941487d1..5190313ad 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -1,6 +1,7 @@ mod alloc; mod barrier; mod budgeted_step_api; +mod buffer_side_tables; mod contract; mod copying; mod copying_side_tables; diff --git a/crates/perry-runtime/src/shared_sab.rs b/crates/perry-runtime/src/shared_sab.rs index 2fa5af840..f91806798 100644 --- a/crates/perry-runtime/src/shared_sab.rs +++ b/crates/perry-runtime/src/shared_sab.rs @@ -22,6 +22,7 @@ use std::alloc::{alloc_zeroed, handle_alloc_error, Layout}; use std::collections::HashSet; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Mutex, OnceLock}; use crate::buffer::BufferHeader; @@ -29,6 +30,17 @@ use crate::buffer::BufferHeader; /// 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` +/// 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 +/// per serialized pointer, #6009) and the GC's dead-buffer scan, which probes +/// every registered buffer on every full trace. Without the latch both take the +/// registry mutex on each miss. The overwhelming majority of processes never +/// allocate a `SharedArrayBuffer` at all, so they can answer `false` from a +/// single relaxed atomic load and never touch the lock. +static SHARED_SAB_NONEMPTY: AtomicBool = AtomicBool::new(false); + fn registry() -> &'static Mutex> { SHARED_SAB_REGISTRY.get_or_init(|| Mutex::new(HashSet::new())) } @@ -63,16 +75,67 @@ pub fn alloc_shared_sab(size: u32) -> *mut BufferHeader { .lock() .unwrap_or_else(|e| e.into_inner()) .insert(buf as usize); + // Release-store AFTER the insert is visible, so a thread that observes the + // latch also observes the entry it was latched for. + SHARED_SAB_NONEMPTY.store(true, Ordering::Release); buf } /// True if `addr` is a process-global `SharedArrayBuffer` backing store. Unlike /// the thread-local `buffer::is_shared_array_buffer`, this answers correctly on /// every thread — used by the cross-thread serializer to recognise a SAB -/// pointer that has no `GcHeader`. +/// pointer that has no `GcHeader`, and by the GC's dead-buffer scan to refuse +/// to treat one as a collectable GC allocation (see +/// `buffer::header::registered_buffer_is_dead_post_trace`). pub fn is_shared_sab(addr: usize) -> bool { + if !SHARED_SAB_NONEMPTY.load(Ordering::Acquire) { + return false; + } registry() .lock() .map(|r| r.contains(&addr)) .unwrap_or(false) } + +/// Snapshot the SAB backing set for one GC dead-buffer scan. +/// +/// The scan tests every registered buffer, and calling [`is_shared_sab`] per +/// buffer would take the registry mutex once per buffer per full trace. The set +/// is tiny (a handful of entries even in heavy `Atomics` users) and the GC is +/// stop-the-world here, so lock it once and hand the scan a plain set. `None` — +/// the case for nearly every process — means no SAB was ever allocated and the +/// scan can skip the check entirely without allocating anything. +pub(crate) fn snapshot_shared_sabs() -> Option> { + if !SHARED_SAB_NONEMPTY.load(Ordering::Acquire) { + return None; + } + registry().lock().ok().map(|r| r.clone()) +} + +/// Test-only: pretend `addr` is a process-global SAB backing. +/// +/// A real backing has no `GcHeader`, so the GC's dead-buffer scan can only +/// mistake it for a collectable object when the malloc metadata preceding the +/// block happens to read as a dead `GC_TYPE_BUFFER` header — a coincidence a +/// test cannot force without writing outside the allocation. Seeding an +/// ordinary GC buffer (whose real header genuinely says "dead") into this +/// registry reproduces the same decision deterministically, so the veto in +/// `buffer::header::registered_buffer_is_dead_post_trace` can be proven rather +/// than assumed. Callers MUST pair this with [`test_unseed_shared_sab`] — the +/// registry is process-global and never cleared. +#[cfg(test)] +pub(crate) fn test_seed_shared_sab(addr: usize) { + registry() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(addr); + SHARED_SAB_NONEMPTY.store(true, Ordering::Release); +} + +#[cfg(test)] +pub(crate) fn test_unseed_shared_sab(addr: usize) { + registry() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .remove(&addr); +}