diff --git a/changelog.d/7179-gc-pointer-publish-barrier.md b/changelog.d/7179-gc-pointer-publish-barrier.md new file mode 100644 index 0000000000..068ccd06ec --- /dev/null +++ b/changelog.d/7179-gc-pointer-publish-barrier.md @@ -0,0 +1,20 @@ +**Two raw pointer stores made the evacuating minor GC lose live objects (#7154).** Perry allocates every heap object in the `POINTER_FREE` layout state and only leaves that state when a store *records* a pointer through `layout_note_slot`. `heap_payload_slot_selection` short-circuits on `POINTER_FREE` and skips the **entire** payload without consulting any mask, so a raw `*slot = bits` into a traced slot is not merely imprecise — it makes the child invisible. The evacuating young-generation minor (default-on since #7019) then neither keeps the child alive nor rewrites the slot when the child moves, and the next read of that slot returns a pointer into reclaimed nursery memory: `TypeError: value is not a function` at a call site unrelated to the store, or a SIGSEGV inside the collector on a later cycle. The same raw store also skips `runtime_write_barrier_slot`, so an old→young edge never reaches the remembered set. + +**Invariant, now stated at both sites:** *a store that publishes a pointer-bearing value into a slot the collector traces — object field, array element, closure capture — must go through the barriered store helper (`js_object_set_field` / `js_closure_set_capture_*` / `runtime_store_jsvalue_slot`), which records the layout bit and dirties the remembered-set page. A raw slot write is only sound for a value that is provably not pointer-bearing, or for an object that is not yet published.* + +Fixed: + +- `bind_reserved_this_slot` (`symbol/properties.rs`), shared by `js_object_set_method_by_name` (#809, the ordered object-literal lowering used when a `...spread` precedes a `this`-reading method) and `js_object_set_symbol_method`, patched the closure's reserved `this` capture slot with a raw `*slot = f64`. The receiver was therefore never traced. Measured on a Perry-compiled zod workload: `PERRY_GC_FROMSPACE_SCAN=1` reported **294 dangling closure-capture edges per cycle → 0**, all pointing at live receivers from freshly-copied survivor closures whose layout read `POINTER_FREE`. +- `js_weakmap_set`'s **overwrite-existing-key** path (`weakref.rs`) wrote the new value into the entry's field 1 — byte offset **+40**, the offset #7154's diagnostic scan reports — with the unbarriered `write_object_field_bits_raw`. The insert path (`weak_entry_new`) already used `js_object_set_field`; the overwrite now matches. For a tenured entry this was a genuine missing remembered-set insert, so a young value could be swept while the mapping was still live. + +Two hardening changes in the same family: + +- `js_closure_alloc` now undefined-initializes the capture slots. They were raw recycled arena bytes, invisible while the layout says `POINTER_FREE` but decoded as references by anything that reads payload words (the conservative scan, `PERRY_GC_FROMSPACE_SCAN`, a later whole-range layout rebuild). Mirrors #7138's HOLE-initialisation of unused array capacity and `js_object_alloc`'s undefined-fill. +- `js_object_set_field_by_name`'s three dynamic-key **append** paths bumped `field_count` *after* publishing the value, with `mirror_class_object_static_write` in between. `gc_field_slot_range` bounds the collector's view of the payload by `field_count`, so a slot the count does not yet cover is invisible to both tracing and evacuation rewriting. The count is now widened first; every physical slot is undefined-initialized at allocation, so the widened range can only expose non-pointer sentinels. Latent today (nothing in that window allocates from the arena) — the ordering is now correct by construction rather than by audit. + +Regression coverage in `gc/tests/copying/pointer_publish_7154.rs`. Each test first interrogates the collector's own child-slot enumerator (deterministic, immune to GC timing and conservative-stack pinning), then asserts end-to-end survival across a collection that is proven to have copied (`copied_objects > 0`, kept true by a rooted young canary so the liveness gate cannot be satisfied by the edge under test). All three are red on the pre-fix stores and green after. + +Review follow-up: `js_object_set_method_by_name` and `js_object_set_symbol_method` now root the receiver, key/symbol and closure across the capture store and re-derive every raw pointer afterwards. The barriered store is not an arena allocator today (`layout_note_slot` only touches thread-local layout maps; `runtime_write_barrier_gc_slot` ends in a remembered-set insert), so nothing was miscompiled — but that is a reachability claim about three other functions, and #7114 is what it costs when one goes stale. `bind_reserved_this_slot` now documents itself as a collection point, and `js_weakmap_set`'s scan loop records why its "no allocation in this loop" invariant still holds (the barriered store is the last thing the match arm does before returning from a handle). + +Coverage note after #7161: the gap test drives 33 copying minors under `PERRY_GC_MOVING_LOOP_POLLS=1` and zero under the now-non-moving shipped default, so its `default` / `verify_evac` / `cons_scan_off*` matrix cells are `UNVER` by design — the bug cannot manifest without a moving collector. The three Rust tests do not depend on that knob: they call `gc_collect_minor` directly and assert `copied_objects > 0`. + diff --git a/crates/perry-runtime/src/closure/alloc.rs b/crates/perry-runtime/src/closure/alloc.rs index 591793a165..1ff5dc1a88 100644 --- a/crates/perry-runtime/src/closure/alloc.rs +++ b/crates/perry-runtime/src/closure/alloc.rs @@ -152,6 +152,18 @@ pub extern "C" fn js_closure_alloc(func_ptr: *const u8, capture_count: u32) -> * (*ptr).func_ptr = func_ptr; (*ptr).capture_count = capture_count; // Preserve flag in high bit (*ptr).type_tag = CLOSURE_MAGIC; + // #7154: a fresh closure's capture slots are raw recycled arena bytes. + // They are invisible to the collector while the layout says + // POINTER_FREE, but any code path (conservative scan, diagnostic + // from-space scan, a later layout rebuild over the whole slot range) + // that reads them decodes garbage as a reference. Initialize them to a + // non-pointer sentinel, mirroring what `js_object_alloc` does for + // object fields and what #7138 did for unused array capacity. + let slots = closure_capture_slots_mut(ptr); + for i in 0..actual_count { + // GC_STORE_AUDIT(INIT): fresh closure capture slot, pointer-free sentinel. + std::ptr::write(slots.add(i), crate::value::TAG_UNDEFINED); + } crate::gc::layout_init_pointer_free(ptr as *mut u8); } diff --git a/crates/perry-runtime/src/gc/tests/copying.rs b/crates/perry-runtime/src/gc/tests/copying.rs index e4ada935d8..cffaf0074f 100644 --- a/crates/perry-runtime/src/gc/tests/copying.rs +++ b/crates/perry-runtime/src/gc/tests/copying.rs @@ -1,3 +1,4 @@ +mod pointer_publish_7154; mod promise_side_tables; mod survival_and_malloc; mod weak_holder_registry; diff --git a/crates/perry-runtime/src/gc/tests/copying/pointer_publish_7154.rs b/crates/perry-runtime/src/gc/tests/copying/pointer_publish_7154.rs new file mode 100644 index 0000000000..ca8511ee9f --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/copying/pointer_publish_7154.rs @@ -0,0 +1,269 @@ +//! #7154 — publishing a pointer into a GC-traced payload slot must RECORD it. +//! +//! Invariant under test: +//! +//! > Any store that publishes a pointer-bearing value into a slot the collector +//! > traces (an object field, an array element, a closure capture) must go +//! > through the barriered store helper, so the store both records the layout +//! > bit (`layout_note_slot`) and dirties the remembered-set page +//! > (`runtime_write_barrier_slot`). A raw `*slot = bits` breaks it silently. +//! +//! Why a raw store is fatal rather than merely conservative: every heap object +//! is allocated in `GC_LAYOUT_POINTER_FREE` and only leaves that state when a +//! store *records* a pointer. `heap_payload_slot_selection` short-circuits on +//! `POINTER_FREE` and skips the WHOLE payload without consulting any mask, so a +//! raw store leaves the child untraced: the evacuating young-generation minor +//! neither keeps it alive nor rewrites the slot when it moves. The next read of +//! that slot returns a pointer into reclaimed nursery memory — `TypeError: +//! value is not a function` at a call site with no relation to the store, or a +//! SIGSEGV inside the collector on a later cycle. +//! +//! Both cases below were live offenders found with `PERRY_GC_FROMSPACE_SCAN=1` +//! on a Perry-compiled zod workload (294 dangling closure-capture edges per +//! cycle, deterministic SIGSEGV; clean under `PERRY_GC_MOVING_LOOP_POLLS=0`). +//! +//! The first assertion in each test is the *deterministic* one: it interrogates +//! the collector's own child-slot enumerator directly, so it is red on the raw +//! store regardless of GC timing or conservative-stack pinning. The survival +//! assertions that follow are the end-to-end check and are gated on the cycle +//! having actually copied something. + +use super::*; + +extern "C" fn test_bound_method_body(_closure: *const crate::closure::ClosureHeader) -> f64 { + 0.0 +} + +/// Addresses of the slots the collector says it will visit inside `user_ptr`'s +/// payload. A `POINTER_FREE` payload yields an empty list — that is exactly the +/// failure mode this file guards. +unsafe fn enumerated_child_slots(user_ptr: usize) -> Vec { + test_heap_child_slots_for_user(user_ptr as *mut u8) + .into_iter() + .filter_map(|slot| match slot { + HeapChildSlot::Child(p, _) => Some(p as usize), + HeapChildSlot::PointerFreeRange(_) => None, + }) + .collect() +} + +/// `js_object_set_method_by_name` (the ordered object-literal lowering used when +/// a `...spread` precedes a `this`-reading method) patches the closure's +/// reserved `this` capture slot with the receiver. That store publishes a +/// pointer into a traced payload slot. +#[test] +fn test_bound_this_capture_is_traced_after_method_bind_7154() { + // Slot 0 closure, slot 1 receiver, slot 2 key. `js_object_set_method_by_name` + // allocates (it interns the key, may clone the keys array and may grow the + // object), so it is a collection point: nothing may be carried across it in a + // raw Rust local. Rust stack locals are not roots and do not pin, so every + // address is parked in a shadow slot and re-read afterwards — otherwise a + // minor landing inside the call would fail this test for a reason unrelated + // to the fix. Same discipline as the WeakMap test below. + let _guard = CopyingNurseryTestGuard::new(3); + + js_shadow_slot_set( + 0, + ptr_bits(crate::closure::js_closure_alloc( + test_bound_method_body as *const u8, + crate::closure::CAPTURES_THIS_FLAG | 1, + ) as usize), + ); + js_shadow_slot_set(1, ptr_bits(crate::object::js_object_alloc(0, 1) as usize)); + js_shadow_slot_set( + 2, + string_bits(crate::string::js_string_from_bytes(b"m".as_ptr(), 1) as usize), + ); + + unsafe { + // Every argument is read from its slot at the call, so the last + // allocation before the call cannot leave a sibling argument stale. + crate::symbol::js_object_set_method_by_name( + f64::from_bits(ptr_bits((js_shadow_slot_get(1) & POINTER_MASK) as usize)), + f64::from_bits(string_bits((js_shadow_slot_get(2) & POINTER_MASK) as usize)), + f64::from_bits(ptr_bits((js_shadow_slot_get(0) & POINTER_MASK) as usize)), + ); + } + + // The `this` slot now holds the receiver. The collector must say so. + // Re-derive the closure — the call above may have moved it. + let closure = (js_shadow_slot_get(0) & POINTER_MASK) as *mut crate::closure::ClosureHeader; + let capture_slot = unsafe { crate::closure::closure_capture_slots_mut(closure) as usize }; + let enumerated = unsafe { enumerated_child_slots(closure as usize) }; + assert!( + enumerated.contains(&capture_slot), + "#7154: the bound `this` capture slot must be enumerated as a child \ + edge after `js_object_set_method_by_name`; the collector reported \ + {enumerated:?} (a raw slot store leaves the closure POINTER_FREE, so \ + the whole capture payload is skipped)" + ); + + // End to end: the closure (slot 0) becomes the ONLY root — drop the direct + // roots on the receiver and the key so the receiver is reachable exclusively + // through the capture edge under test. It has to survive a copying minor + // through that edge, and the slot has to be rewritten to its new address. + js_shadow_slot_set(1, crate::value::TAG_UNDEFINED); + js_shadow_slot_set(2, crate::value::TAG_UNDEFINED); + let trace = collect_minor_trace(GcTriggerKind::Direct); + assert!( + trace.copying_nursery.copied_objects > 0, + "#7154 regression test requires a COPYING minor; a non-moving \ + collection cannot expose a missing rewrite (copied_objects=0)" + ); + + let moved_closure = + (js_shadow_slot_get(0) & POINTER_MASK) as *mut crate::closure::ClosureHeader; + let capture_bits = crate::closure::js_closure_get_capture_bits(moved_closure, 0); + let recovered = (capture_bits & POINTER_MASK) as usize; + assert!( + crate::arena::classify_heap_generation(recovered) != crate::arena::HeapGeneration::Unknown, + "#7154: the bound receiver must still be a live heap object after a \ + copying minor (capture bits {capture_bits:#x})" + ); + unsafe { + let header = header_from_user_ptr(recovered as *const u8); + assert_eq!( + (*header).gc_flags & GC_FLAG_FORWARDED, + 0, + "#7154: the `this` capture slot still points at a FORWARDED \ + from-space copy — the slot was not rewritten" + ); + assert_eq!( + (*header).obj_type, + GC_TYPE_OBJECT, + "#7154: the `this` capture no longer names an object" + ); + } +} + +/// `js_weakmap_set` overwriting an EXISTING key publishes the new value into +/// field 1 (+40 from the user pointer — the offset #7154's diagnostic scan +/// reports) of an already-reachable entry object. +/// +/// The interesting case is the OLD entry: once the entry has been promoted, an +/// overwrite creates an old→young edge, and only the write barrier puts that +/// edge in the remembered set. A raw store leaves the minor blind to it. +#[test] +fn test_weakmap_overwrite_value_is_traced_7154() { + let _guard = CopyingNurseryTestGuard::new(3); + + js_shadow_slot_set(0, ptr_bits(crate::weakref::js_weakmap_new() as usize)); + js_shadow_slot_set(1, ptr_bits(crate::object::js_object_alloc(0, 0) as usize)); + + let live = + |slot: u32| f64::from_bits(ptr_bits((js_shadow_slot_get(slot) & POINTER_MASK) as usize)); + + // Insert, then age the map/key/entry graph until the entry is tenured. + crate::weakref::js_weakmap_set( + live(0), + live(1), + f64::from_bits(ptr_bits(crate::object::js_object_alloc(0, 0) as usize)), + ); + for _ in 0..6 { + let _ = gc_collect_minor(); + } + let entry_addr = weak_entry_addr_for(live(0), live(1)); + assert!( + crate::arena::pointer_in_old_gen(entry_addr), + "#7154 test setup: the WeakMap entry must be promoted to old-gen so the \ + overwrite creates an old->young edge (entry at {entry_addr:#x})" + ); + + // OVERWRITE with a fresh young value — the existing-entry path, which is + // the one that used a raw store. The value is allocated inline so no named + // local holds its address across the collection below. + crate::weakref::js_weakmap_set( + live(0), + live(1), + f64::from_bits(ptr_bits(crate::object::js_object_alloc(0, 1) as usize)), + ); + + let before = crate::weakref::js_weakmap_get(live(0), live(1)).to_bits() & POINTER_MASK; + assert!( + before != 0, + "weakmap overwrite must be observable through `get`" + ); + assert!( + crate::arena::pointer_in_nursery(before as usize), + "#7154 test setup: the overwritten value must be YOUNG" + ); + + // Rooted young canary: keeps `copied_objects > 0` true independently of the + // subject, so the liveness gate below cannot be satisfied (or dissatisfied) + // by the very edge under test. + js_shadow_slot_set(2, ptr_bits(crate::object::js_object_alloc(0, 0) as usize)); + let trace = collect_minor_trace(GcTriggerKind::Direct); + assert!( + trace.copying_nursery.copied_objects > 0, + "#7154 regression test requires a COPYING minor (copied_objects=0)" + ); + + let after = crate::weakref::js_weakmap_get(live(0), live(1)); + let recovered = (after.to_bits() & POINTER_MASK) as usize; + assert!( + recovered != 0 + && crate::arena::classify_heap_generation(recovered) + != crate::arena::HeapGeneration::Unknown, + "#7154: the overwritten WeakMap value must survive a copying minor \ + (got {:#x})", + after.to_bits() + ); + unsafe { + let header = header_from_user_ptr(recovered as *const u8); + assert_eq!( + (*header).gc_flags & GC_FLAG_FORWARDED, + 0, + "#7154: WeakMap value slot still points at a FORWARDED from-space \ + copy — the slot was not rewritten" + ); + assert_eq!( + (*header).obj_type, + GC_TYPE_OBJECT, + "#7154: the WeakMap value slot no longer names an object" + ); + } +} + +/// Address of the entry object a WeakMap holds for `key`, found by walking the +/// map's entries array the same way `js_weakmap_get` does. +fn weak_entry_addr_for(map: f64, key: f64) -> usize { + let map_ptr = (map.to_bits() & POINTER_MASK) as *mut crate::ObjectHeader; + unsafe { + let entries = crate::object::js_object_get_field(map_ptr, 0); + let entries_ptr = (entries.bits() & POINTER_MASK) as *mut crate::array::ArrayHeader; + let len = crate::array::js_array_length(entries_ptr) as usize; + for i in 0..len { + let entry_val = crate::array::js_array_get(entries_ptr, i as u32); + let entry = (entry_val.bits() & POINTER_MASK) as *mut crate::ObjectHeader; + if entry.is_null() { + continue; + } + let stored_key = crate::object::js_object_get_field(entry, 0); + if stored_key.bits() == key.to_bits() { + return entry as usize; + } + } + 0 + } +} + +/// A freshly allocated closure's capture slots must read as a non-pointer +/// sentinel, not raw recycled arena bytes: anything that decodes payload words +/// (the conservative scan, `PERRY_GC_FROMSPACE_SCAN`, a later whole-range layout +/// rebuild) would otherwise follow garbage as a reference. Mirrors #7138's +/// HOLE-initialisation of unused array capacity. +#[test] +fn test_fresh_closure_capture_slots_are_initialized_7154() { + let _guard = CopyingNurseryTestGuard::new(1); + let closure = crate::closure::js_closure_alloc(test_bound_method_body as *const u8, 3); + unsafe { + let slots = crate::closure::closure_capture_slots_mut(closure); + for i in 0..3 { + assert_eq!( + *slots.add(i), + crate::value::TAG_UNDEFINED, + "#7154: fresh closure capture slot {i} must be undefined-initialized" + ); + } + } +} diff --git a/crates/perry-runtime/src/object/field_set_by_name.rs b/crates/perry-runtime/src/object/field_set_by_name.rs index 67c540a784..5d5b0aa62c 100644 --- a/crates/perry-runtime/src/object/field_set_by_name.rs +++ b/crates/perry-runtime/src/object/field_set_by_name.rs @@ -1428,12 +1428,18 @@ pub extern "C" fn js_object_set_field_by_name( // Reallocate fields to hold at least one value // Note: We assume the object has enough field slots pre-allocated - js_object_set_field(obj, 0, JSValue::from_bits(value.to_bits())); - mirror_class_object_static_write(obj, key, value); + // #7154 publication order: `gc_field_slot_range` bounds the + // collector's view of the payload by `field_count`, so a slot at an + // index the count does not yet cover is invisible to BOTH tracing + // and evacuation rewriting. Widen the count FIRST — every physical + // slot is undefined-initialized at allocation, so the widened range + // can only expose non-pointer sentinels — then publish the value. // Bump field_count so Object.keys()/values()/entries() see the new property. if (*obj).field_count == 0 { (*obj).field_count = 1; } + js_object_set_field(obj, 0, JSValue::from_bits(value.to_bits())); + mirror_class_object_static_write(obj, key, value); // Record the null→single-key transition so the next object // that starts with `{}` and sets the same first key hits the // fast path above instead of allocating a fresh 4-elem @@ -1647,11 +1653,17 @@ pub extern "C" fn js_object_set_field_by_name( if !keys_shared { super::shapes::shape_keys_grown(prev_keys_usize, new_keys); } - js_object_set_field(obj, new_index as u32, JSValue::from_bits(value.to_bits())); - mirror_class_object_static_write(obj, key, value); + // #7154 publication order: `gc_field_slot_range` bounds the + // collector's view of the payload by `field_count`, so a slot at an + // index the count does not yet cover is invisible to BOTH tracing + // and evacuation rewriting. Widen the count FIRST — every physical + // slot is undefined-initialized at allocation, so the widened range + // can only expose non-pointer sentinels — then publish the value. if new_index as u32 >= (*obj).field_count { (*obj).field_count = new_index as u32 + 1; } + js_object_set_field(obj, new_index as u32, JSValue::from_bits(value.to_bits())); + mirror_class_object_static_write(obj, key, value); transition_cache_insert( prev_keys_usize, interned_key, @@ -1841,12 +1853,18 @@ pub extern "C" fn js_object_set_field_by_name( } // Set the field at the new index and update logical field_count - js_object_set_field(obj, new_index as u32, JSValue::from_bits(value.to_bits())); - mirror_class_object_static_write(obj, key, value); + // #7154 publication order: `gc_field_slot_range` bounds the + // collector's view of the payload by `field_count`, so a slot at an + // index the count does not yet cover is invisible to BOTH tracing + // and evacuation rewriting. Widen the count FIRST — every physical + // slot is undefined-initialized at allocation, so the widened range + // can only expose non-pointer sentinels — then publish the value. // Bump field_count to reflect the newly added property if new_index as u32 >= (*obj).field_count { (*obj).field_count = new_index as u32 + 1; } + js_object_set_field(obj, new_index as u32, JSValue::from_bits(value.to_bits())); + mirror_class_object_static_write(obj, key, value); // Record the shape transition — see above for semantics. transition_cache_insert( prev_keys_usize, diff --git a/crates/perry-runtime/src/symbol/properties.rs b/crates/perry-runtime/src/symbol/properties.rs index e935e22bcb..b3bb318c3f 100644 --- a/crates/perry-runtime/src/symbol/properties.rs +++ b/crates/perry-runtime/src/symbol/properties.rs @@ -619,8 +619,20 @@ pub unsafe extern "C" fn js_object_set_symbol_method( sym_f64: f64, closure_f64: f64, ) -> f64 { - bind_reserved_this_slot(closure_f64, obj_f64); - js_object_set_symbol_property_infer_name(obj_f64, sym_f64, closure_f64) + // #7154 review: `bind_reserved_this_slot` is a *collection point* (see the + // note on that function). Every value that outlives it — the receiver, the + // symbol, the closure — is rooted and re-derived afterwards rather than + // carried in a raw register across it. + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_h = scope.root_nanbox_f64(obj_f64); + let sym_h = scope.root_nanbox_f64(sym_f64); + let closure_h = scope.root_nanbox_f64(closure_f64); + bind_reserved_this_slot(closure_h.get_nanbox_f64(), obj_h.get_nanbox_f64()); + js_object_set_symbol_property_infer_name( + obj_h.get_nanbox_f64(), + sym_h.get_nanbox_f64(), + closure_h.get_nanbox_f64(), + ) } /// Patch the closure's reserved (LAST) capture slot with `obj_f64` so a @@ -637,6 +649,29 @@ pub unsafe extern "C" fn js_object_set_symbol_method( /// non-closure value (e.g. a Proxy of a function) has no capture array to patch, /// so it is simply stored as-is; the call site then dispatches it through the /// proxy `[[Call]]` path. +/// +/// #7154: the store MUST go through `js_closure_set_capture_f64`, not a raw +/// `*slot = bits`. A closure is allocated `GC_LAYOUT_POINTER_FREE` and only +/// leaves that state when a capture store *records* the pointer +/// (`layout_note_slot`). `heap_payload_slot_selection` short-circuits on +/// `POINTER_FREE` and skips the ENTIRE capture payload without consulting any +/// mask, so a raw store left the receiver untraced: the evacuating young-gen +/// minor neither kept it alive nor rewrote the slot when it moved, and the +/// method later read a pointer into reclaimed nursery memory ("value is not a +/// function", or a SIGSEGV in the collector on the next cycle). The raw store +/// also skipped the write barrier, so an old-gen closure -> young receiver edge +/// never entered the remembered set. +/// +/// ***TREAT THIS FUNCTION AS A COLLECTION POINT.*** The barriered store reaches +/// `layout_note_slot` (which resolves forwarded headers recursively and can +/// populate the layout side tables) and `runtime_write_barrier_gc_slot` (which +/// can insert into the remembered set). Neither allocates from the arena +/// *today*, so no caller is miscompiled right now — but that is a reachability +/// argument about someone else's code, and #7114 is what happens when one of +/// those goes stale. Callers root `obj_f64`/`closure_f64` across this call and +/// re-derive afterwards instead of relying on it; both entry points do. +/// `c_ptr` is derived from `closure_f64` immediately before its only use, with +/// no intervening call. unsafe fn bind_reserved_this_slot(closure_f64: f64, obj_f64: f64) { let c_bits = closure_f64.to_bits(); if c_bits & 0xFFFF_0000_0000_0000 != POINTER_TAG { @@ -649,10 +684,7 @@ unsafe fn bind_reserved_this_slot(closure_f64: f64, obj_f64: f64) { let c_ptr = c_addr as *mut crate::closure::ClosureHeader; let real_count = crate::closure::real_capture_count((*c_ptr).capture_count); if real_count >= 1 { - let captures_ptr = (c_ptr as *mut u8) - .add(std::mem::size_of::()) - as *mut f64; - *captures_ptr.add((real_count - 1) as usize) = obj_f64; + crate::closure::js_closure_set_capture_f64(c_ptr, real_count - 1, obj_f64); } } @@ -675,18 +707,29 @@ pub unsafe extern "C" fn js_object_set_method_by_name( key_f64: f64, closure_f64: f64, ) -> f64 { + // #7154 review: `bind_reserved_this_slot` is a collection point (see its + // doc comment), and step 2 dereferences the receiver as an `ObjectHeader`. + // Root the receiver, key and closure across step 1 and re-derive every raw + // pointer from the handles afterwards, so a minor GC inside the barriered + // capture store cannot leave `obj_ptr` naming a from-space copy — the #7114 + // failure shape (an operand register outliving a collection point). + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_h = scope.root_nanbox_f64(obj_f64); + let key_h = scope.root_nanbox_f64(key_f64); + let closure_h = scope.root_nanbox_f64(closure_f64); + // 1) Patch the closure's reserved (last) `this` capture slot with obj. // No-op for anything that is not a real heap closure (#6320). - bind_reserved_this_slot(closure_f64, obj_f64); + bind_reserved_this_slot(closure_h.get_nanbox_f64(), obj_h.get_nanbox_f64()); // 2) Set the field by name. `js_object_set_field_by_name` strips the // NaN-box tag off `obj` itself, so passing the raw bits is fine; the - // key must be a real `StringHeader*` (tag stripped). - let key_bits = key_f64.to_bits(); - let key_ptr = (key_bits & POINTER_MASK) as *const StringHeader; - let obj_ptr = obj_f64.to_bits() as *mut crate::object::ObjectHeader; + // key must be a real `StringHeader*` (tag stripped). Both pointers are + // re-derived from the handles here, AFTER step 1. + let key_ptr = (key_h.get_nanbox_f64().to_bits() & POINTER_MASK) as *const StringHeader; + let obj_ptr = obj_h.get_nanbox_f64().to_bits() as *mut crate::object::ObjectHeader; if crate::value::addr_class::is_above_handle_band(key_ptr as usize) { - crate::object::js_object_set_field_by_name(obj_ptr, key_ptr, closure_f64); + crate::object::js_object_set_field_by_name(obj_ptr, key_ptr, closure_h.get_nanbox_f64()); } - obj_f64 + obj_h.get_nanbox_f64() } diff --git a/crates/perry-runtime/src/weakref.rs b/crates/perry-runtime/src/weakref.rs index d92af6ea00..07c8cecf9d 100644 --- a/crates/perry-runtime/src/weakref.rs +++ b/crates/perry-runtime/src/weakref.rs @@ -1498,6 +1498,13 @@ pub extern "C" fn js_weakmap_set(map: f64, key: f64, value: f64) -> f64 { // tombstone (an entry whose key the GC collected) so a new key can // reuse the freed slot instead of growing the array unboundedly. This // scan performs no allocation, so `entries_ptr` stays valid throughout. + // + // #7154: the one exception is the barriered `js_object_set_field` on the + // match arm below — treat it as a collection point. It is the LAST thing + // that arm does: the arm returns immediately, deriving its result from + // `map_handle`, so neither `entries_ptr` nor `entry` is read after it. + // ***If you ever make that arm fall through to another iteration, + // re-derive `entries_ptr` from `map_handle` first.*** let mut first_tomb: i64 = -1; for i in 0..len { let entry = weak_entry_at(entries_ptr, i); @@ -1512,10 +1519,18 @@ pub extern "C" fn js_weakmap_set(map: f64, key: f64, value: f64) -> f64 { continue; } if stored_key == key_handle.get_nanbox_f64().to_bits() { - write_object_field_bits_raw( + // #7154: overwriting an EXISTING mapping publishes a new value + // into a long-lived, already-reachable entry object — it must + // take the barriered store, exactly like `weak_entry_new`'s + // insert. The raw write recorded neither the layout bit (so the + // evacuating minor skipped the payload and dropped the value) + // nor the remembered-set page (so an old entry -> young value + // edge was invisible to a minor). Field 1 is +40 from the user + // pointer, the offset the #7154 diagnostic scan reports. + js_object_set_field( entry, - WEAK_ENTRY_VALUE_FIELD, - value_handle.get_nanbox_f64().to_bits(), + WEAK_ENTRY_VALUE_FIELD as u32, + JSValue::from_bits(value_handle.get_nanbox_f64().to_bits()), ); return map_handle.get_nanbox_f64(); } diff --git a/test-files/test_gap_gc_pointer_publish.ts b/test-files/test_gap_gc_pointer_publish.ts new file mode 100644 index 0000000000..4ab69cec51 --- /dev/null +++ b/test-files/test_gap_gc_pointer_publish.ts @@ -0,0 +1,117 @@ +// #7154: publishing a pointer into a GC-traced payload slot must RECORD it. +// +// Three runtime helpers used to write a pointer into a slot the collector +// traces with a raw `*slot = bits`, skipping `layout_note_slot` and the write +// barrier. A heap object is allocated `GC_LAYOUT_POINTER_FREE` and only leaves +// that state when a store RECORDS a pointer; `heap_payload_slot_selection` +// short-circuits on `POINTER_FREE` and skips the WHOLE payload without +// consulting any mask. So the child was never traced: the evacuating young-gen +// minor neither kept it alive nor rewrote the slot when it moved, and the next +// read of the slot returned a pointer into reclaimed nursery memory. +// +// LIVE BY CONSTRUCTION. Each subject is held across escaping allocation churn +// heavy enough to reach the collector, and is READ AFTER the churn — a +// non-moving collection cannot expose this, so the evacuating arms are the ones +// that bite. Keep the churn budgets in sync with the matrix harness: if a +// collector change stops this file collecting, the matrix reports its cells +// UNVER rather than green. + +const sink: unknown[] = new Array(1024); + +function churn(n: number): void { + for (let i = 0; i < n; i++) { + sink[i & 1023] = { a: i, b: "s" + (i & 7), c: [i, i + 1] }; + } +} + +// --------------------------------------------------------------------------- +// 1. Object literal that interleaves a `...spread` with a `this`-reading +// method. That lowering routes through the runtime's +// `js_object_set_method_by_name` (#809), which patches the closure's +// reserved `this` capture slot with the receiver. A raw patch left the +// closure POINTER_FREE, so the receiver was untraced and `this.kind` later +// read reclaimed memory. +// --------------------------------------------------------------------------- +interface Described { + kind: string; + n: number; + describe(): string; +} + +const base = { kind: "base", extra: 7 }; +const described: Described[] = []; +for (let r = 0; r < 40; r++) { + const o = { + ...base, + n: r, + describe(): string { + return this.kind + ":" + this.n; + }, + }; + described.push(o); + churn(3000); +} + +let spreadAcc = ""; +for (let i = 0; i < described.length; i++) { + spreadAcc += described[i].describe() + "|"; +} +console.log("spread-method:", spreadAcc.length, described[0].describe(), described[39].describe()); + +// --------------------------------------------------------------------------- +// 2. Computed symbol-keyed method that reads `this`. Same reserved-`this`-slot +// patch, through `js_object_set_symbol_method`. +// --------------------------------------------------------------------------- +const prim = Symbol.toPrimitive; +const boxes: { value: number }[] = []; +for (let r = 0; r < 40; r++) { + const b = { + value: r, + [prim](): number { + return this.value * 2 + 1; + }, + }; + boxes.push(b); + churn(3000); +} + +let symbolTotal = 0; +for (let i = 0; i < boxes.length; i++) { + symbolTotal += Number(boxes[i]); +} +console.log("symbol-method:", symbolTotal); + +// --------------------------------------------------------------------------- +// 3. WeakMap OVERWRITE of an existing key. The insert path was barriered; the +// overwrite wrote the new value into the entry's field 1 (+40 — the offset +// #7154's diagnostic scan reports) with a raw store. Once the entry is +// tenured, that is a missing remembered-set insert: the young replacement +// value is invisible to the next minor. +// --------------------------------------------------------------------------- +const wm = new WeakMap(); +const keys: object[] = []; +for (let i = 0; i < 40; i++) { + const k = { id: i }; + keys.push(k); + wm.set(k, { v: -1, tag: "seed" }); +} +churn(60000); // age the entries out of the nursery +for (let i = 0; i < keys.length; i++) { + wm.set(keys[i], { v: i, tag: "t" + i }); +} +churn(60000); + +let wmSum = 0; +let wmTags = 0; +for (let i = 0; i < keys.length; i++) { + const e = wm.get(keys[i]); + if (e === undefined) { + throw new Error("weakmap entry lost at " + i); + } + wmSum += e.v; + wmTags += e.tag.length; +} +console.log("weakmap-overwrite:", wmSum, wmTags); + +// Keep the sink observably live so the churn allocations cannot be elided. +console.log("sink:", sink.length, typeof sink[0]); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 9625f297f1..0498503ae5 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -136,6 +136,33 @@ test_gap_repsel_scalar_replaced_locals # the evacuating arms in that frame, not only inside a callee. test_gap_repsel_module_init_canonical +# --- Raw pointer publishes into traced payload slots (#7154) ----------------- +# Not a representation file, so registered explicitly per the header rule. +# Three runtime helpers published a pointer into a GC-traced slot with a raw +# `*slot = bits`, skipping `layout_note_slot` (so the holder stayed +# `POINTER_FREE` and `heap_payload_slot_selection` skipped its WHOLE payload) +# and the write barrier (so an old->young edge never reached the remembered +# set). Every subject is read AFTER the churn that drives collection, so a +# non-moving collection cannot satisfy it. Verified to FAIL on the pre-fix +# stores (SIGBUS, exit 138) and pass after. +# +# ***THIS FILE IS LIVE ONLY ON THE MOVING ARMS, AND THAT IS THE POINT.*** +# Measured 33 copying minors under `PERRY_GC_MOVING_LOOP_POLLS=1` / +# `evac_minor` / `force_evac`, and ZERO under the shipped default -- because +# #7161 flipped the evacuating minor off by default pending #7154. So its +# `default` / `verify_evac` / `cons_scan_off*` cells are UNVER, correctly: the +# bug it guards cannot manifest without a moving collector, and a PASS there +# would be the false confidence this harness exists to remove. +# +# The consequence to keep in view: `loop_polls` (and the other `requires=move` +# arms) are now the ONLY place this file bites end to end. If #7161 is ever +# reverted the cells flip to PASS on their own; if instead the moving-loop knob +# is deleted, this file goes dark and must be re-tuned or retired with it. The +# collector-side coverage does NOT depend on that knob: the three Rust tests in +# `gc/tests/copying/pointer_publish_7154.rs` call `gc_collect_minor` directly, +# which takes the copying path regardless, and each asserts `copied_objects > 0`. +test_gap_gc_pointer_publish + # --- The GC-live member ------------------------------------------------------ # Every file above performs ZERO collections (measured, #6950), which makes the # GC arms inert against them. This one holds each representation's local live