diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 2c8fba185..e1b50ebb5 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -40,6 +40,7 @@ mod unpack; use std::borrow::Cow; use std::collections::{BTreeMap, HashSet}; use std::io::{Read as _, Write}; +use std::sync::Arc; use serde::{Deserialize, Serialize}; use vectorcraft_doc::{Document, ImageBlob}; @@ -534,15 +535,20 @@ pub fn load_file(bytes: &[u8]) -> Result { } // The bytes after the JSON, read in one pass: the images' first, then the profiles'. let entries: Vec<[u64; 2]> = f.images.values().chain(f.profiles.values()).filter_map(|i| i.blob).collect(); - let mut tail = file.entries(&entries)?.into_iter(); + let tail = file.entries(&entries)?; + // Entries naming the same bytes share them; a profile's own copy of shared bytes counts + // against the data's size, so copies never add up to more than the file has (#3574). + let mut distinct = HashSet::new(); + let mut spare: u64 = tail.iter().flatten().filter(|b| distinct.insert(Arc::as_ptr(b))).map(|b| b.len() as u64).sum(); + let mut tail = tail.into_iter(); let mut bytes_of = |img: &Image| match img.blob { Some(_) => tail.next().flatten(), - None => img.inline(), + None => img.inline().map(Arc::new), }; let mut doc = f.document; for (k, img) in f.images { let bytes = bytes_of(&img).ok_or_else(|| FormatError::BadImage(k.clone()))?; - let mut blob = ImageBlob::new(img.mime, bytes); + let mut blob = ImageBlob { mime: img.mime, bytes, proxy: None }; // Until the linked file is read, the preview stands in for it. if img.proxy { blob.proxy = Some(blob.bytes.clone()); @@ -575,7 +581,18 @@ pub fn load_file(bytes: &[u8]) -> Result { // labels and the character styles, within the ranges `document.setup` takes (after the depth // limit: the walk recurses). doc.keep_scripts_in_range(); - let profiles = f.profiles.into_iter().filter_map(|(name, icc)| Some((name, bytes_of(&icc)?))).collect(); + let profiles = f + .profiles + .into_iter() + .filter_map(|(name, icc)| Some((name, bytes_of(&icc)?))) + .map(|(name, icc)| match Arc::try_unwrap(icc) { + Ok(icc) => Ok((name, icc)), + Err(icc) => { + spare = spare.checked_sub(icc.len() as u64).ok_or(FormatError::TooBig)?; + Ok((name, icc.to_vec())) + } + }) + .collect::>()?; Ok(NativeFile { doc, depth_notes, info: FileInfo { version: f.version, legacy: f.format == LEGACY_EXTENSION }, profiles }) } @@ -593,7 +610,7 @@ fn head_member(bytes: &[u8], pick: impl FnOnce(Head) -> Option) -> Option let head: Head = serde_json::from_slice(file.json()).ok()?; let img = pick(head)?; match img.blob { - Some(at) => file.entries(&[at]).ok()?.pop().flatten(), + Some(at) => file.entries(&[at]).ok()?.pop().flatten().map(Arc::unwrap_or_clone), None => img.inline(), } } diff --git a/crates/format/src/unpack.rs b/crates/format/src/unpack.rs index 6e9af01d4..9aa3c81b2 100644 --- a/crates/format/src/unpack.rs +++ b/crates/format/src/unpack.rs @@ -3,7 +3,9 @@ //! the file is never held unpacked as a whole. Limits apply per part ([`MAX_JSON`], [`MAX_BLOB`], //! [`MAX_BLOBS`]), guarding against decompression bombs. +use std::collections::HashMap; use std::io::{self, Read}; +use std::sync::Arc; use flate2::read::GzDecoder; @@ -18,6 +20,9 @@ fn damaged(e: impl std::fmt::Display) -> FormatError { FormatError::NotVectorcraft(format!("damaged compressed data: {e}")) } +/// An entry's bytes (`None`: the data ends before it), shared by the entries naming them. +pub(crate) type Entry = Option>>; + /// A file opened for reading: its JSON, and where the data after it is. pub(crate) struct Unpacked<'a> { json: Json<'a>, @@ -116,27 +121,43 @@ impl<'a> Unpacked<'a> { } /// The entries `[offset, length]` of the data after the JSON → each one's bytes, `None` for - /// one that isn't there (the data ends before it). Errors: the compressed data is damaged, or - /// the entries of a compressed file are larger than [`MAX_BLOB`] each or [`MAX_BLOBS`] - /// together, or those of an uncompressed file larger together than both [`MAX_BLOBS`] and the - /// data (entries sharing bytes, only in hand-made files, would copy them once each). Entries - /// are read in the order of their offsets, each straight into its own buffer. - pub(crate) fn entries(self, wanted: &[[u64; 2]]) -> Result>>, FormatError> { + /// one that isn't there (the data ends before it). Entries naming the same bytes (only in + /// hand-made files) share one buffer. Errors: the compressed data is damaged, or the entries + /// of a compressed file are larger than [`MAX_BLOB`] each or [`MAX_BLOBS`] together, or those + /// of an uncompressed file larger together than both [`MAX_BLOBS`] and the data, or the + /// distinct entries found larger together than the data they are in (overlapping entries + /// would copy the same bytes once each, #3574). Entries are read in the order of their + /// offsets, each straight into its own buffer. + pub(crate) fn entries(self, wanted: &[[u64; 2]]) -> Result, FormatError> { self.entries_within(wanted, MAX_WORK) } /// [`Self::entries`], unpacking at most `work` bytes of a streamed file to find them. - fn entries_within(self, wanted: &[[u64; 2]], work: u64) -> Result>>, FormatError> { - let mut out: Vec>> = vec![None; wanted.len()]; - let in_memory = |tail: &[u8], out: &mut Vec>>| { - for (slot, &[offset, len]) in out.iter_mut().zip(wanted) { - *slot = range(offset, len).and_then(|(a, b)| tail.get(a..b)).map(<[u8]>::to_vec); + fn entries_within(self, wanted: &[[u64; 2]], work: u64) -> Result, FormatError> { + let mut out: Vec = vec![None; wanted.len()]; + // The entries read so far, by `[offset, length]`, and the bytes copied for them. + let mut done: HashMap<[u64; 2], Entry> = HashMap::new(); + let mut copied = 0u64; + let mut in_memory = |tail: &[u8], out: &mut Vec| { + for (slot, &w) in out.iter_mut().zip(wanted) { + if let Some(e) = done.get(&w) { + *slot = e.clone(); + continue; + } + let bytes = range(w[0], w[1]).and_then(|(a, b)| tail.get(a..b)); + copied = copied.saturating_add(bytes.map_or(0, |b| b.len() as u64)); + if copied > tail.len() as u64 { + return Err(FormatError::TooBig); + } + *slot = bytes.map(|b| Arc::new(b.to_vec())); + done.insert(w, slot.clone()); } + Ok(()) }; match self.tail { Tail::Slice(tail) => { within(wanted, u64::MAX, MAX_BLOBS.max(tail.len() as u64))?; - in_memory(tail, &mut out); + in_memory(tail, &mut out)?; } Tail::Whole(at) => { within(wanted, MAX_BLOB, MAX_BLOBS)?; @@ -144,20 +165,31 @@ impl<'a> Unpacked<'a> { Json::Whole { buf, .. } => buf.as_slice(), _ => &[], }; - in_memory(buf.get(at..).unwrap_or_default(), &mut out); + in_memory(buf.get(at..).unwrap_or_default(), &mut out)?; } - Tail::None => in_memory(&[], &mut out), + Tail::None => in_memory(&[], &mut out)?, Tail::Stream { file, at, read, rest } => { within(wanted, MAX_BLOB, MAX_BLOBS)?; drop(self.json); let mut order: Vec = (0..wanted.len()).collect(); order.sort_by_key(|&i| wanted.get(i).map_or(0, |w| w[0])); let mut s = Stream { r: Box::new(io::Cursor::new(read).chain(rest)), pos: 0, file, at, left: work }; + // How far into the data the stream has been: the bytes copied stay within it. + let mut seen = 0u64; for i in order { - let Some(&[offset, len]) = wanted.get(i) else { continue }; - if let Some(slot) = out.get_mut(i) { - *slot = s.entry(offset, len)?; + let Some(&w) = wanted.get(i) else { continue }; + let Some(slot) = out.get_mut(i) else { continue }; + if let Some(e) = done.get(&w) { + *slot = e.clone(); + continue; } + *slot = s.entry(w[0], w[1])?.map(Arc::new); + copied = copied.saturating_add(slot.as_ref().map_or(0, |e| e.len() as u64)); + seen = seen.max(s.pos); + if copied > seen { + return Err(FormatError::TooBig); + } + done.insert(w, slot.clone()); } // To the end, so the stream's checksum is verified as before. s.left = MAX_BLOBS; @@ -278,17 +310,17 @@ mod tests { }; // Entries one after the other: the tail is unpacked once. let parts = read(&[[0, 1000], [1000, 3096]], 4096).unwrap(); - assert_eq!(parts, vec![Some(tail[..1000].to_vec()), Some(tail[1000..].to_vec())]); + assert_eq!(parts, vec![Some(Arc::new(tail[..1000].to_vec())), Some(Arc::new(tail[1000..].to_vec()))]); // One byte at the far end is the whole tail's work. - assert_eq!(read(&[[4095, 1]], 4096).unwrap(), vec![Some(vec![tail[4095]])]); + assert_eq!(read(&[[4095, 1]], 4096).unwrap(), vec![Some(Arc::new(vec![tail[4095]]))]); assert!(matches!(read(&[[4095, 1]], 4000), Err(FormatError::TooBig))); // Entries over the same bytes start the stream again each time: counted, then refused. - let same = vec![[4000, 96]; 50]; + let same: Vec<[u64; 2]> = (0..50).map(|i| [4000, 1 + i]).collect(); assert!(read(&same, 50 * (4096 + 64)).is_ok()); assert!(matches!(read(&same, 3 * 4096), Err(FormatError::TooBig))); // An offset past the data is still just missing, and the default budget reads all of it. assert_eq!(read(&[[1 << 40, 1]], 1 << 20).unwrap(), vec![None]); - assert_eq!(Unpacked::new(&file).unwrap().entries(&[[0, 4096]]).unwrap(), vec![Some(tail.clone())]); + assert_eq!(Unpacked::new(&file).unwrap().entries(&[[0, 4096]]).unwrap(), vec![Some(Arc::new(tail.clone()))]); } #[test] @@ -303,8 +335,61 @@ mod tests { assert!(matches!(Unpacked::new(&whole).unwrap().entries(&[[0, MAX_BLOB + 1]]), Err(FormatError::TooBig))); // Uncompressed files keep no per-entry limit: a length the data doesn't have is just missing. assert_eq!(Unpacked::new(b"{}").unwrap().entries(&[[0, MAX_BLOB + 1]]).unwrap(), vec![None]); - // Within the limits, shared bytes are still read once per entry. + // Within the limits, entries naming the same bytes share them. let file = b"{}\n\0VCBLOBS\0abcd"; - assert_eq!(Unpacked::new(file).unwrap().entries(&[[0, 4], [0, 4]]).unwrap(), vec![Some(b"abcd".to_vec()), Some(b"abcd".to_vec())]); + let shared = Unpacked::new(file).unwrap().entries(&[[0, 4], [0, 4]]).unwrap(); + assert_eq!(shared, vec![Some(Arc::new(b"abcd".to_vec())), Some(Arc::new(b"abcd".to_vec()))]); + assert!(Arc::ptr_eq(shared[0].as_ref().unwrap(), shared[1].as_ref().unwrap())); + } + + /// The bytes held for `entries`, each buffer counted once however many entries share it. + macro_rules! stored { + ($entries:expr) => {{ + let mut bufs = std::collections::HashSet::new(); + $entries.iter().flatten().filter(|e| bufs.insert(e.as_ptr())).map(|e| e.len()).sum::() + }}; + } + + /// #3574: entries naming the same bytes, or overlapping ones, would copy them once each, far + /// past the data the file has: they share one buffer or are refused, in memory and streamed. + #[test] + fn entries_sharing_bytes_hold_no_more_than_the_data() { + let tail: Vec = (0..65536u32).map(|i| (i % 251) as u8).collect(); + let n = tail.len() as u64; + let same = vec![[0, n]; 50]; + let overlapping: Vec<[u64; 2]> = (0..50).map(|i| [i, n - i]).collect(); + let with = |json: &[u8]| { + let mut file = json.to_vec(); + file.extend(crate::BLOB_MAGIC); + file.extend(&tail); + file + }; + let plain = with(b"{}"); + // Compressed, unpacked whole (the marker isn't right after a `}`), and streamed. + let whole = gzip(&with(b"1")); + let streamed = gzip(&plain); + for file in [&plain, &whole, &streamed] { + let u = Unpacked::new(file).unwrap(); + let kind = match u.tail { + Tail::Slice(_) => "in place", + Tail::Whole(_) => "whole", + Tail::Stream { .. } => "streamed", + Tail::None => "no data", + }; + match u.entries(&same) { + Ok(e) => { + assert!(stored!(e) <= tail.len(), "{kind}: {} bytes held for {} of data", stored!(e), tail.len()); + assert!(e.iter().all(|e| e.as_deref().map(|b| &b[..]) == Some(&tail[..])), "{kind}"); + } + Err(e) => assert!(matches!(e, FormatError::TooBig), "{kind}: {e}"), + } + match Unpacked::new(file).unwrap().entries(&overlapping) { + Ok(e) => assert!(stored!(e) <= tail.len(), "{kind}: {} bytes held for {} of data", stored!(e), tail.len()), + Err(e) => assert!(matches!(e, FormatError::TooBig), "{kind}: {e}"), + } + // One entry over all of it is fine. + let one = Unpacked::new(file).unwrap().entries(&same[..1]).unwrap(); + assert_eq!(one[0].as_deref().map(|b| &b[..]), Some(&tail[..]), "{kind}"); + } } }