PPTX: charge repeated chart decodes and sidecar copies to presentation-wide budgets (#1790)

* PPTX: charge repeated chart decodes and sidecar copies to presentation-wide budgets

* address review: count all chart vectors and relationship strings; reject a chart that crosses the presentation limit

* address review: charge chart part names/content types, roll back rejected capture

* address review: bound chart decode work across rejected and repeated references, cache refused sidecar captures

* address review: keep a chart past the presentation chart limits as its original part and closure, so a save writes it back

crates/model/src/chart.rs

@@ -392,6 +392,9 @@ pub struct Chart {
     /// Original chart XML part kept for round-trip of unmodelled options.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub raw: Option<String>,
+    /// The chart was too large to model: `raw` and `kept` are written back unchanged on save.
+    #[serde(skip_serializing_if = "std::ops::Not::not")]
+    pub verbatim: bool,
     /// Package parts the original chart still points at (embedded workbook, styles, drawings).
     /// The PPTX writer clears this before comparing modelled state. Native saves keep it too.
     #[serde(skip_serializing_if = "ChartKept::is_empty")]
@@ -485,6 +488,7 @@ impl Default for Chart {
             bin_count: None,
             bin_size: None,
             raw: None,
+            verbatim: false,
             kept: ChartKept::default(),
         }
     }
@@ -1311,6 +1315,7 @@ impl Chart {
             series: vec![],
             gap_width: 0.0,
             raw: None,
+            verbatim: false,
             subtotals: vec![],
             ..self.clone()
         };

crates/pptx/src/lib.rs

@@ -70,6 +70,13 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxE
     read::import_checked(bytes)
 }
 
