Pptx: budget chart points, chart XML size, inflate work and VML lookups (#401)

- Charts: every category/value/size cache draws from one chart-wide
  budget of 1M decoded slots, so sparse ptCount/idx declarations across
  many series can't expand far beyond the XML.
- Chart parts go through the same 96 MiB XML size guard as other XML.
- Package::open charges every inflated byte, including parts it then
  discards, against a 4 GiB decompression budget and stops reading
  entries once it is spent.
- Legacy OLE previews index each VML drawing once (shape id -> picture
  part) instead of reparsing it and its rels for every object.

Co-authored-by: Cursor <cursoragent@cursor.com>

crates/pptx/src/opc.rs

@@ -12,6 +12,9 @@ pub const MAX_PART: u64 = 512 * 1024 * 1024;
 pub const MAX_XML_PART: usize = 96 * 1024 * 1024;
 /// Total decompressed bytes accepted for one package.
 pub const MAX_TOTAL: u64 = 2 * 1024 * 1024 * 1024;
+/// Total bytes we are willing to decompress for one package, counting parts we then discard
+/// (too large, or failing part-way): bounds inflate work, not just what is kept.
+pub const MAX_INFLATE_WORK: u64 = 2 * MAX_TOTAL;
 /// Maximum number of zip entries.
 pub const MAX_ENTRIES: usize = 50_000;
 
@@ -36,13 +39,22 @@ pub struct Package {
 
 impl Package {
     pub fn open(bytes: &[u8]) -> Result<Self, PptxError> {
+        Self::open_within(bytes, MAX_PART, MAX_TOTAL, MAX_INFLATE_WORK)
+    }
+    /// [`Package::open`] with explicit limits: per part, kept in total, and inflated in total.
+    fn open_within(bytes: &[u8], max_part: u64, max_total: u64, max_work: u64) -> Result<Self, PptxError> {
         let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| PptxError::NotPptx(format!("{e}")))?;
         if zip.len() > MAX_ENTRIES {
             return Err(PptxError::NotPptx(format!("too many entries ({})", zip.len())));
         }
         let mut parts = HashMap::new();
         let mut total: u64 = 0;
+        let mut work: u64 = 0;
         for i in 0..zip.len() {
+            if work >= max_work {
+                log::warn!("pptx: decompression budget spent; skipping the remaining {} entries", zip.len() - i);
+                break;
+            }
             let mut f = match zip.by_index(i) {
                 Ok(f) => f,
                 Err(e) => {
@@ -55,13 +67,16 @@ impl Package {
             }
             let name = f.name().trim_start_matches('/').to_string();
             let declared = f.size();
-            if declared > MAX_PART {
+            if declared > max_part {
                 log::warn!("pptx: skipping oversized part {name} ({declared} bytes)");
                 continue;
             }
             let mut data = Vec::with_capacity(declared.min(16 * 1024 * 1024) as usize);
-            let cap = MAX_PART.min(MAX_TOTAL.saturating_sub(total));
-            match (&mut f).take(cap.saturating_add(1)).read_to_end(&mut data) {
+            let cap = max_part.min(max_total.saturating_sub(total)).min(max_work.saturating_sub(work));
+            let read = (&mut f).take(cap.saturating_add(1)).read_to_end(&mut data);
+            // Charge what was inflated whether or not the part is kept.
+            work = work.saturating_add(data.len() as u64);
+            match read {
                 Ok(_) if data.len() as u64 > cap => {
                     log::warn!("pptx: part {name} exceeds size limits; skipped");
                     continue;
@@ -80,6 +95,15 @@ impl Package {
     pub fn get(&self, name: &str) -> Option<&[u8]> {
         self.parts.get(&norm(name)).map(|(_, d)| d.as_slice())
     }
+    /// An XML part's bytes, when it is within the XML size limit.
+    pub fn xml_bytes(&self, name: &str) -> Option<&[u8]> {
+        let b = self.get(name)?;
+        if b.len() > MAX_XML_PART {
+            log::warn!("pptx: XML part {name} too large");
+            return None;
+        }
+        Some(b)
+    }
     /// Parse an XML part.
     pub fn xml(&self, name: &str) -> Option<xml::Doc> {
         let b = self.get(name)?;
@@ -352,6 +376,39 @@ impl PackageOut {
 mod tests {
     use super::*;
 
+    fn zip_of(entries: &[(&str, usize)]) -> Vec<u8> {
+        let mut zw = zip::ZipWriter::new(Cursor::new(Vec::new()));
+        let o = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated);
+        for (name, len) in entries {
+            zw.start_file(*name, o).unwrap();
+            zw.write_all(&vec![b'x'; *len]).unwrap();
+        }
+        zw.finish().unwrap().into_inner()
+    }
+
+    #[test]
+    fn discarded_parts_count_against_the_inflate_budget() {
+        // Kept bytes cap at 100: b, c and d are inflated past what's left and discarded. That
+        // work is charged, so after d the budget (130) is spent and e is not read, although it
+        // would have fit.
+        let z = zip_of(&[("a", 80), ("b", 80), ("c", 80), ("d", 80), ("e", 5)]);
+        let p = Package::open_within(&z, 1000, 100, 130).unwrap();
+        assert!(p.get("a").is_some());
+        assert!(p.get("b").is_none() && p.get("d").is_none());
+        assert!(p.get("e").is_none(), "the inflate budget stops reading");
+        // With room to spare the same small part is kept.
+        let p = Package::open_within(&z, 1000, 100, 10_000).unwrap();
+        assert!(p.get("e").is_some());
+    }
+
+    #[test]
+    fn xml_bytes_applies_the_xml_limit() {
+        let z = zip_of(&[("small.xml", 10)]);
+        let p = Package::open(&z).unwrap();
+        assert_eq!(p.xml_bytes("small.xml").map(<[u8]>::len), Some(10));
+        assert!(p.xml_bytes("missing.xml").is_none());
+    }
+
     #[test]
     fn path_resolution() {
         assert_eq!(resolve("ppt/slides", "../media/image1.png"), "ppt/media/image1.png");

crates/pptx/src/read/chart.rs

@@ -35,19 +35,34 @@ fn rich_text(t: &El) -> Option<String> {
     tx.find("v").map(|v| v.text())
 }
 
-/// String values from `c:cat`/`c:tx` (str or num refs, caches or literals).
-fn strings(e: &El) -> Vec<String> {
+/// Most category/value/size slots one chart may decode across all its series and caches, so
+/// sparse `ptCount`/`idx` declarations can't expand far beyond the XML they came from.
+pub(crate) const MAX_CHART_POINTS: usize = 1_000_000;
+
+/// Most slots one cache may hold.
+const MAX_CACHE_POINTS: usize = 100_000;
+
+/// String values from `c:cat`/`c:tx` (str or num refs, caches or literals). Every slot returned
+/// is taken from `budget`; once it runs out the values are cut short.
+fn strings(e: &El, budget: &mut usize) -> Vec<String> {
+    let cap = MAX_CACHE_POINTS.min(*budget);
+    let out = strings_capped(e, cap);
+    *budget = budget.saturating_sub(out.len());
+    out
+}
+
+fn strings_capped(e: &El, cap: usize) -> Vec<String> {
     let cache = e.find("strCache").or_else(|| e.find("numCache")).or_else(|| e.find("strLit")).or_else(|| e.find("numLit"));
     if let Some(c) = cache {
-        let n = c.child("ptCount").and_then(|p| p.u32("val")).unwrap_or(0).min(100_000) as usize;
+        let n = (c.child("ptCount").and_then(|p| p.u32("val")).unwrap_or(0) as usize).min(cap);
         let mut out = vec![String::new(); n];
-        for pt in c.children_named("pt").take(100_000) {
+        for pt in c.children_named("pt").take(MAX_CACHE_POINTS) {
             let i = pt.u32("idx").unwrap_or(0) as usize;
+            if i >= cap {
+                continue;
+            }
             let v = pt.child("v").map(|v| v.text()).unwrap_or_default();
             if i >= out.len() {
-                if i >= 100_000 {
-                    continue;
-                }
                 out.resize(i + 1, String::new());
             }
             if let Some(s) = out.get_mut(i) {
@@ -59,17 +74,19 @@ fn strings(e: &El) -> Vec<String> {
     if let Some(ml) = e.find("multiLvlStrCache") {
         // Use the innermost level.
         if let Some(l) = ml.child("lvl") {
-            return l.children_named("pt").map(|p| p.child("v").map(|v| v.text()).unwrap_or_default()).collect();
+            return l.children_named("pt").take(cap).map(|p| p.child("v").map(|v| v.text()).unwrap_or_default()).collect();
         }
     }
-    if let Some(v) = e.child("v") {
+    if cap > 0
+        && let Some(v) = e.child("v")
+    {
         return vec![v.text()];
     }
     vec![]
 }
 
-fn numbers(e: &El) -> Vec<Option<f64>> {
-    strings(e).into_iter().map(|s| s.trim().parse::<f64>().ok().filter(|v| v.is_finite())).collect()
+fn numbers(e: &El, budget: &mut usize) -> Vec<Option<f64>> {
+    strings(e, budget).into_iter().map(|s| s.trim().parse::<f64>().ok().filter(|v| v.is_finite())).collect()
 }
 
 /// Parse a chart part. Returns `None` for something that isn't a chart we can read.
@@ -86,6 +103,11 @@ pub fn read_chart_xml(bytes: &[u8]) -> Option<Chart> {
 }
 
 pub(crate) fn chart_from(root: &El) -> Option<Chart> {
+    chart_within(root, MAX_CHART_POINTS)
+}
+
+/// [`chart_from`] decoding at most `budget` data slots in all.
+fn chart_within(root: &El, mut budget: usize) -> Option<Chart> {
     let ch = root.child("chart")?;
     let pa = ch.child("plotArea")?;
     let mut c = Chart { categories: vec![], series: vec![], title: None, legend: None, gridlines: false, ..Default::default() };
@@ -190,7 +212,7 @@ pub(crate) fn chart_from(root: &El) -> Option<Chart> {
         let secondary = val_axis.is_some() && val_axis != primary_val;
         for s in g.children_named("ser").take(1000) {
             let mut ser = Series {
-                name: s.child("tx").map(strings).and_then(|v| v.into_iter().next()).unwrap_or_default(),
+                name: s.child("tx").map(|t| strings(t, &mut budget)).and_then(|v| v.into_iter().next()).unwrap_or_default(),
                 fill: simple_fill(s.child("spPr")),
                 line: simple_line(s.child("spPr")),
                 smooth: s.child("smooth").and_then(|v| v.bool("val")).unwrap_or(false),
@@ -218,7 +240,7 @@ pub(crate) fn chart_from(root: &El) -> Option<Chart> {
                 }
             }
             if let Some(cat) = s.child("cat").or_else(|| s.child("xVal")) {
-                let cats = strings(cat);
+                let cats = strings(cat, &mut budget);
                 if matches!(kind, ChartType::Scatter | ChartType::Bubble) {
                     ser.x_values = cats.iter().map(|v| v.trim().parse::<f64>().ok()).collect();
                 }
@@ -227,10 +249,10 @@ pub(crate) fn chart_from(root: &El) -> Option<Chart> {
                 }
             }
             if let Some(v) = s.child("val").or_else(|| s.child("yVal")) {
-                ser.values = numbers(v);
+                ser.values = numbers(v, &mut budget);
             }
             if let Some(b) = s.child("bubbleSize") {
-                ser.sizes = numbers(b);
+                ser.sizes = numbers(b, &mut budget);
             }
             c.series.push(ser);
         }
@@ -310,6 +332,32 @@ fn text_size(e: Option<&El>) -> Option<f64> {
 mod tests {
     use super::*;
 
+    #[test]
+    fn sparse_caches_share_one_chart_wide_budget() {
+        // Every series declares 100000 points but writes one: without a chart-wide budget each
+        // would expand to the full count.
+        let ser = |n: usize| {
+            format!(
+                r#"<c:ser><c:idx val="{n}"/><c:val><c:numRef><c:numCache><c:ptCount val="100000"/><c:pt idx="0"><c:v>{n}</c:v></c:pt></c:numCache></c:numRef></c:val></c:ser>"#
+            )
+        };
+        let sers: String = (0..5).map(ser).collect();
+        let xml = format!(
+            r#"<c:chartSpace xmlns:c="http://schemas.openxmlformats.org/drawingml/2006/chart"><c:chart><c:plotArea><c:barChart><c:barDir val="col"/>{sers}</c:barChart></c:plotArea></c:chart></c:chartSpace>"#
+        );
+        let doc = crate::xml::parse(xml.as_bytes()).unwrap();
+        let c = chart_within(&doc.root, 250_000).unwrap();
+        let total: usize = c.series.iter().map(|s| s.values.len()).sum();
+        assert!(total <= 250_000, "{total}");
+        assert_eq!(c.series.len(), 5);
+        assert_eq!(c.series[0].values.len(), 100_000);
+        assert_eq!(c.series[0].values[0], Some(0.0));
+        assert!(c.series[4].values.is_empty(), "the budget ran out before the last series");
+        // A small chart is untouched by the default budget.
+        let c = chart_from(&doc.root).unwrap();
+        assert!(c.series.iter().all(|s| s.values.len() == 100_000));
+    }
+
     #[test]
     fn legend_position_and_overlay_are_read() {
         let legend = r#"<c:legend><c:legendPos val="r"/><c:overlay val="1"/></c:legend>"#;

crates/pptx/src/read/mod.rs

@@ -32,6 +32,8 @@ pub struct Imp<'a> {
     pub custom_show_names: HashMap<String, String>,
     /// GUIDs (upper case) of the table styles the file defines.
     pub table_style_ids: Vec<String>,
+    /// Per legacy VML drawing part: shape id → preview picture part (see `shapes::vml_preview`).
+    pub vml_previews: HashMap<String, HashMap<String, String>>,
 }
 
 impl Imp<'_> {
@@ -233,6 +235,7 @@ pub fn import(bytes: &[u8]) -> Result<Presentation, PptxError> {
         slide_by_part: HashMap::new(),
         custom_show_names: HashMap::new(),
         table_style_ids: Vec::new(),
+        vml_previews: HashMap::new(),
     };
     // Slide ids first so hyperlinks anywhere can point at slides.
     let mut slide_ids: Vec<(u32, String, SlideId)> = vec![];

crates/pptx/src/read/shapes.rs

@@ -367,7 +367,7 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S
         }
         if let Some(c) = gd.child("chart")
             && let Some(rel) = part.rels.get(c.attr("r:id").unwrap_or(""))
-            && let Some(bytes) = imp.pkg.get(&rel.target)
+            && let Some(bytes) = imp.pkg.xml_bytes(&rel.target)
             && let Some(chart) = super::read_chart_xml(bytes)
         {
             s.kind = ShapeKind::Chart(Box::new(chart));
@@ -403,17 +403,39 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S
 /// `v:imagedata` of the legacy VML drawing shape with that id, as PowerPoint 2007 wrote them.
 fn vml_preview(imp: &mut Imp, part: &Part, spid: &str) -> Option<deckcraft_model::MediaId> {
     for rel in part.rels.items.iter().filter(|r| r.kind == "vmlDrawing" && !r.external) {
-        let Some(doc) = imp.pkg.xml(&rel.target) else { continue };
-        let mut shapes = vec![];
-        doc.root.find_all("shape", &mut shapes);
-        let Some(img) = shapes.into_iter().find(|s| s.attr("id") == Some(spid)).and_then(|s| s.find("imagedata")) else { continue };
-        let Some(id) = img.attr("o:relid").or(img.attr("r:id")) else { continue };
-        let target = imp.pkg.rels(&rel.target).get(id).filter(|r| !r.external).map(|r| r.target.clone())?;
+        let key = crate::opc::norm(&rel.target);
+        if !imp.vml_previews.contains_key(&key) {
+            let index = vml_index(imp.pkg, &rel.target);
+            imp.vml_previews.insert(key.clone(), index);
+        }
+        let Some(target) = imp.vml_previews.get(&key).and_then(|m| m.get(spid)).cloned() else { continue };
         return imp.media_for(&target);
     }
     None
 }
 
+/// Every shape id in a VML drawing with a picture, mapped to that picture's part: parsed once per
+/// drawing however many objects look it up.
+fn vml_index(pkg: &crate::opc::Package, vml: &str) -> std::collections::HashMap<String, String> {
+    let mut out = std::collections::HashMap::new();
+    let Some(doc) = pkg.xml(vml) else { return out };
+    let rels = pkg.rels(vml);
+    let mut shapes = vec![];
+    doc.root.find_all("shape", &mut shapes);
+    for s in shapes {
+        let Some(id) = s.attr("id") else { continue };
+        if out.contains_key(id) {
+            continue; // the first shape with an id wins, as the old lookup did
+        }
+        let Some(img) = s.find("imagedata") else { continue };
+        let Some(rid) = img.attr("o:relid").or(img.attr("r:id")) else { continue };
+        if let Some(r) = rels.get(rid).filter(|r| !r.external) {
+            out.insert(id.to_string(), r.target.clone());
+        }
+    }
+    out
+}
+
 /// SmartArt: read the pre-drawn shapes of its drawing part as a group.
 fn smartart(imp: &mut Imp, part: &Part, gd: &El, frame: &Shape, depth: usize) -> Option<Shape> {
     let rel_ids = gd.child("relIds")?;

crates/pptx/tests/ole_vml.rs

@@ -27,6 +27,11 @@ fn png(v: u8) -> Vec<u8> {
 
 /// A new deck whose first slide holds [`FRAME`], with the VML drawing, its rels and two pictures.
 fn deck() -> Vec<u8> {
+    deck_with(FRAME)
+}
+
+/// [`deck`] with `frames` in place of [`FRAME`].
+fn deck_with(frames: &str) -> Vec<u8> {
     let bytes = deckcraft_pptx::export(&defaults::new_presentation(None)).expect("export");
     let mut z = zip::ZipArchive::new(Cursor::new(bytes)).expect("zip");
     let mut out = zip::ZipWriter::new(Cursor::new(Vec::new()));
@@ -39,7 +44,7 @@ fn deck() -> Vec<u8> {
         if name == "ppt/slides/slide1.xml" {
             let s = String::from_utf8(d).expect("utf8");
             let at = s.rfind("</p:spTree>").expect("spTree");
-            let s = format!("{}{FRAME}{}", &s[..at], &s[at..]);
+            let s = format!("{}{frames}{}", &s[..at], &s[at..]);
             let at = s.rfind("</p:cSld>").expect("cSld");
             d = format!("{}{CONTROLS}{}", &s[..at], &s[at..]).into_bytes();
         } else if name == "ppt/slides/_rels/slide1.xml.rels" {
@@ -82,3 +87,32 @@ fn activex_controls_show_their_pictures() {
     assert_eq!((x.x, x.y, x.w, x.h), (1.0, 2.0, 10.0, 5.0));
     assert!(!p.slides[0].shapes.iter().any(|s| s.name == "NoPicture"), "a control without a picture draws nothing");
 }
+
+#[test]
+fn many_objects_sharing_one_vml_drawing_each_get_their_preview() {
+    // 200 frames look up the same drawing (it is parsed once); every other one names a shape id
+    // the drawing doesn't have.
+    let frames: String = (0..200)
+        .map(|i| {
+            let spid = match i % 4 {
+                0 => "_x0000_s2050",
+                1 => "_x0000_s9999",
+                2 => "_x0000_s2049",
+                _ => "missing",
+            };
+            FRAME.replace("id=\"90\" name=\"Object 3\"", &format!("id=\"{}\" name=\"Obj {i}\"", 1000 + i)).replace("_x0000_s2050", spid)
+        })
+        .collect();
+    let p = deckcraft_pptx::import(&deck_with(&frames)).expect("import");
+    for i in 0..200 {
+        let s = p.slides[0].shapes.iter().find(|s| s.name == format!("Obj {i}")).expect("frame");
+        let ShapeKind::Opaque { preview, .. } = &s.kind else { panic!("{:?}", s.kind) };
+        let name = preview.and_then(|m| p.media.iter().find(|x| x.id == m)).map(|m| m.name.as_str());
+        let want = match i % 4 {
+            0 => Some("preview.png"),
+            2 => Some("other.png"),
+            _ => None,
+        };
+        assert_eq!(name, want, "frame {i}");
+    }
+}