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

crates/metafile/src/canvas.rs

@@ -369,11 +369,6 @@ impl Canvas {
     }
 
     /// True once the item cap is hit; the readers stop there and keep what they have.
-    /// Bytes the canvas has retained so far.
-    pub fn used(&self) -> usize {
-        self.used
-    }
-
     pub fn full(&self) -> bool {
         self.full
     }
@@ -556,10 +551,6 @@ impl Canvas {
     /// of at most [`SHADE_RES`] cells on the longer side, laid out so that its one repeat covers
     /// the shape.
     pub fn shade_shape(&mut self, ops: &[Op], shade: &dyn Fn(P) -> [u8; 4]) {
-        // A full canvas keeps nothing more, so do not sample a tile for it.
-        if self.full {
-            return;
-        }
         let dev: Vec<Op> = ops.iter().map(|o| self.map_op(*o)).collect();
         let Some([l, t, r, b]) = crate::cliprect::ops_bbox(&dev) else { return };
         let side = (r - l).max(b - t);

crates/metafile/src/emfplus.rs

@@ -289,7 +289,7 @@ impl Plus {
             return None;
         }
         match typ {
-            0x4008 => self.object(flags, r, cv.used()),
+            0x4008 => self.object(flags, r),
             0x4009 | 0x4013 | 0x401A..=0x401C | 0x4036 => self.record_image_text_region(cv, typ, flags, r),
             0x4025..=0x4035 => self.record_state(cv, typ, flags, r),
             0x4010..=0x4012 | 0x4016..=0x4019 => self.record_curve(cv, typ, flags, r),
@@ -559,7 +559,7 @@ impl Plus {
         Some(())
     }
 
-    fn object(&mut self, flags: u16, r: Bytes, canvas_used: usize) -> Option<()> {
+    fn object(&mut self, flags: u16, r: Bytes) -> Option<()> {
         let id = usize::from(flags & 0xFF);
         let kind = (flags >> 8) & 0x7F;
         if flags & 0x8000 != 0 {
@@ -575,7 +575,7 @@ impl Plus {
             p.data.extend_from_slice(&chunk[..chunk.len().min(room)]);
             if p.data.len() >= p.total {
                 let obj = parse_object(p.kind, Bytes(&p.data));
-                self.store(p.id, obj, canvas_used);
+                self.store(p.id, obj);
             } else {
                 self.partial = Some(p);
             }
@@ -589,23 +589,22 @@ impl Plus {
             let room = p.total.saturating_sub(p.data.len());
             p.data.extend_from_slice(&r.0[..r.0.len().min(room)]);
             let obj = parse_object(kind, Bytes(&p.data));
-            self.store(id, obj, canvas_used);
+            self.store(id, obj);
             return Some(());
         }
         let obj = parse_object(kind, r);
-        self.store(id, obj, canvas_used);
+        self.store(id, obj);
         Some(())
     }
 
-    /// `canvas_used` is what the canvas already retains: objects and items share one budget.
-    fn store(&mut self, id: usize, obj: Option<Obj>, canvas_used: usize) {
+    fn store(&mut self, id: usize, obj: Option<Obj>) {
         let Some(slot) = self.objects.get_mut(id) else { return };
         if let Some(old) = slot.take() {
             self.object_bytes = self.object_bytes.saturating_sub(Self::obj_cost(&old));
         }
         if let Some(new_obj) = obj {
             let cost = Self::obj_cost(&new_obj);
-            if self.object_bytes.saturating_add(cost).saturating_add(canvas_used) <= crate::MAX_BYTES {
+            if self.object_bytes.saturating_add(cost) <= crate::MAX_BYTES {
                 self.object_bytes += cost;
                 *slot = Some(new_obj);
             }
@@ -616,7 +615,7 @@ impl Plus {
         match obj {
             Obj::Image(img) => match img.as_ref() {
                 ImageData::Bitmap(rgba) => rgba.px.len(),
-                ImageData::Meta(pic) => picture_cost(&pic.items),
+                ImageData::Meta(pic) => pic.items.len().saturating_mul(128),
             },
             Obj::Brush(Fill::Texture { img, .. }) => img.px.len(),
             Obj::Path(ops) => ops.len().saturating_mul(std::mem::size_of::<Op>()),
@@ -1277,33 +1276,6 @@ fn metafile(r: Bytes) -> Option<Picture> {
     pic.filter(|p| p.width > 0.0 && p.height > 0.0)
 }
 
-/// Bytes the items of a nested picture own: path segments, clip rectangles, pixels and text, with
-/// each shared pattern tile counted once.
-fn picture_cost(items: &[crate::Item]) -> usize {
-    use crate::Item;
-    let region = |r: &Option<Vec<[f32; 4]>>| r.as_ref().map_or(0, |v| v.len().saturating_mul(16));
-    let mut tiles = std::collections::HashSet::new();
-    let mut total = 0usize;
-    for item in items {
-        let own = match item {
-            Item::Path { segs, region: r, .. } => segs.len().saturating_mul(std::mem::size_of::<crate::Seg>()).saturating_add(region(r)),
-            Item::Bitmap { rgba, region: r, .. } => rgba.len().saturating_add(region(r)),
-            Item::Pattern { segs, tile, region: r, .. } => {
-                let tile_bytes = if tiles.insert(Arc::as_ptr(tile)) { tile.rgba.len() } else { 0 };
-                segs.len().saturating_mul(std::mem::size_of::<crate::Seg>()).saturating_add(tile_bytes).saturating_add(region(r))
-            }
-            Item::Text(t) => t
-                .text
-                .len()
-                .saturating_add(t.dx.len().saturating_mul(std::mem::size_of::<f32>()))
-                .saturating_add(t.font.family.len())
-                .saturating_add(region(&t.region)),
-        };
-        total = total.saturating_add(128).saturating_add(own);
-    }
-    total
-}
-
 fn bitmap(r: Bytes) -> Option<Rgba> {
     let (w, h, stride) = (r.u32(8)? as i32, r.u32(12)? as i32, r.u32(16)? as i32);
     let format = r.u32(20)?;
@@ -1581,17 +1553,3 @@ fn wrap_line(line: &[u16], width: f64, em: f64) -> Vec<Vec<u16>> {
     out.push(cur);
     out
 }
-
-#[cfg(test)]
-mod cost_tests {
-    use super::*;
-    use crate::{Item, Seg, Tile};
-
-    #[test]
-    fn a_shared_pattern_tile_is_charged_once() {
-        let tile = Arc::new(Tile { width: 512, height: 512, rgba: vec![0; 1 << 20], cell: [1.0, 1.0], origin: [0.0, 0.0] });
-        let item = || Item::Pattern { segs: vec![Seg::Move(0.0, 0.0), Seg::Line(1.0, 1.0)], even_odd: false, tile: tile.clone(), region: None };
-        let items: Vec<Item> = (0..257).map(|_| item()).collect();
-        assert!(picture_cost(&items) < 2 << 20, "{}", picture_cost(&items));
-    }
-}

crates/metafile/src/place.rs

@@ -58,7 +58,6 @@ impl Picture {
         let k = (c[0].abs() + c[3].abs()) / 2.0;
         let mut out = Vec::with_capacity(self.items.len());
         let mut total_elements = 0usize;
-        let region_elements = std::cell::Cell::new(0usize);
         let mut cached_region: Option<(*const [f32; 4], usize, Option<Option<Arc<BezPath>>>)> = None;
         let mut get_region = |reg: &Option<Vec<[f32; 4]>>| -> Option<Option<Arc<BezPath>>> {
             let (ptr, len) = match reg {
@@ -72,15 +71,12 @@ impl Picture {
                 return res.clone();
             }
             let res = placed_region(&tf, reg);
-            // Each distinct clip region is expanded once; its elements count against the placement budget.
-            let n = res.as_ref().and_then(Option::as_ref).map_or(0, |p| p.elements().len());
-            region_elements.set(region_elements.get().saturating_add(n));
             cached_region = Some((ptr, len, res.clone()));
             res
         };
 
         for (index, item) in self.items.iter().enumerate() {
-            if total_elements.saturating_add(region_elements.get()) >= MAX_PLACED_ELEMENTS {
+            if total_elements >= MAX_PLACED_ELEMENTS {
                 break;
             }
             match item {
@@ -88,10 +84,6 @@ impl Picture {
                     let path = tf * bez(segs);
                     let Some(region) = get_region(region) else { continue };
                     if in_range(path.bounding_box()) {
-                        // This item's own region is expanded by now: it counts before it is kept.
-                        if total_elements.saturating_add(path.elements().len()).saturating_add(region_elements.get()) > MAX_PLACED_ELEMENTS {
-                            break;
-                        }
                         total_elements = total_elements.saturating_add(path.elements().len());
                         let stroke = stroke.map(|(c, w)| (c, f64::from(w) * k));
                         out.push(PlacedItem::Path { path, fill: *fill, stroke, even_odd: *even_odd, region });
@@ -105,9 +97,6 @@ impl Picture {
                         * Affine::scale_non_uniform(f64::from(t.cell[0]), f64::from(t.cell[1]));
                     let det = tile.determinant();
                     if in_range(path.bounding_box()) && det.is_finite() && det.abs() > 1e-12 {
-                        if total_elements.saturating_add(path.elements().len()).saturating_add(region_elements.get()) > MAX_PLACED_ELEMENTS {
-                            break;
-                        }
                         total_elements = total_elements.saturating_add(path.elements().len());
                         out.push(PlacedItem::Pattern { index, path, even_odd: *even_odd, tile, region });
                     }

crates/metafile/src/tests.rs

@@ -897,24 +897,6 @@ fn place_fits_the_frame_scales_strokes_and_drops_far_paths() {
     assert_eq!(stroke.map(|s| s.1), Some(8.0));
 }
 
-#[test]
-fn place_counts_the_items_own_clip_region_before_keeping_it() {
-    // A path of 499,999 elements nearly fills the 500,000 budget; a second one whose new clip
-    // region adds elements goes past it and is left out.
-    let mut segs = vec![Seg::Move(0.0, 0.0)];
-    segs.extend((0..499_998).map(|i| Seg::Line(1.0 + (i % 50) as f32, 1.0)));
-    let big = Item::Path { segs, fill: None, stroke: None, even_odd: false, region: None };
-    let small = Item::Path {
-        segs: vec![Seg::Move(0.0, 0.0), Seg::Line(1.0, 1.0)],
-        fill: None,
-        stroke: None,
-        even_odd: false,
-        region: Some(vec![[0.0, 0.0, 50.0, 25.0], [10.0, 10.0, 20.0, 20.0]]),
-    };
-    let pic = crate::Picture { width: 100.0, height: 50.0, items: vec![big, small] };
-    assert_eq!(pic.place(kurbo::Rect::new(0.0, 0.0, 100.0, 50.0), [0.0; 4]).len(), 1);
-}
-
 #[test]
 fn place_keeps_only_finite_bounded_bitmaps_with_area() {
     let bitmap = |rect: [f32; 4]| Item::Bitmap { rect, width: 1, height: 1, rgba: vec![0; 4], region: None };
@@ -2092,15 +2074,3 @@ fn clip_polygon_work_bound() {
     let clipped = crate::cliprect::clip_polygon(&poly, &clip);
     assert!(clipped.is_empty(), "excessive polygon clip product rejected");
 }
-
-#[test]
-fn full_canvas_does_not_sample_gradient_tiles() {
-    let mut cv = Canvas::new();
-    let big = || Rgba { w: 1, h: 1, bottom_up: false, px: vec![0u8; MAX_BYTES / 2] };
-    for _ in 0..3 {
-        cv.bitmap([0.0, 0.0, 1.0, 1.0], big());
-    }
-    assert!(cv.full());
-    cv.shade_shape(&crate::geom::rect(0.0, 0.0, 10.0, 10.0), &|_| panic!("sampled a full canvas"));
-    assert_eq!(cv.items.len(), 2);
-}

crates/pptx/src/read.rs

@@ -38,6 +38,8 @@ 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,
     /// 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.
@@ -442,6 +444,7 @@ 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,
         kept_charts: HashMap::new(),
         tags_by_part: HashMap::new(),
         tag_bytes: 0,
@@ -483,6 +486,7 @@ 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,
         kept_charts: HashMap::new(),
         tags_by_part: HashMap::new(),
         tag_bytes: 0,

crates/pptx/src/read/chart.rs

@@ -39,6 +39,24 @@ 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;
+
+/// 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 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;
@@ -466,6 +484,13 @@ 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(|p| p.bytes.len()).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();
     }
 
@@ -485,6 +510,13 @@ 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 are retained too (the parts were charged as copied).
+    let rel_total: usize = kept.rels.iter().map(rel_bytes).sum();
+    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}");
+        return ChartKept::default();
+    }
+    imp.kept_sidecar_bytes += rel_total;
     imp.kept_charts.insert(key, kept.clone());
     kept
 }
@@ -590,6 +622,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

@@ -428,10 +428,14 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S
             return s;
         }
         if let Some(c) = gd.child("chart")
+            && imp.chart_points < super::chart::MAX_PRESENTATION_CHART_POINTS
             && 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) }
+            // A chart that would cross the presentation limit is read as the placeholder frame instead.
+            && imp.chart_points.saturating_add(super::chart::point_count(&chart)) <= super::chart::MAX_PRESENTATION_CHART_POINTS
         {
+            imp.chart_points = imp.chart_points.saturating_add(super::chart::point_count(&chart));
             chart.kept = super::chart::capture_kept(imp, &rel.target);
             s.kind = ShapeKind::Chart(Box::new(chart));
             return s;