From 774cdc7211ce573023a8d5d556c59f98c051963e Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 05:32:06 +0000 Subject: [PATCH] address review: drop tessellation-tolerance, attribute-count and entity-size refusals; keep retained and draw budgets, warn when the draw budget is hit --- Cargo.lock | 1 + crates/svg/Cargo.toml | 1 + crates/svg/src/lib.rs | 92 ++-------------------------------------- crates/svg/src/raster.rs | 3 ++ crates/svg/src/tests.rs | 37 ---------------- 5 files changed, 9 insertions(+), 125 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c43ba9d9c..57aad57e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1891,6 +1891,7 @@ dependencies = [ "effectcraft-path", "effectcraft-raster", "kurbo", + "log", "rayon", "roxmltree", ] diff --git a/crates/svg/Cargo.toml b/crates/svg/Cargo.toml index 0e1af7719..03cacaa7b 100644 --- a/crates/svg/Cargo.toml +++ b/crates/svg/Cargo.toml @@ -16,3 +16,4 @@ effectcraft-path = { workspace = true } kurbo = { workspace = true } roxmltree = "0.21" rayon = { workspace = true } +log = { workspace = true } diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs index a6b73f787..c31617a92 100644 --- a/crates/svg/src/lib.rs +++ b/crates/svg/src/lib.rs @@ -147,27 +147,17 @@ impl Geom { Geom::Rect { x, y, w, h, rx, ry } => { let r = kurbo::Rect::new(*x, *y, x + w, y + h); if *rx > 0.0 || *ry > 0.0 { - if (rx - ry).abs() < 1e-9 { - kurbo::RoundedRect::from_rect(r, *rx).to_path(tolerance(*rx)) - } else { - elliptic_round_rect(*x, *y, *w, *h, *rx, *ry) - } + if (rx - ry).abs() < 1e-9 { kurbo::RoundedRect::from_rect(r, *rx).to_path(0.01) } else { elliptic_round_rect(*x, *y, *w, *h, *rx, *ry) } } else { r.to_path(0.01) } } - Geom::Ellipse { cx, cy, rx, ry } => kurbo::Ellipse::new((*cx, *cy), (*rx, *ry), 0.0).to_path(tolerance(rx.max(*ry))), + Geom::Ellipse { cx, cy, rx, ry } => kurbo::Ellipse::new((*cx, *cy), (*rx, *ry), 0.0).to_path(0.01), Geom::Path(p) => p.clone(), } } } -/// The flattening tolerance for a curve of radius `r`: 0.01, growing with the radius so a huge -/// circle is not split into an unbounded number of curves (as `pathdata` does for arcs). -fn tolerance(r: f64) -> f64 { - 0.01_f64.max(r.abs() * 1e-4) -} - fn elliptic_round_rect(x: f64, y: f64, w: f64, h: f64, rx: f64, ry: f64) -> BezPath { const K: f64 = 0.552_284_749_830_793_6; let (kx, ky) = (rx * K, ry * K); @@ -1247,23 +1237,11 @@ const MAX_XML_DEPTH: usize = 256; /// more than half as many (#1236). const MAX_EXPANDED_ELEMENTS: usize = 500_000; -/// The most attributes one element may have: the XML parser checks each attribute against all -/// earlier ones on its element, so a tag with very many takes quadratic time. -const MAX_XML_ATTRIBUTES: usize = 1024; - /// The deepest element nesting of `text` (a quick scan that skips comments, CDATA, processing /// instructions, declarations and quoted attribute values). -#[cfg(test)] fn xml_depth(text: &str) -> usize { - xml_shape(text).0 -} - -/// The deepest element nesting of `text` and the most attributes (`=` outside quotes) of one -/// of its start tags. -fn xml_shape(text: &str) -> (usize, usize) { let b = text.as_bytes(); let (mut i, mut depth, mut max) = (0usize, 0usize, 0usize); - let mut attrs_max = 0usize; let skip_to = |from: usize, end: &[u8]| b.get(from..).and_then(|r| r.windows(end.len()).position(|w| w == end)).map_or(b.len(), |p| from + p + end.len()); while i < b.len() { if b[i] != b'<' { @@ -1286,19 +1264,16 @@ fn xml_shape(text: &str) -> (usize, usize) { // A start tag: find its end outside quotes; `/>` closes it at once. let mut j = i + 1; let mut quote = None; - let mut attrs = 0usize; while let Some(&c) = b.get(j) { match quote { Some(q) if c == q => quote = None, Some(_) => {} None if c == b'"' || c == b'\'' => quote = Some(c), None if c == b'>' => break, - None if c == b'=' => attrs += 1, None => {} } j += 1; } - attrs_max = attrs_max.max(attrs); if b.get(j.wrapping_sub(1)) != Some(&b'/') { depth += 1; max = max.max(depth); @@ -1306,59 +1281,7 @@ fn xml_shape(text: &str) -> (usize, usize) { i = j + 1; } } - (max, attrs_max) -} - -/// The most bytes the internal entity references of a document may expand to. -const MAX_ENTITY_BYTES: usize = 64 << 20; - -/// What the general entity references (`&name;`) of `text` expand to, in bytes, counting the -/// internal entities its DTD declares (``) and the references inside -/// them. The XML parser bounds the nodes entities make but not the text, so a few repeated -/// references to nested entities could otherwise expand attribute values to gigabytes. -fn entity_bytes(text: &str) -> usize { - use std::collections::HashMap; - // Every declaration of a name is kept, comments and quoted text included: the parser uses - // only one of them, and the largest bounds whichever it is. - let mut decls: HashMap<&str, Vec<&str>> = HashMap::new(); - let mut rest = text; - while let Some(p) = rest.find("`) are not referenced with `&`. - if t.starts_with('%') { - continue; - } - let name_end = t.find(|c: char| c.is_whitespace()).unwrap_or(t.len()); - let (name, after) = t.split_at(name_end); - let after = after.trim_start(); - let Some(q) = after.chars().next().filter(|c| *c == '"' || *c == '\'') else { continue }; - if let Some(end) = after[1..].find(q) { - decls.entry(name).or_default().push(&after[1..1 + end]); - } - } - if decls.is_empty() { - return 0; - } - fn refs(s: &str) -> impl Iterator { - s.split('&').skip(1).filter_map(|r| r.find(';').map(|e| &r[..e])) - } - fn size<'t>(name: &'t str, decls: &HashMap<&'t str, Vec<&'t str>>, memo: &mut HashMap<&'t str, usize>, depth: usize) -> usize { - if let Some(&n) = memo.get(name) { - return n; - } - let Some(values) = decls.get(name) else { return 0 }; - // Nested past any real use (or a cycle, which the parser refuses): too large. - let n = if depth > 32 { - usize::MAX - } else { - values.iter().map(|value| refs(value).fold(value.len(), |n, r| n.saturating_add(size(r, decls, memo, depth + 1)))).max().unwrap_or(0) - }; - memo.insert(name, n); - n - } - let mut memo = HashMap::new(); - refs(text).fold(0usize, |n, r| n.saturating_add(size(r, &decls, &mut memo, 0))) + max } /// Nodes a document may expand to: a few kilobytes of nested internal entities @@ -1369,16 +1292,9 @@ const MAX_XML_NODES: u32 = 4_000_000; pub fn parse(bytes: &[u8]) -> Result { let text = String::from_utf8_lossy(bytes); let text = text.trim_start_matches('\u{feff}'); - let (depth, attributes) = xml_shape(text); - if depth > MAX_XML_DEPTH { + if xml_depth(text) > MAX_XML_DEPTH { return Err(Error::Xml(format!("elements nested more than {MAX_XML_DEPTH} levels deep"))); } - if attributes > MAX_XML_ATTRIBUTES { - return Err(Error::Xml(format!("an element has more than {MAX_XML_ATTRIBUTES} attributes"))); - } - if entity_bytes(text) > MAX_ENTITY_BYTES { - return Err(Error::Xml(format!("its entity references expand to more than {} MiB", MAX_ENTITY_BYTES >> 20))); - } // DTDs stay allowed (Illustrator's SVGs declare entities), but expansion is bounded (#2129). let opts = roxmltree::ParsingOptions { allow_dtd: true, nodes_limit: MAX_XML_NODES, ..Default::default() }; let xml = roxmltree::Document::parse_with_options(text, opts).map_err(|e| Error::Xml(e.to_string()))?; diff --git a/crates/svg/src/raster.rs b/crates/svg/src/raster.rs index 6560bad45..bbcd37a90 100644 --- a/crates/svg/src/raster.rs +++ b/crates/svg/src/raster.rs @@ -421,6 +421,9 @@ fn set_sat(c: [f32; 3], s: f32) -> [f32; 3] { fn draw_node(n: &Node, m: Affine, opacity: f64, dst: &mut Image, budget: &mut SurfaceBudget) { let Some(left) = budget.draws_left.checked_sub(1) else { return }; budget.draws_left = left; + if left == 0 { + log::warn!("svg: draw budget reached, the rest of the drawing is skipped"); + } match n { Node::Group(g) => draw_group(g, m, opacity, dst, budget), Node::Shape(s) => { diff --git a/crates/svg/src/tests.rs b/crates/svg/src/tests.rs index f10057e6f..2d44d6d31 100644 --- a/crates/svg/src/tests.rs +++ b/crates/svg/src/tests.rs @@ -447,43 +447,6 @@ fn at_rule_blocks_are_skipped_by_brace_depth() { assert_eq!(sheet.rules[0].classes, vec!["e".to_string()]); } -#[test] -fn a_huge_circle_is_flattened_into_a_bounded_number_of_curves() { - let g = Geom::Ellipse { cx: 0.0, cy: 0.0, rx: 1e15, ry: 1e15 }; - assert!(g.to_path().elements().len() < 1000, "{}", g.to_path().elements().len()); - let small = Geom::Ellipse { cx: 0.0, cy: 0.0, rx: 10.0, ry: 10.0 }.to_path(); - assert!(!small.elements().is_empty()); -} - -#[test] -fn an_element_with_too_many_attributes_is_refused() { - let attrs: String = (0..2000).map(|i| format!(" a{i}=\"1\"")).collect(); - let svg = format!(""); - assert!(matches!(parse(svg.as_bytes()), Err(Error::Xml(e)) if e.contains("attributes"))); -} - -#[test] -fn entity_references_that_expand_past_the_limit_are_refused() { - // Each level repeats the one below ten times: 10^7 bytes of `x` per `&e;`, referenced 100 times. - let mut dtd = format!("", "x".repeat(10)); - for i in 1..7 { - dtd.push_str(&format!("", format!("&e{};", i - 1).repeat(10))); - } - let svg = format!("", "&e6;".repeat(100)); - assert!(matches!(parse(svg.as_bytes()), Err(Error::Xml(e)) if e.contains("entity"))); - // Illustrator-style entities stay accepted. - let ok = "]>"; - assert!(parse(ok.as_bytes()).is_ok()); -} - -#[test] -fn a_decoy_entity_declaration_does_not_hide_the_real_one() { - // A commented-out `b` comes first; the parser uses the later `b = &e;` (1 MiB each, 100 times). - let dtd = format!("", "x".repeat(1 << 20)); - let svg = format!("", "&b;".repeat(100)); - assert!(matches!(parse(svg.as_bytes()), Err(Error::Xml(e)) if e.contains("entity"))); -} - #[test] fn gradient_stops_copied_past_the_budget_are_refused() { // 4,000 stops referenced by 2,100 shapes would keep 8.4 million stops.