From d2066a1f0779d6c4a15c2c67300839f33bdbbef4 Mon Sep 17 00:00:00 2001 From: Nikita Rakitin Date: Tue, 13 Oct 2026 20:53:49 +0300 Subject: [PATCH] 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 --- crates/pptx/src/opc.rs | 63 +++++++++++++++++++++++++-- crates/pptx/src/read/chart.rs | 78 +++++++++++++++++++++++++++------- crates/pptx/src/read/mod.rs | 3 ++ crates/pptx/src/read/shapes.rs | 36 +++++++++++++--- crates/pptx/tests/ole_vml.rs | 36 +++++++++++++++- 5 files changed, 190 insertions(+), 26 deletions(-) diff --git a/crates/pptx/src/opc.rs b/crates/pptx/src/opc.rs index 4ef21c3d..1c246181 100644 --- a/crates/pptx/src/opc.rs +++ b/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::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 { 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 { let b = self.get(name)?; @@ -352,6 +376,39 @@ impl PackageOut { mod tests { use super::*; + fn zip_of(entries: &[(&str, usize)]) -> Vec { + 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"); diff --git a/crates/pptx/src/read/chart.rs b/crates/pptx/src/read/chart.rs index 6d7541ff..ae68f3e9 100644 --- a/crates/pptx/src/read/chart.rs +++ b/crates/pptx/src/read/chart.rs @@ -35,19 +35,34 @@ fn rich_text(t: &El) -> Option { 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 { +/// 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 { + 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 { 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 { 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> { - strings(e).into_iter().map(|s| s.trim().parse::().ok().filter(|v| v.is_finite())).collect() +fn numbers(e: &El, budget: &mut usize) -> Vec> { + strings(e, budget).into_iter().map(|s| s.trim().parse::().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 { } pub(crate) fn chart_from(root: &El) -> Option { + 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 { 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 { 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 { } } 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::().ok()).collect(); } @@ -227,10 +249,10 @@ pub(crate) fn chart_from(root: &El) -> Option { } } 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 { 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#"{n}"# + ) + }; + let sers: String = (0..5).map(ser).collect(); + let xml = format!( + r#"{sers}"# + ); + 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#""#; diff --git a/crates/pptx/src/read/mod.rs b/crates/pptx/src/read/mod.rs index 057614eb..45c6af52 100644 --- a/crates/pptx/src/read/mod.rs +++ b/crates/pptx/src/read/mod.rs @@ -32,6 +32,8 @@ pub struct Imp<'a> { pub custom_show_names: HashMap, /// GUIDs (upper case) of the table styles the file defines. pub table_style_ids: Vec, + /// Per legacy VML drawing part: shape id → preview picture part (see `shapes::vml_preview`). + pub vml_previews: HashMap>, } impl Imp<'_> { @@ -233,6 +235,7 @@ pub fn import(bytes: &[u8]) -> Result { 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![]; diff --git a/crates/pptx/src/read/shapes.rs b/crates/pptx/src/read/shapes.rs index c4b3f77f..2281cd78 100644 --- a/crates/pptx/src/read/shapes.rs +++ b/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 { 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 { + 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 { let rel_ids = gd.child("relIds")?; diff --git a/crates/pptx/tests/ole_vml.rs b/crates/pptx/tests/ole_vml.rs index 8f6c007b..ccdc803a 100644 --- a/crates/pptx/tests/ole_vml.rs +++ b/crates/pptx/tests/ole_vml.rs @@ -27,6 +27,11 @@ fn png(v: u8) -> Vec { /// A new deck whose first slide holds [`FRAME`], with the VML drawing, its rels and two pictures. fn deck() -> Vec { + deck_with(FRAME) +} + +/// [`deck`] with `frames` in place of [`FRAME`]. +fn deck_with(frames: &str) -> Vec { 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 { if name == "ppt/slides/slide1.xml" { let s = String::from_utf8(d).expect("utf8"); let at = s.rfind("").expect("spTree"); - let s = format!("{}{FRAME}{}", &s[..at], &s[at..]); + let s = format!("{}{frames}{}", &s[..at], &s[at..]); let at = s.rfind("").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}"); + } +}