From 79ebd019352508b0b461b557dc5110d328f61839 Mon Sep 17 00:00:00 2001 From: DWAA1660 Date: Tue, 13 Oct 2026 16:59:08 -0600 Subject: [PATCH] svg: bound expansion by an element budget and skip reference cycles (#1236) Depth-limited expansion still let a few kilobytes of branching references expand to 2^N elements. The parser now counts every element it converts, copies included, against a budget of max(500 000, twice the file's own elements) and fails the import with Error::Xml once it is spent. Targets currently being expanded are tracked, so mutual references stop at the first repeat instead of nesting to the depth limit. Fixes #1236 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01N9kwMcw2pYQ5fSjKa3ZPnh --- crates/svg/src/lib.rs | 26 +++++++++++++++--- crates/svg/src/tests.rs | 60 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs index 7310814bf..b5ddd98e6 100644 --- a/crates/svg/src/lib.rs +++ b/crates/svg/src/lib.rs @@ -486,6 +486,12 @@ struct Parser<'a, 'i> { vw: f64, vh: f64, skipped: Vec, + /// Elements converted so far, `` copies included, and how many may be (#1236). + work: usize, + budget: usize, + /// The `` targets being expanded, outermost first: a reference back into one is a + /// cycle and is skipped. + using: Vec, } fn is_svg_ns(n: &roxmltree::Node) -> bool { @@ -748,9 +754,12 @@ impl<'a, 'i> Parser<'a, 'i> { /// Convert an element (and its subtree) into nodes appended to `out`. fn node(&mut self, n: roxmltree::Node<'a, 'i>, parent: &Style, out: &mut Vec, depth: usize) { - if depth > 64 || !is_svg_ns(&n) { + if depth > 64 || !is_svg_ns(&n) || self.work > self.budget { return; } + // Every element converted counts, each `` copy again: a few references to groups + // that reference each other several times can multiply into billions (#1236). + self.work += 1; let tag = n.tag_name().name(); match tag { "defs" | "title" | "desc" | "metadata" | "style" | "linearGradient" | "radialGradient" | "symbol" | "clipPath" | "mask" | "pattern" | "marker" @@ -816,9 +825,10 @@ impl<'a, 'i> Parser<'a, 'i> { } "use" => { let Some(target) = self.href(n) else { return }; - if target == n || n.ancestors().any(|a| a == target) { + if target == n || n.ancestors().any(|a| a == target) || self.using.contains(&target.id()) { return; } + self.using.push(target.id()); let x = self.attr_len(n, "x", 'x', &st); let y = self.attr_len(n, "y", 'y', &st); transform *= Affine::translate((x, y)); @@ -847,6 +857,7 @@ impl<'a, 'i> Parser<'a, 'i> { } else { self.node(target, &st, &mut children, depth + 1); } + self.using.pop(); out.push(Node::Group(Group { name: Self::element_name(n), transform, @@ -1048,6 +1059,9 @@ pub fn parse_transform(s: &str) -> Affine { /// 2 MiB thread's stack (aborting the app) at a few thousand levels; real documents stay far /// below this. const MAX_XML_DEPTH: usize = 256; +/// The most elements an import converts, `` copies included, unless the file itself has +/// more than half as many (#1236). +const MAX_EXPANDED_ELEMENTS: usize = 500_000; /// The deepest element nesting of `text` (a quick scan that skips comments, CDATA, processing /// instructions, declarations and quoted attribute values). @@ -1121,7 +1135,9 @@ pub fn parse(bytes: &[u8]) -> Result { } } let vb = viewbox(root); - let mut p = Parser { ids, rules, vw: vb.map_or(300.0, |v| v.1), vh: vb.map_or(150.0, |v| v.2), skipped: vec![] }; + // Room for generous reuse, and always for every element of the file itself. + let budget = MAX_EXPANDED_ELEMENTS.max(root.descendants().filter(|n| n.is_element()).count().saturating_mul(2)); + let mut p = Parser { ids, rules, vw: vb.map_or(300.0, |v| v.1), vh: vb.map_or(150.0, |v| v.2), skipped: vec![], work: 0, budget, using: vec![] }; let st = Style::default(); let pct_or = |name: &str, axis: char, def: f64| -> f64 { match root.attribute(name) { @@ -1157,6 +1173,10 @@ pub fn parse(bytes: &[u8]) -> Result { for c in root.children() { p.node(c, &rst, &mut children, 1); } + if p.work > p.budget { + // Never a silently partial drawing. + return Err(Error::Xml(format!("its references expand to more than {} elements", p.budget))); + } let transform = viewbox_transform(root, dw, dh); Ok(Doc { width: dw, diff --git a/crates/svg/src/tests.rs b/crates/svg/src/tests.rs index ed1ea873e..93db715c1 100644 --- a/crates/svg/src/tests.rs +++ b/crates/svg/src/tests.rs @@ -217,3 +217,63 @@ fn deep_nesting_errors_instead_of_overflowing_the_stack() { assert_eq!(crate::xml_depth(&s), 2); assert!(crate::parse(s.as_bytes()).is_ok()); } + +/// How deep `g`'s groups nest, and how many shapes it holds. +fn depth_and_shapes(g: &Group) -> (usize, usize) { + let mut depth = 0; + let mut shapes = 0; + for c in &g.children { + match c { + Node::Group(sub) => { + let (d, s) = depth_and_shapes(sub); + depth = depth.max(d + 1); + shapes += s; + } + Node::Shape(_) => shapes += 1, + Node::Image(_) => {} + } + } + (depth, shapes) +} + +#[test] +fn branching_use_references_stop_at_the_expansion_budget() { + // Each group uses the one before twice: 30 levels are 2^30 copies of one rectangle, from a + // file of a few kilobytes (#1236). + let mut defs = String::from(r#""#); + for i in 1..=30 { + defs += &format!(r##""##, p = i - 1); + } + let svg = format!(r##"{defs}"##); + let t0 = std::time::Instant::now(); + let e = parse(svg.as_bytes()).unwrap_err(); + assert!(e.to_string().contains("expand to more than"), "{e}"); + assert!(t0.elapsed() < std::time::Duration::from_secs(10), "{:?}", t0.elapsed()); + // Below the budget the same pattern imports whole: 2^10 rectangles. + let small = svg.replace(r##""##, r##""##); + let doc = parse(small.as_bytes()).unwrap(); + assert_eq!(depth_and_shapes(&doc.root).1, 1024); +} + +#[test] +fn mutually_referencing_uses_are_cut_at_the_first_repeat() { + // A uses B and B uses A, neither inside the other: once each, not down to the depth limit. + let svg = r##" + + + "##; + let doc = parse(svg.as_bytes()).unwrap(); + let (depth, shapes) = depth_and_shapes(&doc.root); + assert_eq!(shapes, 2, "a's and b's rectangle, once each"); + assert!(depth <= 6, "{depth}"); +} + +#[test] +fn a_symbol_used_a_thousand_times_still_imports() { + let uses: String = (0..1000).map(|i| format!(r##""##)).collect(); + let svg = format!( + r##"{uses}"## + ); + let doc = parse(svg.as_bytes()).unwrap(); + assert_eq!(depth_and_shapes(&doc.root).1, 3000); +}