From 20b5a8a281386a08342c0ec8fca51021599c2e84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 13 Aug 2026 14:38:29 +0200 Subject: [PATCH 1/2] fix(gc): classify array growth forwarding by ownership (#8035) --- crates/perry-runtime/src/array/header.rs | 170 +++++-------------- crates/perry-runtime/src/array/push_pop.rs | 109 ++++-------- crates/perry-runtime/src/array/tests.rs | 110 ++++++++++++ crates/perry-runtime/src/value/addr_class.rs | 133 +++++++++++++++ 4 files changed, 324 insertions(+), 198 deletions(-) diff --git a/crates/perry-runtime/src/array/header.rs b/crates/perry-runtime/src/array/header.rs index 4c586410d5..c90d960e61 100644 --- a/crates/perry-runtime/src/array/header.rs +++ b/crates/perry-runtime/src/array/header.rs @@ -537,15 +537,11 @@ pub(crate) fn test_template_raw_roots() -> (usize, usize) { /// Strip NaN-boxing tags from an array pointer and guard against invalid values. /// -/// Issue #73 follow-up: the `> 0x1000` (4 KB) floor is too permissive -/// for the macOS ARM64 heap layout. A corrupted NaN-box whose 48-bit -/// handle lands in the 1 TB — 2 TB window (e.g. `0x00FF_0000_0000` — -/// a `BufferHeader { length: 0, capacity: 255 }` read as u64) clears -/// the old floor and segfaults `(*arr).length` / SIMD memcpy inside -/// `js_array_slice` / `js_array_length` / etc. Real mimalloc + arena -/// allocations on Darwin consistently land in the 3-5 TB range; -/// constraining to `>= 2 TB && < 128 TB` rejects the observed -/// corruption patterns without cutting off any real heap pointer. +/// Issue #73/#8035 follow-up: address magnitude is not proof of ownership. +/// Corrupted NaN-box payloads can land in a plausible heap window, while real +/// macOS mimalloc arena allocations can land below the former 2 TiB floor. +/// Registry-handle bands are rejected first; GC headers are read only after +/// arena page metadata or the malloc registry proves ownership. /// /// v0.5.85 follow-up: also validate the GC header byte + length/capacity /// sanity. A pointer that passes the range check but points into the @@ -553,52 +549,11 @@ pub(crate) fn test_template_raw_roots() -> (usize, usize) { /// e.g. decoded PostgreSQL text column data) reads garbage length /// values — witnessed `len=775370038 cap=926234674` (both the ASCII /// bytes of `"6+2.2017"`) flowing through `js_array_slice` and -/// triggering 22GB-wide memcpy segfaults. Post-check: obj_type at -/// `handle-8` must equal GC_TYPE_ARRAY (1), and length must be -/// <= capacity <= 16M (same bound as the GC tracer's sanity guard). +/// triggering 22GB-wide memcpy segfaults. The post-check therefore requires +/// `GC_TYPE_ARRAY` and validates length/capacity before any element access +/// (with the registered Buffer/TypedArray and sparse-array exceptions below). #[inline(always)] pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { - // Heap window varies by allocator and run: macOS mimalloc can land well - // below 2 TB (observed around 45 GB in the Rust test harness); - // Android scudo + Linux glibc also allocate MUCH lower (often < 1 TB); Windows - // mimalloc lands well under 1 TB (often in the GB-to-tens-of-GB range). - // iOS / tvOS / watchOS / visionOS *device* targets use libsystem_malloc - // (mimalloc is host-side only) and allocate in the same low range — - // #1136's `for…of` over `split()` reproed empty because the array - // pointer landed below 2 TB and `clean_arr_ptr` silently null-ed it. - // Using the macOS-tight 2 TB floor on Android / Windows / iOS-family - // silently null-s every real array pointer, turning js_array_set_f64 - // into a no-op and — at the read side via js_array_map etc. — - // returning empty arrays for legitimate inputs (issues #385/#386/#387 - // for non-macOS hosts; #1136 for iOS device). - // - // The iOS *simulator* runs on the macOS host's mimalloc and lands in - // the 3-5 TB range like macOS itself; lowering the floor to 4 KB does - // not weaken the guard there because the actual liveness check is the - // GcHeader / obj_type validation downstream. - #[cfg(any( - target_os = "android", - target_os = "macos", - target_os = "linux", - target_os = "windows", - target_os = "ios", - target_os = "tvos", - target_os = "watchos", - target_os = "visionos", - ))] - const HEAP_MIN: u64 = 0x1000; // 4 KB (classic user-space floor) - #[cfg(not(any( - target_os = "android", - target_os = "macos", - target_os = "linux", - target_os = "windows", - target_os = "ios", - target_os = "tvos", - target_os = "watchos", - target_os = "visionos", - )))] - const HEAP_MIN: u64 = 0x200_0000_0000; // 2 TB — retained for unlisted targets - const HEAP_MAX: u64 = 0x8000_0000_0000; // 47-bit userspace cap let bits = arr as u64; let top16 = bits >> 48; let cleaned = if top16 >= 0x7FF8 { @@ -606,24 +561,14 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { return std::ptr::null(); } let cleaned_bits = bits & 0x0000_FFFF_FFFF_FFFF; - if !(HEAP_MIN..HEAP_MAX).contains(&cleaned_bits) { - return std::ptr::null(); - } cleaned_bits as *const ArrayHeader } else { - if !(HEAP_MIN..HEAP_MAX).contains(&bits) { - return std::ptr::null(); - } arr }; - // #5432: reject small-handle ids (fetch/zlib/proxy/common-registry) that - // reached an array helper as the receiver. On non-macOS hosts HEAP_MIN is - // 0x1000 — below the handle band — so a handle like a fetch Headers id - // (0x40000) passes the window check above, and the forwarding-chain / - // obj_type derefs below (`cleaned - 8`) would read unmapped low memory and - // SIGSEGV. Magnitude-classify before any deref and null it (safe - // empty-result no-op), mirroring the addr_class band-map contract. - if crate::value::addr_class::is_handle_band(cleaned as usize) { + // Preserve the permissive window needed by registered Buffer/TypedArray + // receivers, but centralize its platform policy in addr_class. Actual GC + // header reads below require allocator ownership as well. + if !crate::value::addr_class::is_plausible_heap_addr(cleaned as usize) { return std::ptr::null(); } // Issue #233: follow GC_FLAG_FORWARDED forwarding chains. When @@ -641,14 +586,21 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { let mut cleaned = cleaned; unsafe { let mut steps = 0u32; - while (cleaned as usize) >= crate::gc::GC_HEADER_SIZE + 0x1000 { - let gc_header = - (cleaned as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; - if (*gc_header).gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 { + while let Some(gc_header) = + crate::value::addr_class::try_read_tracked_gc_header(cleaned as usize) + { + if gc_header.obj_type != crate::gc::GC_TYPE_ARRAY + || gc_header.gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 + { break; } - let new_user = crate::gc::forwarding_address(gc_header) as u64; - if !(HEAP_MIN..HEAP_MAX).contains(&new_user) { + let new_user = crate::gc::forwarding_address(gc_header) as usize; + let Some(target_header) = + crate::value::addr_class::try_read_tracked_gc_header(new_user) + else { + return std::ptr::null(); + }; + if target_header.obj_type != crate::gc::GC_TYPE_ARRAY { return std::ptr::null(); } cleaned = new_user as *const ArrayHeader; @@ -665,56 +617,32 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { // into a real ArrayHeader and substitute the materialized // pointer for every downstream accessor. O(1) on subsequent // calls (idempotent via the `materialized` cache). - unsafe { - if (cleaned as usize) >= crate::gc::GC_HEADER_SIZE + 0x1000 { - let gc_header = - (cleaned as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; - let obj_type = (*gc_header).obj_type; - if obj_type == crate::gc::GC_TYPE_LAZY_ARRAY { + let addr = cleaned as usize; + let tracked_header = unsafe { crate::value::addr_class::try_read_tracked_gc_header(addr) }; + if let Some(gc_header) = tracked_header { + if gc_header.obj_type == crate::gc::GC_TYPE_LAZY_ARRAY { + unsafe { let lazy = cleaned as *mut crate::json_tape::LazyArrayHeader; if (*lazy).magic == crate::json_tape::LAZY_ARRAY_MAGIC { let materialized = crate::json_tape::force_materialize_lazy(lazy); return materialized as *const ArrayHeader; } } - // #7574: a `GC_TYPE_OBJECT` / `GC_TYPE_CLOSURE` allocation is NOT - // an `ArrayHeader`, and the two layouts overlay field for field — - // `ArrayHeader.length` reads `ObjectHeader.object_type` (= 1), - // `.capacity` reads `class_id`, and the element slots at +8/+16/+24 - // are `parent_class_id ‖ field_count`, `keys_array` and `meta`. The - // sanity check below waves that through (1 <= class_id <= 100M), so - // an element WRITE overwrites two live GC child edges with - // arbitrary doubles and the collector then traces them: `class X - // extends Array` in a `T[]`-annotated binding SIGSEGVs on its - // second `.push()`. - // - // A declared TypeScript type is a hint, never a layout fact - // (CLAUDE.md, *Known Limitations*), so this is reachable from every - // binding form. Refusing it here makes ALL ~190 `clean_arr_ptr` - // call sites fail-closed at once — each degrades through its - // existing null branch instead of dereferencing a forged header — - // and it is the same "resolve at the shared runtime funnel, not at - // one codegen predicate at a time" shape #7573 used for Map/Set. - // - // Correctness (rather than mere safety) for the entry points the - // declared-type tiers actually reach is layered on top: those null - // branches re-enter through `array::subclass::array_object_*`, - // which runs the operation on the spec-generic array-like engine. - // - // Costs one compare on a byte this block already loaded. Buffers - // and typed arrays are `std::alloc`-backed with no `GcHeader`, so - // their preceding bytes are allocator bookkeeping that can read as - // any value — confirm against the registries before nulling, in the - // cold arm only. - if obj_type == crate::gc::GC_TYPE_OBJECT || obj_type == crate::gc::GC_TYPE_CLOSURE { - let addr = cleaned as usize; - if !crate::buffer::is_registered_buffer(addr) - && crate::typedarray::lookup_typed_array_kind(addr).is_none() - { - return std::ptr::null(); - } - } } + // A declared TypeScript type is a hint, never a layout fact. Reject + // every tracked non-array at this shared funnel before treating its + // payload as an ArrayHeader (#7574). + if gc_header.obj_type != crate::gc::GC_TYPE_ARRAY { + return std::ptr::null(); + } + } else if !crate::buffer::is_registered_buffer(addr) + && crate::typedarray::lookup_typed_array_kind(addr).is_none() + { + // Handles, synthetic pointers, and unrelated allocations must be + // rejected before any GcHeader or ArrayHeader dereference. Registered + // Buffer/TypedArray receivers intentionally use the compatible + // length/capacity prefix and carry no GcHeader. + return std::ptr::null(); } // Length/capacity sanity: dense arrays have length <= capacity and // length below 100M (800 MB of element payload — well above legitimate @@ -732,15 +660,11 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { // wave them through; everything else at this size is // almost certainly corrupted. let addr = cleaned as usize; - let sparse_array_shape = if addr >= crate::gc::GC_HEADER_SIZE + 0x1000 { - let gc_header = (cleaned as *const u8).sub(crate::gc::GC_HEADER_SIZE) - as *const crate::gc::GcHeader; - (*gc_header).obj_type == crate::gc::GC_TYPE_ARRAY + let sparse_array_shape = tracked_header.is_some_and(|gc_header| { + gc_header.obj_type == crate::gc::GC_TYPE_ARRAY && hdr.length > hdr.capacity && hdr.capacity <= 1_000_000 - } else { - false - }; + }); if sparse_array_shape { return cleaned; } diff --git a/crates/perry-runtime/src/array/push_pop.rs b/crates/perry-runtime/src/array/push_pop.rs index 8d69c32eab..8f54b18e25 100644 --- a/crates/perry-runtime/src/array/push_pop.rs +++ b/crates/perry-runtime/src/array/push_pop.rs @@ -35,6 +35,32 @@ fn throw_non_writable_length() -> ! { ); } +/// Install an array-growth forwarding stub after `tracked_header_for` proves +/// allocator ownership of `old_user_addr`. Keeping the classifier injectable +/// makes the below-2-TiB macOS case deterministic without mapping a fixed low +/// virtual address in the test process. +/// +/// # Safety +/// The classifier must return the live header for `old_user_addr`, and +/// `new_user_addr` must be a live array allocation. +#[inline] +pub(super) unsafe fn install_array_growth_forwarding_with( + old_user_addr: usize, + new_user_addr: *mut u8, + tracked_header_for: impl FnOnce(usize) -> Option<&'static crate::gc::GcHeader>, +) -> bool { + let Some(header) = tracked_header_for(old_user_addr) else { + return false; + }; + if header.obj_type != crate::gc::GC_TYPE_ARRAY + || header.gc_flags & crate::gc::GC_FLAG_ARENA == 0 + { + return false; + } + crate::gc::set_forwarding_address(std::ptr::from_ref(header).cast_mut(), new_user_addr); + true +} + #[inline] pub(crate) fn guard_writable_length(arr: *const ArrayHeader) { if array_length_is_non_writable(arr) { @@ -135,46 +161,14 @@ pub extern "C" fn js_array_grow(arr: *mut ArrayHeader, min_capacity: u32) -> *mu // ptr. Unlike GC-evacuation originals, array-growth stubs stay // retained because stale array references rely on clean_arr_ptr // following this chain. - // Only valid for arena-allocated arrays (which have a GcHeader - // 8 bytes before the user pointer); guard with a heap-bounds - // check that mirrors clean_arr_ptr's HEAP_MIN to skip pointers - // that don't have a real GcHeader behind them (e.g. test-mode - // synthetic pointers, longlived-arena edge cases). - // #1136: iOS family device allocates via libsystem_malloc in the - // same low range as Android/Linux; mirror `clean_arr_ptr`'s - // platform split so growth forwarding can install a stub for - // arrays that live below 2 TB. - #[cfg(any( - target_os = "android", - target_os = "linux", - target_os = "windows", - target_os = "ios", - target_os = "tvos", - target_os = "watchos", - target_os = "visionos", - ))] - const HEAP_MIN: usize = 0x1000; - #[cfg(not(any( - target_os = "android", - target_os = "linux", - target_os = "windows", - target_os = "ios", - target_os = "tvos", - target_os = "watchos", - target_os = "visionos", - )))] - const HEAP_MIN: usize = 0x200_0000_0000; - if (arr as usize) >= HEAP_MIN + crate::gc::GC_HEADER_SIZE { - // Only forward arrays that came from the GC arena. A - // non-array obj_type would mean something has gone wrong - // upstream; bail out without forwarding rather than corrupt - // an unrelated allocation's header. - if (*old_header).obj_type == crate::gc::GC_TYPE_ARRAY { - crate::gc::set_forwarding_address(old_header, new_ptr as *mut u8); - } - } else { - report_growth_stub_skipped_below_heap_min(arr as usize); - } + // Ownership comes from the same canonical arena/malloc classifier + // that clean_arr_ptr uses while following this stub. In particular, + // valid low-address macOS arena allocations are accepted, while + // handles, synthetic pointers, and unrelated allocations are rejected + // before a header dereference. + let _ = install_array_growth_forwarding_with(arr as usize, new_ptr as *mut u8, |addr| { + crate::value::addr_class::try_read_tracked_gc_header(addr) + }); new_ptr } @@ -1109,38 +1103,3 @@ pub extern "C" fn js_array_unshift_variadic( #[used] static KEEP_UNSHIFT_VARIADIC: extern "C" fn(*mut ArrayHeader, *const f64, u32) -> *mut ArrayHeader = js_array_unshift_variadic; - -/// The `HEAP_MIN` guard above is an **address-conditional silent divergence**: -/// whether a growth forwarding stub is installed depends on where the allocator -/// happened to place the array. Below the floor the stub is skipped, so a stale -/// pre-grow reference stops resolving (issue #233's whole mechanism) — with no -/// signal at all. -/// -/// That silence has already cost real debugging time. While investigating -/// #7022 an experiment replaced arena blocks with `mmap`'d, guard-paged blocks; -/// `mmap` with a NULL hint lands well below macOS's 2 TB floor, so this branch -/// silently disabled every growth stub and the experiment came back **falsely -/// clean**. It was only caught by re-running with a high `MAP_FIXED` hint. -/// -/// Emit once per process so the next person gets a signal instead of a silent -/// behaviour change. One line on stderr, so it cannot perturb a stdout parity -/// comparison. -#[cold] -fn report_growth_stub_skipped_below_heap_min(arr_addr: usize) { - use std::sync::atomic::{AtomicBool, Ordering}; - static REPORTED: AtomicBool = AtomicBool::new(false); - debug_assert!( - false, - "array-growth forwarding stub skipped: array at {arr_addr:#x} is below the platform heap floor" - ); - if REPORTED.swap(true, Ordering::Relaxed) { - return; - } - eprintln!( - "[perry-gc] array-growth forwarding stub SKIPPED for an array at {arr_addr:#x}: \ -address is below this platform's heap floor. Stale pre-grow array references \ -will no longer resolve through the growth chain (issue #233). This is normally \ -unreachable; it usually means the arena is being backed by an allocator that \ -places blocks outside the expected range. Reported once per process." - ); -} diff --git a/crates/perry-runtime/src/array/tests.rs b/crates/perry-runtime/src/array/tests.rs index 8cf29cf12e..b1b92e8858 100644 --- a/crates/perry-runtime/src/array/tests.rs +++ b/crates/perry-runtime/src/array/tests.rs @@ -538,6 +538,116 @@ fn test_array_push_f64_grow_path_preserves_value_and_forwarding() { ); } +#[test] +fn stale_array_reference_survives_three_growths_and_minor_gc() { + let scope = crate::gc::RuntimeHandleScope::new(); + let initial = js_array_alloc(0); + let _root = scope.root_raw_mut_ptr(initial); + let mut head = initial; + + // Capacity progresses 16 -> 32 -> 64 -> 128. Keeping `initial` unchanged + // makes every assertion exercise the complete three-stub chain. + for i in 0..65u32 { + head = js_array_push_f64(head, i as f64); + } + assert_ne!(head, initial); + assert_eq!(clean_arr_ptr_mut(initial), head); + assert_eq!(js_array_length(initial), 65); + for i in 0..65u32 { + assert_eq!(js_array_get_f64(initial, i), i as f64); + } + + // This same test is run under PERRY_GC_FORCE_EVACUATE=1 and + // PERRY_GC_VERIFY_EVACUATION=1 by the issue-specific validation command. + // The rooted stale reference must still lead to the live head afterward. + let _ = crate::gc::gc_collect_minor(); + assert_eq!(js_array_length(initial), 65); + for i in 0..65u32 { + assert_eq!(js_array_get_f64(initial, i), i as f64); + } +} + +#[test] +fn injected_low_address_array_receives_growth_forwarding_stub() { + const LOW_USER: usize = 0x1_0000_0008; + const { assert!(LOW_USER < 0x200_0000_0000) }; + let old = js_array_alloc(0); + let new = js_array_alloc(0); + unsafe { + let old_header = + crate::value::addr_class::try_read_tracked_gc_header(old as usize).unwrap(); + let header_ptr = std::ptr::from_ref(old_header).cast_mut(); + let flags = old_header.gc_flags; + let payload = *(old as *const u64); + + let installed = super::push_pop::install_array_growth_forwarding_with( + LOW_USER, + new as *mut u8, + |candidate| { + assert_eq!(candidate, LOW_USER); + Some(old_header) + }, + ); + let resolved = clean_arr_ptr(old); + + *(old as *mut u64) = payload; + (*header_ptr).gc_flags = flags; + assert!(installed); + assert_eq!(resolved, new); + } +} + +#[test] +fn clean_arr_ptr_rejects_forwarding_cycle() { + let first = js_array_alloc(0); + let second = js_array_alloc(0); + unsafe { + let first_header = std::ptr::from_ref( + crate::value::addr_class::try_read_tracked_gc_header(first as usize).unwrap(), + ) + .cast_mut(); + let second_header = std::ptr::from_ref( + crate::value::addr_class::try_read_tracked_gc_header(second as usize).unwrap(), + ) + .cast_mut(); + let first_flags = (*first_header).gc_flags; + let second_flags = (*second_header).gc_flags; + let first_payload = *(first as *const u64); + let second_payload = *(second as *const u64); + crate::gc::set_forwarding_address(first_header, second as *mut u8); + crate::gc::set_forwarding_address(second_header, first as *mut u8); + + let resolved = clean_arr_ptr(first); + + *(first as *mut u64) = first_payload; + *(second as *mut u64) = second_payload; + (*first_header).gc_flags = first_flags; + (*second_header).gc_flags = second_flags; + assert!(resolved.is_null()); + } +} + +#[test] +fn clean_arr_ptr_rejects_untracked_forwarding_target_without_deref() { + let array = js_array_alloc(0); + let unrelated = 0x20_0000usize as *mut u8; + unsafe { + let header = std::ptr::from_ref( + crate::value::addr_class::try_read_tracked_gc_header(array as usize).unwrap(), + ) + .cast_mut(); + let flags = (*header).gc_flags; + let payload = *(array as *const u64); + crate::gc::set_forwarding_address(header, unrelated); + + let resolved = clean_arr_ptr(array); + + *(array as *mut u64) = payload; + (*header).gc_flags = flags; + assert!(resolved.is_null()); + } +} + #[test] fn test_numeric_array_layout_metadata_preserves_and_downgrades_on_writes() { let mut arr = js_array_alloc(4); diff --git a/crates/perry-runtime/src/value/addr_class.rs b/crates/perry-runtime/src/value/addr_class.rs index afdea46ec8..842ecb205e 100644 --- a/crates/perry-runtime/src/value/addr_class.rs +++ b/crates/perry-runtime/src/value/addr_class.rs @@ -218,6 +218,73 @@ pub(crate) unsafe fn try_read_gc_header(addr: usize) -> Option<&'static GcHeader Some(&*((addr - GC_HEADER_SIZE) as *const GcHeader)) } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum TrackedGcStorage { + Arena, + Malloc, +} + +/// Classify a candidate user address without dereferencing it. +/// +/// The injected lookups keep the safety policy independently testable: a +/// low-address arena range must win on allocator membership, while every +/// handle-band value must be rejected before either lookup runs. +#[inline] +fn classify_tracked_gc_header_with( + addr: usize, + arena_range_base: impl FnOnce(usize) -> Option, + malloc_header_is_tracked: impl FnOnce(*const GcHeader) -> bool, +) -> Option<(usize, TrackedGcStorage)> { + if is_handle_band(addr) { + return None; + } + let header_addr = addr.checked_sub(GC_HEADER_SIZE)?; + if let Some(range_base) = arena_range_base(addr) { + // The payload and its header must belong to the same registered + // range. This prevents a candidate at the first bytes of a mapped + // range from back-reading the preceding page (#7742). + return (header_addr >= range_base).then_some((header_addr, TrackedGcStorage::Arena)); + } + malloc_header_is_tracked(header_addr as *const GcHeader) + .then_some((header_addr, TrackedGcStorage::Malloc)) +} + +/// Read a `GcHeader` only after allocator-owned metadata proves that `addr` is +/// a Perry GC allocation. Unlike [`try_read_gc_header`], this does not use an +/// address-magnitude window as evidence of ownership: arena page membership or +/// an exact malloc-registry hit is required before the first header byte is +/// touched. The header's type, size, and arena flag are then validated. +/// +/// This is the canonical gate for code that must distinguish live Perry GC +/// allocations from registry handles, synthetic pointers, and unrelated +/// allocations before dereferencing a header. +/// +/// # Safety +/// The returned reference is valid only while the allocation remains live and +/// on the current runtime thread. Callers must not retain it across allocation +/// or collection safepoints. +#[inline] +pub(crate) unsafe fn try_read_tracked_gc_header(addr: usize) -> Option<&'static GcHeader> { + let (header_addr, storage) = classify_tracked_gc_header_with( + addr, + |candidate| crate::arena::classify_heap_space_in_range(candidate).map(|(_, base)| base), + crate::gc::gc_malloc_header_is_tracked, + )?; + let header = &*(header_addr as *const GcHeader); + if crate::gc::gc_type_info(header.obj_type).is_none() { + return None; + } + let size = header.size as usize; + if size < GC_HEADER_SIZE || size as u64 > (1u64 << 34) { + return None; + } + let header_is_arena = header.gc_flags & crate::gc::GC_FLAG_ARENA != 0; + if header_is_arena != matches!(storage, TrackedGcStorage::Arena) { + return None; + } + Some(header) +} + #[cfg(test)] mod tests { use super::*; @@ -264,6 +331,72 @@ mod tests { assert!(unsafe { try_read_gc_header(0x7FFD_0000_0000_0000) }.is_none()); } + #[test] + fn tracked_gc_classifier_accepts_injected_low_arena_membership() { + use std::cell::Cell; + + // 4 GiB is well below the legacy 2 TiB macOS floor. Membership in a + // registered arena range, not this magnitude, is the ownership proof. + const LOW_USER: usize = 0x1_0000_0008; + const LOW_RANGE_BASE: usize = LOW_USER - GC_HEADER_SIZE; + const { assert!(LOW_USER < 0x200_0000_0000) }; + let malloc_lookup_ran = Cell::new(false); + assert_eq!( + classify_tracked_gc_header_with( + LOW_USER, + |_| Some(LOW_RANGE_BASE), + |_| { + malloc_lookup_ran.set(true); + false + }, + ), + Some((LOW_RANGE_BASE, TrackedGcStorage::Arena)) + ); + assert!(!malloc_lookup_ran.get()); + } + + #[test] + fn tracked_gc_classifier_rejects_handle_boundaries_before_lookup() { + for addr in [0, 1, COMMON_HANDLE_BAND_END, HANDLE_BAND_MAX - 1] { + assert_eq!( + classify_tracked_gc_header_with( + addr, + |_| panic!("handle must not reach the arena classifier"), + |_| panic!("handle must not reach the malloc registry"), + ), + None + ); + } + + // The first address outside the handle band is still rejected when + // neither allocator owns it, without a header dereference. + assert_eq!( + classify_tracked_gc_header_with(HANDLE_BAND_MAX, |_| None, |_| false), + None + ); + } + + #[test] + fn try_read_tracked_gc_header_rejects_unrelated_allocation() { + #[repr(C)] + struct SyntheticAllocation { + header: GcHeader, + payload: u64, + } + + let synthetic = SyntheticAllocation { + header: GcHeader { + obj_type: crate::gc::GC_TYPE_ARRAY, + gc_flags: 0, + _reserved: 0, + size: std::mem::size_of::() as u32, + }, + payload: 0, + }; + let user = &synthetic.payload as *const u64 as usize; + assert!(unsafe { try_read_tracked_gc_header(user) }.is_none()); + } + #[test] fn stream_id_band_is_above_pointer_handles() { assert!(is_stream_id_band(STREAM_ID_BAND_START)); From 7cfea9c4b5da6ea14dd37a27f7d4958cd89cd458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 13 Aug 2026 15:15:44 +0200 Subject: [PATCH 2/2] fix(gc): address array forwarding review blockers --- changelog.d/8041-array-growth-forwarding.md | 1 + crates/perry-runtime/src/array/header.rs | 31 ++++----- crates/perry-runtime/src/array/push_pop.rs | 21 ++++-- crates/perry-runtime/src/array/tests.rs | 67 +++++++++++++------- crates/perry-runtime/src/gc/mod.rs | 5 ++ crates/perry-runtime/src/gc/tests/support.rs | 10 +-- crates/perry-runtime/src/value/addr_class.rs | 46 +++++++++----- 7 files changed, 116 insertions(+), 65 deletions(-) create mode 100644 changelog.d/8041-array-growth-forwarding.md diff --git a/changelog.d/8041-array-growth-forwarding.md b/changelog.d/8041-array-growth-forwarding.md new file mode 100644 index 0000000000..c62ffb751b --- /dev/null +++ b/changelog.d/8041-array-growth-forwarding.md @@ -0,0 +1 @@ +**Fixed: growing arrays at valid low arena addresses now retain stale-reference forwarding across repeated growth and moving minor GC (#8035).** Array header access now requires allocator ownership before dereferencing, rejects invalid forwarding targets, and installs growth forwarding through a checked raw-header pointer. diff --git a/crates/perry-runtime/src/array/header.rs b/crates/perry-runtime/src/array/header.rs index c90d960e61..aba6685dc0 100644 --- a/crates/perry-runtime/src/array/header.rs +++ b/crates/perry-runtime/src/array/header.rs @@ -584,13 +584,14 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { // practice (1-2 grows) but cap depth at 64 to defend against // cycles from corrupted GC state. let mut cleaned = cleaned; + let mut tracked_header = + unsafe { crate::value::addr_class::try_read_tracked_gc_header(cleaned as usize) }; unsafe { let mut steps = 0u32; - while let Some(gc_header) = - crate::value::addr_class::try_read_tracked_gc_header(cleaned as usize) - { - if gc_header.obj_type != crate::gc::GC_TYPE_ARRAY - || gc_header.gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 + while let Some(gc_header) = tracked_header { + let gc_header = gc_header.as_ptr(); + if (*gc_header).obj_type != crate::gc::GC_TYPE_ARRAY + || (*gc_header).gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 { break; } @@ -600,10 +601,11 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { else { return std::ptr::null(); }; - if target_header.obj_type != crate::gc::GC_TYPE_ARRAY { + if (*target_header.as_ptr()).obj_type != crate::gc::GC_TYPE_ARRAY { return std::ptr::null(); } cleaned = new_user as *const ArrayHeader; + tracked_header = Some(target_header); steps += 1; if steps > 64 { return std::ptr::null(); @@ -618,9 +620,10 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { // pointer for every downstream accessor. O(1) on subsequent // calls (idempotent via the `materialized` cache). let addr = cleaned as usize; - let tracked_header = unsafe { crate::value::addr_class::try_read_tracked_gc_header(addr) }; - if let Some(gc_header) = tracked_header { - if gc_header.obj_type == crate::gc::GC_TYPE_LAZY_ARRAY { + let tracked_obj_type = + tracked_header.map(|gc_header| unsafe { (*gc_header.as_ptr()).obj_type }); + if let Some(obj_type) = tracked_obj_type { + if obj_type == crate::gc::GC_TYPE_LAZY_ARRAY { unsafe { let lazy = cleaned as *mut crate::json_tape::LazyArrayHeader; if (*lazy).magic == crate::json_tape::LAZY_ARRAY_MAGIC { @@ -632,7 +635,7 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { // A declared TypeScript type is a hint, never a layout fact. Reject // every tracked non-array at this shared funnel before treating its // payload as an ArrayHeader (#7574). - if gc_header.obj_type != crate::gc::GC_TYPE_ARRAY { + if obj_type != crate::gc::GC_TYPE_ARRAY { return std::ptr::null(); } } else if !crate::buffer::is_registered_buffer(addr) @@ -660,11 +663,9 @@ pub(crate) fn clean_arr_ptr(arr: *const ArrayHeader) -> *const ArrayHeader { // wave them through; everything else at this size is // almost certainly corrupted. let addr = cleaned as usize; - let sparse_array_shape = tracked_header.is_some_and(|gc_header| { - gc_header.obj_type == crate::gc::GC_TYPE_ARRAY - && hdr.length > hdr.capacity - && hdr.capacity <= 1_000_000 - }); + let sparse_array_shape = tracked_obj_type == Some(crate::gc::GC_TYPE_ARRAY) + && hdr.length > hdr.capacity + && hdr.capacity <= 1_000_000; if sparse_array_shape { return cleaned; } diff --git a/crates/perry-runtime/src/array/push_pop.rs b/crates/perry-runtime/src/array/push_pop.rs index 8f54b18e25..3de073abfb 100644 --- a/crates/perry-runtime/src/array/push_pop.rs +++ b/crates/perry-runtime/src/array/push_pop.rs @@ -47,17 +47,18 @@ fn throw_non_writable_length() -> ! { pub(super) unsafe fn install_array_growth_forwarding_with( old_user_addr: usize, new_user_addr: *mut u8, - tracked_header_for: impl FnOnce(usize) -> Option<&'static crate::gc::GcHeader>, + tracked_header_for: impl FnOnce(usize) -> Option>, ) -> bool { let Some(header) = tracked_header_for(old_user_addr) else { return false; }; - if header.obj_type != crate::gc::GC_TYPE_ARRAY - || header.gc_flags & crate::gc::GC_FLAG_ARENA == 0 + let header = header.as_ptr(); + if (*header).obj_type != crate::gc::GC_TYPE_ARRAY + || (*header).gc_flags & crate::gc::GC_FLAG_ARENA == 0 { return false; } - crate::gc::set_forwarding_address(std::ptr::from_ref(header).cast_mut(), new_user_addr); + crate::gc::set_forwarding_address(header, new_user_addr); true } @@ -166,9 +167,15 @@ pub extern "C" fn js_array_grow(arr: *mut ArrayHeader, min_capacity: u32) -> *mu // valid low-address macOS arena allocations are accepted, while // handles, synthetic pointers, and unrelated allocations are rejected // before a header dereference. - let _ = install_array_growth_forwarding_with(arr as usize, new_ptr as *mut u8, |addr| { - crate::value::addr_class::try_read_tracked_gc_header(addr) - }); + let installed = + install_array_growth_forwarding_with(arr as usize, new_ptr as *mut u8, |addr| { + crate::value::addr_class::try_read_tracked_gc_header(addr) + }); + assert!( + installed, + "array growth could not install a forwarding stub for the tracked source at {:#x}", + arr as usize + ); new_ptr } diff --git a/crates/perry-runtime/src/array/tests.rs b/crates/perry-runtime/src/array/tests.rs index b1b92e8858..6248c7a8eb 100644 --- a/crates/perry-runtime/src/array/tests.rs +++ b/crates/perry-runtime/src/array/tests.rs @@ -539,10 +539,13 @@ fn test_array_push_f64_grow_path_preserves_value_and_forwarding() { } #[test] -fn stale_array_reference_survives_three_growths_and_minor_gc() { - let scope = crate::gc::RuntimeHandleScope::new(); +fn stale_array_reference_survives_three_growths_and_forced_minor_gc() { + let _copying_nursery = crate::gc::CopyingNurseryTestGuard::new(0); + let _triggers = crate::gc::GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + let _force_evacuation = crate::gc::knob_overrides::ForcedEvacuationTestGuard::on(); + let _verify_evacuation = crate::gc::knob_overrides::VerifyEvacuationTestGuard::on(); + crate::gc::register_runtime_handle_root_scanner_for_tests(); let initial = js_array_alloc(0); - let _root = scope.root_raw_mut_ptr(initial); let mut head = initial; // Capacity progresses 16 -> 32 -> 64 -> 128. Keeping `initial` unchanged @@ -557,10 +560,30 @@ fn stale_array_reference_survives_three_growths_and_minor_gc() { assert_eq!(js_array_get_f64(initial, i), i as f64); } - // This same test is run under PERRY_GC_FORCE_EVACUATE=1 and - // PERRY_GC_VERIFY_EVACUATION=1 by the issue-specific validation command. - // The rooted stale reference must still lead to the live head afterward. - let _ = crate::gc::gc_collect_minor(); + // Root the current head, not the deliberately stale first allocation: the + // handle must prove that evacuation moved the live array itself, while + // `initial` independently exercises the three growth stubs afterward. + let scope = crate::gc::RuntimeHandleScope::new(); + let root = scope.root_raw_mut_ptr(head); + let pre_gc_head = head; + let cycles_before = crate::gc::copying_minor_cycles(); + let (_, rooted_head) = root.across_mut::(|| { + let _ = crate::gc::gc_collect_minor(); + }); + let cycles_after = crate::gc::copying_minor_cycles(); + assert!( + cycles_after > cycles_before, + "forced collection must complete a copying minor" + ); + assert_ne!( + rooted_head, pre_gc_head, + "forced evacuation must move the live array head" + ); + assert_eq!( + clean_arr_ptr_mut(initial), + rooted_head, + "the stale three-stub chain must resolve to the relocated rooted head" + ); assert_eq!(js_array_length(initial), 65); for i in 0..65u32 { assert_eq!(js_array_get_f64(initial, i), i as f64); @@ -568,7 +591,10 @@ fn stale_array_reference_survives_three_growths_and_minor_gc() { } #[test] -fn injected_low_address_array_receives_growth_forwarding_stub() { +fn install_array_growth_forwarding_with_installs_stub_for_injected_header() { + // Actual low-address classification is covered by + // value::addr_class::tests::tracked_gc_classifier_accepts_injected_low_arena_membership. + // This test proves the install path uses the injected tracked header. const LOW_USER: usize = 0x1_0000_0008; const { assert!(LOW_USER < 0x200_0000_0000) }; let old = js_array_alloc(0); @@ -576,8 +602,8 @@ fn injected_low_address_array_receives_growth_forwarding_stub() { unsafe { let old_header = crate::value::addr_class::try_read_tracked_gc_header(old as usize).unwrap(); - let header_ptr = std::ptr::from_ref(old_header).cast_mut(); - let flags = old_header.gc_flags; + let header_ptr = old_header.as_ptr(); + let flags = (*header_ptr).gc_flags; let payload = *(old as *const u64); let installed = super::push_pop::install_array_growth_forwarding_with( @@ -602,14 +628,12 @@ fn clean_arr_ptr_rejects_forwarding_cycle() { let first = js_array_alloc(0); let second = js_array_alloc(0); unsafe { - let first_header = std::ptr::from_ref( - crate::value::addr_class::try_read_tracked_gc_header(first as usize).unwrap(), - ) - .cast_mut(); - let second_header = std::ptr::from_ref( - crate::value::addr_class::try_read_tracked_gc_header(second as usize).unwrap(), - ) - .cast_mut(); + let first_header = crate::value::addr_class::try_read_tracked_gc_header(first as usize) + .unwrap() + .as_ptr(); + let second_header = crate::value::addr_class::try_read_tracked_gc_header(second as usize) + .unwrap() + .as_ptr(); let first_flags = (*first_header).gc_flags; let second_flags = (*second_header).gc_flags; let first_payload = *(first as *const u64); @@ -632,10 +656,9 @@ fn clean_arr_ptr_rejects_untracked_forwarding_target_without_deref() { let array = js_array_alloc(0); let unrelated = 0x20_0000usize as *mut u8; unsafe { - let header = std::ptr::from_ref( - crate::value::addr_class::try_read_tracked_gc_header(array as usize).unwrap(), - ) - .cast_mut(); + let header = crate::value::addr_class::try_read_tracked_gc_header(array as usize) + .unwrap() + .as_ptr(); let flags = (*header).gc_flags; let payload = *(array as *const u64); crate::gc::set_forwarding_address(header, unrelated); diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 268963a084..bc4748ca16 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -1415,3 +1415,8 @@ mod tests; /// on a parallel test thread can wipe its entries mid-test. #[cfg(test)] pub(crate) use tests::support::copying_nursery_isolation_lock as global_side_table_test_lock; +#[cfg(test)] +pub(crate) use tests::support::{ + register_runtime_handle_root_scanner_for_tests, CopyingNurseryTestGuard, + GcTriggerThresholdTestGuard, +}; diff --git a/crates/perry-runtime/src/gc/tests/support.rs b/crates/perry-runtime/src/gc/tests/support.rs index 9b022a22cd..38bb9da2e5 100644 --- a/crates/perry-runtime/src/gc/tests/support.rs +++ b/crates/perry-runtime/src/gc/tests/support.rs @@ -222,7 +222,7 @@ pub(super) fn root_scanner_registry_counts() -> (usize, usize, usize, usize) { /// `RuntimeHandleScope` inside such a test is decorative — its handles are /// neither marked nor rewritten, so a raw pointer held across a GC-capable /// call is silently unrooted and the test can pass for the wrong reason. -pub(super) fn register_runtime_handle_root_scanner_for_tests() { +pub(crate) fn register_runtime_handle_root_scanner_for_tests() { gc_register_budgeted_mutable_root_scanner_with_source( scan_runtime_handle_roots_mut, scan_runtime_handle_roots_mut_step, @@ -403,7 +403,7 @@ impl Drop for GcTestIsolationGuard { } } -pub(super) struct CopyingNurseryTestGuard { +pub(crate) struct CopyingNurseryTestGuard { frame: u64, _scanner_guard: ScopedRootScannerRegistryGuard, _lock: std::sync::MutexGuard<'static, ()>, @@ -447,7 +447,7 @@ pub(super) fn reset_copying_nursery_runtime_test_state() { } impl CopyingNurseryTestGuard { - pub(super) fn new(slot_count: u32) -> Self { + pub(crate) fn new(slot_count: u32) -> Self { let lock = copying_nursery_isolation_lock(); let scanner_guard = ScopedRootScannerRegistryGuard::new(); reset_copying_nursery_runtime_test_state(); @@ -475,14 +475,14 @@ impl Drop for CopyingNurseryTestGuard { } } -pub(super) struct GcTriggerThresholdTestGuard { +pub(crate) struct GcTriggerThresholdTestGuard { next_arena_trigger: usize, next_malloc_trigger: usize, malloc_step: usize, } impl GcTriggerThresholdTestGuard { - pub(super) fn suppress_automatic_triggers() -> Self { + pub(crate) fn suppress_automatic_triggers() -> Self { let next_arena_trigger = GC_NEXT_TRIGGER_BYTES.with(|trigger| { let previous = trigger.get(); trigger.set(usize::MAX); diff --git a/crates/perry-runtime/src/value/addr_class.rs b/crates/perry-runtime/src/value/addr_class.rs index 842ecb205e..da3a83a575 100644 --- a/crates/perry-runtime/src/value/addr_class.rs +++ b/crates/perry-runtime/src/value/addr_class.rs @@ -249,10 +249,10 @@ fn classify_tracked_gc_header_with( .then_some((header_addr, TrackedGcStorage::Malloc)) } -/// Read a `GcHeader` only after allocator-owned metadata proves that `addr` is -/// a Perry GC allocation. Unlike [`try_read_gc_header`], this does not use an -/// address-magnitude window as evidence of ownership: arena page membership or -/// an exact malloc-registry hit is required before the first header byte is +/// Locate a `GcHeader` only after allocator-owned metadata proves that `addr` +/// is a Perry GC allocation. Unlike [`try_read_gc_header`], this does not use +/// an address-magnitude window as evidence of ownership: arena page membership +/// or an exact malloc-registry hit is required before the first header byte is /// touched. The header's type, size, and arena flag are then validated. /// /// This is the canonical gate for code that must distinguish live Perry GC @@ -260,25 +260,32 @@ fn classify_tracked_gc_header_with( /// allocations before dereferencing a header. /// /// # Safety -/// The returned reference is valid only while the allocation remains live and -/// on the current runtime thread. Callers must not retain it across allocation -/// or collection safepoints. +/// The returned pointer is valid only while the allocation remains live and on +/// the current runtime thread. Callers must not dereference it after an +/// allocation or collection safepoint. Returning a raw pointer is deliberate: +/// some checked callers install forwarding metadata, so this gate must not +/// manufacture a shared reference and then write through a cast of it. #[inline] -pub(crate) unsafe fn try_read_tracked_gc_header(addr: usize) -> Option<&'static GcHeader> { +pub(crate) unsafe fn try_read_tracked_gc_header( + addr: usize, +) -> Option> { let (header_addr, storage) = classify_tracked_gc_header_with( addr, |candidate| crate::arena::classify_heap_space_in_range(candidate).map(|(_, base)| base), crate::gc::gc_malloc_header_is_tracked, )?; - let header = &*(header_addr as *const GcHeader); - if crate::gc::gc_type_info(header.obj_type).is_none() { + if header_addr % std::mem::align_of::() != 0 { + return None; + } + let header = std::ptr::NonNull::new(header_addr as *mut GcHeader)?; + let header_ptr = header.as_ptr(); + if crate::gc::gc_type_info((*header_ptr).obj_type).is_none() { return None; } - let size = header.size as usize; - if size < GC_HEADER_SIZE || size as u64 > (1u64 << 34) { + if ((*header_ptr).size as usize) < GC_HEADER_SIZE { return None; } - let header_is_arena = header.gc_flags & crate::gc::GC_FLAG_ARENA != 0; + let header_is_arena = (*header_ptr).gc_flags & crate::gc::GC_FLAG_ARENA != 0; if header_is_arena != matches!(storage, TrackedGcStorage::Arena) { return None; } @@ -337,6 +344,8 @@ mod tests { // 4 GiB is well below the legacy 2 TiB macOS floor. Membership in a // registered arena range, not this magnitude, is the ownership proof. + // The array install-path test injects this candidate because mapping a + // fixed low address in the test process would be platform-dependent. const LOW_USER: usize = 0x1_0000_0008; const LOW_RANGE_BASE: usize = LOW_USER - GC_HEADER_SIZE; const { assert!(LOW_USER < 0x200_0000_0000) }; @@ -384,7 +393,7 @@ mod tests { payload: u64, } - let synthetic = SyntheticAllocation { + let make_synthetic = || SyntheticAllocation { header: GcHeader { obj_type: crate::gc::GC_TYPE_ARRAY, gc_flags: 0, @@ -393,8 +402,13 @@ mod tests { }, payload: 0, }; - let user = &synthetic.payload as *const u64 as usize; - assert!(unsafe { try_read_tracked_gc_header(user) }.is_none()); + let stack_synthetic = make_synthetic(); + let stack_user = &stack_synthetic.payload as *const u64 as usize; + assert!(unsafe { try_read_tracked_gc_header(stack_user) }.is_none()); + + let heap_synthetic = Box::new(make_synthetic()); + let heap_user = &heap_synthetic.payload as *const u64 as usize; + assert!(unsafe { try_read_tracked_gc_header(heap_user) }.is_none()); } #[test]