fix(engine): end runaway loops in variable deletion, data-merge markers and nested scripts

crates/engine/src/cmd/datamerge/fill.rs

@@ -334,13 +334,18 @@ fn typed_reps(
 fn find_markers(text: &str) -> Vec<(usize, usize, String)> {
     let mut out = Vec::new();
     let mut i = 0;
+    // The next `>>` at or after the `<<` being looked at: found once, kept until passed, so the
+    // text is scanned once however many `<<` it holds (not again from every one of them).
+    let mut close: Option<usize> = Some(0);
     while i < text.len() {
-        if text[i..].starts_with("<<")
-            && let Some(rel) = text[i + 2..].find(">>")
-        {
-            let name = &text[i + 2..i + 2 + rel];
+        if text[i..].starts_with("<<") {
+            if close.is_some_and(|c| c < i + 2) {
+                close = text[i + 2..].find(">>").map(|rel| i + 2 + rel);
+            }
+            let Some(c) = close else { break };
+            let name = &text[i + 2..c];
             if !name.is_empty() && !name.contains(['<', '>', '\n']) {
-                let end = i + 2 + rel + 2;
+                let end = c + 2;
                 out.push((i, end, name.to_string()));
                 i = end;
                 continue;
@@ -570,6 +575,25 @@ fn cell_of<'a>(fields: &[DataField], row: &'a [String], name: &str) -> &'a str {
     fields.iter().position(|f| f.name == name).and_then(|i| row.get(i)).map(String::as_str).unwrap_or("")
 }
 
+#[cfg(test)]
+mod marker_tests {
+    use super::find_markers;
+
+    #[test]
+    fn markers_are_found_in_one_pass() {
+        let found = find_markers;
+        assert_eq!(found("a <<Name>> b <<x>>"), [(2, 10, "Name".to_string()), (13, 18, "x".to_string())]);
+        assert_eq!(found("<<>> <<a<b>> <<ok>>"), [(13, 19, "ok".to_string())]);
+        assert_eq!(found("<<<<a>>"), [(2, 7, "a".to_string())]);
+        // Many `<<` with no `>>` to close them: linear, not a search from each of them.
+        let open = "<<".repeat(200_000);
+        let t = std::time::Instant::now();
+        assert!(found(&open).is_empty());
+        assert!(found(&format!("{open}>>")).is_empty(), "a name made of `<` isn't one");
+        assert!(t.elapsed() < std::time::Duration::from_secs(5), "{:?}", t.elapsed());
+    }
+}
+
 #[cfg(test)]
 mod link_tests {
     use super::*;

crates/engine/src/cmd/variables.rs

@@ -361,16 +361,21 @@ fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth
         return;
     }
     if st.text.chars().any(|c| designcraft_doc::vars::var_index(c).is_some_and(|k| k >= i)) {
+        // The instances as they are now, replaced last first so earlier positions stay valid:
+        // text put in isn't searched again (a value holding the marker would never end).
+        let instances = |text: &str, target: &str| {
+            let mut at: Vec<usize> = text.match_indices(target).map(|(pos, _)| pos).collect();
+            at.reverse();
+            at
+        };
         let target = var_char(i).unwrap_or(' ').to_string();
-        let mut occurrences: Vec<usize> = st.text.match_indices(&target).map(|(pos, _)| pos).collect();
-        occurrences.reverse();
-        for pos in occurrences {
+        for pos in instances(&st.text, &target) {
             st.replace(pos..pos + target.len(), value);
             on_replace(pos, target.len());
         }
         for k in i + 1..n {
             let (from, to) = (var_char(k).unwrap_or(' ').to_string(), var_char(k - 1).unwrap_or(' ').to_string());
-            while let Some(pos) = st.text.find(&from) {
+            for pos in instances(&st.text, &from) {
                 st.replace(pos..pos + from.len(), &to);
             }
         }

crates/engine/src/cmd/variables/tests.rs

@@ -290,6 +290,22 @@ fn delete_variable_reaches_table_cells() {
     assert_eq!(t.cells[0].text.text, format!("Alpha|{}", var_char(nb).unwrap()));
 }
 
+/// A custom variable whose text holds its own instance marker: deleting it puts that text in
+/// once per instance and ends (the text put in isn't searched for instances again).
+#[test]
+fn delete_variable_whose_value_holds_its_marker_ends() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({})).unwrap();
+    s.execute("variables.define", &json!({"name": "A", "type": "custom", "text": "a"})).unwrap();
+    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();
+    s.execute("variables.delete", &json!({"name": "A"})).unwrap();
+    let d = &s.doc().unwrap().doc;
+    assert_eq!(d.stories.values().next().unwrap().text, format!("x<{marker}>y<{marker}>z"));
+}
+
 /// Deleting a variable flattens its instances in footnotes and endnotes too and moves the
 /// instances of later variables down.
 #[test]

crates/engine/src/lib.rs

@@ -392,6 +392,10 @@ pub struct Session {
     active: Option<usize>,
     /// An undoable command is running: the commands it runs are part of its undo step.
     in_undo_step: bool,
+    /// Scripts running one inside another, and the steps the outermost may still run
+    /// ([`script::MAX_SCRIPT_DEPTH`], [`script::MAX_SCRIPT_STEPS`]).
+    pub(crate) script_depth: usize,
+    pub(crate) script_steps: usize,
     pub prefs: Prefs,
     pub cache: Arc<Cache>,
     pub(crate) tool: Box<dyn designcraft_tools::Tool>,
@@ -461,6 +465,8 @@ impl Session {
             docs: vec![],
             active: None,
             in_undo_step: false,
+            script_depth: 0,
+            script_steps: 0,
             prefs: Prefs::default(),
             cache: Arc::new(Cache::new()),
             tool: designcraft_tools::create("selection"),

crates/engine/src/script.rs

@@ -110,6 +110,10 @@ fn is_reference(r: &str) -> bool {
     head_ok && parts.all(|p| !p.is_empty() && p.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-'))
 }
 
+/// Longest string a `${…}` interpolation may build: a step that pastes an earlier result into a
+/// story, then interpolates that story into the next, could otherwise double its size each step.
+pub const MAX_INTERPOLATED_BYTES: usize = 16 << 20;
+
 /// Replace references in `params` with values from earlier `results`.
 pub fn resolve(params: &Value, results: &[Value]) -> Result<Value, String> {
     Ok(match params {
@@ -134,6 +138,10 @@ pub fn resolve(params: &Value, results: &[Value]) -> Result<Value, String> {
                         Value::String(t) => out.push_str(&t),
                         other => out.push_str(&other.to_string()),
                     }
+                    if out.len() > MAX_INTERPOLATED_BYTES {
+                        return Err(format!("the interpolated text of {} is longer than {MAX_INTERPOLATED_BYTES} bytes", s.get(..40).unwrap_or(s))
+                            .replace('\n', " "));
+                    }
                     rest = &inner[end + 1..];
                 } else {
                     out.push('$');
@@ -182,10 +190,33 @@ pub fn run(steps: &[Step], mut exec: impl FnMut(&str, Value) -> Result<Value, St
     Report { results, failed: None }
 }
 
+/// How deep scripts may run scripts (`script.run` is a command a step may run).
+pub const MAX_SCRIPT_DEPTH: usize = 8;
+/// Steps one outermost script may run, the scripts it runs included: a script that runs itself
+/// (its text read back from a story, say) ends here rather than never.
+pub const MAX_SCRIPT_STEPS: usize = 100_000;
+
 impl crate::Session {
     /// Run a script on this session (see [`crate::script`]).
     pub fn run_script(&mut self, steps: &[Step]) -> Report {
-        run(steps, |c, p| self.execute(c, &p).map_err(|e| e.to_string()))
+        if self.script_depth >= MAX_SCRIPT_DEPTH {
+            let command = steps.first().map(|s| s.command.clone()).unwrap_or_default();
+            return Report {
+                results: vec![],
+                failed: Some((0, command, format!("scripts nest deeper than {MAX_SCRIPT_DEPTH} (a script running a script…)"))),
+            };
+        }
+        if self.script_depth == 0 {
+            self.script_steps = MAX_SCRIPT_STEPS;
+        }
+        self.script_depth += 1;
+        let report = run(steps, |c, p| {
+            self.script_steps =
+                self.script_steps.checked_sub(1).ok_or_else(|| format!("the script and the scripts it runs exceed {MAX_SCRIPT_STEPS} steps"))?;
+            self.execute(c, &p).map_err(|e| e.to_string())
+        });
+        self.script_depth -= 1;
+        report
     }
 }
 
@@ -275,6 +306,35 @@ mod tests {
         assert_eq!(r.results[3]["text"], "a\u{a0}v\u{a0}Praze");
     }
 
+    /// A script that runs scripts stops at the depth limit, and the steps of all of them count
+    /// against one budget; the session goes on afterwards.
+    #[test]
+    fn nested_scripts_are_bounded() {
+        let mut s = crate::Session::new();
+        let steps = parse("file.new\ndocument.inspect\ndocument.inspect").unwrap();
+        s.script_depth = MAX_SCRIPT_DEPTH;
+        let e = s.run_script(&steps).failed.expect("must fail").2;
+        assert!(e.contains("nest deeper"), "{e}");
+        s.script_depth = 1;
+        s.script_steps = 2;
+        let r = s.run_script(&steps);
+        assert_eq!(r.results.len(), 2);
+        assert!(r.failed.as_ref().is_some_and(|f| f.2.contains("exceed")), "{r:?}");
+        s.script_depth = 0;
+        let r = s.run_script(&steps);
+        assert!(r.failed.is_none(), "{r:?}");
+        assert_eq!(s.script_depth, 0);
+    }
+
+    #[test]
+    fn interpolation_is_capped() {
+        let big = "x".repeat(MAX_INTERPOLATED_BYTES / 2 + 1);
+        let results = vec![json!({"text": big})];
+        assert!(resolve(&json!("${0.text}"), &results).is_ok());
+        let e = resolve(&json!("${0.text}${0.text}"), &results).unwrap_err();
+        assert!(e.contains("longer than"), "{e}");
+    }
+
     #[test]
     fn runs_on_a_session_with_references() {
         let mut s = crate::Session::new();