From 6708ff55b467cb4ae6e9e454d8ae425b5ed9029d Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 12:14:58 -0400 Subject: [PATCH 1/2] Never crash: SVG path data no longer loops forever on a number after Z Path data like "M0 0 L10 0 Z9" repeated Z implicitly for the stray number; Z consumes no numbers, so the parser looped forever appending close-paths until memory ran out (a 1-byte change to a valid SVG used 24 GB and was killed). A number after Z is a syntax error, so parsing now stops there and keeps what was read, as the spec asks. Also denies the panic lints in the crate (it had no panic sites). Co-Authored-By: Claude Opus 5.5 --- clippy.toml | 4 ++++ crates/svg/src/lib.rs | 2 ++ crates/svg/src/pathdata.rs | 14 ++++++++++++-- 3 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 000000000..4b6c79f55 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +allow-unwrap-in-tests = true +allow-expect-in-tests = true +allow-panic-in-tests = true +allow-indexing-slicing-in-tests = true diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs index 2825cdfaa..06ae671af 100644 --- a/crates/svg/src/lib.rs +++ b/crates/svg/src/lib.rs @@ -4,6 +4,8 @@ //! ([`Doc`]): groups with transforms and opacity, and shapes with fills and strokes in their own //! user space. [`rasterize`] draws it at any scale; the engine converts it to shape layers. +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] + mod color; mod pathdata; mod raster; diff --git a/crates/svg/src/pathdata.rs b/crates/svg/src/pathdata.rs index 842efb45d..c8fa7f67f 100644 --- a/crates/svg/src/pathdata.rs +++ b/crates/svg/src/pathdata.rs @@ -109,8 +109,9 @@ pub fn parse_path_data(d: &str) -> BezPath { let c = match l.command() { Some(c) => c, None => match cmd { - // Implicit repeat (a moveto repeats as lineto). - Some(c) if l.at_number() => match c { + // Implicit repeat (a moveto repeats as lineto). Z takes no numbers, so a number + // after it is an error: repeating Z would loop forever without consuming input. + Some(c) if l.at_number() && !c.eq_ignore_ascii_case(&b'z') => match c { b'M' => b'L', b'm' => b'l', other => other, @@ -284,6 +285,15 @@ mod tests { assert!(parse_path_data("").elements().is_empty()); } + #[test] + fn number_after_close_stops_instead_of_looping() { + // "Z9": Z was repeated implicitly without consuming the number, appending close-paths + // until memory ran out. + let p = parse_path_data("M0 0 L10 0 L10 10 Z9 C1 2 3 4 5 6"); + assert_eq!(p.elements().len(), 4); + assert!(parse_path_data("M0 0 L10 0 z 9 9").elements().len() <= 4); + } + #[test] fn number_lists() { assert_eq!(numbers("0 0,100 50"), vec![0.0, 0.0, 100.0, 50.0]); From 19c1d2ea1dd6a31d3c22f4ea7daa45f622d9048c Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 14:05:58 -0400 Subject: [PATCH 2/2] Never crash: reject SVGs nested too deeply roxmltree recurses per element and overflowed a 2 MiB stack at a few thousand nested elements (aborting the app). parse() now scans the element depth first and returns Error::Xml beyond 256 levels. Co-Authored-By: Claude Opus 5.5 --- crates/svg/src/lib.rs | 55 +++++++++++++++++++++++++++++++++++++++++ crates/svg/src/tests.rs | 17 +++++++++++++ 2 files changed, 72 insertions(+) diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs index 06ae671af..d0e62c0d1 100644 --- a/crates/svg/src/lib.rs +++ b/crates/svg/src/lib.rs @@ -1044,10 +1044,65 @@ pub fn parse_transform(s: &str) -> Affine { m } +/// The deepest element nesting accepted. The XML parser recurses per level and overflowed a +/// 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 deepest element nesting of `text` (a quick scan that skips comments, CDATA, processing +/// instructions, declarations and quoted attribute values). +fn xml_depth(text: &str) -> usize { + let b = text.as_bytes(); + let (mut i, mut depth, mut max) = (0usize, 0usize, 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'<' { + i += 1; + continue; + } + let rest = &b[i..]; + if rest.starts_with(b""); + } else if rest.starts_with(b""); + } else if rest.starts_with(b""); + } else if rest.starts_with(b""); + } else if rest.starts_with(b""); + } else { + // A start tag: find its end outside quotes; `/>` closes it at once. + let mut j = i + 1; + let mut quote = None; + 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 => {} + } + j += 1; + } + if b.get(j.wrapping_sub(1)) != Some(&b'/') { + depth += 1; + max = max.max(depth); + } + i = j + 1; + } + } + max +} + /// Parse an SVG document. pub fn parse(bytes: &[u8]) -> Result { let text = String::from_utf8_lossy(bytes); let text = text.trim_start_matches('\u{feff}'); + if xml_depth(text) > MAX_XML_DEPTH { + return Err(Error::Xml(format!("elements nested more than {MAX_XML_DEPTH} levels deep"))); + } let opts = roxmltree::ParsingOptions { allow_dtd: true, ..Default::default() }; let xml = roxmltree::Document::parse_with_options(text, opts).map_err(|e| Error::Xml(e.to_string()))?; let root = xml.root_element(); diff --git a/crates/svg/src/tests.rs b/crates/svg/src/tests.rs index c367412f4..a7a9bef3f 100644 --- a/crates/svg/src/tests.rs +++ b/crates/svg/src/tests.rs @@ -106,3 +106,20 @@ fn transforms() { let p = m * kurbo::Point::new(0.0, 1.0); assert!((p.x - 6.0).abs() < 1e-9 && (p.y - 7.0).abs() < 1e-9); } + +#[test] +fn deep_nesting_errors_instead_of_overflowing_the_stack() { + // 100 000 nested groups overflowed the XML parser's stack and aborted the process. + let deep = |n: usize| format!("{}{}", "".repeat(n), "".repeat(n)); + assert!(matches!(crate::parse(deep(100_000).as_bytes()), Err(crate::Error::Xml(_)))); + assert!(crate::parse(deep(100).as_bytes()).is_ok()); + // Comments, CDATA, quoted '>' and self-closing tags don't count. + let s = format!( + "{}b\">{}", + "".repeat(1000), + "".repeat(1000), + "".repeat(1000) + ); + assert_eq!(crate::xml_depth(&s), 2); + assert!(crate::parse(s.as_bytes()).is_ok()); +}