diff --git a/crates/format/src/chart_parts_tests.rs b/crates/format/src/chart_parts_tests.rs index df74a183c..89b20e8b6 100644 --- a/crates/format/src/chart_parts_tests.rs +++ b/crates/format/src/chart_parts_tests.rs @@ -85,14 +85,14 @@ fn chart_parts_share_media_byte_and_item_budgets() { let mut p = fixture(vec![7; 600]); p.add_media("a.bin", "application/octet-stream", vec![8; 600]).unwrap(); let bytes = save(&p).unwrap(); - assert!(load_within(&bytes, Budget { total: 1200, items: 2 }).is_ok()); - assert!(load_within(&bytes, Budget { total: 1199, items: 2 }).is_err(), "chart payloads share the cumulative media budget"); - assert!(load_within(&bytes, Budget { total: 1200, items: 1 }).is_err(), "chart parts share the media item budget"); + assert!(load_within(&bytes, Budget::media(1200, 2)).is_ok()); + assert!(load_within(&bytes, Budget::media(1199, 2)).is_err(), "chart payloads share the cumulative media budget"); + assert!(load_within(&bytes, Budget::media(1200, 1)).is_err(), "chart parts share the media item budget"); let duplicated = rewrite_manifest(&bytes, |manifest| { let media = manifest["presentation"]["media"].as_array_mut().unwrap(); media.push(media[0].clone()); }); - assert!(load_within(&duplicated, Budget { total: 1200, items: 2 }).is_err(), "duplicate media IDs still count as listed items"); + assert!(load_within(&duplicated, Budget::media(1200, 2)).is_err(), "duplicate media IDs still count as listed items"); } #[test] @@ -104,9 +104,9 @@ fn legacy_inline_chart_bytes_are_budgeted_and_preserved() { part.remove("nativeEntry"); part.insert("bytes".into(), json!(vec![9_u8; 600])); }); - assert_eq!(load_within(&legacy, Budget { total: 1200, items: 2 }).unwrap().0, p); - assert!(load_within(&legacy, Budget { total: 1199, items: 2 }).is_err()); - assert!(load_within(&legacy, Budget { total: 1200, items: 1 }).is_err()); + assert_eq!(load_within(&legacy, Budget::media(1200, 2)).unwrap().0, p); + assert!(load_within(&legacy, Budget::media(1199, 2)).is_err()); + assert!(load_within(&legacy, Budget::media(1200, 1)).is_err()); } #[test] @@ -207,7 +207,7 @@ fn corrupt_chart_members_stop_before_reusing_the_inflation_budget() { } // A failed CRC may already have inflated bytes. Continuing with the same remaining // budget would let each corrupt member consume that work budget again. - let result = load_within(&bytes, Budget { total: 10, items: 2 }); + let result = load_within(&bytes, Budget::media(10, 2)); assert!(result.is_err(), "two corrupt 10-byte members must not reuse a 10-byte budget"); assert!(result.unwrap_err().to_string().contains("chart-parts/0")); } @@ -228,7 +228,7 @@ fn duplicate_physical_zip_names_are_indexed_and_inflated_once() { assert_eq!(replaced, 2); let z = zip::ZipArchive::new(Cursor::new(bytes.as_slice())).unwrap(); assert_eq!(z.file_names().filter(|n| n.starts_with("chart-parts/")).count(), 1); - let (q, damaged) = load_within(&bytes, Budget { total: 10, items: 2 }).unwrap(); + let (q, damaged) = load_within(&bytes, Budget::media(10, 2)).unwrap(); assert_eq!(damaged.len(), 1, "the second reference is missing, not inflated again"); let ShapeKind::Group { children, .. } = &q.slides[0].shapes[0].kind else { panic!("group") }; let ShapeKind::Chart(chart) = &children[0].kind else { panic!("chart") }; diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 7610398e6..eabe30345 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -29,12 +29,24 @@ const MAX_ENTRY: u64 = 2 << 30; const MAX_TOTAL_MEDIA: u64 = 4 << 30; /// Media items and chart sidecar parts one file may list. const MAX_MEDIA_ITEMS: usize = 100_000; +/// Largest `presentation.json` we will inflate. The manifest holds no media bytes, so it is far +/// smaller than a media entry may be; inflating and parsing one is what dominates memory. +const MAX_MANIFEST: u64 = 256 << 20; /// What one load may inflate. #[derive(Clone, Copy)] struct Budget { total: u64, items: usize, + manifest: u64, +} + +#[cfg(test)] +impl Budget { + /// A media budget with the default manifest limit. + fn media(total: u64, items: usize) -> Self { + Budget { total, items, manifest: MAX_MANIFEST } + } } #[derive(Debug, thiserror::Error)] @@ -107,6 +119,7 @@ pub fn sniff(bytes: &[u8]) -> bool { bytes.starts_with(b"PK") && [MIMETYPE, LEGACY_MIMETYPE].iter().any(|m| bytes.windows(m.len()).take(200).any(|w| w == m.as_bytes())) } +#[cfg(test)] fn read_entry(z: &mut zip::ZipArchive>, name: &str) -> Result> { read_entry_max(z, name, MAX_ENTRY) } @@ -114,13 +127,30 @@ fn read_entry(z: &mut zip::ZipArchive>, name: &str) -> Result>, name: &str, max: u64) -> Result> { + read_entry_counted(z, name, max, &mut 0) +} + +/// [`read_entry_max`] within `left` (and [`MAX_ENTRY`]), charging `left` for every byte inflated, +/// including those inflated before a decode or CRC error, so a member that fails late still +/// spends the shared budget it used. +fn read_entry_charged(z: &mut zip::ZipArchive>, name: &str, left: &mut u64) -> Result> { + let mut spent = 0; + let out = read_entry_counted(z, name, (*left).min(MAX_ENTRY), &mut spent); + *left = left.saturating_sub(spent); + out +} + +/// [`read_entry_max`], adding the bytes inflated to `spent` whether or not the read succeeds. +fn read_entry_counted(z: &mut zip::ZipArchive>, name: &str, max: u64, spent: &mut u64) -> Result> { let f = z.by_name(name).map_err(|e| FormatError::Damaged(format!("{name}: {e}")))?; if f.size() > max { return Err(FormatError::Damaged(format!("{name} is too large"))); } let mut out = Vec::with_capacity(f.size().min(64 << 20) as usize); let mut f = f.take(max.saturating_add(1)); - f.read_to_end(&mut out).map_err(|e| FormatError::Damaged(format!("{name}: {e}")))?; + let read = f.read_to_end(&mut out); + *spent = spent.saturating_add(out.len() as u64); + read.map_err(|e| FormatError::Damaged(format!("{name}: {e}")))?; if out.len() as u64 > max { return Err(FormatError::Damaged(format!("{name} is too large"))); } @@ -137,12 +167,15 @@ pub fn load(bytes: &[u8]) -> Result { /// A chart member that is present but fails decoding, CRC or its entry limit refuses the file. /// Linked media saved without its bytes isn't damage. pub fn load_checked(bytes: &[u8]) -> Result<(Presentation, Vec)> { - load_within(bytes, Budget { total: MAX_TOTAL_MEDIA, items: MAX_MEDIA_ITEMS }) + load_within(bytes, Budget { total: MAX_TOTAL_MEDIA, items: MAX_MEDIA_ITEMS, manifest: MAX_MANIFEST }) } fn load_within(bytes: &[u8], budget: Budget) -> Result<(Presentation, Vec)> { let mut z = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| FormatError::NotOurs(e.to_string()))?; - let json = read_entry(&mut z, "presentation.json").map_err(|_| FormatError::NotOurs("no presentation.json".into()))?; + if z.index_for_name("presentation.json").is_none() { + return Err(FormatError::NotOurs("no presentation.json".into())); + } + let json = read_entry_max(&mut z, "presentation.json", budget.manifest)?; let m = parse_manifest(&json)?; if m.format != "deckcraft" { return Err(FormatError::NotOurs(m.format)); @@ -164,11 +197,9 @@ fn load_within(bytes: &[u8], budget: Budget) -> Result<(Presentation, Vec left) { return Err(FormatError::Damaged("embedded media is larger than DeckCraft can open".into())); } - match read_entry_max(&mut z, &name, left.min(MAX_ENTRY)) { - Ok(data) => { - left -= data.len() as u64; - item.data = Arc::new(data); - } + // A failed read is charged for what it inflated, so later members can't reuse that budget. + match read_entry_charged(&mut z, &name, &mut left) { + Ok(data) => item.data = Arc::new(data), Err(_) if linked_only => {} Err(e) => damaged.push(e.to_string()), } @@ -188,9 +219,11 @@ pub fn repair(p: &mut Presentation) { } p.fix_next_id(); let first_layout = p.masters.first().and_then(|m| m.layouts.first()).map(|l| l.id).unwrap_or_default(); + // Indexed once: a per-slide scan of every layout is slides × layouts work. + let layouts: std::collections::HashSet<_> = p.masters.iter().flat_map(|m| m.layouts.iter().map(|l| l.id)).collect(); let mut seen = std::collections::HashSet::new(); for i in 0..p.slides.len() { - let needs_layout = p.slides.get(i).is_some_and(|s| p.layout(s.layout).is_none()); + let needs_layout = p.slides.get(i).is_some_and(|s| !layouts.contains(&s.layout)); let dup = p.slides.get(i).is_some_and(|s| !seen.insert(s.id)); if needs_layout || dup { // An exhausted id space leaves a duplicate slide as it is: the engine refuses to @@ -331,16 +364,20 @@ fn failure(src: &[u8]) -> std::result::Result Result { - let mut src = json.to_vec(); - let mut last = match failure(&src) { + // Parse the inflated text as it is: only a file that fails on a `null` number is copied for + // patching (the copy is bounded by `MAX_MANIFEST`). + let mut last = match failure(json) { Ok(m) => return Ok(m), Err(f) => f, }; + if !is_null_number(&last.0) { + return Err(FormatError::Damaged(last.0)); + } + let mut src = json.to_vec(); let mut budget = MAX_REPARSE_BYTES; 'fixes: for _ in 0..MAX_NULL_FIXES { let (msg, pos) = &last; - let number = msg.starts_with("invalid type: null, expected f") || msg.starts_with("invalid type: null, expected a number"); - if !number { + if !is_null_number(msg) { break; } let end = offset(&src, pos.0, pos.1).unwrap_or(src.len()); @@ -374,6 +411,11 @@ fn parse_manifest(json: &[u8]) -> Result { Err(FormatError::Damaged(last.0)) } +/// Is this parse error a `null` where a number belongs (a NaN or infinity serde_json wrote)? +fn is_null_number(msg: &str) -> bool { + msg.starts_with("invalid type: null, expected f") || msg.starts_with("invalid type: null, expected a number") +} + /// Byte offset of a 1-based `line`:`column`. fn offset(src: &[u8], line: usize, column: usize) -> Option { let mut start = 0; diff --git a/crates/format/src/tests.rs b/crates/format/src/tests.rs index dcd156367..46c7f7e96 100644 --- a/crates/format/src/tests.rs +++ b/crates/format/src/tests.rs @@ -93,9 +93,60 @@ fn media_budget_is_cumulative() { p.add_media("b.bin", "application/octet-stream", vec![2; 600]); let bytes = save(&p).unwrap(); // Exactly at the limit opens; one byte under is refused, though each item fits. - assert!(load_within(&bytes, Budget { total: 1200, items: 2 }).is_ok()); - assert!(load_within(&bytes, Budget { total: 1199, items: 2 }).is_err()); - assert!(load_within(&bytes, Budget { total: 1200, items: 1 }).is_err()); + assert!(load_within(&bytes, Budget::media(1200, 2)).is_ok()); + assert!(load_within(&bytes, Budget::media(1199, 2)).is_err()); + assert!(load_within(&bytes, Budget::media(1200, 1)).is_err()); +} + +#[test] +fn corrupt_media_is_charged_for_what_it_inflated() { + let mut p = Presentation::default(); + p.add_media("a.png", "image/png", b"PICTURE-AAAAAAAAAAAA".to_vec()); + p.add_media("b.png", "image/png", b"PICTURE-BBBBBBBBBBBB".to_vec()); + let mut bytes = save(&p).unwrap(); + // Corrupt both stored members, so each fails its CRC only after inflating all its bytes. + for tag in [b"PICTURE-AAAA", b"PICTURE-BBBB"] { + let at = bytes.windows(tag.len()).position(|w| w == tag).unwrap(); + bytes[at] ^= 0xff; + } + // Each member fits the budget on its own; the second must not reuse what the first spent. + let result = load_within(&bytes, Budget::media(20, 2)); + assert!(result.is_err(), "two corrupt 20-byte members must not share a 20-byte budget"); + // With room for both, the file opens and reports both. + let (_, damaged) = load_within(&bytes, Budget::media(40, 2)).unwrap(); + assert_eq!(damaged.len(), 2, "{damaged:?}"); +} + +#[test] +fn oversized_manifest_is_refused_before_inflating() { + let bytes = save(&Presentation::default()).unwrap(); + let mut z = zip::ZipArchive::new(Cursor::new(bytes.as_slice())).unwrap(); + let size = z.by_name("presentation.json").unwrap().size(); + assert!(load_within(&bytes, Budget { total: 0, items: 0, manifest: size }).is_ok()); + let err = load_within(&bytes, Budget { total: 0, items: 0, manifest: size - 1 }).unwrap_err(); + assert!(err.to_string().contains("too large"), "{err}"); +} + +#[test] +fn many_slides_on_missing_layouts_are_repaired() { + let mut p = Presentation::default(); + let template = p.slides[0].clone(); + let good = template.layout; + p.slides = (0..2000u32) + .map(|i| { + let mut s = (*template).clone(); + s.id = deckcraft_model::SlideId(10_000 + i); + if i % 2 == 1 { + s.layout = deckcraft_model::LayoutId(900_000 + i); + } + Arc::new(s) + }) + .collect(); + p.next_id = 20_000; + repair(&mut p); + assert!(p.validate().is_empty(), "{:?}", p.validate().first()); + assert!(p.slides.iter().all(|s| p.layout(s.layout).is_some())); + assert_eq!(p.slides[0].layout, good); } #[test] @@ -124,7 +175,7 @@ fn repeated_media_ids_are_read_once() { } let bytes = out.finish().unwrap().into_inner(); // 51 references to one 1000-byte member fit a 1000-byte budget: it's inflated once. - let q = load_within(&bytes, Budget { total: 1000, items: 51 }).unwrap().0; + let q = load_within(&bytes, Budget::media(1000, 51)).unwrap().0; assert_eq!(q.media.len(), 1); assert_eq!(q.media[0].data.len(), 1000); // And the deck saves again. diff --git a/crates/model/src/lib.rs b/crates/model/src/lib.rs index 1289e47ab..8547bc60c 100644 --- a/crates/model/src/lib.rs +++ b/crates/model/src/lib.rs @@ -1112,11 +1112,13 @@ impl Presentation { pub fn validate(&self) -> Vec { let mut out = vec![]; let mut seen = std::collections::HashSet::new(); + // Indexed once: a per-slide scan of every layout is slides × layouts work. + let layouts: std::collections::HashSet<_> = self.masters.iter().flat_map(|m| m.layouts.iter().map(|l| l.id)).collect(); for s in &self.slides { if !seen.insert(s.id) { out.push(format!("duplicate slide id {}", s.id)); } - if self.layout(s.layout).is_none() { + if !layouts.contains(&s.layout) { out.push(format!("slide {} uses missing layout {}", s.id, s.layout)); } let mut ids = std::collections::HashSet::new();