+/// [`import`] with a lower presentation-wide chart point budget, so tests reach the over-budget
+/// path without a multi-million-point chart.
+#[doc(hidden)]
+pub fn import_with_chart_budget(bytes: &[u8], points: usize) -> Result<Presentation, PptxError> {
+    read::import_with_chart_points(bytes, points).map(|(p, _)| p)
+}
+
 /// Read the theme of a `.thmx` file, or of a `.pptx` / `.potx`'s first master, with its media.
 pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec<deckcraft_model::MediaItem>), PptxError> {
     read::import_theme(bytes)

crates/pptx/src/read.rs

@@ -38,8 +38,17 @@ pub struct Imp<'a> {
     pub vml_previews: HashMap<String, HashMap<String, String>>,
     /// Aggregate byte count of retained chart sidecars across the presentation.
     pub kept_sidecar_bytes: usize,
+    /// Data slots of the charts decoded so far (see `chart::MAX_PRESENTATION_CHART_POINTS`).
+    pub chart_points: usize,
+    /// Points the presentation may hold across modelled charts.
+    pub chart_points_cap: usize,
     /// Cached kept sidecars by normalized chart part name.
     pub kept_charts: HashMap<String, deckcraft_model::chart::ChartKept>,
+    /// Chart parts whose sidecar capture was refused, with the retained-byte total it started from:
+    /// the same start gives the same refusal, so the copy is not repeated.
+    pub rejected_kept: HashMap<String, usize>,
+    /// Bytes of chart parts decoded so far, rejected ones included (see `chart::MAX_CHART_DECODE_BYTES`).
+    pub chart_decode_bytes: usize,
     /// 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`]).
@@ -445,7 +454,11 @@ pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec<
         table_style_ids: Vec::new(),
         vml_previews: HashMap::new(),
         kept_sidecar_bytes: 0,
+        chart_points: 0,
+        chart_points_cap: chart::MAX_PRESENTATION_CHART_POINTS,
         kept_charts: HashMap::new(),
+        rejected_kept: HashMap::new(),
+        chart_decode_bytes: 0,
         tags_by_part: HashMap::new(),
         tag_bytes: 0,
         tags_capped: false,
@@ -463,6 +476,11 @@ pub fn import(bytes: &[u8]) -> Result<Presentation, PptxError> {
 /// [`import`], plus the parts of the slide list the file names but that couldn't be read (a
 /// missing or truncated slide): those slides are left out, so a caller can say so (#203).
 pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxError> {
+    import_with_chart_points(bytes, chart::MAX_PRESENTATION_CHART_POINTS)
+}
+
+/// [`import_checked`] with the presentation's chart point budget set (tests lower it).
+pub(crate) fn import_with_chart_points(bytes: &[u8], chart_points_cap: usize) -> Result<(Presentation, Vec<String>), PptxError> {
     let mut lost_slides: Vec<String> = vec![];
     let pkg = Package::open(bytes)?;
     let root_rels = pkg.rels("");
@@ -487,7 +505,11 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxE
         table_style_ids: Vec::new(),
         vml_previews: HashMap::new(),
         kept_sidecar_bytes: 0,
+        chart_points: 0,
+        chart_points_cap,
         kept_charts: HashMap::new(),
+        rejected_kept: HashMap::new(),
+        chart_decode_bytes: 0,
         tags_by_part: HashMap::new(),
         tag_bytes: 0,
         tags_capped: false,

crates/pptx/src/read/chart.rs

@@ -39,6 +39,33 @@ pub(crate) fn rich_text(t: &El) -> Option<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;
+/// Data slots (values and categories) the charts of one presentation may hold in all; a chart past
+/// it is read as a placeholder frame. Each frame that references a chart decodes its own copy.
+pub(crate) const MAX_PRESENTATION_CHART_POINTS: usize = 4_000_000;
+
+/// Chart part bytes all chart frames of a presentation may decode, counting every attempt: a
+/// chart that is rejected, or has no data slots, would otherwise be decoded again for each frame
+/// that shows it.
+pub(crate) const MAX_CHART_DECODE_BYTES: usize = 256 * 1024 * 1024;
+
+/// Slots a decoded chart holds, counting the chart itself as at least one.
+pub(crate) fn point_count(c: &Chart) -> usize {
+    let series = |s: &deckcraft_model::chart::Series| {
+        let bars = s.error_bars.as_ref().map_or(0, |e| e.plus.len().saturating_add(e.minus.len()));
+        s.values.len().saturating_add(s.x_values.len()).saturating_add(s.sizes.len()).saturating_add(bars)
+    };
+    let levels: usize = c.category_levels.iter().map(Vec::len).sum();
+    1usize.saturating_add(c.categories.len()).saturating_add(levels).saturating_add(c.series.iter().map(series).fold(0, usize::saturating_add))
+}
+
+/// Bytes of the strings a kept relationship owns.
+fn part_bytes(p: &deckcraft_model::chart::ChartPart) -> usize {
+    p.bytes.len() + p.name.len() + p.content_type.len()
+}
+
+fn rel_bytes(r: &deckcraft_model::chart::ChartRel) -> usize {
+    r.id.len() + r.rel_type.len() + r.target.len() + r.part.len()
+}
 
 /// Most slots one cache may hold.
 const MAX_CACHE_POINTS: usize = 100_000;
@@ -103,6 +130,48 @@ pub fn read_chart_xml(bytes: &[u8]) -> Option<Chart> {
     Some(c)
 }
 
+/// The chart a graphic frame points at. A chart that would cross the presentation's point or
+/// decode limits is not modelled: it is kept as its original part with the relationships it
+/// reaches (`verbatim`), so a save writes it back whole instead of dropping it and its workbook.
+pub(crate) fn frame_chart(imp: &mut super::Imp, part: &super::Part, rid: &str, uri: &str) -> Option<Chart> {
+    let rel = part.rels.get(rid)?;
+    let chartex = uri.ends_with("/chartex");
+    let bytes = imp.pkg.xml_bytes(&rel.target)?;
+    if imp.chart_decode_bytes.saturating_add(bytes.len()) > MAX_CHART_DECODE_BYTES {
+        log::warn!("pptx: chart {} is past the presentation's chart decode limit and is not kept", rel.target);
+        return None;
+    }
+    imp.chart_decode_bytes += bytes.len();
+    let parsed = if imp.chart_points < imp.chart_points_cap {
+        let c = if chartex { super::read_chartex_xml(bytes) } else { super::read_chart_xml(bytes) }?;
+        let n = point_count(&c);
+        if imp.chart_points.saturating_add(n) <= imp.chart_points_cap {
+            imp.chart_points = imp.chart_points.saturating_add(n);
+            let mut c = c;
+            c.kept = capture_kept(imp, &rel.target);
+            return Some(c);
+        }
+        Some(c)
+    } else {
+        None
+    };
+    // Past the limit: keep the part itself (charged to the sidecar budget) and show an empty chart.
+    if chartex || imp.kept_sidecar_bytes.saturating_add(bytes.len()) > MAX_KEPT_TOTAL_BYTES {
+        log::warn!("pptx: chart {} is past the presentation's chart limits and is not kept", rel.target);
+        return None;
+    }
+    imp.kept_sidecar_bytes += bytes.len();
+    log::warn!("pptx: chart {} is past the presentation's chart limits; shown empty and written back unchanged", rel.target);
+    let mut c = parsed.unwrap_or_default();
+    c.series.clear();
+    c.categories.clear();
+    c.category_levels.clear();
+    c.raw = Some(String::from_utf8_lossy(bytes).into_owned());
+    c.verbatim = true;
+    c.kept = capture_kept(imp, &rel.target);
+    Some(c)
+}
+
 pub(crate) fn chart_from(root: &El) -> Option<Chart> {
     chart_within(root, MAX_CHART_POINTS)
 }
@@ -466,9 +535,20 @@ pub(crate) fn capture_kept(imp: &mut super::Imp, chart_part: &str) -> deckcraft_
 
     let key = crate::opc::norm(chart_part);
     if let Some(cached) = imp.kept_charts.get(&key) {
+        // Every frame that shows this chart gets its own copy, so each copy is charged like the first.
+        let bytes: usize = cached.parts.iter().map(part_bytes).sum::<usize>() + cached.rels.iter().map(rel_bytes).sum::<usize>();
+        if imp.kept_sidecar_bytes.saturating_add(bytes) > MAX_KEPT_TOTAL_BYTES {
+            log::warn!("pptx: chart sidecar retained byte budget exceeded for {chart_part}");
+            return ChartKept::default();
+        }
+        imp.kept_sidecar_bytes += bytes;
         return cached.clone();
     }
 
+    let charged_before = imp.kept_sidecar_bytes;
+    if imp.rejected_kept.get(&key) == Some(&charged_before) {
+        return ChartKept::default();
+    }
     let mut kept = ChartKept::default();
     // The chart document itself is rewritten; don't store a second copy of it.
     let mut seen = std::collections::HashMap::from([(key.clone(), true)]);
@@ -485,6 +565,18 @@ pub(crate) fn capture_kept(imp: &mut super::Imp, chart_part: &str) -> deckcraft_
             kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, part: r.target, external: false });
         }
     }
