From 0679a708f78526f46cfc81fefe16696f545f1d21 Mon Sep 17 00:00:00 2001 From: Andrew DiZenzo Date: Wed, 10 Jun 2026 17:39:49 -0400 Subject: [PATCH 1/2] fix(gc): avoid closure side-table GC deadlock --- .../src/closure/dynamic_props.rs | 163 ++++++++++++++---- crates/perry/tests/gc_write_barrier_stress.rs | 79 ++++++++- 2 files changed, 200 insertions(+), 42 deletions(-) diff --git a/crates/perry-runtime/src/closure/dynamic_props.rs b/crates/perry-runtime/src/closure/dynamic_props.rs index 8a2185b27..5ecab9c56 100644 --- a/crates/perry-runtime/src/closure/dynamic_props.rs +++ b/crates/perry-runtime/src/closure/dynamic_props.rs @@ -128,6 +128,26 @@ fn merge_closure_prop_map( } } +fn forwarded_heap_owner(owner: usize) -> Option { + if owner == 0 { + return None; + } + if matches!( + crate::arena::classify_heap_generation(owner), + crate::arena::HeapGeneration::Unknown + ) { + return None; + } + unsafe { + let header = + (owner as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; + if (*header).gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 { + return None; + } + Some(crate::gc::forwarding_address(header) as usize) + } +} + pub(crate) fn closure_dynamic_props_owner_moved(old_owner: usize, new_owner: usize) { if old_owner == 0 || new_owner == 0 || old_owner == new_owner { return; @@ -207,46 +227,64 @@ pub(crate) fn visit_closure_static_prototype_slot_mut( /// ajv's `validate.errors = [{ msg }]`) had its element objects freed /// behind the still-live array. pub fn scan_closure_dynamic_props_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { - let mut moved = Vec::new(); - if let Ok(mut props) = get_closure_props().lock() { - for (&owner, closure_props) in props.iter_mut() { - // Metadata key rewrite. Only fires in rewrite-phase modes; - // mark phases return `false` here without recording the key - // as a root (so the side-table entry doesn't itself keep - // the closure alive — same semantics as overflow-field - // metadata, see `visit_metadata_usize_slot`). - let mut new_owner = owner; - if visitor.visit_metadata_usize_slot(&mut new_owner) { - moved.push((owner, new_owner)); - } - // #1802: trace every stored value in every phase. In `Mark` - // / `CopyingMark` this keeps `fn.errors = [...]` and its - // transitive contents reachable; in rewrite phases it - // updates the slot bits when a value was forwarded. - for value in closure_props.values_mut() { - visitor.visit_nanbox_f64_slot(value); - } + let prop_owners = get_closure_props() + .lock() + .ok() + .map(|props| props.keys().copied().collect::>()) + .unwrap_or_default(); + for owner in prop_owners { + let Some(mut closure_props) = get_closure_props() + .lock() + .ok() + .and_then(|mut props| props.remove(&owner)) + else { + continue; + }; + + // Metadata key rewrite. Only fires in rewrite-phase modes; mark phases + // return `false` here without recording the key as a root (so the + // side-table entry doesn't itself keep the closure alive). + let mut new_owner = owner; + visitor.visit_metadata_usize_slot(&mut new_owner); + // #1802: trace every stored value in every phase. In `Mark` / + // `CopyingMark` this keeps `fn.errors = [...]` and its transitive + // contents reachable; in rewrite phases it updates slot bits when a + // value was forwarded. + for value in closure_props.values_mut() { + visitor.visit_nanbox_f64_slot(value); } - for (old_owner, new_owner) in moved { - if let Some(old_props) = props.remove(&old_owner) { - merge_closure_prop_map(&mut props, new_owner, old_props); - } + if new_owner == owner { + new_owner = forwarded_heap_owner(owner).unwrap_or(owner); + } + + if let Ok(mut props) = get_closure_props().lock() { + merge_closure_prop_map(&mut props, new_owner, closure_props); } } - let mut moved_prototypes = Vec::new(); - if let Ok(mut prototypes) = get_closure_prototypes().lock() { - for (owner, proto_bits) in prototypes.iter_mut() { - let old_owner = *owner; - let mut new_owner = old_owner; - if visitor.visit_metadata_usize_slot(&mut new_owner) { - moved_prototypes.push((old_owner, new_owner)); - } - visitor.visit_nanbox_u64_slot(proto_bits); + + let prototype_owners = get_closure_prototypes() + .lock() + .ok() + .map(|prototypes| prototypes.keys().copied().collect::>()) + .unwrap_or_default(); + for owner in prototype_owners { + let Some(mut proto_bits) = get_closure_prototypes() + .lock() + .ok() + .and_then(|mut prototypes| prototypes.remove(&owner)) + else { + continue; + }; + + let mut new_owner = owner; + visitor.visit_metadata_usize_slot(&mut new_owner); + visitor.visit_nanbox_u64_slot(&mut proto_bits); + if new_owner == owner { + new_owner = forwarded_heap_owner(owner).unwrap_or(owner); } - for (old_owner, new_owner) in moved_prototypes { - if let Some(proto_bits) = prototypes.remove(&old_owner) { - prototypes.insert(new_owner, proto_bits); - } + + if let Ok(mut prototypes) = get_closure_prototypes().lock() { + prototypes.insert(new_owner, proto_bits); } } // #3655: re-key the deleted-keys side table when a closure moves. The @@ -316,6 +354,10 @@ pub extern "C" fn js_value_is_closure(value_bits: i64) -> i32 { /// Get a dynamic property stored on a closure. /// Returns TAG_UNDEFINED if not found. pub fn closure_get_dynamic_prop(ptr: usize, prop: &str) -> f64 { + if !is_closure_ptr(ptr) { + return f64::from_bits(crate::value::TAG_UNDEFINED); + } + if let Some(acc) = crate::object::get_accessor_descriptor(ptr, prop) { if acc.get == 0 { return f64::from_bits(crate::value::TAG_UNDEFINED); @@ -591,6 +633,8 @@ pub extern "C" fn js_closure_unbind_this(val: f64) -> f64 { mod tests_1802 { use super::*; + static SIDE_TABLE_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// #1802: the side-table values must be visited in mark phases, not /// only during the metadata-rewrite tail. Pre-fix /// `scan_closure_dynamic_props_roots_mut` early-returned unless @@ -600,6 +644,7 @@ mod tests_1802 { /// removed, the adapter sees every stored value's bits. #[test] fn dyn_prop_values_are_visited_in_mark_phase() { + let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap(); // A unique synthetic closure address (just an integer key — the // scanner doesn't deref it during value visitation; the // metadata-key visitor is a no-op for non-heap addresses). @@ -631,6 +676,52 @@ mod tests_1802 { } } + #[test] + fn dyn_prop_scanner_visits_values_without_holding_props_lock() { + let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap(); + let owner: usize = 0xC10C_AB1E_0000_1803; + let value_bits: u64 = 0x7FFD_AAAA_BBBB_CCCD; + closure_set_dynamic_prop(owner, "errors", f64::from_bits(value_bits)); + + let mut saw_value = false; + let mut lock_was_free = false; + { + let mut mark = |v: f64| { + if v.to_bits() == value_bits { + saw_value = true; + lock_was_free = get_closure_props().try_lock().is_ok(); + } + }; + let mut visitor = crate::gc::RuntimeRootVisitor::for_copy(&mut mark); + scan_closure_dynamic_props_roots_mut(&mut visitor); + } + + assert!( + saw_value, + "scanner did not visit the stored closure prop value" + ); + assert!( + lock_was_free, + "scanner must not hold CLOSURE_PROPS while visitor callbacks can move closures" + ); + + if let Ok(mut props) = get_closure_props().lock() { + props.remove(&owner); + } + } + + #[test] + fn dyn_prop_get_ignores_non_closure_receivers() { + let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap(); + let obj = crate::object::js_object_alloc(0, 0) as usize; + + assert_eq!( + closure_get_dynamic_prop(obj, "payload").to_bits(), + crate::value::TAG_UNDEFINED, + "ordinary objects must not enter closure-only Function.prototype fallback" + ); + } + /// #4740: `is_closure_ptr` must NOT dereference an address in the /// `[0x10000, 0x100000)` native-handle band. Web Fetch response handles /// (`0x40000+`), node:http / axios / fastify ids live there and are not diff --git a/crates/perry/tests/gc_write_barrier_stress.rs b/crates/perry/tests/gc_write_barrier_stress.rs index 58943f7d2..ecf6fcc7e 100644 --- a/crates/perry/tests/gc_write_barrier_stress.rs +++ b/crates/perry/tests/gc_write_barrier_stress.rs @@ -19,7 +19,24 @@ //! literals are avoided too (compile-time blowup, #4880). use std::path::PathBuf; -use std::process::Command; +use std::process::{Command, Output, Stdio}; +use std::time::{Duration, Instant}; + +#[cfg(unix)] +use std::os::unix::process::CommandExt; + +#[cfg(unix)] +unsafe extern "C" { + fn kill(pid: i32, sig: i32) -> i32; + fn setsid() -> i32; +} + +#[cfg(unix)] +const SIGTERM: i32 = 15; +#[cfg(unix)] +const SIGKILL: i32 = 9; + +const COMPILED_BINARY_TIMEOUT: Duration = Duration::from_secs(120); fn perry_bin() -> PathBuf { PathBuf::from(env!("CARGO_BIN_EXE_perry")) @@ -46,11 +63,61 @@ fn compile_and_run(source: &str) -> std::process::Output { String::from_utf8_lossy(&compile.stderr) ); - Command::new(&output) - .env("PERRY_GC_FORCE_EVACUATE", "1") - .env("PERRY_GC_VERIFY_EVACUATION", "1") - .output() - .expect("run compiled binary") + let mut run = Command::new(&output); + run.env("PERRY_GC_FORCE_EVACUATE", "1") + .env("PERRY_GC_VERIFY_EVACUATION", "1"); + run_compiled_binary_with_timeout(run, COMPILED_BINARY_TIMEOUT) +} + +fn run_compiled_binary_with_timeout(mut command: Command, timeout: Duration) -> Output { + command.stdout(Stdio::piped()).stderr(Stdio::piped()); + #[cfg(unix)] + unsafe { + command.pre_exec(|| { + if setsid() == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + + let mut child = command.spawn().expect("run compiled binary"); + let start = Instant::now(); + loop { + match child.try_wait().expect("poll compiled binary") { + Some(_) => { + return child + .wait_with_output() + .expect("collect compiled binary output") + } + None if start.elapsed() >= timeout => { + #[cfg(unix)] + unsafe { + let pgid = -(child.id() as i32); + kill(pgid, SIGTERM); + std::thread::sleep(Duration::from_millis(250)); + if child.try_wait().expect("poll after SIGTERM").is_none() { + kill(pgid, SIGKILL); + } + } + #[cfg(not(unix))] + { + child.kill().expect("kill timed out compiled binary"); + } + let output = child + .wait_with_output() + .expect("collect timed out compiled binary output"); + panic!( + "compiled binary timed out after {:?}\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + timeout, + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + } + None => std::thread::sleep(Duration::from_millis(50)), + } + } } fn assert_ok_output(run: &std::process::Output, expected: &str) { From de45d0d03b5264b2f5ad11ab0c95fb14f458d48b Mon Sep 17 00:00:00 2001 From: Andrew DiZenzo Date: Wed, 10 Jun 2026 18:45:15 -0400 Subject: [PATCH 2/2] fix(gc): use addr-class helper for forwarded owner --- crates/perry-runtime/src/closure/dynamic_props.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/perry-runtime/src/closure/dynamic_props.rs b/crates/perry-runtime/src/closure/dynamic_props.rs index 5ecab9c56..3f7a86716 100644 --- a/crates/perry-runtime/src/closure/dynamic_props.rs +++ b/crates/perry-runtime/src/closure/dynamic_props.rs @@ -139,12 +139,11 @@ fn forwarded_heap_owner(owner: usize) -> Option { return None; } unsafe { - let header = - (owner as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; - if (*header).gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 { + let header = crate::value::addr_class::try_read_gc_header(owner)?; + if header.gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 { return None; } - Some(crate::gc::forwarding_address(header) as usize) + Some(crate::gc::forwarding_address(header as *const _) as usize) } }