From efa0c94d2bc9591b20be6a24f293ab9a89fddae7 Mon Sep 17 00:00:00 2001 From: vangel76 Date: Sun, 8 Nov 2026 03:24:05 +0100 Subject: [PATCH] aep container: review fixes (#3275) - read.rs: reserve with try_reserve_exact, so a raised chunk limit on a small target cannot abort before parsing; the capped reservation and a deep, retry-heavy nest of broken lists are pinned by tests. - write.rs: a tree past the 32-bit container size fails with TooLarge before anything is serialised. - aepx.rs: drop an unneeded local and lifetime annotation. --- crates/aep/src/container/aepx.rs | 7 ++-- crates/aep/src/container/read.rs | 64 +++++++++++++++++++++++++++++-- crates/aep/src/container/write.rs | 32 ++++++++++++---- 3 files changed, 87 insertions(+), 16 deletions(-) diff --git a/crates/aep/src/container/aepx.rs b/crates/aep/src/container/aepx.rs index db78797ef..21c30ae8b 100644 --- a/crates/aep/src/container/aepx.rs +++ b/crates/aep/src/container/aepx.rs @@ -122,7 +122,7 @@ struct Reader<'d> { trailer: Option>, } -impl<'d> Reader<'d> { +impl Reader<'_> { fn run(mut self) -> Result, AepError> { if self.data.starts_with(&[0xef, 0xbb, 0xbf]) { self.pos = 3; @@ -227,8 +227,7 @@ impl<'d> Reader<'d> { let at = self.pos; self.pos = self.pos.saturating_add(1); let name = self.name()?; - let data = self.data; - let mut attrs: Vec<((usize, usize), Cow<'d, [u8]>)> = Vec::new(); + let mut attrs = Vec::new(); let self_closing = loop { self.skip_ws(); let rest = self.rest(); @@ -256,7 +255,7 @@ impl<'d> Reader<'d> { } let vstart = self.pos.saturating_add(1); let vend = self.find(vstart, &[quote]).ok_or(xml(vstart, "an attribute value is not closed"))?; - let raw = data.get(vstart..vend).unwrap_or(&[]); + let raw = self.data.get(vstart..vend).unwrap_or(&[]); if raw.contains(&b'<') { return Err(xml(vstart, "`<` inside an attribute value")); } diff --git a/crates/aep/src/container/read.rs b/crates/aep/src/container/read.rs index 596404d15..8c43d9314 100644 --- a/crates/aep/src/container/read.rs +++ b/crates/aep/src/container/read.rs @@ -70,10 +70,10 @@ struct Parser<'a> { impl<'a> Parser<'a> { /// Parses the sibling chunks that exactly fill `start..end`. fn sequence(&mut self, start: usize, end: usize, depth: usize) -> Result>, AepError> { - // Sized once: a well-formed sequence fills exactly this many slots. The count is bounded by - // the input (one per 8 bytes at most) and by the chunks the limit still allows. - let capacity = self.count_siblings(start, end).min(self.limits.max_chunks.saturating_sub(self.count)); - let mut out = Vec::with_capacity(capacity); + // Sized once: a well-formed sequence fills exactly this many slots. `try_reserve` so a + // limit raised past what the target can allocate fails nothing here (the Vec then grows). + let mut out = Vec::new(); + let _ = out.try_reserve_exact(self.capacity_for(start, end)); let mut pos = start; while pos < end { let short = AepError::Malformed { offset: pos, what: "bytes left over that are too short for a chunk header" }; @@ -102,6 +102,12 @@ impl<'a> Parser<'a> { Ok(out) } + /// The slots to reserve for the sequence `start..end`: its header count, bounded by the input + /// (one per 8 bytes at most) and by the chunks the limit still allows. + fn capacity_for(&self, start: usize, end: usize) -> usize { + self.count_siblings(start, end).min(self.limits.max_chunks.saturating_sub(self.count)) + } + /// The number of chunk headers that tile `start..end` the way [`Self::sequence`] walks them, /// stopping at the first that does not fit. Never more than `(end - start) / 8`. fn count_siblings(&self, start: usize, end: usize) -> usize { @@ -182,6 +188,8 @@ mod tests { let mut vecs = vec![&parsed.chunks]; child_vecs(&parsed.chunks, &mut vecs); assert_eq!(vecs.len(), 11); + // `try_reserve_exact` gives exactly the requested capacity with std's allocator today; a + // Vec grown push by push would have rounded up to a power of two (37 -> 64, 9 -> 16). for v in vecs { assert_eq!(v.capacity(), v.len(), "a sibling Vec grew past its length"); } @@ -208,5 +216,53 @@ mod tests { // Under a tighter limit the parse fails as before, without sizing for more than allowed. let tight = Limits { max_depth: 64, max_chunks: 10 }; assert_eq!(parse_with_limits(&data, tight), Err(AepError::TooManyChunks)); + // The reservation never exceeds what the chunk limit still allows. + let mut parser = Parser { data: &data, limits: tight, count: 0 }; + assert_eq!(parser.capacity_for(12, data.len()), 10); + parser.count = 7; + assert_eq!(parser.capacity_for(12, data.len()), 3); + parser.count = 11; + assert_eq!(parser.capacity_for(12, data.len()), 0); + let parser = Parser { data: &data, limits: Limits::default(), count: 0 }; + assert_eq!(parser.capacity_for(12, data.len()), 1000); + } + + /// `LIST Fold` around `inner`, followed by 6 bytes too short for a header, so the list's + /// sequence fails and the list is kept opaque. + fn broken_level(inner: &[u8]) -> Vec { + let mut body = b"Fold".to_vec(); + body.extend_from_slice(inner); + body.extend_from_slice(&[0xee; 6]); + let mut out = b"LIST".to_vec(); + out.extend_from_slice(&u32::try_from(body.len()).expect("small").to_be_bytes()); + out.extend_from_slice(&body); + out + } + + #[test] + fn nested_lists_with_broken_tails_parse_each_header_once() { + // A wide, well-formed list at the bottom, then 50 levels that each fail and fall back to + // opaque: every level re-parses its child before giving up. + let mut wide = b"Fold".to_vec(); + wide.extend(std::iter::repeat_n(*b"abcd\0\0\0\0", 1000).flatten()); + let mut level = b"LIST".to_vec(); + level.extend_from_slice(&u32::try_from(wide.len()).expect("small").to_be_bytes()); + level.extend_from_slice(&wide); + for _ in 0..50 { + level = broken_level(&level); + } + let mut data = b"RIFX\0\0\0\0Egg!".to_vec(); + data.extend_from_slice(&level); + let declared = u32::try_from(data.len() - 8).expect("small"); + data[4..8].copy_from_slice(&declared.to_be_bytes()); + // 1 + 50 + 1000 headers in the file: the parse counts each exactly once (the count spans + // every attempt), so the work stays linear in the input. + let headers = 1 + 50 + 1000; + let f = parse_with_limits(&data, Limits { max_depth: 64, max_chunks: headers }).expect("parse"); + assert_eq!(parse_with_limits(&data, Limits { max_depth: 64, max_chunks: headers - 1 }), Err(AepError::TooManyChunks)); + // The outermost list keeps its bytes, and the file writes back unchanged. + let [top] = f.chunks.as_slice() else { panic!("one top-level chunk") }; + assert!(matches!(&top.body, Body::OpaqueList { list_type, data } if list_type == b"Fold" && data.len() + 12 == level.len())); + assert_eq!(crate::container::write(&f).expect("write"), data); } } diff --git a/crates/aep/src/container/write.rs b/crates/aep/src/container/write.rs index 38410c080..35a5b6276 100644 --- a/crates/aep/src/container/write.rs +++ b/crates/aep/src/container/write.rs @@ -17,14 +17,17 @@ const MAX_WRITE_DEPTH: usize = 256; /// [`AepError::TooDeep`] for an absurdly deep hand-built tree. pub fn write(file: &AepFile<'_>) -> Result, AepError> { let mut out = Vec::new(); - // Sized once to the exact output when the tree fits a container (a tree that does not fails - // below anyway). `try_reserve` so a hand-built tree that repeats one large borrowed body many - // times cannot abort on the allocation; the writer then grows as it goes, as before. - if let Some(len) = sequence_len(&file.chunks, 1) - && u32::try_from(len.saturating_add(4)).is_ok() - && let Some(total) = len.checked_add(12).and_then(|n| n.checked_add(file.trailer.len())) - { - let _ = out.try_reserve_exact(total); + // Sized once to the exact output. A tree past the 32-bit container size fails at once (the + // same error the write below would give after serialising it). `try_reserve` so a hand-built + // tree that repeats one large borrowed body many times cannot abort on the allocation; the + // writer then grows as it goes, as before. + if let Some(len) = sequence_len(&file.chunks, 1) { + if u32::try_from(len.saturating_add(4)).is_err() { + return Err(AepError::TooLarge); + } + if let Some(total) = len.checked_add(12).and_then(|n| n.checked_add(file.trailer.len())) { + let _ = out.try_reserve_exact(total); + } } out.extend_from_slice(b"RIFX"); out.extend_from_slice(&[0; 4]); @@ -143,10 +146,23 @@ mod tests { for file in &trees { let out = write(file).expect("write"); assert_eq!(sequence_len(&file.chunks, 1).map(|n| n + 12 + file.trailer.len()), Some(out.len())); + // Exact with std's allocator today; grown push by push it would be rounded up. assert_eq!(out.capacity(), out.len(), "the output grew past its size"); } } + #[cfg(target_pointer_width = "64")] + #[test] + fn a_tree_past_the_container_size_fails_before_serialising() { + // 4097 chunks borrowing one 1 MiB body: over 4 GiB of output, none of it written. + let body = vec![0u8; 1 << 20]; + let chunks: Vec> = (0..4097).map(|_| Chunk::data(*b"cdat", &body[..])).collect(); + let file = AepFile { chunks, trailer: Cow::Borrowed(&[]) }; + assert_eq!(write(&file), Err(AepError::TooLarge)); + // Just under the limit still sizes, and fails nothing up front. + assert!(sequence_len(&file.chunks[..4000], 1).is_some_and(|n| u32::try_from(n + 4).is_ok())); + } + #[test] fn a_tree_past_the_depth_limit_has_no_size() { let mut deep = Chunk::data(*b"cdat", vec![0]);