From edd2f24bb06aa0d31fac050718e21abdeda87c43 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 05:11:14 +0000 Subject: [PATCH] address review: cap whole-reference strings and the resolved parameters of a step; range adjustment uses the inserted length --- crates/engine/src/cmd/variables.rs | 14 +++--- crates/engine/src/cmd/variables/tests.rs | 9 +++- crates/engine/src/script.rs | 56 ++++++++++++++++++++++-- 3 files changed, 68 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/variables.rs b/crates/engine/src/cmd/variables.rs index 28fd6d37a..87479c214 100644 --- a/crates/engine/src/cmd/variables.rs +++ b/crates/engine/src/cmd/variables.rs @@ -53,9 +53,9 @@ pub fn specs() -> Vec { } let st = d.story_mut(sid).ok_or(designcraft_doc::DocError::NoStory(sid))?; let mut replacements = Vec::new(); - unvar(st, i, n, &value, 0, &mut |pos, len| replacements.push((pos, len))); - for (pos, tlen) in replacements { - d.adjust_text_ranges(sid, pos..pos + tlen, value.len()); + unvar(st, i, n, &value, 0, &mut |pos, len, ins| replacements.push((pos, len, ins))); + for (pos, tlen, ins) in replacements { + d.adjust_text_ranges(sid, pos..pos + tlen, ins); } } d.text_variables.remove(i); @@ -356,7 +356,7 @@ fn needs_unvar(st: &designcraft_doc::Story, i: usize, depth: usize) -> bool { /// `i + 1..n` down by one, including table cells and foot/endnotes nested in the story. Only /// touches (and unshares) the parts that hold such instances; a story whose nested text changed /// gets a new revision. -fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth: usize, on_replace: &mut dyn FnMut(usize, usize)) { +fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth: usize, on_replace: &mut dyn FnMut(usize, usize, usize)) { if depth > MAX_DEPTH { return; } @@ -374,7 +374,7 @@ fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth let value = var_char(i).map_or_else(|| value.to_string(), |c| value.replace(c, "")); for pos in at { st.replace(pos..pos + target.len(), &value); - on_replace(pos, target.len()); + on_replace(pos, target.len(), value.len()); } } for k in i + 1..n { @@ -391,14 +391,14 @@ fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth } for c in &mut std::sync::Arc::make_mut(t).cells { if needs_unvar(&c.text, i, depth + 1) { - unvar(&mut c.text, i, n, value, depth + 1, &mut |_, _| {}); + unvar(&mut c.text, i, n, value, depth + 1, &mut |_, _, _| {}); } } nested = true; } for note in st.notes.iter_mut().chain(st.endnotes.iter_mut()) { if needs_unvar(¬e.text, i, depth + 1) { - unvar(&mut std::sync::Arc::make_mut(note).text, i, n, value, depth + 1, &mut |_, _| {}); + unvar(&mut std::sync::Arc::make_mut(note).text, i, n, value, depth + 1, &mut |_, _, _| {}); nested = true; } } diff --git a/crates/engine/src/cmd/variables/tests.rs b/crates/engine/src/cmd/variables/tests.rs index beb775424..726f8d6a8 100644 --- a/crates/engine/src/cmd/variables/tests.rs +++ b/crates/engine/src/cmd/variables/tests.rs @@ -301,10 +301,17 @@ fn delete_variable_whose_value_holds_its_marker_ends() { let a = s.doc().unwrap().doc.variable("A").unwrap(); let marker = var_char(a).unwrap(); s.execute("variables.define", &json!({"name": "A", "type": "custom", "text": format!("<{marker}>")})).unwrap(); - s.execute("frame.create", &json!({"rect": [72, 72, 400, 300], "content": "text", "text": format!("x{marker}y{marker}z")})).unwrap(); + let r = s.execute("frame.create", &json!({"rect": [72, 72, 400, 300], "content": "text", "text": format!("x{marker}y{marker}z")})).unwrap(); + let sid = s.doc().unwrap().doc.item(designcraft_doc::ItemId(r["id"].as_u64().unwrap())).unwrap().text_frame().unwrap().story; + // A link on the last "z": its range follows the text put in (without the marker). + let z = format!("x{marker}y{marker}").len(); + s.execute("text.select", &json!({"story": sid.0, "anchor": z, "focus": z + 1})).unwrap(); + s.execute("hyperlink.create", &json!({"url": "https://example.org"})).unwrap(); s.execute("variables.delete", &json!({"name": "A"})).unwrap(); let d = &s.doc().unwrap().doc; assert_eq!(d.stories.values().next().unwrap().text, "x<>y<>z"); + let designcraft_doc::HyperlinkSource::Text { start, end, .. } = &d.hyperlinks[0].source else { panic!("a text link") }; + assert_eq!((*start, *end), (6, 7)); } /// Deleting a variable flattens its instances in footnotes and endnotes too and moves the diff --git a/crates/engine/src/script.rs b/crates/engine/src/script.rs index 52efddf78..1ccd6f29c 100644 --- a/crates/engine/src/script.rs +++ b/crates/engine/src/script.rs @@ -120,14 +120,49 @@ fn too_long(s: &str) -> String { format!("the interpolated text of {preview} is longer than {MAX_INTERPOLATED_BYTES} bytes").replace('\n', " ") } +/// Longest string a whole reference (`$last.text`) may copy in. A story is far smaller; the cap +/// stops a loop of "read the story, paste it back" from doubling it until memory runs out. +pub const MAX_REFERENCED_BYTES: usize = 64 << 20; + +/// Most bytes of strings one step's parameters may hold once its references are resolved: the +/// limits above apply to each string, this one to a whole parameter tree (an array of 256 +/// references to a large string would pass each of them). +pub const MAX_STEP_PARAM_BYTES: usize = 256 << 20; + +/// Roughly how many bytes cloning `v` allocates (its strings, plus a word per node). +fn weight(v: &Value) -> usize { + match v { + Value::String(s) => s.len(), + Value::Array(a) => a.iter().fold(8, |n, x| n.saturating_add(weight(x))), + Value::Object(o) => o.iter().fold(8, |n, (k, x)| n.saturating_add(k.len()).saturating_add(weight(x))), + _ => 8, + } +} + +fn charge(budget: &mut usize, bytes: usize) -> Result<(), String> { + *budget = + budget.checked_sub(bytes).ok_or_else(|| format!("the parameters of a step are longer than {MAX_STEP_PARAM_BYTES} bytes once resolved"))?; + Ok(()) +} + /// Replace references in `params` with values from earlier `results`. pub fn resolve(params: &Value, results: &[Value]) -> Result { + resolve_in(params, results, MAX_REFERENCED_BYTES, &mut { MAX_STEP_PARAM_BYTES }) +} + +fn resolve_in(params: &Value, results: &[Value], max_ref: usize, budget: &mut usize) -> Result { Ok(match params { Value::String(s) => { if let Some(r) = s.strip_prefix('$').filter(|r| is_reference(r)) { - return lookup(r, results).cloned(); + let v = lookup(r, results)?; + if matches!(v, Value::String(t) if t.len() > max_ref) { + return Err(format!("`${r}` is longer than {max_ref} bytes")); + } + charge(budget, weight(v))?; + return Ok(v.clone()); } if !s.contains("${") && !s.contains("$$") { + charge(budget, s.len())?; return Ok(params.clone()); } let mut out = String::new(); @@ -159,10 +194,13 @@ pub fn resolve(params: &Value, results: &[Value]) -> Result { return Err(too_long(s)); } out.push_str(rest); + charge(budget, out.len())?; Value::String(out) } - Value::Array(a) => Value::Array(a.iter().map(|v| resolve(v, results)).collect::>()?), - Value::Object(o) => Value::Object(o.iter().map(|(k, v)| Ok((k.clone(), resolve(v, results)?))).collect::>()?), + Value::Array(a) => Value::Array(a.iter().map(|v| resolve_in(v, results, max_ref, budget)).collect::>()?), + Value::Object(o) => { + Value::Object(o.iter().map(|(k, v)| Ok((k.clone(), resolve_in(v, results, max_ref, budget)?))).collect::>()?) + } v => v.clone(), }) } @@ -240,6 +278,18 @@ mod tests { use super::*; + /// A whole reference to a long string is refused, and the strings of one step's parameters + /// share a budget (small limits here: the real ones would mean allocating them). + #[test] + fn whole_references_and_the_parameter_tree_are_bounded() { + let results = vec![json!({"text": "x".repeat(100)})]; + assert!(resolve_in(&json!("$0.text"), &results, 100, &mut 1000).is_ok()); + assert!(resolve_in(&json!("$0.text"), &results, 99, &mut 1000).unwrap_err().contains("longer than 99")); + let many = json!(["$0.text", "$0.text", "${0.text}"]); + assert!(resolve_in(&many, &results, 100, &mut 300).is_ok()); + assert!(resolve_in(&many, &results, 100, &mut 299).unwrap_err().contains("once resolved")); + } + #[test] fn leading_blank_lines_keep_line_numbers() { let e = parse("\n\n# c\nfile.new\nframe.create {bad").unwrap_err();