Engine: bounded DTD validation, compound-path hole orientation

* dtd: bounded matching steps, element depth and problem count

* address review: stop DTD matching when a step charge fails; refuse hole orientation past its budget

* Apply review fixes

- crates/engine/src/dtd.rs

---------

Co-authored-by: autocraft-bot[bot] <354215646+autocraft-bot[bot]@users.noreply.github.com>

crates/engine/src/cmd/paths.rs

@@ -4,7 +4,7 @@
 use std::sync::Arc;
 
 use designcraft_doc::{Content, Document, Fill, Item, ItemId, Selection, Shape, SpreadRef, Stroke};
-use designcraft_geom::{Affine, BezPath, PathData, SubPath};
+use designcraft_geom::{Affine, BezPath, PathData, Shape as _, SubPath};
 use serde_json::{Value, json};
 
 use super::{CommandSpec, bad, cmd, has_selection, point_param, targets};
@@ -75,20 +75,40 @@ fn spread_xf(d: &Document, id: ItemId) -> Option<(SpreadRef, Affine)> {
     Some((loc.spread, d.parent_xf(&loc) * d.item_at(&loc)?.xf))
 }
 
+/// Containment tests `orient_holes` makes at most (after the bounding-box filter). A compound path
+/// of thousands of nested contours would otherwise take quadratic time; past this the command is
+/// refused rather than leave later subpaths with the wrong direction.
+const MAX_HOLE_TESTS: usize = 4_000_000;
+
 /// Orient subpaths so those nested an odd number of times run against the others: with the
 /// non-zero rule they become holes.