+    // The first capture's relationship strings and the parts' names and content types are retained
+    // too (the part bytes were charged as copied).
+    let rel_total: usize =
+        kept.rels.iter().map(rel_bytes).sum::<usize>() + kept.parts.iter().map(|p| p.name.len() + p.content_type.len()).sum::<usize>();
+    if imp.kept_sidecar_bytes.saturating_add(rel_total) > MAX_KEPT_TOTAL_BYTES {
+        log::warn!("pptx: chart sidecar retained byte budget exceeded for {chart_part}");
+        // The dropped parts must not stay charged against later charts.
+        imp.kept_sidecar_bytes = charged_before;
+        imp.rejected_kept.insert(key, charged_before);
+        return ChartKept::default();
+    }
+    imp.kept_sidecar_bytes += rel_total;
     imp.kept_charts.insert(key, kept.clone());
     kept
 }
@@ -590,6 +682,16 @@ fn text_size(e: Option<&El>) -> Option<f64> {
 mod tests {
     use super::*;
 
+    #[test]
+    fn point_count_charges_every_retained_vector() {
+        let mut c = Chart { categories: vec!["a".into()], category_levels: vec![vec!["x".into(); 2], vec!["y".into(); 3]], ..Default::default() };
+        let mut s =
+            deckcraft_model::chart::Series { values: vec![Some(1.0); 4], x_values: vec![None; 5], sizes: vec![None; 6], ..Default::default() };
+        s.error_bars = Some(deckcraft_model::chart::ErrorBars { plus: vec![None; 7], minus: vec![None; 8], ..Default::default() });
+        c.series = vec![s];
+        assert_eq!(point_count(&c), 1 + 1 + 5 + 4 + 5 + 6 + 15);
+    }
+
     #[test]
     fn sparse_caches_share_one_chart_wide_budget() {
         // Every series declares 100000 points but writes one: without a chart-wide budget each

crates/pptx/src/read/shapes.rs

@@ -434,11 +434,8 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S
             return 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.xml_bytes(&rel.target)
-            && let Some(mut chart) = if uri.ends_with("/chartex") { super::read_chartex_xml(bytes) } else { super::read_chart_xml(bytes) }
+            && let Some(chart) = super::chart::frame_chart(imp, part, c.attr("r:id").unwrap_or(""), uri)
         {
-            chart.kept = super::chart::capture_kept(imp, &rel.target);
             s.kind = ShapeKind::Chart(Box::new(chart));
             return s;
         }

crates/pptx/src/write/chart.rs

@@ -610,6 +610,18 @@ fn kept_chartex(c: &Chart) -> Option<Vec<u8>> {
 
 /// The chart part XML, and whether it is the original part (so its relationships still apply).
 pub fn chart_xml(c: &Chart) -> (Vec<u8>, bool) {
+    // A chart too large to model is written back as it was read.
+    if c.verbatim
+        && let Some(raw) = &c.raw
+        && let Ok(doc) = crate::xml::parse(raw.as_bytes())
+        && doc.root.is("chartSpace")
+    {
+        let mut keep = doc.root;
+        drop_unkept_refs(&mut keep, c);
+        let mut w = W::new();
+        w.raw(&keep.to_xml());
+        return (w.finish(), true);
+    }
     // Reuse the kept part when the chart is unchanged since it was read.
     if let Some(raw) = &c.raw
         && !raw.contains(NS_CX)

crates/pptx/tests/it/chart_embed.rs

@@ -328,3 +328,30 @@ fn an_unchanged_chartex_keeps_its_embedded_workbook() {
     assert!(saved.iter().all(|(n, _)| n != "ppt/charts/_rels/chart1.xml.rels"), "the fallback chart has no workbook");
     assert!(!text_of(&saved, "ppt/charts/chart1.xml").contains("externalData"));
 }
+
+/// A chart past the presentation's point budget is not modelled, but a save writes the original
+/// chart part and its workbook back instead of dropping both.
+#[test]
+fn a_chart_past_the_point_budget_is_saved_whole() {
+    let src = with_workbook(&chart_pptx());
+    let p = deckcraft_pptx::import_with_chart_budget(&src, 0).expect("import");
+    let chart = p
+        .slides
+        .iter()
+        .flat_map(|s| s.shapes.iter())
+        .find_map(|s| match &s.kind {
+            ShapeKind::Chart(c) => Some(c.clone()),
+            _ => None,
+        })
+        .expect("the chart stays a chart");
+    assert!(chart.verbatim && chart.series.is_empty());
+    let saved = parts(&deckcraft_pptx::export(&p).expect("export"));
+    let workbook = saved.iter().find(|(n, _)| n == "ppt/embeddings/kept1.xlsx").map(|(_, d)| d.as_slice());
+    assert_eq!(workbook, Some(WORKBOOK));
+    let xml = saved
+        .iter()
+        .find(|(n, _)| n.starts_with("ppt/charts/chart") && n.ends_with(".xml"))
+        .map(|(_, d)| String::from_utf8_lossy(d).into_owned())
+        .expect("chart xml");
+    assert!(xml.contains("externalData") && xml.contains("c:pie"), "{xml}");
+}