Format: overflow-safe blob decode and bounds-checked ZIP reads (#3486) (#3501)

- store: `max_blob_bytes as u64 + 1` overflowed for usize::MAX (debug
  panic; release take(0), so every blob failed). The blob read and the
  tile decode now share decompress_bounded(), which saturates.
- zip: ZipReader::read sliced `data_offset..data_offset + compressed`
  unchecked although Entry is pub + Clone; a foreign entry is now a
  Corrupt error.
- store: Collect::tile reuses tile_le(); Object::content() gives the
  stored bytes once (existing_object_is_valid no longer builds them twice).

Co-authored-by: Claude Opus 5.5 <[email protected]>
Co-authored-by: Brandon Thomas <[email protected]>

crates/format/src/store.rs

@@ -30,14 +30,24 @@ fn compress(data: &[u8]) -> Vec<u8> {
     ruzstd::encoding::compress_to_vec(data, ruzstd::encoding::CompressionLevel::Fastest)
 }
 
-fn decompress(data: &[u8], expected: usize, what: &str) -> Result<Vec<u8>> {
-    let mut dec = ruzstd::decoding::StreamingDecoder::new(data).map_err(|e| FormatError::corrupt(format!("{what}: zstd: {e}")))?;
-    let mut out = Vec::with_capacity(expected.min(64 << 20));
-    (&mut dec).take(expected as u64 + 1).read_to_end(&mut out).map_err(|e| FormatError::corrupt(format!("{what}: zstd: {e}")))?;
-    if out.len() != expected {
-        return Err(FormatError::corrupt(format!("{what}: expected {expected} bytes, got {}", out.len())));
+/// Decodes a zstd frame of at most `max` bytes (`LimitExceeded` beyond). With `exact`, the
+/// decoded length must be exactly that (`Corrupt` otherwise).
+fn decompress_bounded(data: &[u8], max: usize, exact: Option<usize>, what: &str) -> Result<Vec<u8>> {
+    let zstd = |e: &dyn std::fmt::Display| FormatError::corrupt(format!("{what}: zstd: {e}"));
+    let mut dec = ruzstd::decoding::StreamingDecoder::new(data).map_err(|e| zstd(&e))?;
+    let mut out = Vec::with_capacity(exact.unwrap_or(0).min(max).min(64 << 20));
+    // `max` may be `usize::MAX`: one byte past it must not overflow.
+    let limit = u64::try_from(max).unwrap_or(u64::MAX).saturating_add(1);
+    (&mut dec).take(limit).read_to_end(&mut out).map_err(|e| zstd(&e))?;
+    match exact {
+        Some(n) if out.len() != n => Err(FormatError::corrupt(format!("{what}: expected {n} bytes, got {}", out.len()))),
+        None if out.len() > max => Err(FormatError::LimitExceeded(format!("{what} exceeds {max} bytes"))),
+        _ => Ok(out),
     }
-    Ok(out)
+}
+
+fn decompress(data: &[u8], expected: usize, what: &str) -> Result<Vec<u8>> {
+    decompress_bounded(data, expected, Some(expected), what)
 }
 
 fn hash_bytes(b: &[u8]) -> Hash {
@@ -228,12 +238,7 @@ impl Fetch for LoadFetch<'_> {
         let z = self.src.get(&path, self.opts.max_blob_bytes)?;
         // Blob length is not in the manifest; the frame header bounds it and
         // we cap at max_blob_bytes.
-        let mut dec = ruzstd::decoding::StreamingDecoder::new(&z[..]).map_err(|e| FormatError::corrupt(format!("{path}: zstd: {e}")))?;
-        let mut data = Vec::new();
-        (&mut dec).take(self.opts.max_blob_bytes as u64 + 1).read_to_end(&mut data).map_err(|e| FormatError::corrupt(format!("{path}: zstd: {e}")))?;
-        if data.len() > self.opts.max_blob_bytes {
-            return Err(FormatError::LimitExceeded(format!("{path} exceeds max_blob_bytes")));
-        }
+        let data = decompress_bounded(&z, self.opts.max_blob_bytes, None, &path)?;
         self.account(data.len())?;
         if hash_bytes(&data) != hash {
             return Err(FormatError::corrupt(format!("{path}: content does not match its hash")));
@@ -313,13 +318,7 @@ impl Sink for Collect<'_> {
             self.tiles.entry(h.clone()).or_insert_with(|| (tile.clone(), format.sample));
             return h;
         }
-        let h = if cfg!(target_endian = "big") {
-            let mut b = tile.bytes().to_vec();
-            swap_to_le(&mut b, format.sample);
-            hash_bytes(&b)
-        } else {
-            hash_bytes(tile.bytes())
-        };
+        let h = hash_bytes(&tile_le(tile, format.sample));
         if let Some(cache) = self.hash_cache.as_deref_mut() {
             cache.insert(key, (Arc::downgrade(tile), h.clone()));
         }
@@ -414,21 +413,23 @@ enum Object {
 
 impl Object {
     fn compressed(&self) -> Vec<u8> {
-        match self {
-            Object::Tile(t, s) => compress(&tile_le(t, *s)),
-            Object::Blob(b) => compress(b),
-        }
+        compress(&self.content())
     }
 
-    fn matches_content_hash(&self, compressed: &[u8], what: &str) -> bool {
-        let expected = match self {
+    /// The bytes this object stores (tiles little-endian).
+    fn content(&self) -> std::borrow::Cow<'_, [u8]> {
+        match self {
             Object::Tile(tile, sample) => tile_le(tile, *sample),
             Object::Blob(blob) => std::borrow::Cow::Borrowed(blob.as_slice()),
-        };
-        decompress(compressed, expected.len(), what).is_ok_and(|actual| hash_bytes(&actual) == hash_bytes(&expected))
+        }
     }
 }
 
+/// Whether `compressed` decodes to exactly `expected`.
+fn content_matches(expected: &[u8], compressed: &[u8], what: &str) -> bool {
+    decompress(compressed, expected.len(), what).is_ok_and(|actual| hash_bytes(&actual) == hash_bytes(expected))
+}
+
 fn file_signature(metadata: &std::fs::Metadata) -> Option<FileSignature> {
     #[cfg(unix)]
     let unix = {
@@ -587,10 +588,8 @@ fn existing_object_is_valid(path: &Path, object: &Object, what: &str) -> Result<
     }
     let signature = file_signature(&metadata);
 
-    let expected_len = match object {
-        Object::Tile(tile, sample) => tile_le(tile, *sample).len(),
-        Object::Blob(blob) => blob.len(),
-    };
+    let expected = object.content();
+    let expected_len = expected.len();
     // This is the same bound used by the tile loader. It comfortably includes
     // zstd framing overhead while keeping damaged files bounded when inspected.
     let max_compressed_len = expected_len.saturating_add(expected_len / 8).saturating_add(4096);
@@ -604,7 +603,7 @@ fn existing_object_is_valid(path: &Path, object: &Object, what: &str) -> Result<
     if compressed.len() > max_compressed_len {
         return Ok((false, None));
     }
-    Ok((object.matches_content_hash(&compressed, what), signature))
+    Ok((content_matches(&expected, &compressed, what), signature))
 }
 
 /// A bundle path and its compressed bytes.

crates/format/src/zip.rs

@@ -201,7 +201,12 @@ impl<'a> ZipReader<'a> {
         if e.uncompressed > max {
             return Err(FormatError::LimitExceeded(format!("zip entry `{}` is {} bytes (max {max})", e.name, e.uncompressed)));
         }
-        let raw = &self.bytes[e.data_offset..e.data_offset + e.compressed];
+        // `Entry` is public and cloneable: one from another archive must not index out of range.
+        let raw = e
+            .data_offset
+            .checked_add(e.compressed)
+            .and_then(|end| self.bytes.get(e.data_offset..end))
+            .ok_or_else(|| FormatError::corrupt(format!("zip entry `{}` out of range", e.name)))?;
         let data = match e.method {
             0 => raw.to_vec(),
             8 => {
@@ -366,6 +371,19 @@ mod tests {
         assert!(matches!(archive.read_by_name("manifest.json", usize::MAX), Err(FormatError::Corrupt(_))));
     }
 
+    #[test]
+    fn an_entry_from_another_archive_is_an_error_not_a_panic() {
+        let mut big = ZipWriter::new();
+        big.add("a", &[7; 4096]).unwrap();
+        let big = big.finish().unwrap();
+        let foreign = ZipReader::new(&big).unwrap().entries[0].clone();
+        let mut small = ZipWriter::new();
+        small.add("a", b"x").unwrap();
+        let small = small.finish().unwrap();
+        let r = ZipReader::new(&small).unwrap();
+        assert!(matches!(r.read(&foreign, usize::MAX), Err(FormatError::Corrupt(_))));
+    }
+
     #[test]
     fn offset_access_rejects_overflow() {
         assert!(u16_at(&[], usize::MAX).is_err());

crates/format/tests/corruption.rs

@@ -419,6 +419,15 @@ fn tile_coordinates_outside_the_i32_pixel_range_are_rejected() {
     }
 }
 
+#[test]
+fn an_unlimited_blob_budget_still_loads_blobs() {
+    // `max_blob_bytes + 1` overflowed (debug panic; release read zero bytes, failing every blob).
+    let doc = rich_doc(ColorMode::Rgb, SampleType::U8);
+    let bytes = save_to_bytes(&doc, &SaveOptions::default()).unwrap();
+    let opts = LoadOptions { max_blob_bytes: usize::MAX, ..LoadOptions::default() };
+    assert_eq!(load_from_bytes_with(&bytes, &opts).unwrap(), doc);
+}
+
 /// Rewrites the first pattern's declared size (its tiles stay as saved).
 fn with_pattern_size(w: u64, h: u64) -> Vec<u8> {
     rebuild(sample_cached(), |n, d| {