PPTX: parse each tags part once, cap copied tags, and bound cache (#1164)

- Parse each tags part once per import and cache in `Imp::tags_by_part`
- Cap total copied tags into the model at `MAX_TAG_BYTES` (64 MiB), charging each pair a fixed size_of::<(String, String)>() in addition to text
- Bound the cache: once `tags_capped` is set, return early before parsing or caching any subsequent part
- Tests covering deduplicated part copies, empty-tag pair budgets, and verifying unread tag parts are not parsed or cached after the budget is reached

crates/pptx/src/lib.rs

@@ -14,6 +14,7 @@ mod maps;
 mod opc;
 mod png;
 pub(crate) mod read;
+pub use read::MAX_TAG_BYTES;
 mod tables;
 mod write;
 mod xml;

crates/pptx/src/read.rs

@@ -40,6 +40,12 @@ pub struct Imp<'a> {
     pub kept_sidecar_bytes: usize,
     /// Cached kept sidecars by normalized chart part name.
     pub kept_charts: HashMap<String, deckcraft_model::chart::ChartKept>,
+    /// Add-in tags parts, parsed once each, by normalized part name.
+    pub tags_by_part: HashMap<String, Arc<[(String, String)]>>,
+    /// Bytes of tags copied into the model so far (see [`MAX_TAG_BYTES`]).
+    pub tag_bytes: usize,
+    /// The tag budget ran out and the warning was given.
+    pub tags_capped: bool,
 }
 
 impl Imp<'_> {
@@ -196,16 +202,64 @@ fn choice_has_model3d(el: &El, rels: &Rels) -> bool {
     rec(el, rels, 0)
 }
 
+/// Bytes of tags one import copies into the model, every tags part and reference together: a
+/// small compressed tags part listed by many shapes, or many times by one, must not grow without
+/// bound (#1164). The package budgets (`opc::MAX_XML_PART`, `MAX_TOTAL`, #1283) bound what is
+/// parsed; this bounds what is copied from it.
+pub const MAX_TAG_BYTES: usize = 64 << 20;
+/// What each copied pair costs besides its text, so empty pairs count too.
+const TAG_PAIR_COST: usize = std::mem::size_of::<(String, String)>();
+
 /// Add-in tags named by `holder`'s `p:custDataLst`: the name/value pairs of each tags part.
