Merge pull request #3801 from Silvertree2010/fix/3574-audit-native-format-image-entrie

Share native-format entries naming the same bytes; cap copies at data
This commit is contained in:
BFlat
2026-10-24 02:02:18 +07:00
committed by GitHub
2 changed files with 130 additions and 28 deletions
+22 -5
View File
@@ -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<NativeFile, FormatError> {
}
// 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<NativeFile, FormatError> {
// 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::<Result<_, FormatError>>()?;
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<Image>) -> 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(),
}
}
+108 -23
View File
@@ -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<Arc<Vec<u8>>>;
/// 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<Vec<Option<Vec<u8>>>, 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<Vec<Entry>, 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<Vec<Option<Vec<u8>>>, FormatError> {
let mut out: Vec<Option<Vec<u8>>> = vec![None; wanted.len()];
let in_memory = |tail: &[u8], out: &mut Vec<Option<Vec<u8>>>| {
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<Vec<Entry>, FormatError> {
let mut out: Vec<Entry> = 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<Entry>| {
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<usize> = (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::<usize>()
}};
}
/// #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<u8> = (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}");
}
}
}