Format: a saved document opens whatever the size of its placed images (#2166)

save writes every asset into the archive with no size limit, but load refused any asset entry over 1 GiB and any archive expanding past 4 GiB, so a document with large placed files saved normally and then couldn't be reopened. The caps guard against archives whose claimed sizes lie; a stored entry can't lie about its size, since it is its bytes in the archive, already in memory, and all stored entries together can't exceed the archive. A stored entry (every placed PNG, JPEG, GIF and WebP, which save stores as is) now opens at its own size and counts for nothing against the budget; deflated entries (PDF, EPS, SVG) and the JSON keep their caps, so a zip bomb is still refused. The limits are a struct the test can shrink.

Verified locally on a288a87b: fmt, clippy, format tests including stored_assets_open_at_any_size_while_deflated_ones_stay_capped.

crates/format/src/lib.rs

@@ -177,20 +177,36 @@ pub fn load(bytes: &[u8]) -> Result<Document, FormatError> {
 
 /// Most entries a `.designcraft` archive may hold.
 const MAX_ENTRIES: usize = 100_000;
-/// Most bytes `meta.json` or `document.json` may expand to.
-const MAX_JSON_BYTES: u64 = 256 << 20;
-/// Most bytes one embedded asset may expand to.
-const MAX_ASSET_BYTES: u64 = 1 << 30;
-/// Most bytes all the entries read from one archive may expand to together.
-const MAX_TOTAL_BYTES: u64 = 4 << 30;
 
-/// Read entry `name` of `z`, or `None` when it is missing or unreadable. Refuses an entry that
-/// expands past `limit` bytes or past what is `left` of the archive's budget, without
-/// decompressing more than that: the sizes the archive claims are not trusted.
+/// How much a `.designcraft` archive may expand to when read. The sizes an archive claims are
+/// not trusted, so deflated entries are capped; a *stored* entry can't expand (its size is its
+/// bytes in the archive, which is already in memory, and all stored entries together can't
+/// exceed the archive), so a placed image — stored as is by [`save`] — opens whatever its size
+/// and counts for nothing against the budget. Saving has no cap, and this keeps every saved
+/// document openable (#2166).
+#[derive(Clone, Copy, Debug)]
+struct Limits {
+    /// Most bytes `meta.json` or `document.json` may expand to.
+    json: u64,
+    /// Most bytes one deflated asset may expand to.
+    asset: u64,
+    /// Most bytes all the deflated entries read from one archive may expand to together.
+    total: u64,
+}
+
+impl Limits {
+    const DEFAULT: Limits = Limits { json: 256 << 20, asset: 1 << 30, total: 4 << 30 };
+}
+
+/// Read entry `name` of `z`, or `None` when it is missing or unreadable. Refuses a deflated entry
+/// that expands past `limit` bytes or past what is `left` of the archive's budget, without
+/// decompressing more than that: the sizes the archive claims are not trusted. A stored entry
+/// is read at its size in the archive and leaves the budget alone.
 fn read_entry(z: &mut zip::ZipArchive<Cursor<&[u8]>>, name: &str, limit: u64, left: &mut u64) -> Result<Option<Vec<u8>>, FormatError> {
-    let too_big = || FormatError::Io(format!("{name} is larger than the {limit}-byte limit (or the archive is larger than {MAX_TOTAL_BYTES} bytes)"));
     let Ok(f) = z.by_name(name) else { return Ok(None) };
-    let limit = limit.min(*left);
+    let stored = f.compression() == zip::CompressionMethod::Stored;
+    let limit = if stored { f.compressed_size() } else { limit.min(*left) };
+    let too_big = || FormatError::Io(format!("{name} is larger than the {limit}-byte limit (or the archive expands past its {}-byte budget)", *left));
     if f.size() > limit {
         return Err(too_big());
     }
@@ -202,24 +218,30 @@ fn read_entry(z: &mut zip::ZipArchive<Cursor<&[u8]>>, name: &str, limit: u64, le
     if n > limit {
         return Err(too_big());
     }
-    *left = left.saturating_sub(n);
+    if !stored {
+        *left = left.saturating_sub(n);
+    }
     Ok(Some(v))
 }
 
 fn load_zip(bytes: &[u8]) -> Result<Document, FormatError> {
+    load_zip_within(bytes, Limits::DEFAULT)
+}
+
+fn load_zip_within(bytes: &[u8], limits: Limits) -> Result<Document, FormatError> {
     let mut z = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| FormatError::NotOurs(e.to_string()))?;
     if z.len() > MAX_ENTRIES {
         return Err(FormatError::NotOurs(format!("{} entries (at most {MAX_ENTRIES})", z.len())));
     }
-    let mut left = MAX_TOTAL_BYTES;
-    let version = read_entry(&mut z, "meta.json", MAX_JSON_BYTES, &mut left)?
+    let mut left = limits.total;
+    let version = read_entry(&mut z, "meta.json", limits.json, &mut left)?
         .and_then(|meta| serde_json::from_slice::<Value>(&meta).ok())
         .and_then(|m| m.get("version").and_then(Value::as_u64))
         .unwrap_or(1);
     if version > u64::from(VERSION) {
         return Err(FormatError::TooNew(version));
     }
-    let json = read_entry(&mut z, "document.json", MAX_JSON_BYTES, &mut left)?.ok_or_else(|| FormatError::NotOurs("missing document.json".into()))?;
+    let json = read_entry(&mut z, "document.json", limits.json, &mut left)?.ok_or_else(|| FormatError::NotOurs("missing document.json".into()))?;
     let mut v: Value = serde_json::from_slice(&json).map_err(|e| FormatError::NotOurs(e.to_string()))?;
     upgrade_json(&mut v);
     let mut doc: Document = serde_json::from_value(v).map_err(|e| FormatError::NotOurs(e.to_string()))?;
@@ -235,7 +257,7 @@ fn load_zip(bytes: &[u8]) -> Result<Document, FormatError> {
         if !doc.assets.contains_key(&AssetId(id)) {
             continue;
         }
-        if let (Some(data), Some(a)) = (read_entry(&mut z, &name, MAX_ASSET_BYTES, &mut left)?, doc.assets.get_mut(&AssetId(id))) {
+        if let (Some(data), Some(a)) = (read_entry(&mut z, &name, limits.asset, &mut left)?, doc.assets.get_mut(&AssetId(id))) {
             let same = loaded.entry(fingerprint.hash_one(data.as_slice())).or_default();
             let data = match same.iter().find(|d| ***d == data) {
                 Some(d) => d.clone(),

crates/format/src/tests.rs

@@ -54,6 +54,85 @@ fn save_uncompacted(d: &Document) -> Vec<u8> {
     buf.into_inner()
 }
 
+/// A `.designcraft` with the given asset entries, each stored as is or deflated.
+fn archive_with(d: &Document, entries: &[(AssetId, &[u8], bool)]) -> Vec<u8> {
+    let mut buf = Cursor::new(Vec::new());
+    let mut z = zip::ZipWriter::new(&mut buf);
+    let stored = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored);
+    let deflate = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated);
+    z.start_file("mimetype", stored).unwrap();
+    z.write_all(MIME.as_bytes()).unwrap();
+    z.start_file("meta.json", deflate).unwrap();
+    z.write_all(json!({"format": "designcraft", "version": VERSION}).to_string().as_bytes()).unwrap();
+    z.start_file("document.json", deflate).unwrap();
+    z.write_all(&serde_json::to_vec(d).unwrap()).unwrap();
+    for (id, data, as_is) in entries {
+        z.start_file(format!("assets/{}.png", id.0), if *as_is { stored } else { deflate }).unwrap();
+        z.write_all(data).unwrap();
+    }
+    z.finish().unwrap();
+    buf.into_inner()
+}
+
+/// Bytes that don't compress (a simple generator, so the test needs no crate).
+fn noise(n: usize) -> Vec<u8> {
+    let mut x: u32 = 0x9e37_79b9;
+    (0..n)
+        .map(|_| {
+            x ^= x << 13;
+            x ^= x >> 17;
+            x ^= x << 5;
+            (x >> 24) as u8
+        })
+        .collect()
+}
+
+/// #2166: saving has no size cap, so opening must keep every saved document openable. A stored
+/// entry (how placed images are saved) can't expand and opens at any size; the total budget is
+/// never below the archive's own size. Deflated entries stay capped, so a zip bomb is refused.
+#[test]
+fn stored_assets_open_at_any_size_while_deflated_ones_stay_capped() {
+    let mut d = Document::new(&NewDocument::default());
+    let big = noise(50_000);
+    let a = png_asset(&mut d, &big);
+    let b = png_asset(&mut d, &big);
+    d.assets.values_mut().for_each(|a| Arc::make_mut(a).data = Arc::new(Vec::new()));
+    place(&mut d, a, 0.0);
+    place(&mut d, b, 0.0);
+    // A budget that fits the document's JSON but not the two 50 000-byte assets together.
+    let json_len = serde_json::to_vec(&d).unwrap().len() as u64;
+    let tight = Limits { json: 256 << 20, asset: 1_000, total: json_len + 10_000 };
+    assert!(tight.total < 100_000, "the JSON is {json_len} bytes; the budget must stay under the two assets");
+
+    // Two stored assets, each over the asset cap and together over the total: both open.
+    let bytes = archive_with(&d, &[(a, &big, true), (b, &big, true)]);
+    let doc = load_zip_within(&bytes, tight).expect("stored entries open at their own size");
+    assert_eq!(doc.assets[&a].data.len(), 50_000);
+    // (Identical bytes come back as one asset: `load` merges duplicates.)
+    assert_eq!(doc.assets.len(), 1);
+    assert!(doc.all_items().iter().filter_map(|i| doc.item(*i)?.graphic()).all(|g| g.asset == a), "both placements show it");
+
+    // A deflated entry past the cap is refused, whether it really is that big ...
+    let bytes = archive_with(&d, &[(a, &big, false), (b, &big, true)]);
+    let e = load_zip_within(&bytes, tight).expect_err("a deflated entry over the cap").to_string();
+    assert!(e.contains("larger than the 1000-byte limit"), "{e}");
+    // ... or claims to be (a few KB that expand to megabytes).
+    let bomb = vec![0u8; 3 << 20];
+    let bytes = archive_with(&d, &[(a, &bomb, false), (b, &big, true)]);
+    assert!(bytes.len() < 64 << 10, "the bomb is small on disk: {}", bytes.len());
+    assert!(load_zip_within(&bytes, tight).is_err(), "a zip bomb is refused");
+
+    // The default limits read the same stored archive, and what `save` writes round-trips.
+    let bytes = archive_with(&d, &[(a, &big, true), (b, &big, true)]);
+    assert_eq!(load_zip(&bytes).unwrap().assets[&a].data.len(), 50_000);
+    Arc::make_mut(d.assets.get_mut(&a).unwrap()).data = Arc::new(big.clone());
+    Arc::make_mut(d.assets.get_mut(&b).unwrap()).data = Arc::new(big.clone());
+    let saved = save(&d).unwrap();
+    let doc = load_zip_within(&saved, tight).expect("what save writes opens under a cap smaller than its images");
+    // (`save` merges the two identical assets into one.)
+    assert!(!doc.assets.is_empty() && doc.assets.values().all(|x| x.data.len() == 50_000), "every saved image is back");
+}
+
 #[test]
 fn explicit_all_on_pdf_layers_require_a_capable_reader_only_when_needed() {
     let mut d = Document::new(&NewDocument::default());