From 1b4ff98924eb5d9424c8118385f5717943e2d3a7 Mon Sep 17 00:00:00 2001 From: logbie Date: Thu, 16 Jul 2026 10:29:48 -0500 Subject: [PATCH 1/2] Make cyclic values safe to format and clone --- CHANGELOG.md | 4 + Dev diary/2026-07-16-cycle-safe-values.md | 41 +++ src/interpreter/tests.rs | 25 ++ src/interpreter/value.rs | 333 ++++++++++++++++------ tests/value_cycle_safety.rs | 96 +++++++ 5 files changed, 412 insertions(+), 87 deletions(-) create mode 100644 Dev diary/2026-07-16-cycle-safe-values.md create mode 100644 tests/value_cycle_safety.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 061ce563..2887bc5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased] ### Security +- **Cyclic values no longer abort the interpreter during display, diagnostics, + or isolated-module cloning.** List/object formatting now detects cycles and + caps nesting depth, while deep clones preserve cycles and shared references + inside the cloned graph. - **Subprocess policy is enforced on every process launch** (shell path and direct-exec / `with arguments` path). Previously, `shell_execution_mode` and related checks ran only when the engine believed a shell was required, so diff --git a/Dev diary/2026-07-16-cycle-safe-values.md b/Dev diary/2026-07-16-cycle-safe-values.md new file mode 100644 index 00000000..3c842712 --- /dev/null +++ b/Dev diary/2026-07-16-cycle-safe-values.md @@ -0,0 +1,41 @@ +# Dev Diary — Cycle-safe values (2026-07-16) + +## Context + +Lists and objects are reference-counted mutable values, so valid WFL can build +self-referential and mutually recursive graphs. Value equality already handled +those graphs, but `Display`, `Debug`, and the deep clone used for module +isolation still traversed them recursively without cycle detection. Displaying +a cyclic value (or including one in a diagnostic) could therefore exhaust the +native stack, and cloning one could do the same during isolated lookup. + +## What changed + +- `Display` and `Debug` now carry per-format traversal state. A container that + reappears on the active path renders as ``; shared acyclic values still + render normally each time they appear. +- Formatting stops after 64 nested containers and renders ``, keeping + very deep acyclic graphs comfortably below the native stack limit. +- Formatting uses `try_borrow`, so an incidental outstanding mutable borrow is + rendered as a marker instead of causing a `RefCell` panic. +- `Value::deep_clone` now memoizes list, object, and container-instance + placeholders before cloning their contents. Cycles point into the cloned + graph, shared references remain shared within that graph, and the clone stays + isolated from the source. +- Container parent links are cloned through the same memo instead of retaining + a reference into the source graph. + +## Compatibility + +Acyclic values below the depth limit retain their existing display and debug +forms. Only values that previously recursed indefinitely, exceeded the new +nesting guard, or were formatted while mutably borrowed receive marker text. + +## Tests + +- Rust-level self-cycle and list/object mutual-cycle formatting regressions. +- Deep-clone assertions for source isolation, back-reference preservation, and + shared identity. +- A depth-bound regression for acyclic nesting. +- An interpreter regression that constructs a self-referential list with WFL's + `push` statement and displays it as `[]` without aborting. diff --git a/src/interpreter/tests.rs b/src/interpreter/tests.rs index 57fb32e5..15b956e1 100644 --- a/src/interpreter/tests.rs +++ b/src/interpreter/tests.rs @@ -583,3 +583,28 @@ async fn test_header_access_case_insensitive_via_request_object() { "absent header should be nothing, got {result:?}" ); } + +/// A WFL program can insert a list into itself through `push`. Displaying that +/// value used to recurse on the native stack until the whole process aborted. +#[tokio::test] +async fn test_display_self_referential_list_is_cycle_safe() { + let source = r#" +create list items: +end list +push with items and items +display items +"#; + let tokens = lex_wfl_with_positions(source); + let mut parser = Parser::new(&tokens); + let program = parser.parse().expect("parse self-referential list program"); + let mut interpreter = Interpreter::new(); + + let output = std::rc::Rc::new(std::cell::RefCell::new(String::new())); + let result = { + let _capture = super::io_capture::push_capture(std::rc::Rc::clone(&output)); + interpreter.interpret(&program).await + }; + + result.expect("displaying a self-referential list must not abort or error"); + assert_eq!(&*output.borrow(), "[]\n"); +} diff --git a/src/interpreter/value.rs b/src/interpreter/value.rs index 7d033a6a..302390ee 100644 --- a/src/interpreter/value.rs +++ b/src/interpreter/value.rs @@ -156,6 +156,32 @@ pub struct ActionSignature { pub column: usize, } +/// Keep value formatting comfortably below the native stack limit even when a +/// program builds a very deeply nested (but acyclic) container graph. +const MAX_VALUE_FORMAT_DEPTH: usize = 64; + +type ListStorage = RefCell>; +type ObjectStorage = RefCell>; +type ContainerInstanceStorage = RefCell; + +#[derive(Default)] +struct ValueFormatState { + active_lists: HashSet<*const ListStorage>, + active_objects: HashSet<*const ObjectStorage>, +} + +/// Memoized mutable containers created during one deep-clone operation. +/// +/// The placeholder is inserted before its contents are cloned. That both +/// breaks cycles and preserves aliases: two references to one source container +/// become two references to one cloned container. +#[derive(Default)] +struct DeepCloneMemo { + lists: HashMap<*const ListStorage, Rc>, + objects: HashMap<*const ObjectStorage, Rc>, + container_instances: HashMap<*const ContainerInstanceStorage, Rc>, +} + impl Value { pub fn type_name(&self) -> &'static str { match self { @@ -207,86 +233,169 @@ impl Value { /// Deep clone a value, creating independent copies of reference-counted containers. /// This is used for module isolation to prevent mutations from affecting parent scopes. pub fn deep_clone(&self) -> Self { + self.deep_clone_with_memo(&mut DeepCloneMemo::default()) + } + + fn deep_clone_with_memo(&self, memo: &mut DeepCloneMemo) -> Self { match self { - // For List, create a new Rc> with recursively cloned elements Value::List(list) => { - let cloned_vec = list + let source_id = Rc::as_ptr(list); + if let Some(cloned) = memo.lists.get(&source_id) { + return Value::List(Rc::clone(cloned)); + } + + let cloned = Rc::new(RefCell::new(Vec::new())); + memo.lists.insert(source_id, Rc::clone(&cloned)); + + let cloned_items = list .borrow() .iter() - .map(|v| v.deep_clone()) + .map(|value| value.deep_clone_with_memo(memo)) .collect::>(); - Value::List(Rc::new(RefCell::new(cloned_vec))) + *cloned.borrow_mut() = cloned_items; + + Value::List(cloned) } - // For Object, create a new Rc> with recursively cloned values Value::Object(obj) => { - let cloned_map = obj + let source_id = Rc::as_ptr(obj); + if let Some(cloned) = memo.objects.get(&source_id) { + return Value::Object(Rc::clone(cloned)); + } + + let cloned = Rc::new(RefCell::new(HashMap::new())); + memo.objects.insert(source_id, Rc::clone(&cloned)); + + let cloned_entries = obj .borrow() .iter() - .map(|(k, v)| (k.clone(), v.deep_clone())) + .map(|(key, value)| (key.clone(), value.deep_clone_with_memo(memo))) .collect::>(); - Value::Object(Rc::new(RefCell::new(cloned_map))) + *cloned.borrow_mut() = cloned_entries; + + Value::Object(cloned) } - // For ContainerInstance, create a new Rc> with deep cloned properties Value::ContainerInstance(instance) => { - let inst = instance.borrow(); - let cloned_properties = inst - .properties - .iter() - .map(|(k, v)| (k.clone(), v.deep_clone())) - .collect::>(); - let cloned_parent = inst.parent.as_ref().map(|p| { - // Clone the parent reference, not deep clone (to avoid infinite recursion) - Rc::clone(p) - }); - Value::ContainerInstance(Rc::new(RefCell::new(ContainerInstanceValue { - container_type: inst.container_type.clone(), - properties: cloned_properties, - parent: cloned_parent, - line: inst.line, - column: inst.column, - }))) + Value::ContainerInstance(Self::deep_clone_container_instance(instance, memo)) } // For all other types, use regular clone (they're either primitives or immutable Rc types) _ => self.clone(), } } -} -impl fmt::Debug for Value { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fn deep_clone_container_instance( + instance: &Rc>, + memo: &mut DeepCloneMemo, + ) -> Rc> { + let source_id = Rc::as_ptr(instance); + if let Some(cloned) = memo.container_instances.get(&source_id) { + return Rc::clone(cloned); + } + + let cloned = Rc::new(RefCell::new(ContainerInstanceValue { + container_type: String::new(), + properties: HashMap::new(), + parent: None, + line: 0, + column: 0, + })); + memo + .container_instances + .insert(source_id, Rc::clone(&cloned)); + + let source = instance.borrow(); + let cloned_properties = source + .properties + .iter() + .map(|(key, value)| (key.clone(), value.deep_clone_with_memo(memo))) + .collect(); + let cloned_parent = source + .parent + .as_ref() + .map(|parent| Self::deep_clone_container_instance(parent, memo)); + + *cloned.borrow_mut() = ContainerInstanceValue { + container_type: source.container_type.clone(), + properties: cloned_properties, + parent: cloned_parent, + line: source.line, + column: source.column, + }; + + cloned + } + + fn fmt_debug_with_state( + &self, + f: &mut fmt::Formatter, + state: &mut ValueFormatState, + depth: usize, + ) -> fmt::Result { match self { Value::Number(n) => write!(f, "{n}"), Value::Text(s) => write!(f, "\"{s}\""), Value::Bool(b) => write!(f, "{b}"), Value::Nothing => write!(f, "nothing"), - Value::List(l) => { - let values = l.borrow(); - write!(f, "[")?; - for (i, v) in values.iter().enumerate() { - if i > 0 { - write!(f, ", ")?; - } - write!(f, "{v:?}")?; + Value::List(list) => { + if depth >= MAX_VALUE_FORMAT_DEPTH { + return write!(f, ""); } - write!(f, "]") - } - Value::Object(o) => { - let map = o.borrow(); - write!(f, "{{")?; - for (i, (k, v)) in map.iter().enumerate() { - if i > 0 { - write!(f, ", ")?; + + let id = Rc::as_ptr(list); + if !state.active_lists.insert(id) { + return write!(f, ""); + } + + let result = (|| { + let values = match list.try_borrow() { + Ok(values) => values, + Err(_) => return write!(f, ""), + }; + + write!(f, "[")?; + for (index, value) in values.iter().enumerate() { + if index > 0 { + write!(f, ", ")?; + } + value.fmt_debug_with_state(f, state, depth + 1)?; } - write!(f, "{k}: {v:?}")?; + write!(f, "]") + })(); + + state.active_lists.remove(&id); + result + } + Value::Object(obj) => { + if depth >= MAX_VALUE_FORMAT_DEPTH { + return write!(f, ""); } - write!(f, "}}") + + let id = Rc::as_ptr(obj); + if !state.active_objects.insert(id) { + return write!(f, ""); + } + + let result = (|| { + let map = match obj.try_borrow() { + Ok(map) => map, + Err(_) => return write!(f, ""), + }; + + write!(f, "{{")?; + for (index, (key, value)) in map.iter().enumerate() { + if index > 0 { + write!(f, ", ")?; + } + write!(f, "{key}: ")?; + value.fmt_debug_with_state(f, state, depth + 1)?; + } + write!(f, "}}") + })(); + + state.active_objects.remove(&id); + result } Value::Function(func) => { - write!( - f, - "Function({})", - func.name.as_ref().unwrap_or(&"anonymous".to_string()) - ) + write!(f, "Function({})", func.name.as_deref().unwrap_or("anonymous")) } Value::NativeFunction(name, _) => write!(f, "NativeFunction({name})"), Value::Future(_) => write!(f, "[Future]"), @@ -306,54 +415,92 @@ impl fmt::Debug for Value { Value::InterfaceDefinition(interface) => write!(f, "", interface.name), } } -} -impl fmt::Display for Value { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fn fmt_display_with_state( + &self, + f: &mut fmt::Formatter, + state: &mut ValueFormatState, + depth: usize, + ) -> fmt::Result { match self { Value::Number(n) => write!(f, "{n}"), Value::Text(s) => write!(f, "{s}"), Value::Bool(b) => write!(f, "{}", if *b { "yes" } else { "no" }), Value::Nothing => write!(f, "nothing"), Value::List(list) => { - let items = list.borrow(); - write!(f, "[")?; - for (i, v) in items.iter().enumerate() { - if i > 0 { - write!(f, ", ")?; - } - write!(f, "{v}")?; + if depth >= MAX_VALUE_FORMAT_DEPTH { + return write!(f, ""); } - write!(f, "]") - } - Value::Object(o) => { - let map = o.borrow(); - if map.len() == 1 { - if let Some((_, value)) = map.iter().next() { - write!(f, "{value}") - } else { - write!(f, "[Object]") - } - } else if map.is_empty() { - write!(f, "[Object]") - } else { - write!(f, "{{")?; - for (i, (k, v)) in map.iter().enumerate() { - if i > 0 { + + let id = Rc::as_ptr(list); + if !state.active_lists.insert(id) { + return write!(f, ""); + } + + let result = (|| { + let items = match list.try_borrow() { + Ok(items) => items, + Err(_) => return write!(f, ""), + }; + + write!(f, "[")?; + for (index, value) in items.iter().enumerate() { + if index > 0 { write!(f, ", ")?; } - write!(f, "{k}: {v}")?; + value.fmt_display_with_state(f, state, depth + 1)?; } - write!(f, "}}") - } + write!(f, "]") + })(); + + state.active_lists.remove(&id); + result } - Value::Function(func) => { - write!( - f, - "action {}", - func.name.as_ref().unwrap_or(&"anonymous".to_string()) - ) + Value::Object(obj) => { + if depth >= MAX_VALUE_FORMAT_DEPTH { + return write!(f, ""); + } + + let id = Rc::as_ptr(obj); + if !state.active_objects.insert(id) { + return write!(f, ""); + } + + let result = (|| { + let map = match obj.try_borrow() { + Ok(map) => map, + Err(_) => return write!(f, ""), + }; + + if map.len() == 1 { + if let Some((_, value)) = map.iter().next() { + value.fmt_display_with_state(f, state, depth + 1) + } else { + write!(f, "[Object]") + } + } else if map.is_empty() { + write!(f, "[Object]") + } else { + write!(f, "{{")?; + for (index, (key, value)) in map.iter().enumerate() { + if index > 0 { + write!(f, ", ")?; + } + write!(f, "{key}: ")?; + value.fmt_display_with_state(f, state, depth + 1)?; + } + write!(f, "}}") + } + })(); + + state.active_objects.remove(&id); + result } + Value::Function(func) => write!( + f, + "action {}", + func.name.as_deref().unwrap_or("anonymous") + ), Value::NativeFunction(name, _) => write!(f, "native {name}"), Value::Future(_) => write!(f, "[Future]"), Value::Date(d) => write!(f, "{}", d.format("%Y-%m-%d")), @@ -374,6 +521,18 @@ impl fmt::Display for Value { } } +impl fmt::Debug for Value { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.fmt_debug_with_state(f, &mut ValueFormatState::default(), 0) + } +} + +impl fmt::Display for Value { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + self.fmt_display_with_state(f, &mut ValueFormatState::default(), 0) + } +} + impl PartialEq for Value { fn eq(&self, other: &Self) -> bool { // Optimization: Mismatched types are never equal. diff --git a/tests/value_cycle_safety.rs b/tests/value_cycle_safety.rs new file mode 100644 index 00000000..9a8d2f13 --- /dev/null +++ b/tests/value_cycle_safety.rs @@ -0,0 +1,96 @@ +use std::cell::RefCell; +use std::collections::HashMap; +use std::rc::Rc; +use wfl::interpreter::value::Value; + +#[test] +fn self_cycle_formats_and_deep_clones_without_recursing_forever() { + let source_list = Rc::new(RefCell::new(Vec::new())); + let source = Value::List(Rc::clone(&source_list)); + source_list.borrow_mut().push(source.clone()); + + assert_eq!(source.to_string(), "[]"); + assert_eq!(format!("{source:?}"), "[]"); + + let cloned = source.deep_clone(); + let Value::List(cloned_list) = cloned else { + panic!("a cloned list must remain a list"); + }; + assert!( + !Rc::ptr_eq(&source_list, &cloned_list), + "deep_clone must isolate the clone from its source" + ); + + let cloned_child = cloned_list.borrow()[0].clone(); + let Value::List(cloned_child_list) = cloned_child else { + panic!("the self-reference must remain a list reference"); + }; + assert!( + Rc::ptr_eq(&cloned_list, &cloned_child_list), + "the cloned self-reference must point at the cloned list" + ); +} + +#[test] +fn mutual_cycle_preserves_cycles_and_shared_identity_in_the_clone() { + let source_list = Rc::new(RefCell::new(Vec::new())); + let source_object = Rc::new(RefCell::new(HashMap::new())); + + let shared_object = Value::Object(Rc::clone(&source_object)); + source_list + .borrow_mut() + .extend([shared_object.clone(), shared_object]); + source_object + .borrow_mut() + .insert("back".to_string(), Value::List(Rc::clone(&source_list))); + let source = Value::List(Rc::clone(&source_list)); + + assert_eq!(source.to_string(), "[, ]"); + assert_eq!( + format!("{source:?}"), + "[{back: }, {back: }]" + ); + + let Value::List(cloned_list) = source.deep_clone() else { + panic!("a cloned list must remain a list"); + }; + let cloned_items = cloned_list.borrow(); + let Value::Object(first_object) = &cloned_items[0] else { + panic!("the cloned list must contain its object"); + }; + let Value::Object(second_object) = &cloned_items[1] else { + panic!("the cloned list must contain its shared object twice"); + }; + assert!( + Rc::ptr_eq(first_object, second_object), + "shared source objects must remain shared within the cloned graph" + ); + assert!( + !Rc::ptr_eq(&source_object, first_object), + "the cloned object must be independent from its source" + ); + + let cloned_back_reference = first_object + .borrow() + .get("back") + .expect("cloned object must retain its back-reference") + .clone(); + let Value::List(cloned_back_list) = cloned_back_reference else { + panic!("the cloned back-reference must remain a list"); + }; + assert!( + Rc::ptr_eq(&cloned_list, &cloned_back_list), + "the mutual cycle must point back into the cloned graph" + ); +} + +#[test] +fn formatting_stops_at_a_bounded_depth_for_acyclic_values() { + let mut value = Value::Number(1.0); + for _ in 0..128 { + value = Value::List(Rc::new(RefCell::new(vec![value]))); + } + + assert!(value.to_string().contains("")); + assert!(format!("{value:?}").contains("")); +} From 26fb238323950ba5b18491a3a5595f5bacfe839b Mon Sep 17 00:00:00 2001 From: logbie Date: Thu, 16 Jul 2026 10:33:10 -0500 Subject: [PATCH 2/2] Apply rustfmt formatting --- src/interpreter/value.rs | 17 +++++++++-------- tests/value_cycle_safety.rs | 5 +---- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/interpreter/value.rs b/src/interpreter/value.rs index 302390ee..584d4de5 100644 --- a/src/interpreter/value.rs +++ b/src/interpreter/value.rs @@ -298,8 +298,7 @@ impl Value { line: 0, column: 0, })); - memo - .container_instances + memo.container_instances .insert(source_id, Rc::clone(&cloned)); let source = instance.borrow(); @@ -395,7 +394,11 @@ impl Value { result } Value::Function(func) => { - write!(f, "Function({})", func.name.as_deref().unwrap_or("anonymous")) + write!( + f, + "Function({})", + func.name.as_deref().unwrap_or("anonymous") + ) } Value::NativeFunction(name, _) => write!(f, "NativeFunction({name})"), Value::Future(_) => write!(f, "[Future]"), @@ -496,11 +499,9 @@ impl Value { state.active_objects.remove(&id); result } - Value::Function(func) => write!( - f, - "action {}", - func.name.as_deref().unwrap_or("anonymous") - ), + Value::Function(func) => { + write!(f, "action {}", func.name.as_deref().unwrap_or("anonymous")) + } Value::NativeFunction(name, _) => write!(f, "native {name}"), Value::Future(_) => write!(f, "[Future]"), Value::Date(d) => write!(f, "{}", d.format("%Y-%m-%d")), diff --git a/tests/value_cycle_safety.rs b/tests/value_cycle_safety.rs index 9a8d2f13..a1168a8a 100644 --- a/tests/value_cycle_safety.rs +++ b/tests/value_cycle_safety.rs @@ -46,10 +46,7 @@ fn mutual_cycle_preserves_cycles_and_shared_identity_in_the_clone() { let source = Value::List(Rc::clone(&source_list)); assert_eq!(source.to_string(), "[, ]"); - assert_eq!( - format!("{source:?}"), - "[{back: }, {back: }]" - ); + assert_eq!(format!("{source:?}"), "[{back: }, {back: }]"); let Value::List(cloned_list) = source.deep_clone() else { panic!("a cloned list must remain a list");