From ec9720950ccc341912cd77bdb2868e1ad7c9f368 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sun, 16 Aug 2026 16:12:30 -0400 Subject: [PATCH] fix(gc): unify Map/Set tripwire on is_plausible_heap_addr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The defensive entries/elements tripwires added in #8251 used a local `addr >> 47 != 0` cutoff to catch the NaN-box signature (top bits 0x7FFD) of a fabricated Map/Set header. CodeRabbit noted two gaps: * The cutoff only rejects the upper bits; a low garbage address (below the handle band) or a handle-band id reads as `0 >> 47 == 0` and is accepted, so the collector could still derive a slot range from an unmapped / unrelated low address. * The cutoff is platform-wrong on aarch64 Linux, where user space reaches bit 48 (HEAP_MAX = 0x1_0000_0000_0000) but `>> 47` rejects bit 47+, so a genuine entries pointer in that range would be false-rejected. Replace both local checks with the shared `crate::value::addr_class::is_plausible_heap_addr` predicate already used across the gc module (forwarding, fromspace_scan, dead_owner). It pairs `is_above_handle_band` with the platform-correct `is_valid_obj_ptr` range, so it rejects low / handle-band / NaN-box garbage while accepting every genuine entries/elements pointer. Safety: Map entries and Set elements are always system-allocator pointers (`std::alloc::alloc`), never arena or slab, and a capacity-0 alloc is coerced to 4 so the pointer is real and non-null. System malloc returns heap-range addresses above the handle band on every supported platform, so `is_plausible_heap_addr` accepts all genuine entries and the change is strictly more conservative — it can only reject more garbage, never a live object. Regression test: `test_gc_element_slot_range_rejects_implausible_elements` covers NaN-box, low-addr, and handle-band `elements` words (all now rejected) plus a genuine Set (accepted). `cargo test -p perry-runtime --lib`: 2556 passed, 0 failed, 4 ignored. Follow-up to #8251 (CodeRabbit Major finding on layout_slot_visit / set.rs). --- .../perry-runtime/src/gc/layout_slot_visit.rs | 23 ++++--- crates/perry-runtime/src/set.rs | 61 +++++++++++++++++-- 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/crates/perry-runtime/src/gc/layout_slot_visit.rs b/crates/perry-runtime/src/gc/layout_slot_visit.rs index 622781eac4..ebc0aeabb8 100644 --- a/crates/perry-runtime/src/gc/layout_slot_visit.rs +++ b/crates/perry-runtime/src/gc/layout_slot_visit.rs @@ -203,17 +203,22 @@ pub(super) unsafe fn visit_gc_rewrite_slot_descriptors( } // Defensive tripwire (# fabricated-Map): if a fabricated Map // header ever slips past `plausible_gc_header`, the `entries` - // field would be a NaN-boxed JSValue (top bits 0x7FFD…) read - // from the original object's payload — an impossible pointer - // on x86-64 where user-space addresses stay below bit 47. - // Reject it rather than dereference a derived slot range from - // a garbage base. This is a backstop; the primary fix is the - // fixed-layout size check in `plausible_gc_header`. - if ((*map).entries as usize) >> 47 != 0 { + // field would be a NaN-boxed JSValue (top bits 0x7FFD…) or some + // other non-pointer word read from the original object's + // payload. Reject it rather than dereference a derived slot + // range from a garbage base. This is a backstop; the primary + // fix is the fixed-layout size check in `plausible_gc_header`. + // Use the shared heap-address classifier so the bound is + // platform-correct (a fixed `>> 47` cutoff would false-reject + // genuine entries on aarch64 Linux, where user space reaches + // bit 48) and so low / handle-band garbage is rejected too, + // not only the NaN-box signature. + let entries_addr = (*map).entries as usize; + if !crate::value::addr_class::is_plausible_heap_addr(entries_addr) { if crate::gc::gc_diag_enabled() { eprintln!( - "[gc-tripwire] Map entries has implausible top bits: {:#x} (fabricated Map?)", - (*map).entries as usize + "[gc-tripwire] Map entries is not a plausible heap address: {:#x} (fabricated Map?)", + entries_addr ); } return; diff --git a/crates/perry-runtime/src/set.rs b/crates/perry-runtime/src/set.rs index 1a8b54a9db..868e81b3a6 100644 --- a/crates/perry-runtime/src/set.rs +++ b/crates/perry-runtime/src/set.rs @@ -557,12 +557,18 @@ pub(crate) unsafe fn gc_element_slot_range( } // Defensive tripwire (cf. Map's entries check in layout_slot_visit): // a NaN-boxed JSValue read as `elements` carries 0x7FFD in the top - // bits — an impossible x86-64 user-space pointer. - if ((*set).elements as usize) >> 47 != 0 { + // bits, and other non-pointer payload words can land below the heap + // range or in the handle band. Reject any `elements` that is not a + // plausible heap address rather than dereference a derived slot range + // from a garbage base. Uses the shared platform-correct classifier + // (a fixed `>> 47` cutoff would false-reject genuine elements on + // aarch64 Linux, where user space reaches bit 48). + let elements_addr = (*set).elements as usize; + if !crate::value::addr_class::is_plausible_heap_addr(elements_addr) { if crate::gc::gc_diag_enabled() { eprintln!( - "[gc-tripwire] Set elements has implausible top bits: {:#x} (fabricated Set?)", - (*set).elements as usize + "[gc-tripwire] Set elements is not a plausible heap address: {:#x} (fabricated Set?)", + elements_addr ); } return None; @@ -2446,4 +2452,51 @@ mod tests { "cross-tag strings with same content should be equal" ); } + + /// `gc_element_slot_range`'s defensive tripwire must reject an + /// `elements` word that is not a plausible heap address — a NaN-boxed + /// JSValue (top bits 0x7FFD), a low garbage address below the handle + /// band, and a handle-band id — while still accepting a genuine + /// system-allocator `elements` pointer. The previous `>> 47` cutoff + /// only caught the NaN-box signature and accepted low / handle-band + /// garbage; the unified `is_plausible_heap_addr` classifier closes + /// both gaps and is platform-correct. + #[test] + fn test_gc_element_slot_range_rejects_implausible_elements() { + // Genuine Set: elements is a real system-allocator pointer. + let set = js_set_alloc(4); + js_set_add(set, 1.0); + assert!( + unsafe { gc_element_slot_range(set) }.is_some(), + "genuine Set elements must yield a slot range" + ); + + // Build a stack SetHeader so we can control `elements` directly. + let mut header = SetHeader { + size: 1, + capacity: 4, + elements: std::ptr::null_mut(), + }; + + let cases: &[(&str, *mut f64)] = &[ + // NaN-boxed JSValue (POINTER_TAG 0x7FFD) — the fabricated-Map + // signature that crashes on dereference. + ("nan-box", 0x7FFD_0000_0000_1000u64 as *mut f64), + // Low address below the handle band — the old `>> 47` check + // accepted this (0 >> 47 == 0), letting the collector derive a + // slot range from an unmapped / unrelated low address. + ("low-addr", 0x4000u64 as *mut f64), + // Handle-band id (just below HANDLE_BAND_MAX = 0x100000). + ("handle-band", 0x80000u64 as *mut f64), + ]; + + for &(label, fake_elements) in cases { + header.elements = fake_elements; + let got = unsafe { gc_element_slot_range(&mut header as *mut SetHeader) }; + assert!( + got.is_none(), + "elements={label} must be rejected by the tripwire, got {got:?}" + ); + } + } }