From db1401f7ab537b2de33a8e1c229a4b86db82d230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 5 Sep 2026 20:15:41 +0200 Subject: [PATCH 1/4] fix(gc): allow retained array-growth aliases in copying verification --- changelog.d/4644-retained-growth-verifier.md | 1 + crates/perry-runtime/src/gc/copying.rs | 2 +- crates/perry-runtime/src/gc/cycle.rs | 4 +- crates/perry-runtime/src/gc/roots.rs | 37 ++-- .../src/gc/tests/forwarding_verification.rs | 152 ++++++++++++++++ crates/perry-runtime/src/gc/tests/mod.rs | 1 + .../tests/runtime_roots/callback_scanners.rs | 20 ++- .../runtime_roots/side_table_scanners.rs | 2 +- crates/perry-runtime/src/gc/verify.rs | 163 +++++++++++++----- scripts/gc_runtime_root_holders.json | 4 +- 10 files changed, 305 insertions(+), 81 deletions(-) create mode 100644 changelog.d/4644-retained-growth-verifier.md create mode 100644 crates/perry-runtime/src/gc/tests/forwarding_verification.rs diff --git a/changelog.d/4644-retained-growth-verifier.md b/changelog.d/4644-retained-growth-verifier.md new file mode 100644 index 0000000000..f67793ceb8 --- /dev/null +++ b/changelog.d/4644-retained-growth-verifier.md @@ -0,0 +1 @@ +- Fix a false evacuation-verifier abort when a copying minor encounters a retained, non-moving array-growth alias, such as Solid's effect dependency array. Verification still follows the full forwarding chain and rejects nursery evacuation originals; old-page evacuation retains its strict checks. diff --git a/crates/perry-runtime/src/gc/copying.rs b/crates/perry-runtime/src/gc/copying.rs index 9724feb04c..b9f8519701 100644 --- a/crates/perry-runtime/src/gc/copying.rs +++ b/crates/perry-runtime/src/gc/copying.rs @@ -1552,7 +1552,7 @@ pub(super) fn run_copied_minor_attempt( if gc_verify_evacuation_enabled() { let phase_start = trace_phase_start(trace); let valid_ptrs = build_valid_pointer_set(); - verify_evacuated_no_stale_forwarded_refs(&valid_ptrs); + verify_evacuated_no_stale_forwarded_refs(EvacuationVerifier::copying_minor(&valid_ptrs)); trace_phase_record(trace, "evacuation_verify", phase_start); } diff --git a/crates/perry-runtime/src/gc/cycle.rs b/crates/perry-runtime/src/gc/cycle.rs index b34e9eff3e..66777e520a 100644 --- a/crates/perry-runtime/src/gc/cycle.rs +++ b/crates/perry-runtime/src/gc/cycle.rs @@ -1411,7 +1411,9 @@ impl GcCycleState { trace_phase_record(&mut self.trace, "reference_rewrite", phase_start); if gc_verify_evacuation_enabled() { let phase_start = trace_phase_start(&self.trace); - verify_evacuated_no_stale_forwarded_refs(valid_ptrs); + verify_evacuated_no_stale_forwarded_refs(EvacuationVerifier::all_forwarded( + valid_ptrs, + )); trace_phase_record(&mut self.trace, "evacuation_verify", phase_start); } let released = diff --git a/crates/perry-runtime/src/gc/roots.rs b/crates/perry-runtime/src/gc/roots.rs index 054bee16f7..c40c29cf9c 100644 --- a/crates/perry-runtime/src/gc/roots.rs +++ b/crates/perry-runtime/src/gc/roots.rs @@ -728,7 +728,7 @@ pub(super) enum RuntimeRootVisitMode<'a> { valid_ptrs: &'a ValidPointerSet, }, Verify { - valid_ptrs: &'a ValidPointerSet, + verifier: EvacuationVerifier<'a>, surface: &'static str, }, Copy { @@ -807,12 +807,9 @@ impl<'a> RuntimeRootVisitor<'a> { } } - pub(super) fn for_verify(valid_ptrs: &'a ValidPointerSet, surface: &'static str) -> Self { + pub(super) fn for_verify(verifier: EvacuationVerifier<'a>, surface: &'static str) -> Self { Self { - mode: RuntimeRootVisitMode::Verify { - valid_ptrs, - surface, - }, + mode: RuntimeRootVisitMode::Verify { verifier, surface }, root_source_stats: None, young_scope: false, } @@ -909,11 +906,8 @@ impl<'a> RuntimeRootVisitor<'a> { RuntimeRootVisitMode::Rewrite { valid_ptrs } => { try_rewrite_nanboxed_value(bits, valid_ptrs) } - RuntimeRootVisitMode::Verify { - valid_ptrs, - surface, - } => { - if let Some(new_bits) = try_rewrite_nanboxed_value(bits, valid_ptrs) { + RuntimeRootVisitMode::Verify { verifier, surface } => { + if let Some(new_bits) = verifier.stale_nanboxed_value(bits) { panic_stale_forwarded_reference(surface, 0, bits, new_bits); } None @@ -941,11 +935,8 @@ impl<'a> RuntimeRootVisitor<'a> { collector.rewrite_value_bits(bits) } RuntimeRootVisitMode::Rewrite { valid_ptrs } => try_rewrite_value(bits, valid_ptrs), - RuntimeRootVisitMode::Verify { - valid_ptrs, - surface, - } => { - if let Some(new_bits) = try_rewrite_value(bits, valid_ptrs) { + RuntimeRootVisitMode::Verify { verifier, surface } => { + if let Some(new_bits) = verifier.stale_value(bits) { panic_stale_forwarded_reference(surface, 0, bits, new_bits); } None @@ -981,11 +972,8 @@ impl<'a> RuntimeRootVisitor<'a> { RuntimeRootVisitMode::CopyingMark { collector } => collector.visit_raw_addr(addr), RuntimeRootVisitMode::CopyingRewrite { collector } => collector.rewrite_raw_addr(addr), RuntimeRootVisitMode::Rewrite { valid_ptrs } => try_rewrite_raw_addr(addr, valid_ptrs), - RuntimeRootVisitMode::Verify { - valid_ptrs, - surface, - } => { - if let Some(new_addr) = try_rewrite_raw_addr(addr, valid_ptrs) { + RuntimeRootVisitMode::Verify { verifier, surface } => { + if let Some(new_addr) = verifier.stale_raw_addr(addr) { panic_stale_forwarded_reference( surface, 0, @@ -1012,11 +1000,8 @@ impl<'a> RuntimeRootVisitor<'a> { RuntimeRootVisitMode::CopyingCheck { .. } => None, RuntimeRootVisitMode::CopyingMark { .. } => None, RuntimeRootVisitMode::CopyingRewrite { collector } => collector.rewrite_raw_addr(addr), - RuntimeRootVisitMode::Verify { - valid_ptrs, - surface, - } => { - if let Some(new_addr) = try_rewrite_raw_addr(addr, valid_ptrs) { + RuntimeRootVisitMode::Verify { verifier, surface } => { + if let Some(new_addr) = verifier.stale_raw_addr(addr) { panic_stale_forwarded_reference(surface, 0, addr as u64, new_addr as u64); } None diff --git a/crates/perry-runtime/src/gc/tests/forwarding_verification.rs b/crates/perry-runtime/src/gc/tests/forwarding_verification.rs new file mode 100644 index 0000000000..5e74aa4d40 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/forwarding_verification.rs @@ -0,0 +1,152 @@ +use super::super::*; +use super::support::*; + +// Solid's effect.sources array grows after promotion. Its owning computation +// retains the old array address, which remains a supported growth alias. +#[test] +fn copying_verifier_accepts_retained_array_growth_alias_in_old_field() { + let _guard = CopyingNurseryTestGuard::new(2); + let _verify_guard = VerifyEvacuationTestGuard::on(); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + let (stub, _) = unsafe { alloc_old_test_array(1) }; + let (holder, field) = unsafe { alloc_old_test_object(1) }; + unsafe { + layout_init_pointer_free(stub as *mut u8); + layout_init_pointer_free(holder as *mut u8); + crate::object::store_object_field_slot(holder, 0, ptr_bits(stub as usize)); + } + let grown = crate::array::js_array_push_f64(stub, 42.0); + assert_ne!(stub, grown); + assert!(crate::arena::pointer_in_old_gen(stub as usize)); + assert!(crate::arena::pointer_in_old_gen(grown as usize)); + js_shadow_slot_set(0, ptr_bits(holder as usize)); + let young = young_leaf(); + js_shadow_slot_set(1, ptr_bits(young)); + + let trace = collect_minor_trace(GcTriggerKind::Direct); + + assert_copied_minor_trace(&trace, true, CopiedMinorFallbackReason::None, false); + assert!(trace.phase_us.contains_key("evacuation_verify")); + assert_ne!(js_shadow_slot_get(1), ptr_bits(young)); + assert_eq!(unsafe { *field }, ptr_bits(stub as usize)); + assert_eq!(crate::array::js_array_get_f64(stub, 1), 42.0); +} + +#[test] +fn copying_verifier_accepts_retained_growth_chains_across_root_formats() { + let _guard = CopyingNurseryTestGuard::new(0); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + let (stub, _) = unsafe { alloc_old_test_array(1) }; + let next = crate::array::js_array_grow(stub, 2); + let target = crate::array::js_array_grow(next, 4); + assert_ne!(stub, next); + assert_ne!(next, target); + let valid_ptrs = build_valid_pointer_set(); + let verifier = EvacuationVerifier::copying_minor(&valid_ptrs); + let bits = ptr_bits(stub as usize); + + // The ordinary rewrite and non-copying verifier still canonicalize every + // hop, including old arrays moved during old-page evacuation. + assert_eq!( + try_rewrite_value(bits, &valid_ptrs), + Some(ptr_bits(target as usize)) + ); + assert_eq!( + EvacuationVerifier::all_forwarded(&valid_ptrs).stale_value(bits), + Some(ptr_bits(target as usize)) + ); + let mut visitor = RuntimeRootVisitor::for_verify(verifier, "retained growth root"); + assert_eq!(visitor.visit_nanbox_bits(bits), None); + assert_eq!(visitor.visit_heap_word_bits(stub as usize as u64), None); + assert_eq!( + visitor.visit_tagged_raw_addr(stub as usize, POINTER_TAG), + None + ); + assert_eq!(visitor.visit_metadata_raw_addr(stub as usize), None); + verify_copy_only_scanner_bits(bits, verifier, "retained copy-only root"); + let mut context = verifier; + perry_ffi_verify_root( + f64::from_bits(bits), + &mut context as *mut EvacuationVerifier<'_> as *mut c_void, + ); +} + +#[test] +fn copying_verifier_rejects_from_space_hops_even_through_retained_stubs() { + let _guard = CopyingNurseryTestGuard::new(0); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + let (stub, _) = unsafe { alloc_old_test_array(1) }; + let from_space = crate::array::js_array_alloc(1); + let (target, _) = unsafe { alloc_old_test_array(1) }; + assert!(super::super::fromspace_scan::is_from_space( + crate::arena::classify_heap_space(from_space as usize) + )); + unsafe { + // Sabotage: array growth forbids an old -> young forwarding edge. + // The verifier must still reject it if it somehow occurs, even when + // the first hop is a retained old array and only the second is stale. + set_forwarding_address( + header_from_user_ptr(stub as *const u8), + from_space as *mut u8, + ); + } + let valid_ptrs = build_valid_pointer_set(); + let verifier = EvacuationVerifier::copying_minor(&valid_ptrs); + assert_eq!( + verifier.stale_raw_addr(stub as usize), + Some(from_space as usize), + "a retained stub must not reference even an unforwarded young array" + ); + unsafe { + set_forwarding_address( + header_from_user_ptr(from_space as *const u8), + target as *mut u8, + ); + } + for source in [stub, from_space] { + let bits = ptr_bits(source as usize); + assert_eq!( + verifier.stale_raw_addr(source as usize), + Some(target as usize) + ); + assert_eq!( + verifier.stale_value(source as usize as u64), + Some(target as usize as u64) + ); + assert_eq!( + verifier.stale_nanboxed_value(bits), + Some(ptr_bits(target as usize)) + ); + for format in 0..4 { + let failure = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let mut visitor = RuntimeRootVisitor::for_verify(verifier, "from-space control"); + match format { + 0 => { + visitor.visit_nanbox_bits(bits); + } + 1 => { + visitor.visit_heap_word_bits(source as usize as u64); + } + 2 => { + visitor.visit_tagged_raw_addr(source as usize, POINTER_TAG); + } + _ => { + visitor.visit_metadata_raw_addr(source as usize); + } + } + })); + assert!( + failure.is_err(), + "root format {format} must reject a stale hop" + ); + } + let failure = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| unsafe { + verify_slot(&bits, verifier, "from-space heap control"); + })); + assert!(failure.is_err()); + } + // Leave a valid retained alias for subsequent tests' heap walks. + unsafe { + set_forwarding_address(header_from_user_ptr(stub as *const u8), target as *mut u8); + } +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 2d3300e1cd..186fe29187 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -22,6 +22,7 @@ mod error_side_tables; mod evacuation; mod forwarded_stub_membership; mod forwarding_target_validation; +mod forwarding_verification; mod fromspace_protect; mod fromspace_scan; mod global_bootstrap; diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/callback_scanners.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/callback_scanners.rs index 26aaa9fc0d..bcf72ccefd 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots/callback_scanners.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/callback_scanners.rs @@ -976,7 +976,7 @@ fn test_evacuation_verify_detects_stale_forwarded_root_slot() { js_shadow_slot_set(0, fixture.nursery_bits); assert_panics_with("shadow stack roots", || { - verify_mutable_root_slots(&fixture.valid_ptrs); + verify_mutable_root_slots(EvacuationVerifier::all_forwarded(&fixture.valid_ptrs)); }); js_shadow_frame_pop(shadow); @@ -994,8 +994,10 @@ fn test_evacuation_verify_detects_stale_forwarded_runtime_scanner_slot() { ); assert_panics_with("runtime mutable root scanner", || { - let mut visitor = - RuntimeRootVisitor::for_verify(&fixture.valid_ptrs, "runtime mutable root scanner"); + let mut visitor = RuntimeRootVisitor::for_verify( + EvacuationVerifier::all_forwarded(&fixture.valid_ptrs), + "runtime mutable root scanner", + ); promise_mutable_root_scanner(&mut visitor); }); @@ -1020,7 +1022,7 @@ fn test_evacuation_verify_detects_stale_forwarded_dirty_range_slot() { } assert_panics_with("remembered dirty ranges", || { - verify_remembered_dirty_ranges(&valid_ptrs); + verify_remembered_dirty_ranges(EvacuationVerifier::all_forwarded(&valid_ptrs)); }); remembered_set_clear(); @@ -1036,7 +1038,11 @@ fn test_evacuation_verify_detects_stale_forwarded_heap_field() { let header = header_from_user_ptr(old_obj as *const u8); (*header).gc_flags |= GC_FLAG_MARKED; assert_panics_with("heap fields", || { - verify_heap_object_fields(header, &fixture.valid_ptrs, "heap fields"); + verify_heap_object_fields( + header, + EvacuationVerifier::all_forwarded(&fixture.valid_ptrs), + "heap fields", + ); }); (*header).gc_flags &= !GC_FLAG_MARKED; } @@ -1051,7 +1057,7 @@ fn test_evacuation_verify_copy_only_pinned_root_allows_non_forwarded_target() { } verify_copy_only_scanner_bits( POINTER_TAG | (user as u64 & POINTER_MASK), - &valid_ptrs, + EvacuationVerifier::all_forwarded(&valid_ptrs), "copy-only root scanner", ); unsafe { @@ -1065,7 +1071,7 @@ fn test_evacuation_verify_copy_only_root_rejects_forwarded_target() { assert_panics_with("copy-only root scanner", || { verify_copy_only_scanner_bits( fixture.nursery_bits, - &fixture.valid_ptrs, + EvacuationVerifier::all_forwarded(&fixture.valid_ptrs), "copy-only root scanner", ); }); diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/side_table_scanners.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/side_table_scanners.rs index 10ee93d3fe..4e14a61002 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots/side_table_scanners.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/side_table_scanners.rs @@ -503,7 +503,7 @@ fn test_class_inheritance_side_table_roots_mark_and_rewrite() { // A verify pass must not panic now that the slots point at the live // (non-forwarded) evacuated objects. scan_class_inheritance_roots_mut(&mut RuntimeRootVisitor::for_verify( - &valid_ptrs, + EvacuationVerifier::all_forwarded(&valid_ptrs), "class inheritance side-table roots (test)", )); diff --git a/crates/perry-runtime/src/gc/verify.rs b/crates/perry-runtime/src/gc/verify.rs index 0332bcf3b8..9d6bb0b26f 100644 --- a/crates/perry-runtime/src/gc/verify.rs +++ b/crates/perry-runtime/src/gc/verify.rs @@ -26,9 +26,11 @@ pub(super) fn try_rewrite_nanboxed_value(bits: u64, valid_ptrs: &ValidPointerSet /// #8174: refuses a forwarding target that is not a heap object start, in /// lockstep with [`CopyingNurseryCollector::rewrite_raw_addr`](super::copying). /// -/// The lockstep is the point. This function is what the VERIFY pass runs -/// (`RuntimeRootVisitMode::Verify`), and it panics whenever it can rewrite a -/// slot the rewrite pass left alone. Tightening only the rewrite pass would +/// The lockstep is the point. The VERIFY pass shares this forwarding walker +/// and panics when it finds a stale alias the rewrite pass left alone. +/// Copying verification permits retained array-growth aliases through +/// [`EvacuationVerifier`], without changing source/target validation. +/// Tightening only the rewrite pass would /// therefore have turned a silently-corrupt rewrite into a `PERRY_GC_VERIFY_ /// EVACUATION` abort blaming an innocent scanner — the two walkers must reach /// the same verdict or the verifier is measuring the difference between them @@ -36,6 +38,82 @@ pub(super) fn try_rewrite_nanboxed_value(bits: u64, valid_ptrs: &ValidPointerSet /// stronger than the copier's heap-region test, so this only changes the case /// where a genuinely LIVE forwarded object's target word is corrupt. pub(super) fn try_rewrite_raw_addr(ptr_addr: usize, valid_ptrs: &ValidPointerSet) -> Option { + follow_forwarding_raw_addr(ptr_addr, valid_ptrs, |_, _| true) +} + +/// The copying minor retains non-moving array-growth stubs. Other evacuation +/// paths rewrite every forwarding alias before releasing moved originals. +/// Carry that distinction through every verifier surface, including FFI roots. +#[derive(Clone, Copy)] +pub(super) struct EvacuationVerifier<'a> { + valid_ptrs: &'a ValidPointerSet, + copying_minor: bool, +} + +impl<'a> EvacuationVerifier<'a> { + pub(super) fn all_forwarded(valid_ptrs: &'a ValidPointerSet) -> Self { + Self { + valid_ptrs, + copying_minor: false, + } + } + + /// Must run before the copying minor resets from-space and flips survivors. + pub(super) fn copying_minor(valid_ptrs: &'a ValidPointerSet) -> Self { + Self { + valid_ptrs, + copying_minor: true, + } + } + + pub(super) fn stale_raw_addr(self, addr: usize) -> Option { + follow_forwarding_raw_addr(addr, self.valid_ptrs, |source, target| { + if !self.copying_minor { + return true; + } + // Only array growth creates permanent forwarding aliases. The + // copying minor neither moves nor frees these non-moving sources. + // Both ends must be retained arrays. An old -> young growth edge + // is forbidden even if the young target was never forwarded. + // Follow the whole chain so an indirect unsafe hop is also caught. + !self.retained_growth_array(source) || !self.retained_growth_array(target) + }) + } + + fn retained_growth_array(self, addr: usize) -> bool { + if !self.valid_ptrs.contains(&addr) { + return false; + } + let header = unsafe { header_from_user_ptr(addr as *const u8) }; + (unsafe { (*header).obj_type == GC_TYPE_ARRAY }) + && matches!( + crate::arena::classify_heap_space(addr), + crate::arena::HeapSpace::Old + | crate::arena::HeapSpace::Longlived + | crate::arena::HeapSpace::PromotedYoung + ) + } + + pub(super) fn stale_value(self, bits: u64) -> Option { + let word = decode_root_word(bits)?; + Some(word.encode(self.stale_raw_addr(word.addr())?)) + } + + pub(super) fn stale_nanboxed_value(self, bits: u64) -> Option { + let tag = bits & TAG_MASK; + if tag != POINTER_TAG && tag != STRING_TAG && tag != BIGINT_TAG { + return None; + } + let addr = self.stale_raw_addr((bits & POINTER_MASK) as usize)?; + Some(tag | (addr as u64 & POINTER_MASK)) + } +} + +fn follow_forwarding_raw_addr( + ptr_addr: usize, + valid_ptrs: &ValidPointerSet, + must_rewrite: impl Fn(usize, usize) -> bool, +) -> Option { if ptr_addr == 0 { return None; } @@ -57,8 +135,8 @@ pub(super) fn try_rewrite_raw_addr(ptr_addr: usize, valid_ptrs: &ValidPointerSet if !accept_forwarding_target(next) { return None; } + rewrote |= must_rewrite(current, next); current = next; - rewrote = true; } } rewrote.then_some(current) @@ -87,9 +165,13 @@ pub(super) unsafe fn rewrite_slot(slot: *mut u64, valid_ptrs: &ValidPointerSet) } #[inline] -pub(super) unsafe fn verify_slot(slot: *const u64, valid_ptrs: &ValidPointerSet, surface: &str) { +pub(super) unsafe fn verify_slot( + slot: *const u64, + verifier: EvacuationVerifier<'_>, + surface: &str, +) { let bits = *slot; - if let Some(new_bits) = try_rewrite_value(bits, valid_ptrs) { + if let Some(new_bits) = verifier.stale_value(bits) { panic_stale_forwarded_reference(surface, slot as usize, bits, new_bits); } } @@ -1122,7 +1204,7 @@ pub(super) fn verify_minor_unmarked_young_children_report(phase: &str) { pub(super) unsafe fn verify_heap_object_fields( header: *mut GcHeader, - valid_ptrs: &ValidPointerSet, + verifier: EvacuationVerifier<'_>, surface: &'static str, ) { let flags = (*header).gc_flags; @@ -1131,7 +1213,7 @@ pub(super) unsafe fn verify_heap_object_fields( } visit_gc_rewrite_slots(header, |slot| unsafe { slot.record_layout_read(); - verify_slot(slot.slot as *const u64, valid_ptrs, surface); + verify_slot(slot.slot as *const u64, verifier, surface); }); } @@ -1263,13 +1345,13 @@ pub(super) fn rewrite_mutable_registered_roots_with_sources( visit_ffi_mutable_registered_roots_with_sources(&mut visitor, root_sources); } -pub(super) fn verify_mutable_root_slots(valid_ptrs: &ValidPointerSet) { +pub(super) fn verify_mutable_root_slots(verifier: EvacuationVerifier<'_>) { visit_mutable_root_slots(|slot| unsafe { let bits = slot.read(); if bits == 0 { return; } - if let Some(new_bits) = try_rewrite_value(bits, valid_ptrs) { + if let Some(new_bits) = verifier.stale_value(bits) { let surface = match slot.kind { MutableRootSlotKind::ShadowStack => "shadow stack roots", MutableRootSlotKind::NativeStack => "native stack-map roots", @@ -1280,9 +1362,9 @@ pub(super) fn verify_mutable_root_slots(valid_ptrs: &ValidPointerSet) { }); } -pub(super) fn verify_mutable_registered_roots(valid_ptrs: &ValidPointerSet) { +pub(super) fn verify_mutable_registered_roots(verifier: EvacuationVerifier<'_>) { let scanners: Vec = MUTABLE_ROOT_SCANNERS.with(|s| s.borrow().clone()); - let mut visitor = RuntimeRootVisitor::for_verify(valid_ptrs, "runtime mutable root scanner"); + let mut visitor = RuntimeRootVisitor::for_verify(verifier, "runtime mutable root scanner"); for entry in scanners { (entry.scanner)(&mut visitor); } @@ -1291,72 +1373,67 @@ pub(super) fn verify_mutable_registered_roots(valid_ptrs: &ValidPointerSet) { pub(super) fn verify_copy_only_scanner_bits( bits: u64, - valid_ptrs: &ValidPointerSet, + verifier: EvacuationVerifier<'_>, surface: &'static str, ) { - if let Some(new_bits) = try_rewrite_nanboxed_value(bits, valid_ptrs) { + if let Some(new_bits) = verifier.stale_nanboxed_value(bits) { panic_stale_forwarded_reference(surface, 0, bits, new_bits); } } -pub(super) struct RegisteredRootVerifyContext { - pub(super) valid_ptrs: *const ValidPointerSet, -} - pub(super) extern "C" fn perry_ffi_verify_root(value: f64, ctx: *mut c_void) { if ctx.is_null() { return; } - let ctx = unsafe { &*(ctx as *const RegisteredRootVerifyContext) }; - if ctx.valid_ptrs.is_null() { - return; - } - let valid_ptrs = unsafe { &*ctx.valid_ptrs }; - verify_copy_only_scanner_bits(value.to_bits(), valid_ptrs, "ffi copy-only root scanner"); + let verifier = unsafe { *(ctx as *const EvacuationVerifier<'_>) }; + verify_copy_only_scanner_bits(value.to_bits(), verifier, "ffi copy-only root scanner"); } -pub(super) fn verify_copy_only_registered_roots(valid_ptrs: &ValidPointerSet) { +pub(super) fn verify_copy_only_registered_roots(verifier: EvacuationVerifier<'_>) { let scanners: Vec = ROOT_SCANNERS.with(|s| s.borrow().clone()); for scanner in scanners { scanner(&mut |value: f64| { - verify_copy_only_scanner_bits(value.to_bits(), valid_ptrs, "copy-only root scanner"); + verify_copy_only_scanner_bits(value.to_bits(), verifier, "copy-only root scanner"); }); } let ffi_scanners: Vec = FFI_ROOT_SCANNERS.with(|s| s.borrow().clone()); - let mut ctx = RegisteredRootVerifyContext { - valid_ptrs: valid_ptrs as *const ValidPointerSet, - }; - let ctx = &mut ctx as *mut RegisteredRootVerifyContext as *mut c_void; + let mut ctx = verifier; + let ctx = &mut ctx as *mut EvacuationVerifier<'_> as *mut c_void; for scanner in ffi_scanners { scanner(perry_ffi_verify_root, ctx); } } -pub(super) fn verify_remembered_dirty_ranges(valid_ptrs: &ValidPointerSet) { +pub(super) fn verify_remembered_dirty_ranges(verifier: EvacuationVerifier<'_>) { let snapshot = remembered_dirty_snapshot(); let mut stats = RememberedSetTraceStats::default(); let mut verify_dirty_slot = |slot: *mut u64, _stats: &mut RememberedSetTraceStats| unsafe { - verify_slot(slot as *const u64, valid_ptrs, "remembered dirty ranges"); + verify_slot(slot as *const u64, verifier, "remembered dirty ranges"); }; - scan_remembered_dirty_slot_ranges(&snapshot, valid_ptrs, &mut stats, &mut verify_dirty_slot); + scan_remembered_dirty_slot_ranges( + &snapshot, + verifier.valid_ptrs, + &mut stats, + &mut verify_dirty_slot, + ); for header_addr in snapshot.fallback_headers { let user_ptr = header_addr + GC_HEADER_SIZE; - if !valid_ptrs.contains(&user_ptr) { + if !verifier.valid_ptrs.contains(&user_ptr) { continue; } unsafe { verify_heap_object_fields( header_addr as *mut GcHeader, - valid_ptrs, + verifier, "remembered fallback headers", ); } } } -pub(super) fn verify_heap_objects(valid_ptrs: &ValidPointerSet) { +pub(super) fn verify_heap_objects(verifier: EvacuationVerifier<'_>) { let verify_one = |header: *mut GcHeader| unsafe { let flags = (*header).gc_flags; if flags & GC_FLAG_FORWARDED != 0 { @@ -1373,7 +1450,7 @@ pub(super) fn verify_heap_objects(valid_ptrs: &ValidPointerSet) { return; } } - verify_heap_object_fields(header, valid_ptrs, "heap fields"); + verify_heap_object_fields(header, verifier, "heap fields"); }; crate::arena::arena_walk_objects(|hp| verify_one(hp as *mut GcHeader)); MALLOC_STATE.with(|s| { @@ -1384,12 +1461,12 @@ pub(super) fn verify_heap_objects(valid_ptrs: &ValidPointerSet) { }); } -pub(super) fn verify_evacuated_no_stale_forwarded_refs(valid_ptrs: &ValidPointerSet) { - verify_mutable_root_slots(valid_ptrs); - verify_mutable_registered_roots(valid_ptrs); - verify_copy_only_registered_roots(valid_ptrs); - verify_remembered_dirty_ranges(valid_ptrs); - verify_heap_objects(valid_ptrs); +pub(super) fn verify_evacuated_no_stale_forwarded_refs(verifier: EvacuationVerifier<'_>) { + verify_mutable_root_slots(verifier); + verify_mutable_registered_roots(verifier); + verify_copy_only_registered_roots(verifier); + verify_remembered_dirty_ranges(verifier); + verify_heap_objects(verifier); } /// Top-level Phase C4b-γ-2 entry: rewrite every reference site we diff --git a/scripts/gc_runtime_root_holders.json b/scripts/gc_runtime_root_holders.json index d2dc1990d0..33fbe97090 100644 --- a/scripts/gc_runtime_root_holders.json +++ b/scripts/gc_runtime_root_holders.json @@ -276,7 +276,7 @@ "file": "crates/perry-runtime/src/gc/census.rs", "name": "PASS1_MARKED", "verdict": "non_moving_snapshot", - "why": "Real GC header addresses, deliberately untraced so the diagnostic does not keep its observed objects alive. Populated only at the end of mark propagation of a synchronous full cycle; consumed at sweep entry in the same run_to_completion invocation. The intervening full-cycle phases do not relocate or run JS callbacks. The Vec is used for membership comparisons and dropped with the census before sweep. Budgeted and minor cycles skip both boundaries. Pin re-audited 2026-09-05 after #9760 touched `gc/mod.rs`: that change is `mod heap_stats;` plus a `pub(crate) use` re-export and alters no mark/sweep control flow. `heap_stats()` is reached only from `js_bun_jsc_heap_stats` (the JS-facing `bun:jsc.heapStats()`), i.e. from mutator code, never inside a cycle, and its own module contract forbids allocation or collection during its walk. The mark-complete \u2192 sweep-entry window is unchanged. Re-audited 2026-09-05 (train125) after #9769 and #9771 touched pinned files. #9769 adds one `reg_scanner!` registration to `gc/mod.rs`; #9771 adds a feature-gated `alloc_census_init()` there and a feature-gated Rust-heap dump inside `take_census`. `alloc-census` is not in the default feature set, and decisively: `census_take_if_armed_at_full_sweep_start` does `PASS1_MARKED.with(|p| p.borrow_mut().take())` BEFORE calling `take_census`, so the snapshot has already left the thread-local by the time #9771's code runs \u2014 it cannot affect the window. Neither change alters mark/sweep control flow. Re-audited 2026-09-05 (train126) after #9755 restructured `gc/cycle.rs`. Its hunks are all root-scan machinery (`RootScanSubphase`, `RootScanCycleState`, the mutable-scanner iteration state), which runs BEFORE mark propagation completes; `gc/mod.rs` gains only a `mod young_log;` declaration. The bracketing is unchanged \u2014 `census_pass1_if_armed` is still inside `step_mark_propagation` and `census_take_if_armed_at_full_sweep_start` inside `step_sweep` \u2014 and a synchronous full mark-sweep still moves nothing between them. Re-pinned 2026-09-05 for the #9740 hot-TLS conversion of this file: the sole change is `thread_local!` \u2192 `crate::perry_thread_local!`, a macro-name swap with identical declaration syntax and `.with()` call sites. No control flow, no phase boundary, and no storage semantics change. Re-audited 2026-09-06 (train128) after #9794's GC diagnostics touched `gc/mod.rs` and `gc/policy.rs`: both gain diagnostic module declarations and counters only \u2014 no mark/sweep control flow, and the census bracketing in `step_mark_propagation` / `step_sweep` is unchanged. Re-audited for #9794's GC diagnostics: `gc/mod.rs` gains `mod diag_sites;` / `mod survival_diag;`, a re-export, a `diag_sites::full_started(...)` call at TRIGGER time (before mark propagation begins), and exit-time reporting. Nothing executes between mark-complete and sweep-entry, so the window is unchanged.", + "why": "Real GC header addresses, deliberately untraced so the diagnostic does not keep its observed objects alive. Populated only at the end of mark propagation of a synchronous full cycle; consumed at sweep entry in the same run_to_completion invocation. The intervening full-cycle phases do not relocate or run JS callbacks. The Vec is used for membership comparisons and dropped with the census before sweep. Budgeted and minor cycles skip both boundaries. Pin re-audited 2026-09-05 after #9760 touched `gc/mod.rs`: that change is `mod heap_stats;` plus a `pub(crate) use` re-export and alters no mark/sweep control flow. `heap_stats()` is reached only from `js_bun_jsc_heap_stats` (the JS-facing `bun:jsc.heapStats()`), i.e. from mutator code, never inside a cycle, and its own module contract forbids allocation or collection during its walk. The mark-complete \u2192 sweep-entry window is unchanged. Re-audited 2026-09-05 (train125) after #9769 and #9771 touched pinned files. #9769 adds one `reg_scanner!` registration to `gc/mod.rs`; #9771 adds a feature-gated `alloc_census_init()` there and a feature-gated Rust-heap dump inside `take_census`. `alloc-census` is not in the default feature set, and decisively: `census_take_if_armed_at_full_sweep_start` does `PASS1_MARKED.with(|p| p.borrow_mut().take())` BEFORE calling `take_census`, so the snapshot has already left the thread-local by the time #9771's code runs \u2014 it cannot affect the window. Neither change alters mark/sweep control flow. Re-audited 2026-09-05 (train126) after #9755 restructured `gc/cycle.rs`. Its hunks are all root-scan machinery (`RootScanSubphase`, `RootScanCycleState`, the mutable-scanner iteration state), which runs BEFORE mark propagation completes; `gc/mod.rs` gains only a `mod young_log;` declaration. The bracketing is unchanged \u2014 `census_pass1_if_armed` is still inside `step_mark_propagation` and `census_take_if_armed_at_full_sweep_start` inside `step_sweep` \u2014 and a synchronous full mark-sweep still moves nothing between them. Re-pinned 2026-09-05 for the #9740 hot-TLS conversion of this file: the sole change is `thread_local!` \u2192 `crate::perry_thread_local!`, a macro-name swap with identical declaration syntax and `.with()` call sites. No control flow, no phase boundary, and no storage semantics change. Re-audited 2026-09-06 (train128) after #9794's GC diagnostics touched `gc/mod.rs` and `gc/policy.rs`: both gain diagnostic module declarations and counters only \u2014 no mark/sweep control flow, and the census bracketing in `step_mark_propagation` / `step_sweep` is unchanged. Re-audited for #9794's GC diagnostics: `gc/mod.rs` gains `mod diag_sites;` / `mod survival_diag;`, a re-export, a `diag_sites::full_started(...)` call at TRIGGER time (before mark propagation begins), and exit-time reporting. Nothing executes between mark-complete and sweep-entry, so the window is unchanged. Re-audited 2026-09-06 for the retained array-growth verifier fix: the cycle.rs change passes the existing non-copying evacuation verifier an explicit all-forwarded policy. That call remains in minor finalization, outside the synchronous full-cycle census window; its root and heap reads do not allocate GC objects, move objects, or invoke JS callbacks. The mark-complete and sweep-entry boundaries are unchanged.", "window": { "start": { "file": "crates/perry-runtime/src/gc/census.rs", @@ -292,7 +292,7 @@ }, "sources": { "crates/perry-runtime/src/gc/census.rs": "388414f9629f196e84673e91bebd04bdcdcabdaa180252d2dfe4b82d1b49ca5a", - "crates/perry-runtime/src/gc/cycle.rs": "763d552271b8e983a796b4e9648cd8ee984a0602b2b56aeefdb8713c0049c31f", + "crates/perry-runtime/src/gc/cycle.rs": "2e2f5adca2229f74409e01a1cb571e2147cd8a33f58d0976711fce98d4777309", "crates/perry-runtime/src/gc/mod.rs": "7dd42b9506a97e6844fd3225dc53dfd59512631784750f58ff72208d68595481", "crates/perry-runtime/src/gc/policy.rs": "fa8e9fa188d50bd92c3fbe23950a195906baf387f3a208497791bdd23f1c72db", "crates/perry-runtime/src/gc/progress.rs": "a5ad3971bbe4047229ca57325234780daa85921dbc778e1c08dff4ad07ccfb96" From 350d1b4639046d0cb7557dd11bd1390b01cd82ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 5 Sep 2026 20:16:07 +0200 Subject: [PATCH 2/4] docs: number retained-growth verifier changeset for PR 9822 --- ...tained-growth-verifier.md => 9822-retained-growth-verifier.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{4644-retained-growth-verifier.md => 9822-retained-growth-verifier.md} (100%) diff --git a/changelog.d/4644-retained-growth-verifier.md b/changelog.d/9822-retained-growth-verifier.md similarity index 100% rename from changelog.d/4644-retained-growth-verifier.md rename to changelog.d/9822-retained-growth-verifier.md From da2a84403eb7fa1c849ff1ad70b240624043c6a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 10:34:22 +0200 Subject: [PATCH 3/4] fix(gc): root for-in and proxy descriptor callbacks --- changelog.d/4644-for-in-callback-roots.md | 1 + crates/perry-runtime/src/gc/tests/mod.rs | 1 + .../src/gc/tests/rooted_for_in.rs | 224 ++++++++++++++++++ .../src/object/field_get_set/enumeration.rs | 44 ++-- crates/perry-runtime/src/proxy/reflect.rs | 120 ++++++---- ...test_gap_gc_for_in_proxy_callback_roots.ts | 46 ++++ test-parity/gc_repsel_corpus.txt | 3 + 7 files changed, 382 insertions(+), 57 deletions(-) create mode 100644 changelog.d/4644-for-in-callback-roots.md create mode 100644 crates/perry-runtime/src/gc/tests/rooted_for_in.rs create mode 100644 test-files/test_gap_gc_for_in_proxy_callback_roots.ts diff --git a/changelog.d/4644-for-in-callback-roots.md b/changelog.d/4644-for-in-callback-roots.md new file mode 100644 index 0000000000..3c6dc52939 --- /dev/null +++ b/changelog.d/4644-for-in-callback-roots.md @@ -0,0 +1 @@ +- Keep `for…in` receivers, accumulated keys, and Proxy descriptor state rooted across Proxy callbacks and moving garbage collection. Fixes stale pointers when Solid's universal renderer enumerates reactive spread properties. diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 186fe29187..ad15e1a247 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -48,6 +48,7 @@ mod retention_9628_9629; mod root_words; mod rooted_container_values; mod rooted_define_property; +mod rooted_for_in; mod roots; mod runtime_roots; mod scan_fallback; diff --git a/crates/perry-runtime/src/gc/tests/rooted_for_in.rs b/crates/perry-runtime/src/gc/tests/rooted_for_in.rs new file mode 100644 index 0000000000..34c4c24fc8 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/rooted_for_in.rs @@ -0,0 +1,224 @@ +//! Enumeration retains its output and receiver across Proxy callbacks (#4644). + +use super::super::*; +use super::support::*; +use crate::gc::{RuntimeHandle, RuntimeHandleScope}; + +thread_local! { + static COPIED: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +extern "C" fn moving_own_keys(_closure: *const crate::closure::ClosureHeader, target: f64) -> f64 { + let scope = RuntimeHandleScope::new(); + let target = scope.root_nanbox_f64(target); + let trace = collect_minor_trace(GcTriggerKind::Direct); + COPIED.with(|count| count.set(count.get() + trace.copying_nursery.copied_objects)); + crate::object::js_object_get_own_property_names(target.get_nanbox_f64()) +} + +fn object(scope: &RuntimeHandleScope) -> RuntimeHandle<'_> { + scope.root_raw_mut_ptr(crate::object::js_object_alloc(0, 0)) +} + +fn boxed(handle: RuntimeHandle<'_>) -> f64 { + handle.with_const_ptr(|ptr: *const crate::object::ObjectHeader| { + f64::from_bits(ptr_bits(ptr as usize)) + }) +} + +fn set(handle: RuntimeHandle<'_>, name: &str, value: f64) { + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + handle.with_mut_ptr(|ptr| crate::object::js_object_set_field_by_name(ptr, key, value)); +} + +fn run(inherited_proxy: bool, descriptor_trap: bool) { + let _guard = CopyingNurseryTestGuard::new(0); + let _triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + gc_register_mutable_root_scanner_with_source( + scan_runtime_handle_roots_mut, + MutableRootScannerSource::RuntimeHandles, + ); + gc_register_mutable_root_scanner(crate::proxy::scan_proxy_roots_mut); + COPIED.with(|count| count.set(0)); + let scope = RuntimeHandleScope::new(); + let target = object(&scope); + crate::object::js_object_set_prototype_of( + boxed(target), + f64::from_bits(crate::value::TAG_NULL), + ); + for index in 0..14 { + set(target, &format!("property_{index}"), index as f64); + } + let handler = object(&scope); + let (name, function) = if descriptor_trap { + ("getOwnPropertyDescriptor", moving_descriptor as *const u8) + } else { + ("ownKeys", moving_own_keys as *const u8) + }; + let callback = crate::closure::js_closure_alloc(function, 0); + set(handler, name, f64::from_bits(ptr_bits(callback as usize))); + let proxy = scope.root_nanbox_f64(crate::proxy::js_proxy_new(boxed(target), boxed(handler))); + let receiver = object(&scope); + if inherited_proxy { + // These entries grow the result before reaching the Proxy prototype. + for index in 0..10 { + set(receiver, &format!("local_{index}"), index as f64); + } + crate::object::js_object_set_prototype_of(boxed(receiver), proxy.get_nanbox_f64()); + } + let target_before = boxed(target).to_bits(); + let receiver_before = boxed(receiver).to_bits(); + let result = crate::object::js_for_in_keys_value(if inherited_proxy { + boxed(receiver) + } else { + proxy.get_nanbox_f64() + }); + let result = scope.root_raw_const_ptr(result); + assert!( + COPIED.with(|count| count.get()) > 0, + "the callback must move live objects" + ); + assert_ne!( + boxed(target).to_bits(), + target_before, + "the Proxy target must relocate" + ); + assert_ne!( + boxed(receiver).to_bits(), + receiver_before, + "the receiver must relocate" + ); + let local_count = if inherited_proxy { 10 } else { 0 }; + assert_eq!( + result.with_const_ptr(|array| crate::array::js_array_length(array)), + local_count + 14 + ); + for index in 0..local_count + 14 { + let expected = if index < local_count { + format!("local_{index}") + } else { + format!("property_{}", index - local_count) + }; + let value = result.with_const_ptr(|ptr| crate::array::js_array_get(ptr, index)); + unsafe { + assert_string_bytes( + (value.bits() & POINTER_MASK) as *const crate::StringHeader, + expected.as_bytes(), + ); + } + } +} + +#[test] +fn for_in_result_survives_own_keys_collection() { + run(false, false); +} + +#[test] +fn for_in_grown_result_and_receiver_survive_prototype_collection() { + run(true, false); +} + +extern "C" fn moving_descriptor( + _closure: *const crate::closure::ClosureHeader, + target: f64, + key: f64, +) -> f64 { + let scope = RuntimeHandleScope::new(); + let target = scope.root_nanbox_f64(target); + let key = scope.root_nanbox_f64(key); + let trace = collect_minor_trace(GcTriggerKind::Direct); + COPIED.with(|count| count.set(count.get() + trace.copying_nursery.copied_objects)); + crate::object::js_object_get_own_property_descriptor( + target.get_nanbox_f64(), + key.get_nanbox_f64(), + ) +} + +#[test] +fn descriptor_trap_collection_preserves_for_in_target_and_keys() { + run(false, true); +} + +extern "C" fn moving_value(_closure: *const crate::closure::ClosureHeader) -> f64 { + let trace = collect_minor_trace(GcTriggerKind::Direct); + COPIED.with(|count| count.set(count.get() + trace.copying_nursery.copied_objects)); + 23.0 +} + +extern "C" fn descriptor_with_moving_field( + _closure: *const crate::closure::ClosureHeader, + _target: f64, + _key: f64, +) -> f64 { + let scope = RuntimeHandleScope::new(); + let result = object(&scope); + for name in ["enumerable", "configurable", "writable"] { + set(result, name, f64::from_bits(crate::value::TAG_TRUE)); + } + let getter = crate::closure::js_closure_alloc(moving_value as *const u8, 0); + let descriptor = object(&scope); + set(descriptor, "get", f64::from_bits(ptr_bits(getter as usize))); + let key = crate::string::js_string_from_bytes(b"value".as_ptr(), 5); + crate::object::js_object_define_property( + boxed(result), + f64::from_bits(string_bits(key as usize)), + boxed(descriptor), + ); + boxed(result) +} + +#[test] +fn descriptor_completion_reloads_after_field_getter_collection() { + let _guard = CopyingNurseryTestGuard::new(0); + let _triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + gc_register_mutable_root_scanner_with_source( + scan_runtime_handle_roots_mut, + MutableRootScannerSource::RuntimeHandles, + ); + gc_register_mutable_root_scanner(crate::proxy::scan_proxy_roots_mut); + COPIED.with(|count| count.set(0)); + let scope = RuntimeHandleScope::new(); + let target = object(&scope); + set(target, "property_name", 7.0); + let handler = object(&scope); + let callback = crate::closure::js_closure_alloc(descriptor_with_moving_field as *const u8, 0); + set( + handler, + "getOwnPropertyDescriptor", + f64::from_bits(ptr_bits(callback as usize)), + ); + let proxy = scope.root_nanbox_f64(crate::proxy::js_proxy_new(boxed(target), boxed(handler))); + let key = crate::string::js_string_from_bytes(b"property_name".as_ptr(), 13); + let before = boxed(target).to_bits(); + let result = crate::proxy::js_reflect_get_own_property_descriptor( + proxy.get_nanbox_f64(), + f64::from_bits(string_bits(key as usize)), + ); + let result = scope.root_nanbox_f64(result); + assert!(COPIED.with(|count| count.get()) > 0); + assert_ne!( + boxed(target).to_bits(), + before, + "the descriptor getter must move live objects" + ); + for (name, expected) in [ + ("value", 23.0), + ("writable", f64::from_bits(crate::value::TAG_TRUE)), + ("enumerable", f64::from_bits(crate::value::TAG_TRUE)), + ("configurable", f64::from_bits(crate::value::TAG_TRUE)), + ] { + let value = unsafe { + crate::value::js_get_property( + result.get_nanbox_f64(), + name.as_ptr() as i64, + name.len() as i64, + ) + }; + assert_eq!( + value.to_bits(), + expected.to_bits(), + "descriptor field {name}" + ); + } +} diff --git a/crates/perry-runtime/src/object/field_get_set/enumeration.rs b/crates/perry-runtime/src/object/field_get_set/enumeration.rs index 30098dd73d..5dc0c6f53b 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -280,17 +280,25 @@ pub extern "C" fn js_for_in_keys_value(value: f64) -> *mut ArrayHeader { if jv.is_null() || jv.is_undefined() { return crate::array::js_array_alloc(0); } - let mut out = crate::array::js_array_alloc(8); + // ownKeys, getOwnPropertyDescriptor and getPrototypeOf can invoke user + // callbacks. Keep every value needed after them in relocatable handles, + // including the output accumulated while walking earlier prototypes. + let scope = crate::gc::RuntimeHandleScope::new(); + let current = scope.root_nanbox_f64(value); + let out = scope.root_raw_mut_ptr(crate::array::js_array_alloc(8)); // Non-pointer primitives (number/boolean, boxed string) have only their own // enumerable keys; every prototype property they inherit is non-enumerable. if !jv.is_pointer() { - let own = js_object_keys_value(value); - let n = crate::array::js_array_length(own); + let own = scope.root_raw_const_ptr(js_object_keys_value(current.get_nanbox_f64())); + let n = own.with_const_ptr(|array| crate::array::js_array_length(array)); for i in 0..n { - let kv = crate::array::js_array_get(own, i); - out = crate::array::js_array_push_f64(out, f64::from_bits(kv.bits())); + let kv = own.with_const_ptr(|own| crate::array::js_array_get(own, i)); + let updated = out.with_mut_ptr(|out| { + crate::array::js_array_push_f64(out, f64::from_bits(kv.bits())) + }); + out.set_raw_mut_ptr(updated); } - return out; + return out.with_mut_ptr(|out: *mut ArrayHeader| out); } let key_string = |kv: JSValue, scratch: &mut [u8; crate::value::SHORT_STRING_MAX_LEN]| { unsafe { crate::string::js_string_key_bytes(kv, scratch) } @@ -298,30 +306,34 @@ pub extern "C" fn js_for_in_keys_value(value: f64) -> *mut ArrayHeader { }; let mut seen: std::collections::HashSet = std::collections::HashSet::new(); let mut scratch = [0u8; crate::value::SHORT_STRING_MAX_LEN]; - let mut current = value; // Depth cap guards against pathological / cyclic prototype graphs. for _ in 0..1000 { - let cv = JSValue::from_bits(current.to_bits()); + let cv = JSValue::from_bits(current.get_nanbox_u64()); if cv.is_null() || cv.is_undefined() || !cv.is_pointer() { break; } // Emit this level's enumerable own keys (OrdinaryOwnPropertyKeys order), // skipping any name already shadowed by a closer level. - let enum_arr = js_object_keys_value(current); - let en = crate::array::js_array_length(enum_arr); + let level = crate::gc::RuntimeHandleScope::new(); + let enum_arr = level.root_raw_const_ptr(js_object_keys_value(current.get_nanbox_f64())); + let en = enum_arr.with_const_ptr(|array| crate::array::js_array_length(array)); for i in 0..en { - let kv = crate::array::js_array_get(enum_arr, i); + let kv = enum_arr.with_const_ptr(|keys| crate::array::js_array_get(keys, i)); let name = match key_string(kv, &mut scratch) { Some(s) => s, None => continue, }; if seen.insert(name) { - out = crate::array::js_array_push_f64(out, f64::from_bits(kv.bits())); + let updated = out.with_mut_ptr(|out| { + crate::array::js_array_push_f64(out, f64::from_bits(kv.bits())) + }); + out.set_raw_mut_ptr(updated); } } // Mark ALL own names (incl non-enumerable) seen so they shadow the // remainder of the chain. - let all_f64 = super::super::descriptors::js_object_get_own_property_names(current); + let all_f64 = + super::super::descriptors::js_object_get_own_property_names(current.get_nanbox_f64()); let all_arr = (all_f64.to_bits() & crate::value::POINTER_MASK) as *mut ArrayHeader; if !all_arr.is_null() { let an = crate::array::js_array_length(all_arr); @@ -332,9 +344,11 @@ pub extern "C" fn js_for_in_keys_value(value: f64) -> *mut ArrayHeader { } } } - current = super::super::object_ops::js_object_get_prototype_of(current); + current.set_nanbox_f64(super::super::object_ops::js_object_get_prototype_of( + current.get_nanbox_f64(), + )); } - out + out.with_mut_ptr(|out: *mut ArrayHeader| out) } fn closure_dynamic_enumerable_props(ptr: usize) -> Vec<(String, f64)> { diff --git a/crates/perry-runtime/src/proxy/reflect.rs b/crates/perry-runtime/src/proxy/reflect.rs index cecf2299cd..38ed6001dc 100644 --- a/crates/perry-runtime/src/proxy/reflect.rs +++ b/crates/perry-runtime/src/proxy/reflect.rs @@ -236,10 +236,11 @@ fn descriptor_key(name: &[u8]) -> (*const crate::StringHeader, f64) { } pub(super) unsafe fn descriptor_field_present(desc: f64, name: &[u8]) -> bool { + let scope = crate::gc::RuntimeHandleScope::new(); + let desc_handle = scope.root_nanbox_f64(desc); let (key, key_value) = descriptor_key(name); + let desc = desc_handle.get_nanbox_f64(); if lookup(desc).is_some() { - let scope = crate::gc::RuntimeHandleScope::new(); - let desc_handle = scope.root_nanbox_f64(desc); let key_handle = scope.root_nanbox_f64(key_value); return crate::value::js_is_truthy(js_proxy_has( desc_handle.get_nanbox_f64(), @@ -251,10 +252,11 @@ pub(super) unsafe fn descriptor_field_present(desc: f64, name: &[u8]) -> bool { } unsafe fn descriptor_field(desc: f64, name: &[u8]) -> f64 { + let scope = crate::gc::RuntimeHandleScope::new(); + let desc_handle = scope.root_nanbox_f64(desc); let (key, key_value) = descriptor_key(name); + let desc = desc_handle.get_nanbox_f64(); if lookup(desc).is_some() { - let scope = crate::gc::RuntimeHandleScope::new(); - let desc_handle = scope.root_nanbox_f64(desc); let key_handle = scope.root_nanbox_f64(key_value); return js_proxy_get(desc_handle.get_nanbox_f64(), key_handle.get_nanbox_f64()); } @@ -266,10 +268,12 @@ unsafe fn descriptor_field(desc: f64, name: &[u8]) -> f64 { } pub(super) unsafe fn descriptor_bool_field(desc: f64, name: &[u8]) -> Option { - if !descriptor_field_present(desc, name) { + let scope = crate::gc::RuntimeHandleScope::new(); + let desc_handle = scope.root_nanbox_f64(desc); + if !descriptor_field_present(desc_handle.get_nanbox_f64(), name) { return None; } - Some(crate::value::js_is_truthy(descriptor_field(desc, name)) != 0) + Some(crate::value::js_is_truthy(descriptor_field(desc_handle.get_nanbox_f64(), name)) != 0) } unsafe fn complete_proxy_descriptor_result(desc: f64) -> f64 { @@ -278,36 +282,42 @@ unsafe fn complete_proxy_descriptor_result(desc: f64) -> f64 { } let scope = crate::gc::RuntimeHandleScope::new(); let desc_handle = scope.root_nanbox_f64(desc); - let desc = desc_handle.get_nanbox_f64(); - let has_enumerable = descriptor_field_present(desc, b"enumerable"); - let has_configurable = descriptor_field_present(desc, b"configurable"); - let has_value = descriptor_field_present(desc, b"value"); - let has_writable = descriptor_field_present(desc, b"writable"); - let has_get = descriptor_field_present(desc, b"get"); - let has_set = descriptor_field_present(desc, b"set"); + let has_enumerable = descriptor_field_present(desc_handle.get_nanbox_f64(), b"enumerable"); + let has_configurable = descriptor_field_present(desc_handle.get_nanbox_f64(), b"configurable"); + let has_value = descriptor_field_present(desc_handle.get_nanbox_f64(), b"value"); + let has_writable = descriptor_field_present(desc_handle.get_nanbox_f64(), b"writable"); + let has_get = descriptor_field_present(desc_handle.get_nanbox_f64(), b"get"); + let has_set = descriptor_field_present(desc_handle.get_nanbox_f64(), b"set"); - let enumerable = - has_enumerable && crate::value::js_is_truthy(descriptor_field(desc, b"enumerable")) != 0; + let enumerable = has_enumerable + && crate::value::js_is_truthy(descriptor_field( + desc_handle.get_nanbox_f64(), + b"enumerable", + )) != 0; let configurable = has_configurable - && crate::value::js_is_truthy(descriptor_field(desc, b"configurable")) != 0; + && crate::value::js_is_truthy(descriptor_field( + desc_handle.get_nanbox_f64(), + b"configurable", + )) != 0; let value = if has_value { - descriptor_field(desc, b"value") + descriptor_field(desc_handle.get_nanbox_f64(), b"value") } else { f64::from_bits(TAG_UNDEFINED) }; let value_handle = scope.root_nanbox_f64(value); - let writable = - has_writable && crate::value::js_is_truthy(descriptor_field(desc, b"writable")) != 0; + let writable = has_writable + && crate::value::js_is_truthy(descriptor_field(desc_handle.get_nanbox_f64(), b"writable")) + != 0; let getter = if has_get { - descriptor_field(desc, b"get") + descriptor_field(desc_handle.get_nanbox_f64(), b"get") } else { f64::from_bits(TAG_UNDEFINED) }; let getter_handle = scope.root_nanbox_f64(getter); let setter = if has_set { - descriptor_field(desc, b"set") + descriptor_field(desc_handle.get_nanbox_f64(), b"set") } else { f64::from_bits(TAG_UNDEFINED) }; @@ -360,40 +370,61 @@ pub extern "C" fn js_reflect_get_own_property_descriptor(target: f64, key: f64) return revoked_return(); } - let trap = handler_trap(handler, "getOwnPropertyDescriptor"); + // The handler lookup, trap, and descriptor field getters can all run JS. + // A root is useful only if each post-callback read reloads its current value. + let inner_handle = scope.root_nanbox_f64(inner); + let handler_handle = scope.root_nanbox_f64(handler); + let trap = handler_trap(handler_handle.get_nanbox_f64(), "getOwnPropertyDescriptor"); let trap_bits = trap.to_bits(); if trap_bits == TAG_UNDEFINED || trap_bits == TAG_NULL { // No trap — forward to the target's [[GetOwnProperty]]. When the target // is itself a Proxy, recurse through the Reflect entry point rather than // the ordinary object path, which would deref the fake proxy pointer. - if lookup(inner).is_some() { - return js_reflect_get_own_property_descriptor(inner, property_key); + if lookup(inner_handle.get_nanbox_f64()).is_some() { + return js_reflect_get_own_property_descriptor( + inner_handle.get_nanbox_f64(), + property_key_handle.get_nanbox_f64(), + ); } - return crate::object::js_object_get_own_property_descriptor(inner, property_key); + return crate::object::js_object_get_own_property_descriptor( + inner_handle.get_nanbox_f64(), + property_key_handle.get_nanbox_f64(), + ); } if !is_callable_function(trap) { return throw_type_error("proxy getOwnPropertyDescriptor trap is not a function"); } - let rebound = crate::closure::clone_closure_rebind_this(trap_bits, handler); + let rebound = + crate::closure::clone_closure_rebind_this(trap_bits, handler_handle.get_nanbox_f64()); let closure = closure_from(f64::from_bits(rebound)); if closure.is_null() { return throw_type_error("proxy getOwnPropertyDescriptor trap is not a function"); } let this_scope = crate::gc::RuntimeHandleScope::new(); // #9445 - let prev = this_scope.root_nanbox_f64(crate::object::js_implicit_this_set(handler)); - let result = js_closure_call2(closure, inner, property_key); + let prev = this_scope.root_nanbox_f64(crate::object::js_implicit_this_set( + handler_handle.get_nanbox_f64(), + )); + let result = js_closure_call2( + closure, + inner_handle.get_nanbox_f64(), + property_key_handle.get_nanbox_f64(), + ); crate::object::js_implicit_this_set(prev.get_nanbox_f64()); let result_handle = scope.root_nanbox_f64(result); - let target_desc = crate::object::js_object_get_own_property_descriptor(inner, property_key); + let target_desc = crate::object::js_object_get_own_property_descriptor( + inner_handle.get_nanbox_f64(), + property_key_handle.get_nanbox_f64(), + ); let target_desc_handle = scope.root_nanbox_f64(target_desc); let result = result_handle.get_nanbox_f64(); - let target_desc = target_desc_handle.get_nanbox_f64(); if result.to_bits() == TAG_UNDEFINED { - if target_desc.to_bits() != TAG_UNDEFINED - && (crate::object::obj_value_no_extend(inner) - || unsafe { descriptor_bool_field(target_desc, b"configurable") } == Some(false)) + if target_desc_handle.get_nanbox_u64() != TAG_UNDEFINED + && (crate::object::obj_value_no_extend(inner_handle.get_nanbox_f64()) + || unsafe { + descriptor_bool_field(target_desc_handle.get_nanbox_f64(), b"configurable") + } == Some(false)) { return throw_type_error( "proxy getOwnPropertyDescriptor trap cannot hide target property", @@ -407,16 +438,17 @@ pub extern "C" fn js_reflect_get_own_property_descriptor(target: f64, key: f64) } let result = unsafe { complete_proxy_descriptor_result(result) }; let result_handle = scope.root_nanbox_f64(result); - let result = result_handle.get_nanbox_f64(); - if target_desc.to_bits() == TAG_UNDEFINED { - if crate::object::obj_value_no_extend(inner) { + if target_desc_handle.get_nanbox_u64() == TAG_UNDEFINED { + if crate::object::obj_value_no_extend(inner_handle.get_nanbox_f64()) { return throw_type_error( "proxy getOwnPropertyDescriptor trap reports new property on non-extensible target", ); } - } else if unsafe { descriptor_bool_field(target_desc, b"configurable") } == Some(false) - && unsafe { descriptor_bool_field(result, b"configurable") } == Some(true) + } else if unsafe { descriptor_bool_field(target_desc_handle.get_nanbox_f64(), b"configurable") } + == Some(false) + && unsafe { descriptor_bool_field(result_handle.get_nanbox_f64(), b"configurable") } + == Some(true) { return throw_type_error( "proxy getOwnPropertyDescriptor trap reports incompatible descriptor", @@ -425,9 +457,13 @@ pub extern "C" fn js_reflect_get_own_property_descriptor(target: f64, key: f64) // [[GetOwnProperty]] step 21.a: a non-configurable result descriptor is only // valid for a non-configurable existing target property. - if unsafe { descriptor_bool_field(result, b"configurable") } == Some(false) { - let target_configurable = target_desc.to_bits() == TAG_UNDEFINED - || unsafe { descriptor_bool_field(target_desc, b"configurable") } != Some(false); + if unsafe { descriptor_bool_field(result_handle.get_nanbox_f64(), b"configurable") } + == Some(false) + { + let target_configurable = target_desc_handle.get_nanbox_u64() == TAG_UNDEFINED + || unsafe { + descriptor_bool_field(target_desc_handle.get_nanbox_f64(), b"configurable") + } != Some(false); if target_configurable { return throw_type_error( "proxy getOwnPropertyDescriptor trap reports a non-configurable descriptor for a configurable or absent target property", @@ -435,5 +471,5 @@ pub extern "C" fn js_reflect_get_own_property_descriptor(target: f64, key: f64) } } - result + result_handle.get_nanbox_f64() } diff --git a/test-files/test_gap_gc_for_in_proxy_callback_roots.ts b/test-files/test_gap_gc_for_in_proxy_callback_roots.ts new file mode 100644 index 0000000000..cc2d14d882 --- /dev/null +++ b/test-files/test_gap_gc_for_in_proxy_callback_roots.ts @@ -0,0 +1,46 @@ +// for...in retains its output and current receiver while Proxy traps run JS. +// The inherited proxy fires after own keys have already grown the output past +// its initial capacity. Churn in each trap exposes stale locals under the GC +// schedule/protection matrix without relying on a Node-only gc() function. +let trapCalls = 0; +function churn() { + for (let i = 0; i < 24; i++) { + const garbage = { value: ["temporary", i, trapCalls] }; + if (garbage.value.length !== 3) throw new Error("allocation witness"); + } + trapCalls++; +} +function wrapped(target: any): any { + return new Proxy(target, { + ownKeys(value) { churn(); return Reflect.ownKeys(value); }, + getOwnPropertyDescriptor(value, key) { + churn(); return Reflect.getOwnPropertyDescriptor(value, key); + }, + getPrototypeOf(value) { churn(); return Reflect.getPrototypeOf(value); }, + }); +} +const inherited = wrapped({ inheritedA: 1, inheritedB: 2, hidden: 3 }); +const target: any = Object.create(inherited); +for (let i = 0; i < 14; i++) target["own" + i] = i; +Object.defineProperty(target, "hidden", { value: 4, enumerable: false, configurable: true }); +for (const receiver of [target, wrapped(target)]) { + const keys: string[] = []; + for (const key in receiver) keys.push(key); + const expected = Array.from({ length: 14 }, (_, i) => "own" + i).concat(["inheritedA", "inheritedB"]); + if (keys.join(",") !== expected.join(",")) throw new Error("enumeration lost keys: " + keys.join(",")); + console.log(keys.join(",")); +} +if (trapCalls === 0) throw new Error("traps were not invoked"); +const descriptorProxy = new Proxy({ property_name: 23 }, { + getOwnPropertyDescriptor(value, key) { + return { + enumerable: true, configurable: true, writable: true, + get value() { churn(); return value[key]; }, + }; + }, +}); +const descriptor = Reflect.getOwnPropertyDescriptor(descriptorProxy, "property_name")!; +if (descriptor.value !== 23 || !descriptor.writable || !descriptor.enumerable || !descriptor.configurable) { + throw new Error("descriptor fields lost across collection"); +} +console.log("PASS for-in callback roots"); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 7f76d12528..80310c49f9 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -856,3 +856,6 @@ test_gap_gc_string_repeat_reentrant_count # Pre-fix: SIGBUS after the first copying minor under # PERRY_GC_PROTECT_FROMSPACE=1; post-fix: `600000 0.35 0.25 0.1 100000 0.35`. test_gap_gc_coalesce_local_root + +# for-in output/receiver custody across Proxy callbacks (#4644) +test_gap_gc_for_in_proxy_callback_roots From fcbb0e50312df043d83e2fec49b373ba0a06c572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 11:04:46 +0200 Subject: [PATCH 4/4] docs: number for-in callback roots changeset for PR 9864 --- ...644-for-in-callback-roots.md => 9864-for-in-callback-roots.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{4644-for-in-callback-roots.md => 9864-for-in-callback-roots.md} (100%) diff --git a/changelog.d/4644-for-in-callback-roots.md b/changelog.d/9864-for-in-callback-roots.md similarity index 100% rename from changelog.d/4644-for-in-callback-roots.md rename to changelog.d/9864-for-in-callback-roots.md