-fn orient_holes(path: &mut PathData) {
+fn orient_holes(path: &mut PathData, command: &str) -> Result<()> {
+    orient_holes_within(path, command, MAX_HOLE_TESTS)
+}
+
+fn orient_holes_within(path: &mut PathData, command: &str, max_tests: usize) -> Result<()> {
     let beziers: Vec<BezPath> = path.subpaths.iter().map(|s| PathData::single(s.clone()).to_bezpath()).collect();
+    // A point outside a contour's box is outside the contour: most pairs are skipped without a winding count.
+    let boxes: Vec<_> = beziers.iter().map(BezPath::bounding_box).collect();
     let n = path.subpaths.len();
+    let mut tests = 0usize;
     for i in 0..n {
         let Some(p) = path.subpaths[i].anchors.first().map(|a| a.p) else { continue };
-        let depth = (0..n).filter(|&j| j != i && designcraft_geom::hit::fill_contains(&beziers[j], designcraft_geom::FillRule::NonZero, p)).count();
+        let mut depth = 0;
+        for j in (0..n).filter(|&j| j != i && boxes[j].inflate(1e-6, 1e-6).contains(p)) {
+            tests += 1;
+            if tests > max_tests {
+                return Err(bad(command, "too many nested contours to tell holes from fills"));
+            }
+            depth += usize::from(designcraft_geom::hit::fill_contains(&beziers[j], designcraft_geom::FillRule::NonZero, p));
+        }
         let sp = &mut path.subpaths[i];
         let cw = sp.area() > 0.0;
         if cw == (depth % 2 == 1) {
             sp.reverse();
         }
     }
+    Ok(())
 }
 
 fn make_compound(s: &mut Session, p: &Value) -> Result<Value> {
@@ -115,7 +135,7 @@ fn make_compound(s: &mut Session, p: &Value) -> Result<Value> {
         }
         let it = d.item_mut(base_id).ok_or(designcraft_doc::DocError::NoItem(base_id))?;
         it.path = PathData::new(subpaths);
-        orient_holes(&mut it.path);
+        orient_holes(&mut it.path, "object.makeCompoundPath")?;
         it.shape = Shape::Path;
         *sel = Selection::items(vec![base_id]);
         Ok(json!({"id": base_id.0}))
@@ -159,7 +179,7 @@ fn pathfinder(s: &mut Session, p: &Value) -> Result<Value> {
         }
         let it = d.item_mut(keep).ok_or(designcraft_doc::DocError::NoItem(keep))?;
         it.path = result.transformed(kxf.inverse());
-        orient_holes(&mut it.path);
+        orient_holes(&mut it.path, "object.pathfinder")?;
         it.shape = Shape::Path;
         it.corners = Default::default();
         *sel = Selection::items(vec![keep]);
@@ -409,6 +429,38 @@ mod tests {
         s.doc().unwrap().doc.check().unwrap();
     }
 
+    #[test]
+    fn orient_holes_alternates_nested_contours_and_skips_distant_ones() {
+        use designcraft_geom::{Rect, shapes::rectangle};
+        let mut subs: Vec<SubPath> = Vec::new();
+        for k in 0..3 {
+            let o = 10.0 * f64::from(k);
+            subs.extend(rectangle(Rect::new(o, o, 100.0 - o, 100.0 - o)).subpaths);
+        }
+        // Thousands of separate contours cost no winding counts (their boxes miss each other).
+        for k in 0..3000 {
+            let x = 200.0 + 20.0 * f64::from(k);
+            subs.extend(rectangle(Rect::new(x, 0.0, x + 10.0, 10.0)).subpaths);
+        }
+        let mut path = PathData::new(subs);
+        orient_holes(&mut path, "test").unwrap();
+        let cw: Vec<bool> = path.subpaths.iter().map(|s| s.area() > 0.0).collect();
+        assert_eq!(cw[..3], [true, false, true]);
+        assert!(cw[3..].iter().all(|c| *c));
+    }
+
+    #[test]
+    fn orient_holes_refuses_past_its_budget_instead_of_half_orienting() {
+        use designcraft_geom::{Rect, shapes::rectangle};
+        let mut subs: Vec<SubPath> = Vec::new();
+        for k in 0..4 {
+            let o = 10.0 * f64::from(k);
+            subs.extend(rectangle(Rect::new(o, o, 100.0 - o, 100.0 - o)).subpaths);
+        }
+        assert!(orient_holes_within(&mut PathData::new(subs.clone()), "test", 3).is_err());
+        assert!(orient_holes_within(&mut PathData::new(subs), "test", 100).is_ok());
+    }
+
     #[test]
     fn pathfinder_subtracts_front_from_back() {
         let mut s = Session::new();

crates/engine/src/dtd.rs

@@ -309,7 +309,39 @@ pub fn parse(src: &str) -> Result<Dtd, String> {
 
 /// What `ends` has worked out for one sequence of children: the end positions of a group (by the
 /// address of its node, which lives as long as the model) from a position.
-type EndsMemo = HashMap<(usize, usize), Vec<usize>>;
+struct EndsMemo {
+    map: HashMap<(usize, usize), Vec<usize>>,
+    /// Matching steps left (shared by a whole validation); at zero `ends` stops and `over` is set.
+    left: usize,
+    /// End positions held in `map`; past [`MAX_STORED_ENDS`] results are no longer kept.
+    stored: usize,
+    over: bool,
+}
+
+/// Matching steps one validation may take across all its elements (a step is a position handled or
+/// compared). Valid documents with thousands of children per element use a small fraction.
+const MAX_MATCH_STEPS: usize = 8 * 1024 * 1024;
+/// End positions the memo may hold at once (8 bytes each).
+const MAX_STORED_ENDS: usize = 4 * 1024 * 1024;
+/// Element nesting depth: `check` recurses and keeps each level's full path.
+const MAX_XML_DEPTH: usize = 256;
+/// Elements in the document being validated.
+const MAX_XML_NODES: usize = 4 * 1024 * 1024;
+/// Problems reported before the list stops.
+const MAX_PROBLEMS: usize = 10_000;
+
+impl EndsMemo {
+    /// Spend `n` steps; false once the budget is gone.
+    fn spend(&mut self, n: usize) -> bool {
+        if self.over || n > self.left {
+            self.left = 0;
+            self.over = true;
+            return false;
+        }
+        self.left -= n;
+        true
+    }
+}
 
 /// End positions after `cp` matches a prefix of `seq[i..]`.
 ///
@@ -321,8 +353,16 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec<usize> {
         return if seq.get(i) == Some(&n.as_str()) { vec![i + 1] } else { vec![] };
     }
     let key = (std::ptr::from_ref(cp) as usize, i);
-    if let Some(done) = memo.get(&key) {
-        return done.clone();
+    if let Some(done) = memo.map.get(&key) {
+        // Charged before the clone, and a failed charge ends the match.
+        let len = done.len();
+        if !memo.spend(len) {
+            return vec![];
+        }
+        return memo.map.get(&key).cloned().unwrap_or_default();
+    }
+    if !memo.spend(1) {
+        return vec![];
     }
     let mut out: Vec<usize> = match cp {
         Cp::Name(_) => vec![],
@@ -334,6 +374,9 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec<usize> {
                 for p in at {
                     next.extend(ends(c, seq, p, memo));
                 }
+                if !memo.spend(next.len()) {
+                    return vec![];
+                }
                 next.sort_unstable();
                 next.dedup();
                 at = next;
@@ -349,16 +392,21 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec<usize> {
         }
         Cp::Rep(c, r) => {
             let mut all = if *r == '+' { vec![] } else { vec![i] };
+            // `all` as a set too: a list search per position made long repeats quadratic.
+            let mut have: std::collections::HashSet<usize> = all.iter().copied().collect();
             let mut frontier = ends(c, seq, i, memo);
             if *r == '?' {
                 all.extend(frontier);
             } else {
                 while !frontier.is_empty() {
-                    let new: Vec<usize> = frontier.iter().copied().filter(|p| !all.contains(p)).collect();
+                    if !memo.spend(frontier.len()) {
+                        break;
+                    }
+                    let new: Vec<usize> = frontier.iter().copied().filter(|p| have.insert(*p)).collect();
                     all.extend(&new);
                     let mut next = Vec::new();
                     for p in new.into_iter().filter(|&p| p > i) {
-                        next.extend(ends(c, seq, p, memo).into_iter().filter(|p| !all.contains(p)));
+                        next.extend(ends(c, seq, p, memo).into_iter().filter(|p| !have.contains(p)));
                     }
                     // Two paths to one position are one position: kept apart they multiply per round.
                     next.sort_unstable();
@@ -371,7 +419,10 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec<usize> {
     };
     out.sort_unstable();
     out.dedup();
-    memo.insert(key, out.clone());
+    if !memo.over && memo.stored + out.len() <= MAX_STORED_ENDS {
+        memo.stored += out.len();
+        memo.map.insert(key, out.clone());
+    }
     out
 }
 
@@ -396,6 +447,7 @@ fn read_tree(xml: &str) -> Result<Node, String> {
     use quick_xml::events::Event;
     let mut r = quick_xml::Reader::from_str(xml);
     let mut stack: Vec<Node> = Vec::new();
+    let mut count = 0usize;
     let node = |e: &quick_xml::events::BytesStart| Node {
         name: String::from_utf8_lossy(e.name().as_ref()).to_string(),
         attrs: e.attributes().flatten().map(|a| String::from_utf8_lossy(a.key.as_ref()).to_string()).collect(),
@@ -403,8 +455,20 @@ fn read_tree(xml: &str) -> Result<Node, String> {
         text: false,
     };
     loop {
-        match r.read_event().map_err(|e| e.to_string())? {
-            Event::Start(e) => stack.push(node(&e)),
+        let event = r.read_event().map_err(|e| e.to_string())?;
+        if matches!(event, Event::Start(_) | Event::Empty(_)) {
+            count += 1;
+            if count > MAX_XML_NODES {
+                return Err(format!("more than {MAX_XML_NODES} elements"));
+            }
+        }
+        match event {
+            Event::Start(e) => {
+                if stack.len() >= MAX_XML_DEPTH {
+                    return Err(format!("elements nested more than {MAX_XML_DEPTH} deep"));
+                }
+                stack.push(node(&e));
+            }
             Event::Empty(e) => {
                 let n = node(&e);
                 match stack.last_mut() {
@@ -437,8 +501,15 @@ fn read_tree(xml: &str) -> Result<Node, String> {
     }
 }
 
-fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec<Problem>) {
-    let mut problem = |m: String| out.push(Problem { path: path.to_string(), message: m });
+fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec<Problem>, steps: &mut usize) {
+    if out.len() >= MAX_PROBLEMS {
+        return;
+    }
+    let mut problem = |m: String| {
+        if out.len() < MAX_PROBLEMS {
+            out.push(Problem { path: path.to_string(), message: m });
+        }
+    };
     match dtd.model(&n.name) {
         None => problem(format!("`{}` isn't declared in the DTD", n.name)),
         Some(Model::Any) => {}
@@ -459,7 +530,12 @@ fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec<Problem>) {
                 problem(format!("`{}` can't contain text", n.name));
             }
             let seq: Vec<&str> = n.children.iter().map(|c| c.name.as_str()).collect();
-            if !ends(cp, &seq, 0, &mut EndsMemo::new()).contains(&seq.len()) {
+            let mut memo = EndsMemo { map: HashMap::new(), left: *steps, stored: 0, over: false };
+            let matched = ends(cp, &seq, 0, &mut memo).contains(&seq.len());
+            *steps = memo.left;
+            if memo.over {
+                problem(format!("`{}` has too many children to check against its content model", n.name));
+            } else if !matched {
                 let got = if seq.is_empty() { "nothing".to_string() } else { seq.join(", ") };
                 problem(format!("`{}` contains {got}; the DTD asks for {}", n.name, show(cp)));
             }
@@ -474,7 +550,7 @@ fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec<Problem>) {
     for c in &n.children {
         let k = seen.entry(c.name.as_str()).or_default();
         *k += 1;
-        check(dtd, c, &format!("{path}/{}[{k}]", c.name), out);
+        check(dtd, c, &format!("{path}/{}[{k}]", c.name), out, steps);
     }
 }
 
@@ -484,7 +560,10 @@ pub fn validate(dtd: &Dtd, xml: &str) -> Vec<Problem> {
         Ok(root) => {
             let mut out = Vec::new();
             let path = root.name.clone();
-            check(dtd, &root, &path, &mut out);
+            check(dtd, &root, &path, &mut out, &mut { MAX_MATCH_STEPS });
+            if out.len() >= MAX_PROBLEMS {
+                out.push(Problem { path: String::new(), message: "further problems are not listed".into() });
+            }
             out
         }
         Err(e) => vec![Problem { path: String::new(), message: format!("not well-formed XML: {e}") }],
@@ -743,4 +822,29 @@ mod tests {
         assert_eq!(problems.len(), 1, "{problems:?}");
         assert!(problems[0].message.starts_with("`r` contains a, a, a"), "{problems:?}");
     }
+
+    #[test]
+    fn nested_repeats_over_many_children_stop_at_the_step_budget() {
+        let dtd = parse("<!ELEMENT r ((a*)*)><!ELEMENT a EMPTY>").unwrap();
+        assert_eq!(validate(&dtd, &format!("<r>{}</r>", "<a/>".repeat(1000))), vec![]);
+        let problems = validate(&dtd, &format!("<r>{}</r>", "<a/>".repeat(20_000)));
+        assert_eq!(problems.len(), 1, "{problems:?}");
+        assert!(problems[0].message.contains("too many children"), "{problems:?}");
+    }
+
+    #[test]
+    fn xml_nesting_is_bounded() {
+        let dtd = parse("<!ELEMENT a ANY>").unwrap();
+        let deep = |n: usize| format!("{}{}", "<a>".repeat(n), "</a>".repeat(n));
+        assert_eq!(validate(&dtd, &deep(MAX_XML_DEPTH)), vec![]);
+        let problems = validate(&dtd, &deep(MAX_XML_DEPTH + 1));
+        assert!(problems[0].message.contains("nested more than"), "{problems:?}");
+    }
+
+    #[test]
+    fn problems_are_capped() {
+        let dtd = parse("<!ELEMENT r EMPTY>").unwrap();
+        let problems = validate(&dtd, &format!("<r>{}</r>", "<x/>".repeat(MAX_PROBLEMS + 50)));
+        assert!(problems.len() <= MAX_PROBLEMS + 1, "{}", problems.len());
+    }
 }