From 9b1fe3ff23ca55ace1acd97a7376f5dcebfa6e22 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 06:28:38 -0500 Subject: [PATCH] PPTX: evaluate a custom geometry's guide lists once per coordinate space * PPTX: evaluate a custom geometry's guide lists once per coordinate space * address review: exempt guide-free geometries from the space cap * address review: budget guide-name bytes cached across coordinate spaces * Apply review fixes - crates/pptx/src/custgeom.rs * fix(pptx): warn when custom geometry paths are left out for the guide-cache limits --------- Co-authored-by: autocraft-bot[bot] <354215646+autocraft-bot[bot]@users.noreply.github.com> --- crates/pptx/src/custgeom.rs | 114 +++++++++++++++++++++++++++++++++--- 1 file changed, 105 insertions(+), 9 deletions(-) diff --git a/crates/pptx/src/custgeom.rs b/crates/pptx/src/custgeom.rs index 981cb2e24..383113847 100644 --- a/crates/pptx/src/custgeom.rs +++ b/crates/pptx/src/custgeom.rs @@ -15,6 +15,14 @@ use crate::xml::{A, El, W}; const DEG: f64 = 60_000.0; +/// Distinct path coordinate spaces whose guides are evaluated for one custom geometry; paths in +/// further spaces are left out. +const MAX_SPACES: usize = 16; + +/// Guide-name bytes all cached spaces may hold between them; the first space counts towards it but is always kept. +/// Every space copies the names, which the XML limits bound only once. +const MAX_SPACE_NAME_BYTES: usize = 4 << 20; + /// Guide variables for one shape. pub struct Guides { vars: HashMap, @@ -66,6 +74,10 @@ impl Guides { set("7cd8", 18_900_000.0); Guides { vars: v } } + /// Bytes of the guide names held. + fn name_bytes(&self) -> usize { + self.vars.keys().map(String::len).sum() + } pub fn get(&self, name: &str) -> f64 { let name = name.trim(); if let Some(v) = self.vars.get(name) { @@ -149,16 +161,51 @@ fn pt(g: &Guides, e: &El) -> (f64, f64) { /// Convert `a:custGeom` to custom paths. `w`/`h` is the shape size in EMU (used for guides and for /// paths that don't give their own coordinate space). pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec { + read_cust_geom_counting(cg, w_emu, h_emu).0 +} + +/// [`read_cust_geom`], and how many paths were left out for lack of room in the guide caches +/// (`(too many coordinate spaces, too many guide-name bytes)`); each cause is also logged once. +fn read_cust_geom_counting(cg: &El, w_emu: f64, h_emu: f64) -> (Vec, (usize, usize)) { let mut out = vec![]; - let Some(pl) = cg.child("pathLst") else { return out }; + let mut dropped = (0usize, 0usize); + let Some(pl) = cg.child("pathLst") else { return (out, dropped) }; + let mut spaces: HashMap<(u64, u64), Guides> = HashMap::new(); + let mut name_bytes = 0usize; + let mut budget_full = false; + // The cap on cached spaces saves guide evaluation: a geometry without guides has none to save. + let guided = ["avLst", "gdLst"].iter().any(|n| cg.child(n).is_some_and(|l| l.elements().next().is_some())); for p in pl.children_named("path").take(1000) { let pw = p.f64("w").unwrap_or(0.0); let ph = p.f64("h").unwrap_or(0.0); // Guides are evaluated in the path's own space when it has one. let (gw, gh) = if pw > 0.0 || ph > 0.0 { (pw, ph) } else { (w_emu, h_emu) }; - let mut g = Guides::new(gw, gh); - g.eval_list(cg.child("avLst")); - g.eval_list(cg.child("gdLst")); + // The guide lists are evaluated once per coordinate space, not once per path. + let key = (gw.to_bits(), gh.to_bits()); + if !spaces.contains_key(&key) { + if guided && (budget_full || spaces.len() >= MAX_SPACES) { + if budget_full { + dropped.1 += 1; + } else { + dropped.0 += 1; + } + continue; + } + let mut g = Guides::new(gw, gh); + g.eval_list(cg.child("avLst")); + g.eval_list(cg.child("gdLst")); + // The first space is always kept; later ones only while the cache stays within budget. + let bytes = g.name_bytes(); + if guided && !spaces.is_empty() && name_bytes.saturating_add(bytes) > MAX_SPACE_NAME_BYTES { + // Every space holds the same names, so no later space fits either. + budget_full = true; + dropped.1 += 1; + continue; + } + name_bytes = name_bytes.saturating_add(bytes); + spaces.insert(key, g); + } + let Some(g) = spaces.get(&key) else { continue }; let mut d = String::new(); let mut cur = (0.0, 0.0); let mut start = (0.0, 0.0); @@ -166,7 +213,7 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec { match c.local() { "moveTo" => { if let Some(q) = c.child("pt") { - cur = pt(&g, q); + cur = pt(g, q); start = cur; d.push_str("M "); num(&mut d, cur.0); @@ -177,7 +224,7 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec { } "lnTo" => { if let Some(q) = c.child("pt") { - cur = pt(&g, q); + cur = pt(g, q); d.push_str("L "); num(&mut d, cur.0); d.push(' '); @@ -186,7 +233,7 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec { } } "cubicBezTo" => { - let pts: Vec<(f64, f64)> = c.children_named("pt").map(|q| pt(&g, q)).collect(); + let pts: Vec<(f64, f64)> = c.children_named("pt").map(|q| pt(g, q)).collect(); if let [a, b, e] = pts.as_slice() { d.push_str("C "); for (x, y) in [a, b, e] { @@ -199,7 +246,7 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec { } } "quadBezTo" => { - let pts: Vec<(f64, f64)> = c.children_named("pt").map(|q| pt(&g, q)).collect(); + let pts: Vec<(f64, f64)> = c.children_named("pt").map(|q| pt(g, q)).collect(); if let [a, e] = pts.as_slice() { d.push_str("Q "); for (x, y) in [a, e] { @@ -236,7 +283,16 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec { let (ow, oh) = if pw > 0.0 || ph > 0.0 { (pw, ph) } else { (w_emu.max(1.0), h_emu.max(1.0)) }; out.push(CustomPath { w: ow, h: oh, d: d.trim_end().to_string(), fill, stroke: p.bool("stroke").unwrap_or(true) }); } - out + if dropped.0 > 0 { + log::warn!("pptx: a custom geometry uses more than {MAX_SPACES} distinct path coordinate spaces; {} later path(s) are not kept", dropped.0); + } + if dropped.1 > 0 { + log::warn!( + "pptx: a custom geometry's guide names pass {MAX_SPACE_NAME_BYTES} bytes across its coordinate spaces; {} later path(s) are not kept", + dropped.1 + ); + } + (out, dropped) } /// Ellipse parametric angle for a visual angle `a` (radians). @@ -470,6 +526,46 @@ mod tests { assert_eq!(paths[1].d, "M 1 1 Q 2 2 3 1"); } + #[test] + fn paths_share_guides_and_distinct_spaces_are_capped() { + let path = |w: usize| format!(r#""#); + let paths: String = (1..=40).map(|i| path(10 * i)).collect(); + let x = format!( + r#"{paths}{}"#, + path(10) + ); + let d = parse(x.as_bytes()).unwrap(); + let (out, dropped) = read_cust_geom_counting(&d.root, 1.0, 1.0); + assert_eq!(out.len(), MAX_SPACES + 1, "16 spaces plus the repeat of the first"); + assert_eq!(dropped, (40 - MAX_SPACES, 0), "the paths of later spaces are counted, to be warned about"); + assert!(out[0].d.starts_with("M 5 1"), "{}", out[0].d); + assert!(out[MAX_SPACES].d.starts_with("M 5 1"), "{}", out[MAX_SPACES].d); + } + + #[test] + fn guide_name_bytes_across_spaces_are_budgeted() { + let name = "n".repeat(3 << 20); + let path = |w: usize| format!(r#""#); + let x = format!( + r#"{}{}{}"#, + path(10), + path(20), + path(10) + ); + let d = parse(x.as_bytes()).unwrap(); + let (out, dropped) = read_cust_geom_counting(&d.root, 1.0, 1.0); + assert_eq!(out.len(), 2, "the second 3 MiB space is over budget and left out; the first space's repeat stays"); + assert_eq!(dropped, (0, 1), "the path left out is counted, to be warned about"); + } + + #[test] + fn guide_free_geometries_are_not_capped_by_space() { + let paths: String = (1..=40).map(|i| format!(r#""#, 10 * i)).collect(); + let x = format!(r#"{paths}"#); + let d = parse(x.as_bytes()).unwrap(); + assert_eq!(read_cust_geom(&d.root, 1.0, 1.0).len(), 40); + } + #[test] fn write_then_read_path() { let p = CustomPath { w: 100.0, h: 50.0, d: "M 0 0 L 100 0 C 100 10 90 50 50 50 Q 0 50 0 0 Z".into(), fill: FillMode::Norm, stroke: true };