-pub fn tags(imp: &Imp, part: &Part, holder: &El) -> Vec<(String, String)> {
+/// Each part is parsed once per import and counted once per holder. Once [`MAX_TAG_BYTES`] is
+/// reached, no further tag parts are parsed or cached (#1164).
+pub fn tags(imp: &mut Imp, part: &Part, holder: &El) -> Vec<(String, String)> {
+    if imp.tags_capped {
+        return vec![];
+    }
     let Some(list) = holder.child("custDataLst") else { return vec![] };
     let mut out = Vec::new();
+    let mut seen = HashSet::new();
     for t in list.children_named("tags").take(100) {
-        let Some(doc) = t.attr("r:id").and_then(|id| part.rels.get(id)).and_then(|r| imp.pkg.xml(&r.target)) else { continue };
-        for tag in doc.root.children_named("tag").take(10_000) {
-            if let Some(name) = tag.attr("name") {
-                out.push((name.to_string(), tag.attr("val").unwrap_or("").to_string()));
+        if imp.tags_capped {
+            return out;
+        }
+        let Some(target) = t.attr("r:id").and_then(|id| part.rels.get(id)).map(|r| opc::norm(&r.target)) else { continue };
+        if !seen.insert(target.clone()) {
+            continue;
+        }
+        let parsed = match imp.tags_by_part.get(&target) {
+            Some(p) => p.clone(),
+            None => {
+                if imp.tags_capped {
+                    return out;
+                }
+                let p: Arc<[(String, String)]> = imp
+                    .pkg
+                    .xml(&target)
+                    .map(|doc| {
+                        doc.root
+                            .children_named("tag")
+                            .take(10_000)
+                            .filter_map(|tag| Some((tag.attr("name")?.to_string(), tag.attr("val").unwrap_or("").to_string())))
+                            .collect()
+                    })
+                    .unwrap_or_default();
+                imp.tags_by_part.insert(target, p.clone());
+                p
             }
+        };
+        for (name, val) in parsed.iter() {
+            let n = imp.tag_bytes.saturating_add(TAG_PAIR_COST + name.len() + val.len());
+            if n > MAX_TAG_BYTES {
+                if !imp.tags_capped {
+                    log::warn!("pptx: add-in tags over {MAX_TAG_BYTES} bytes; the rest are dropped");
+                    imp.tags_capped = true;
+                }
+                return out;
+            }
+            imp.tag_bytes = n;
+            out.push((name.clone(), val.clone()));
         }
     }
     out
@@ -389,6 +443,9 @@ pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec<
         vml_previews: HashMap::new(),
         kept_sidecar_bytes: 0,
         kept_charts: HashMap::new(),
+        tags_by_part: HashMap::new(),
+        tag_bytes: 0,
+        tags_capped: false,
     };
     let part = Part::new(&pkg, &theme_path, &doc);
     let theme = theme::read_theme(&mut imp, &part, &doc.root);
@@ -427,6 +484,9 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxE
         vml_previews: HashMap::new(),
         kept_sidecar_bytes: 0,
         kept_charts: HashMap::new(),
+        tags_by_part: HashMap::new(),
+        tag_bytes: 0,
+        tags_capped: false,
     };
     // Slide ids first so hyperlinks anywhere can point at slides.
     let slide_ids = register_slides(&mut imp, &docs, &slide_entries, &mut lost_slides);
@@ -441,7 +501,7 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxE
     let layout_by_part = read_masters(&mut imp, &mut p, &master_paths, &docs);
     let file_slide_ids = read_slides(&mut imp, &mut p, &pres_part, &docs, &slide_ids, &layout_by_part);
     read_sections(&mut p, &pres_part, pres_el, &file_slide_ids);
-    p.tags = tags(&imp, &pres_part, pres_el);
+    p.tags = tags(&mut imp, &pres_part, pres_el);
     read_custom_shows(&mut p, &imp, &pres_part, pres_el);
     read_guides_and_show(&mut p, &imp, &pres_part);
     read_notes_master(&mut imp, &mut p, &pres_part);

crates/pptx/tests/it/tags.rs

@@ -36,3 +36,111 @@ fn tags_survive_round_trip() {
     assert_eq!(first.tags, vec![t("THINKCELLSHAPEDONOTDELETE", "t!abc")]);
     assert!(back.slides.iter().skip(1).all(|s| s.tags.is_empty()));
 }
+
+/// #1164: the same tags part listed over and over is read once, and the tags one import copies
+/// into the model are capped however often a shape lists them.
+#[test]
+fn a_tags_part_listed_many_times_is_copied_once_and_capped() {
+    let mut s = Session::new();
+    deckcraft_engine::sample::open_sample(&mut s).expect("sample");
+    for _ in 0..6 {
+        s.execute("shape.insert", &serde_json::json!({"preset": "rect", "rect": [10, 10, 50, 50]})).expect("shape");
+    }
+    let mut p = s.doc().expect("doc").doc.as_ref().clone();
+    let slide = Arc::make_mut(&mut p.slides[0]);
+    // A quarter of the budget with its name and pair cost: the fifth shape listing it goes over.
+    let pair = std::mem::size_of::<(String, String)>();
+    slide.shapes[0].tags = vec![t("BIG", &"x".repeat(deckcraft_pptx::MAX_TAG_BYTES / 4 - 3 - pair))];
+    let bytes = deckcraft_pptx::export(&p).expect("export");
+
+    // Every shape on the first slide lists that one tags part 100 times.
+    let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).expect("zip");
+    let mut xml = String::new();
+    zip.by_name("ppt/slides/slide1.xml").expect("slide").read_to_string(&mut xml).expect("read");
+    let start = xml.find("<p:custDataLst>").expect("tags listed");
+    let end = xml[start..].find("</p:custDataLst>").expect("list ends") + start + "</p:custDataLst>".len();
+    let list = &xml[start..end];
+    let inner = list.trim_start_matches("<p:custDataLst>").trim_end_matches("</p:custDataLst>");
+    let many = format!("<p:custDataLst>{}</p:custDataLst>", inner.repeat(100));
+    let xml = xml.replace(list, "").replace("<p:nvPr/>", "<p:nvPr></p:nvPr>").replace("<p:nvPr>", &format!("<p:nvPr>{many}"));
+    assert!(xml.matches(&many).count() >= 5, "enough shapes to go over the budget");
+    let mut out = zip::ZipWriter::new(Cursor::new(Vec::new()));
+    for i in 0..zip.len() {
+        let mut f = zip.by_index(i).expect("entry");
+        let name = f.name().to_string();
+        let mut data = vec![];
+        f.read_to_end(&mut data).expect("read");
+        out.start_file(&name, zip::write::SimpleFileOptions::default()).expect("start");
+        std::io::Write::write_all(&mut out, if name == "ppt/slides/slide1.xml" { xml.as_bytes() } else { &data }).expect("write");
+    }
+    let bytes = out.finish().expect("finish").into_inner();
+
+    let back = deckcraft_pptx::import(&bytes).expect("import");
+    let tagged: Vec<_> = back.slides[0].shapes.iter().filter(|x| !x.tags.is_empty()).collect();
+    assert_eq!(tagged.len(), 4, "four copies fit the budget");
+    assert!(tagged.iter().all(|x| x.tags.len() == 1), "each shape keeps the part's tag once, not 100 times");
+}
+
+/// #1164: empty tags cost something too, or a part of 10,000 empty pairs listed by every shape
+/// would be copied without ever reaching the budget.
+#[test]
+fn empty_tags_count_against_the_budget() {
+    let pair = std::mem::size_of::<(String, String)>();
+    let per_shape = 10_000 * pair;
+    let shapes = deckcraft_pptx::MAX_TAG_BYTES / per_shape + 2;
+    let mut s = Session::new();
+    deckcraft_engine::sample::open_sample(&mut s).expect("sample");
+    let mut p = s.doc().expect("doc").doc.as_ref().clone();
+    let slide = Arc::make_mut(&mut p.slides[0]);
+    let template = slide.shapes[0].clone();
+    slide.shapes.clear();
+    for i in 0..shapes {
+        let mut sh = template.clone();
+        sh.id = deckcraft_model::ShapeId(10_000 + i as u32);
+        sh.ph = None;
+        sh.tags = vec![t("", ""); 10_000];
+        slide.shapes.push(sh);
+    }
+    let bytes = deckcraft_pptx::export(&p).expect("export");
+    let back = deckcraft_pptx::import(&bytes).expect("import");
+    let pairs: usize = back.slides[0].shapes.iter().map(|x| x.tags.len()).sum();
+    assert!(pairs * pair <= deckcraft_pptx::MAX_TAG_BYTES, "{pairs} pairs copied");
+    assert!(pairs < shapes * 10_000, "the budget stopped the copying");
+}
+
+/// #1164: once the budget is exhausted, further tags parts are not parsed or cached into
+/// `tags_by_part`.
+#[test]
+fn new_tag_parts_are_not_parsed_or_cached_after_the_budget_is_exhausted() {
+    let mut s = Session::new();
+    deckcraft_engine::sample::open_sample(&mut s).expect("sample");
+    s.execute("shape.insert", &serde_json::json!({"preset": "rect", "rect": [10, 10, 50, 50]})).expect("shape");
+    let mut p = s.doc().expect("doc").doc.as_ref().clone();
+    let slide = Arc::make_mut(&mut p.slides[0]);
+    assert!(slide.shapes.len() >= 2);
+    // The first shape's tag fits the budget, and a second pair pushes it over the cap.
+    let pair = std::mem::size_of::<(String, String)>();
+    slide.shapes[0].tags = vec![t("BIG", &"x".repeat(deckcraft_pptx::MAX_TAG_BYTES - 3 - pair)), t("OVER", "1")];
+    // The second shape has its own separate tag part.
+    slide.shapes[1].tags = vec![t("SHAPE2_TAG", "dropped")];
+    let bytes = deckcraft_pptx::export(&p).expect("export");
+
+    // Corrupt the second shape's tag part (`tag2.xml`) with invalid XML in the zip.
+    // If it were parsed, XML loading would fail. Because the budget was capped, it is never read.
+    let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).expect("zip");
+    let mut out = zip::ZipWriter::new(Cursor::new(Vec::new()));
+    for i in 0..zip.len() {
+        let mut f = zip.by_index(i).expect("entry");
+        let name = f.name().to_string();
+        let mut data = vec![];
+        f.read_to_end(&mut data).expect("read");
+        out.start_file(&name, zip::write::SimpleFileOptions::default()).expect("start");
+        let payload = if name == "ppt/tags/tag2.xml" { b"<not valid xml<<<<" as &[u8] } else { &data };
+        std::io::Write::write_all(&mut out, payload).expect("write");
+    }
+    let corrupted = out.finish().expect("finish").into_inner();
+
+    let back = deckcraft_pptx::import(&corrupted).expect("import succeeds because tag2.xml is not parsed");
+    assert!(!back.slides[0].shapes[0].tags.is_empty(), "first shape tag was loaded");
+    assert!(back.slides[0].shapes[1].tags.is_empty(), "second shape tag was dropped and never parsed");
+}