Merge pull request #15 from storytold/pr/never-crash-svg

Never crash: SVG path data loop and deep nesting

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;
@@ -1042,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"<!--") {
+            i = skip_to(i + 4, b"-->");
+        } else if rest.starts_with(b"<![CDATA[") {
+            i = skip_to(i + 9, b"]]>");
+        } else if rest.starts_with(b"<?") {
+            i = skip_to(i + 2, b"?>");
+        } else if rest.starts_with(b"<!") {
+            i = skip_to(i + 2, b">");
+        } else if rest.starts_with(b"</") {
+            depth = depth.saturating_sub(1);
+            i = skip_to(i + 2, 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<Doc, Error> {
     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();

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]);

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!("<svg xmlns=\"http://www.w3.org/2000/svg\">{}<rect width=\"5\" height=\"5\"/>{}</svg>", "<g>".repeat(n), "</g>".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!(
+        "<svg xmlns=\"http://www.w3.org/2000/svg\"><!-- {} --><![CDATA[{}]]><g title=\"a>b\">{}</g></svg>",
+        "<g>".repeat(1000),
+        "<g>".repeat(1000),
+        "<rect/>".repeat(1000)
+    );
+    assert_eq!(crate::xml_depth(&s), 2);
+    assert!(crate::parse(s.as_bytes()).is_ok());
+}