From b7174dc92dca0129ae82969bbf7cf4c5dbc0128e Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:27:59 -0500 Subject: [PATCH] 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 | 62 ++++++++++++++-- crates/engine/src/dtd.rs | 130 +++++++++++++++++++++++++++++---- 2 files changed, 174 insertions(+), 18 deletions(-) diff --git a/crates/engine/src/cmd/paths.rs b/crates/engine/src/cmd/paths.rs index 7800b3189..946da4215 100644 --- a/crates/engine/src/cmd/paths.rs +++ b/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 = 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 { @@ -115,7 +135,7 @@ fn make_compound(s: &mut Session, p: &Value) -> Result { } 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 { } 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 = 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 = 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 = 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(); diff --git a/crates/engine/src/dtd.rs b/crates/engine/src/dtd.rs index 6b48ba3ab..7128ba84e 100644 --- a/crates/engine/src/dtd.rs +++ b/crates/engine/src/dtd.rs @@ -309,7 +309,39 @@ pub fn parse(src: &str) -> Result { /// 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>; +struct EndsMemo { + map: HashMap<(usize, usize), Vec>, + /// 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 { 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 = match cp { Cp::Name(_) => vec![], @@ -334,6 +374,9 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec { 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 { } 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 = 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 = frontier.iter().copied().filter(|p| !all.contains(p)).collect(); + if !memo.spend(frontier.len()) { + break; + } + let new: Vec = 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 { }; 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 { use quick_xml::events::Event; let mut r = quick_xml::Reader::from_str(xml); let mut stack: Vec = 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 { 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 { } } -fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec) { - 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, 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(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) { 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 { 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("").unwrap(); + assert_eq!(validate(&dtd, &format!("{}", "".repeat(1000))), vec![]); + let problems = validate(&dtd, &format!("{}", "".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("").unwrap(); + let deep = |n: usize| format!("{}{}", "".repeat(n), "".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("").unwrap(); + let problems = validate(&dtd, &format!("{}", "".repeat(MAX_PROBLEMS + 50))); + assert!(problems.len() <= MAX_PROBLEMS + 1, "{}", problems.len()); + } }