Merge pull request #7283 from dotCooCoo/fix/nested-svg-image-count

SVG import: broken nested SVG images do not use up the image allowance

crates/svg/src/import.rs

@@ -20,7 +20,7 @@ use text::TextSlots;
 
 pub use depth::MAX_ELEMENT_DEPTH;
 pub(crate) use depth::xml_too_deep;
-pub use files::MAX_NESTED_SVG_IMAGES;
+pub use files::{MAX_NESTED_SVG_IMAGES, MAX_NESTED_SVG_READS, MAX_NESTED_SVG_TEXT};
 pub(crate) use fx::BLEND;
 
 /// Most objects an imported SVG may expand to: a guard against `<use>` and nested-image

crates/svg/src/import/files.rs

@@ -23,6 +23,13 @@ use crate::{ImportOptions, MAX_ELEMENT_DEPTH, MAX_SVGZ, SvgError};
 /// Most nested SVG images an imported SVG may decode (#3564).
 pub const MAX_NESTED_SVG_IMAGES: usize = 256;
 
+/// Most nested SVG images an imported SVG may read, decoded or not: each one read is checked and
+/// parsed, and an image that fails is read again wherever it is shown.
+pub const MAX_NESTED_SVG_READS: usize = 1024;
+
+/// Most bytes of nested SVG image text an imported SVG may read, decoded or not, all reads together.
+pub const MAX_NESTED_SVG_TEXT: u64 = 512 << 20;
+
 /// The files an SVG's images link to.
 #[derive(Default)]
 pub(in crate::import) struct Files {
@@ -48,6 +55,12 @@ pub(in crate::import) struct Files {
     svg_images: AtomicUsize,
     /// SVG images were left out for exceeding [`MAX_NESTED_SVG_IMAGES`].
     too_many_svg_images: AtomicBool,
+    /// How many nested SVG images were read so far, decoded or not.
+    svg_reads: AtomicUsize,
+    /// The bytes of text those reads took.
+    svg_text: AtomicU64,
+    /// SVG images were left out for exceeding [`MAX_NESTED_SVG_READS`] or [`MAX_NESTED_SVG_TEXT`].
+    too_much_svg_read: AtomicBool,
     /// The CMYK TIFF stored in place of a turned CMYK image, by the address of the PNG usvg got
     /// (kept here too, so the address stays its own).
     cmyk: Mutex<HashMap<usize, (Bytes, Bytes)>>,
@@ -253,6 +266,12 @@ impl Files {
         if self.too_many_svg_images.load(Ordering::Relaxed) {
             notes.push(format!("SVG images were left out: more than {MAX_NESTED_SVG_IMAGES} nested SVG images"));
         }
+        if self.too_much_svg_read.load(Ordering::Relaxed) {
+            notes.push(format!(
+                "SVG images were left out: nested SVG images, valid or not, are read at most {MAX_NESTED_SVG_READS} times and {} MB of text",
+                MAX_NESTED_SVG_TEXT >> 20
+            ));
+        }
         notes
     }
 
@@ -308,11 +327,33 @@ impl Files {
 
     /// The SVG image `text` as usvg reads it with `opts`. `None`, the image left out as one usvg
     /// can't read, when it isn't SVG or when its elements nest deeper than the SVGs around it
-    /// leave: usvg reads it while it reads them, on the same stack.
+    /// leave: usvg reads it while it reads them, on the same stack. Only a decoded image, with the
+    /// images decoded inside it, counts toward [`MAX_NESTED_SVG_IMAGES`]; every image read counts
+    /// toward [`MAX_NESTED_SVG_READS`] and [`MAX_NESTED_SVG_TEXT`].
     fn svg(&self, text: &str, opts: &usvg::Options) -> Option<ImageKind> {
-        if self.svg_images.fetch_add(1, Ordering::Relaxed) >= MAX_NESTED_SVG_IMAGES {
+        let decoded = self.svg_images.load(Ordering::Relaxed);
+        if decoded >= MAX_NESTED_SVG_IMAGES {
             return leave_out(&self.too_many_svg_images, true);
         }
+        let reads = self.svg_reads.load(Ordering::Relaxed);
+        let read = self.svg_text.load(Ordering::Relaxed).saturating_add(text.len() as u64);
+        if reads >= MAX_NESTED_SVG_READS || read > MAX_NESTED_SVG_TEXT {
+            return leave_out(&self.too_much_svg_read, true);
+        }
+        self.svg_reads.store(reads + 1, Ordering::Relaxed);
+        self.svg_text.store(read, Ordering::Relaxed);
+        // The image's place is taken while usvg reads it, so the images inside it get what is left.
+        // usvg reads them all on this thread, so no other read comes between these steps.
+        self.svg_images.store(decoded + 1, Ordering::Relaxed);
+        let tree = self.svg_tree(text, opts);
+        if tree.is_none() {
+            self.svg_images.store(decoded, Ordering::Relaxed);
+        }
+        tree
+    }
+
+    /// [`Files::svg`]'s image once it is counted.
+    fn svg_tree(&self, text: &str, opts: &usvg::Options) -> Option<ImageKind> {
         // Measured as the SVG holding it is (`import`): the text before the XML reader.
         let used = self.depth.load(Ordering::Relaxed);
         let left = MAX_ELEMENT_DEPTH.saturating_sub(used);
@@ -323,11 +364,12 @@ impl Files {
         let Some(height) = depth::height(&xml, left) else { return leave_out(&self.too_deep, true) };
         // usvg's parser would overflow on it, as on the SVG holding it (`import`).
         super::convolve_matrices_ok(&xml).ok()?;
-        self.note_text(text);
         self.depth.store(used + height, Ordering::Relaxed);
         let tree = usvg::Tree::from_data_nested(text.as_bytes(), opts);
         self.depth.store(used, Ordering::Relaxed);
-        tree.ok().map(ImageKind::SVG)
+        let tree = tree.ok()?;
+        self.note_text(text);
+        Some(ImageKind::SVG(tree))
     }
 }
 
@@ -587,4 +629,34 @@ mod tests {
         assert!(!read("2147483647"));
         assert!(read("1"));
     }
+
+    /// The nested SVG images read, decoded or not, take at most [`MAX_NESTED_SVG_TEXT`] bytes of
+    /// text together: an image whose text goes past it is left out and noted. A broken image's
+    /// text counts too, and its decoded place is given back.
+    #[test]
+    fn svg_images_share_the_text_cap() {
+        let svg = r#"<svg xmlns="http://www.w3.org/2000/svg" width="2" height="2"><rect width="1" height="1"/></svg>"#;
+        let broken = &svg[..20];
+        let files = Files::default();
+        let resolver = files.resolver(1);
+        let opts = usvg::Options::default();
+        let read =
+            |text: &str| matches!((resolver.resolve_data)("image/svg+xml", Arc::new(text.as_bytes().to_vec()), &opts), Some(ImageKind::SVG(_)));
+        assert!(!read(broken));
+        assert_eq!(files.svg_text.load(Ordering::Relaxed), broken.len() as u64);
+        assert_eq!((files.svg_reads.load(Ordering::Relaxed), files.svg_images.load(Ordering::Relaxed)), (1, 0));
+        files.svg_text.store(MAX_NESTED_SVG_TEXT - svg.len() as u64, Ordering::Relaxed);
+        assert!(read(svg));
+        assert_eq!((files.svg_reads.load(Ordering::Relaxed), files.svg_images.load(Ordering::Relaxed)), (2, 1));
+        assert!(files.left_out().is_empty());
+        assert!(!read(svg) && !read(broken));
+        assert_eq!(files.svg_text.load(Ordering::Relaxed), MAX_NESTED_SVG_TEXT, "past the cap, nothing is read");
+        assert_eq!(
+            files.left_out(),
+            [format!(
+                "SVG images were left out: nested SVG images, valid or not, are read at most {MAX_NESTED_SVG_READS} times and {} MB of text",
+                MAX_NESTED_SVG_TEXT >> 20
+            )]
+        );
+    }
 }

crates/svg/src/lib.rs

@@ -576,7 +576,7 @@ pub fn svg_filters(svg: &str, room: usize, taken: impl Fn(&str) -> bool) -> Resu
 }
 
 pub use glyph::glyph_art;
-pub use import::{MAX_ELEMENT_DEPTH, MAX_FOREIGN, MAX_FOREIGN_VALUE, MAX_NESTED_SVG_IMAGES, MAX_NODES};
+pub use import::{MAX_ELEMENT_DEPTH, MAX_FOREIGN, MAX_FOREIGN_VALUE, MAX_NESTED_SVG_IMAGES, MAX_NESTED_SVG_READS, MAX_NESTED_SVG_TEXT, MAX_NODES};
 
 /// Import an SVG document reading the files it links to as `opts` says, also returning warnings.
 pub fn import_with(svg: &str, opts: &ImportOptions) -> Result<(Document, Vec<String>), SvgError> {

crates/svg/tests/it/import_details.rs

@@ -6,7 +6,7 @@ use std::path::{Path, PathBuf};
 
 use vectorcraft_doc::{Document, ImageObject, LinkInfo, Node, NodeKind};
 use vectorcraft_geom::Rect;
-use vectorcraft_svg::{ImportOptions, MAX_NESTED_SVG_IMAGES, SvgError, import_with, import_with_report, import_within};
+use vectorcraft_svg::{ImportOptions, MAX_NESTED_SVG_IMAGES, MAX_NESTED_SVG_READS, SvgError, import_with, import_with_report, import_within};
 use vectorcraft_testkit::raster::{Image, assert_similar, render_artboard};
 
 fn svg(body: &str) -> String {
@@ -683,6 +683,87 @@ fn nested_svg_images_past_count_cap_are_left_out() {
     assert!(warnings.iter().any(|w| w.contains(&format!("more than {MAX_NESTED_SVG_IMAGES} nested SVG images"))));
 }
 
+/// Nested SVG images that can't be read: text that isn't XML, and an SVG without a size.
+const BROKEN_SVG_IMAGES: [&str; 2] =
+    [r#"<svg xmlns="http://www.w3.org/2000/svg" width="10"#, r#"<svg xmlns="http://www.w3.org/2000/svg" width="0" height="0"/>"#];
+const SMALL_SVG_IMAGE: &str = r#"<svg xmlns="http://www.w3.org/2000/svg" width="10" height="10"><rect width="5" height="5"/></svg>"#;
+
+/// `broken` images showing the broken SVG images in turn from data URLs, then `good` images
+/// showing a rectangle.
+fn broken_then_good(broken: usize, good: usize) -> String {
+    let broken: String = (0..broken).map(|i| image_of("image/svg+xml", BROKEN_SVG_IMAGES[i % 2])).collect();
+    format!(r#"<svg {SVG_NS} width="500" height="500">{broken}{}</svg>"#, image_of("image/svg+xml", SMALL_SVG_IMAGE).repeat(good))
+}
+
+/// Only nested SVG images that decode count toward `MAX_NESTED_SVG_IMAGES`: an image after that
+/// many broken ones, from data URLs or a linked file shown again and again, is imported, and the
+/// images past that many decoded ones are still left out.
+#[test]
+fn broken_nested_svg_images_do_not_use_up_the_image_allowance() {
+    let allowance = |w: &[String]| w.iter().any(|w| w.contains(&format!("more than {MAX_NESTED_SVG_IMAGES} nested SVG images")));
+    let (d, w) = import_with(&broken_then_good(MAX_NESTED_SVG_IMAGES + 10, 1), &ImportOptions::default()).unwrap();
+    assert_eq!(rects(&d), 1, "{w:?}");
+    assert!(!w.iter().any(|w| w.contains("left out")), "{w:?}");
+    let (d, w) = import_with(&broken_then_good(10, MAX_NESTED_SVG_IMAGES + 1), &ImportOptions::default()).unwrap();
+    assert_eq!(rects(&d), MAX_NESTED_SVG_IMAGES, "{w:?}");
+    assert!(allowance(&w), "{w:?}");
+
+    let dir = folder("brokensvgimages");
+    std::fs::write(dir.join("img").join("broken.svg"), BROKEN_SVG_IMAGES[0]).unwrap();
+    std::fs::write(dir.join("img").join("good.svg"), SMALL_SVG_IMAGE).unwrap();
+    let reads = std::sync::atomic::AtomicUsize::new(0);
+    let counted = |path: &str| {
+        reads.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+        read(path)
+    };
+    let body = format!(
+        r#"{}<image href="img/good.svg" width="10" height="10"/>"#,
+        r#"<image href="img/broken.svg" width="10" height="10"/>"#.repeat(MAX_NESTED_SVG_IMAGES + 10)
+    );
+    let folder = dir.to_string_lossy();
+    let (d, w) = import_with(&svg(&body), &ImportOptions { folder: Some(&folder), read: Some(&counted) }).unwrap();
+    assert_eq!(rects(&d), 1, "{w:?}");
+    assert!(!w.iter().any(|w| w.contains("left out")), "{w:?}");
+    assert_eq!(reads.into_inner(), 2, "each file is read once");
+}
+
+/// Every nested SVG image read, decoded or not, counts toward `MAX_NESTED_SVG_READS`: past it,
+/// images are left out with a warning, also when `<use>` shows one broken linked file many times.
+#[test]
+fn nested_svg_images_are_read_at_most_max_nested_svg_reads_times() {
+    let past = |w: &[String]| w.iter().any(|w| w.contains(&format!("read at most {MAX_NESTED_SVG_READS} times")));
+    let (d, w) = import_with(&broken_then_good(MAX_NESTED_SVG_READS - 1, 1), &ImportOptions::default()).unwrap();
+    assert_eq!(rects(&d), 1, "{w:?}");
+    assert!(!past(&w), "{w:?}");
+    let (d, w) = import_with(&broken_then_good(MAX_NESTED_SVG_READS, 1), &ImportOptions::default()).unwrap();
+    assert_eq!(rects(&d), 0, "{w:?}");
+    assert!(past(&w), "{w:?}");
+    assert!(!w.iter().any(|w| w.contains(&format!("more than {MAX_NESTED_SVG_IMAGES} nested SVG images"))), "{w:?}");
+
+    let dir = folder("brokensvguses");
+    std::fs::write(dir.join("img").join("broken.svg"), BROKEN_SVG_IMAGES[0]).unwrap();
+    std::fs::write(dir.join("img").join("good.svg"), SMALL_SVG_IMAGE).unwrap();
+    let uses = r##"<use href="#broken"/>"##.repeat(2 * MAX_NESTED_SVG_READS);
+    let body = format!(
+        r#"<defs><g id="broken"><image href="img/broken.svg" width="10" height="10"/></g></defs>{uses}<image href="img/good.svg" width="10" height="10"/>"#
+    );
+    let (d, w) = open_in(&dir, &body);
+    assert_eq!(rects(&d), 0, "{w:?}");
+    assert!(past(&w), "{w:?}");
+}
+
+/// A nested SVG image that can't be read and is left out gives no warning about its text.
+#[test]
+fn broken_nested_svg_images_do_not_warn_about_their_text() {
+    let text = |svg: &str| {
+        let file = format!(r#"<svg {SVG_NS} width="50" height="50">{}</svg>"#, image_of("image/svg+xml", svg));
+        let (_, w) = import_with(&file, &ImportOptions::default()).unwrap();
+        w.iter().any(|w| w.contains("text inside an SVG image"))
+    };
+    assert!(!text(r#"<svg xmlns="http://www.w3.org/2000/svg" width="0" height="0"><text>Hi</text></svg>"#));
+    assert!(text(r#"<svg xmlns="http://www.w3.org/2000/svg" width="10" height="10"><text>Hi</text></svg>"#));
+}
+
 /// #3566: entity references in element content count toward the expansion limit, as in
 /// attributes. A 10 KB entity, a second entity that refers to it 250 times, and eight references
 /// to the second inside `<desc>` would expand to about 20 MB from a file of a few KB: the import

docs/target-app-parity.md

@@ -1711,6 +1711,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur
   - A photo whose EXIF orientation turns or flips it (a camera held upright or upside down) is placed and opened upright, as it was taken; the image is stored as it is seen (#4284). A quarter turn swaps the resolution's axes, so the image keeps its physical size. TIFFs read without the image decoder (grayscale with extra channels, CCITT Group 3, CMYK with one plane per ink) follow their Orientation tag too, and a turned CMYK JPEG or TIFF keeps its inks, at the 8 or 16 bits the file has, as a CMYK TIFF.
   - An image that Open and Place store again keeps the ICC profile its file embeds and its 16-bit samples. A turned PNG, JPEG, WebP or TIFF stored as PNG, and a TIFF the image decoder reads, keep their gray or RGB samples at 8 or 16 bits with their gray or RGB profile in the PNG's `iCCP` chunk ([PNG specification, iCCP](https://www.w3.org/TR/png-3/#11iCCP)). A CMYK TIFF stored with one plane per ink and a turned CMYK JPEG or TIFF keep their CMYK profile as TIFF tag 34675, a JPEG's put together from all its APP2 segments ([ICC image-format guide](https://github.com/InternationalColorConsortium/iccDEV/blob/master/iccdev-mcp/docs/icc-format-guide.md)). A profile whose color space differs from the stored samples' is left out. The profile is kept in the stored image; how CMYK inks are drawn and exported to PDF does not change (`crates/engine/src/cmd/fileio/tests.rs`).
   - SVG import shows a raster `<image>` whose EXIF orientation or TIFF Orientation tag turns or flips it upright, linked or as a data URL, and inside an SVG shown as an image: the pixels are stored turned, the width and height swap, and the `<image>` box fits the turned image under `preserveAspectRatio`, as CSS `image-orientation: from-image` describes (<https://www.w3.org/TR/css-images-3/#the-image-orientation>). A turned CMYK JPEG or TIFF keeps its inks as a CMYK TIFF, as does a CMYK TIFF that is not turned. TIFF data URLs (`image/tiff`, or untyped TIFF bytes) are read, and TIFFs with an alpha or other extra channel, CCITT Group 3 TIFFs and CMYK TIFFs stored one plane per ink are read as Open and Place read them. The pixels are decoded within the decode allowance, and a TIFF claiming more strips or tiles than the decoder can count is refused before it is decoded (`crates/svg/tests/it/image_orientation.rs`).
+  - SVG import leaves out an SVG shown as an image that it can't read (text that isn't XML, an SVG without a size) and goes on with the images after it, as SVG 2's error processing describes (<https://www.w3.org/TR/SVG2/conform.html>). Only images that decode count toward the 256 nested SVG images an SVG may show, so an image after many broken ones is imported. Every nested SVG image read, decoded or not, counts toward VectorCraft's own limits of 1,024 reads and 512 MB of text per import; past them, nested SVG images are left out with a warning (`crates/svg/tests/it/import_details.rs`).
   - File › Place and a file dropped on the canvas refuse a PNG, JPEG, GIF or WebP file whose pixels can't be read, such as one cut short by a download or a sync still running, and the status bar shows "Couldn't place <name>: the image's pixels can't be read". Paste and Relink refuse such an image too, and Update Links, or Open when it reads the file again, counts it as missing. A JPEG cut short still places when its pixels can be decoded (`file.place`, `crates/engine/src/tests_place.rs`).
   - On the desktop, Copy and Cut make the system clipboard's formats (text, SVG, PDF, PNG) from a copy of the clipboard on the thread that writes background saves and exports, and write them when they are ready; while that thread has a save, an export, a print or a recovery copy to finish, they are made and written at once. Until they are written, Paste in VectorCraft takes the internal clipboard, and while a text field or the Type tool has the keyboard the app waits for them. Another Copy or Cut, or a copy of text in VectorCraft, drops them.
   - In the Linux Flatpak on GNOME, where the clipboard goes through the compositor's core Wayland protocol, one Copy offers the PNG (`image/png`), the SVG (`image/svg+xml`, with Include SVG Code on) and the text, and Paste takes another app's SVG, text or PNG in that order, a picture over blank text or text that only names it; the Paste menu item is enabled by the types offered (`clipboard::wayland` tests in `apps/vectorcraft/src/clipboard.rs`; not yet run in the Flatpak on a GNOME session).