Merge pull request #1937 from storytold/fix/sec-format-charge-failed-reads

fix(format): charge an archive member's unpacking even when the read fails

crates/format/src/lib.rs

@@ -707,9 +707,16 @@ impl Unpacked {
         if self.exceeded(z, name) && z.by_name(name).is_ok_and(|f| f.size() <= limit) {
             return Err(FormatError::Corrupt(format!("{name}: {}", self.passed())));
         }
-        let data = read_member(z, name, limit.min(self.left))?;
-        self.left = self.left.saturating_sub(data.len() as u64);
-        Ok(data)
+        let cap = limit.min(self.left);
+        let size = z.by_name(name).map_or(0, |f| f.size());
+        let data = read_member(z, name, cap);
+        // A read that failed part way (a bad checksum at the end, say) did its unpacking all the
+        // same: it is charged in full, so an index naming that member again can't repeat it. One
+        // refused for its recorded size before any unpacking costs nothing.
+        if size <= cap {
+            self.left = self.left.saturating_sub(data.as_ref().map_or(size, |d| d.len() as u64));
+        }
+        data
     }
 }
 

crates/format/src/sketch.rs

@@ -282,9 +282,11 @@ fn capped_member(
     if size > allowed {
         return Err(corrupt(format!("{name} is too large")));
     }
-    let data = crate::read_member(z, name, allowed)?;
-    *total += data.len() as u64;
-    Ok(data)
+    let data = crate::read_member(z, name, allowed);
+    // A failed read (a bad checksum at the end, say) unpacked all the same: charged in full, so
+    // a page list naming the member again can't repeat it within the budget.
+    *total += data.as_ref().map_or(size, |d| d.len() as u64);
+    data
 }
 
 /// JSON member `name`, read by [`capped_member`] within the file's [`MAX_JSON_TOTAL`].

crates/format/src/unpacked_total_tests.rs

@@ -93,3 +93,26 @@ fn the_total_leaves_room_for_the_largest_real_files() {
     // Below what the per-area caps added up to (about 10 GB).
     const { assert!(MAX_UNPACKED < 10 * 1024 * 1024 * 1024) };
 }
+
+#[test]
+fn a_read_that_fails_part_way_still_spends_the_total() {
+    // A member whose compressed bytes are damaged near the end unpacks almost all of itself
+    // before failing: that work is charged, so an index naming it again can't repeat it for free.
+    let body: Vec<u8> = (0..200_000u32).map(|i| (i.wrapping_mul(2_654_435_761) >> 13) as u8).collect();
+    // Stored, so a byte flipped in the middle is read through and fails only on the checksum.
+    let mut buf = std::io::Cursor::new(Vec::new());
+    let mut w = zip::ZipWriter::new(&mut buf);
+    w.start_file("snapshots/v1.json", zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored)).unwrap();
+    w.write_all(&body).unwrap();
+    w.finish().unwrap();
+    let mut bytes = buf.into_inner();
+    let at = 30 + "snapshots/v1.json".len() + body.len() / 2;
+    bytes[at] ^= 0xff;
+    let mut z = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap();
+    let size = z.by_name("snapshots/v1.json").unwrap().size();
+    assert_eq!(size, body.len() as u64);
+    let mut total = crate::Unpacked::new(size + size / 2);
+    assert!(total.read(&mut z, "snapshots/v1.json", 1 << 20).is_err(), "the damaged member fails");
+    let again = total.read(&mut z, "snapshots/v1.json", 1 << 20).unwrap_err().to_string();
+    assert!(again.contains("together pass"), "the second read is refused for the total, not read again: {again}");
+}