diff --git a/changelog.d/gc-pointer-free-mask-invariant.md b/changelog.d/gc-pointer-free-mask-invariant.md new file mode 100644 index 0000000000..8492f2f991 --- /dev/null +++ b/changelog.d/gc-pointer-free-mask-invariant.md @@ -0,0 +1,7 @@ +fix(gc): two moving-collector soundness hardenings for arrays. + +1. `layout_note_slot` now restores `SIDE_MASK` state whenever it records a pointer into an array's existing element mask. Previously a stale `POINTER_FREE` state could linger with a populated mask (e.g. after an array was truncated to a numeric/empty prefix), and `heap_payload_slot_selection` treats `POINTER_FREE` as "no pointers" — skipping the whole payload without consulting the mask. The evacuating young-gen minor then dropped live within-length pointer elements, later read as a garbage pointer (`TypeError: value is not a function`). Same class as #6831. + +2. `js_array_alloc` / `js_array_grow` now HOLE-initialize the `[length, capacity)` slack instead of leaving it as raw arena bytes. Uninitialized slack could hold stale pointer-shaped bits that any beyond-`length` scan/trace would follow into freed/relocated memory; HOLE is a non-pointer sentinel (matching `js_array_alloc_with_length`). + +Verified with `PERRY_GC_FROMSPACE_SCAN`: within-length stale array→young edges drop 202→0; uninitialized-slack false-positive edges drop from ~4000/cycle to ~0. `cargo test -p perry-runtime` array/layout suites green. diff --git a/crates/perry-runtime/src/array/alloc.rs b/crates/perry-runtime/src/array/alloc.rs index bb9ee19329..9286cf2ce1 100644 --- a/crates/perry-runtime/src/array/alloc.rs +++ b/crates/perry-runtime/src/array/alloc.rs @@ -47,6 +47,13 @@ pub extern "C" fn js_array_alloc(capacity: u32) -> *mut ArrayHeader { // Initialize header (*ptr).length = 0; (*ptr).capacity = actual_capacity; + // HOLE-initialize the whole capacity so the unused [length, capacity) + // slack never holds stale arena bits that the whole-heap from-space + // scan misreads as live from-space pointers. + let elements_ptr = (ptr as *mut u8).add(std::mem::size_of::()) as *mut u64; + for i in 0..actual_capacity as usize { + std::ptr::write(elements_ptr.add(i), crate::value::TAG_HOLE); + } set_array_numeric_layout(ptr, NumericArrayLayout::RawF64); crate::gc::layout_init_pointer_free(ptr as *mut u8); } diff --git a/crates/perry-runtime/src/array/push_pop.rs b/crates/perry-runtime/src/array/push_pop.rs index f622d069a4..0f4daba730 100644 --- a/crates/perry-runtime/src/array/push_pop.rs +++ b/crates/perry-runtime/src/array/push_pop.rs @@ -91,6 +91,16 @@ pub extern "C" fn js_array_grow(arr: *mut ArrayHeader, min_capacity: u32) -> *mu ptr::copy_nonoverlapping(arr as *const u8, new_ptr as *mut u8, old_size); (*new_ptr).capacity = new_capacity; + // HOLE-initialize the newly added [old_capacity, new_capacity) slack + // so it never holds stale arena bits the whole-heap from-space scan + // misreads as live from-space pointers. + { + let new_elems = + (new_ptr as *mut u8).add(std::mem::size_of::()) as *mut u64; + for i in old_capacity as usize..new_capacity as usize { + ptr::write(new_elems.add(i), crate::value::TAG_HOLE); + } + } let old_header = (arr as *mut u8).sub(crate::gc::GC_HEADER_SIZE) as *mut crate::gc::GcHeader; let new_header = diff --git a/crates/perry-runtime/src/gc/layout.rs b/crates/perry-runtime/src/gc/layout.rs index aff1978b64..1e88fca8fe 100644 --- a/crates/perry-runtime/src/gc/layout.rs +++ b/crates/perry-runtime/src/gc/layout.rs @@ -748,6 +748,20 @@ pub(crate) fn layout_note_slot(parent_user: usize, slot_index: usize, value_bits if pointer { if let Some(mask) = masks.get_mut(&parent_user) { mask.set_slot(slot_index); + // A non-empty pointer mask MUST be reflected by SIDE_MASK + // state: `heap_payload_slot_selection` treats POINTER_FREE + // as "no pointers" and skips the WHOLE payload without ever + // consulting the mask. If a stale POINTER_FREE lingers here + // (an array truncated to a numeric/empty prefix flips to + // POINTER_FREE while its element mask is retained), recording + // a pointer would leave every masked element untraced — the + // evacuating minor then reclaims/relocates the child out from + // under the live slot, later read+called as a garbage pointer + // ("value is not a function"). Recording a pointer proves the + // object is not pointer-free, so restore SIDE_MASK. + if (*header)._reserved & GC_LAYOUT_STATE_MASK != GC_LAYOUT_SIDE_MASK { + set_layout_state(header, GC_LAYOUT_SIDE_MASK); + } } else if (*header)._reserved & GC_LAYOUT_STATE_MASK == GC_LAYOUT_POINTER_FREE { let mut mask = LayoutSlotMask::Inline(0); mask.set_slot(slot_index); diff --git a/crates/perry-runtime/src/gc/tests/layout_trace.rs b/crates/perry-runtime/src/gc/tests/layout_trace.rs index 1f52121ece..f4288f36bc 100644 --- a/crates/perry-runtime/src/gc/tests/layout_trace.rs +++ b/crates/perry-runtime/src/gc/tests/layout_trace.rs @@ -259,6 +259,77 @@ fn test_layout_mask_small_mixed_array_scans_exact_pointer_slot() { clear_mark_seeds(); } +#[test] +fn test_pointer_store_restores_side_mask_from_stale_pointer_free() { + // Regression: an array can end up in POINTER_FREE layout state while + // still carrying a populated element pointer-mask (e.g. after a + // numeric-layout rebuild observed a short/empty prefix while the mask + // lingered). `heap_payload_slot_selection` short-circuits POINTER_FREE and + // skips the WHOLE payload without consulting the mask, so the evacuating + // minor would drop live pointer elements. `layout_note_slot` must restore + // SIDE_MASK whenever it records a pointer into an existing mask. + clear_marks(); + clear_mark_seeds(); + + let child0 = crate::string::js_string_from_bytes(b"c0".as_ptr(), 2) as *mut u8; + let child1 = crate::string::js_string_from_bytes(b"c1".as_ptr(), 2) as *mut u8; + let child0_h = unsafe { header_from_user_ptr(child0) }; + let child1_h = unsafe { header_from_user_ptr(child1) }; + + let arr = crate::array::js_array_alloc_with_length(2); + // Store a pointer at index 0 -> SIDE_MASK + mask{0}. + crate::array::js_array_set_f64( + arr, + 0, + f64::from_bits(STRING_TAG | (child0 as u64 & POINTER_MASK)), + ); + assert_eq!(test_layout_pointer_slot_count(arr as usize, 2), Some(1)); + + // Reproduce the stale-state hazard: force POINTER_FREE while the mask{0} + // entry is still present (`set_layout_state` only touches the state bits). + let arr_header = unsafe { header_from_user_ptr(arr as *mut u8) }; + unsafe { + set_layout_state(arr_header, GC_LAYOUT_POINTER_FREE); + } + + // Store a pointer at index 1 through the normal array store path. + crate::array::js_array_set_f64( + arr, + 1, + f64::from_bits(STRING_TAG | (child1 as u64 & POINTER_MASK)), + ); + + // The fix: recording a pointer must have restored SIDE_MASK, so the mask is + // consulted and BOTH pointer slots are visible to the tracer. + unsafe { + assert_eq!( + (*arr_header)._reserved & GC_LAYOUT_STATE_MASK, + GC_LAYOUT_SIDE_MASK, + "recording a pointer into an existing mask must restore SIDE_MASK" + ); + } + assert_eq!(test_layout_pointer_slot_count(arr as usize, 2), Some(2)); + + let valid_ptrs = build_valid_pointer_set(); + let mut worklist = Vec::new(); + unsafe { + trace_array(arr as *mut u8, &valid_ptrs, &mut worklist); + assert_ne!( + (*child0_h).gc_flags & GC_FLAG_MARKED, + 0, + "index 0 pointer must be traced" + ); + assert_ne!( + (*child1_h).gc_flags & GC_FLAG_MARKED, + 0, + "index 1 pointer (stored under stale POINTER_FREE) must be traced" + ); + } + + clear_marks(); + clear_mark_seeds(); +} + #[test] fn test_layout_mask_heap_conversion_keeps_sparse_words_zeroed() { clear_marks();