format: bound Storage reads that hostile size fields could stretch

On a backend without the file in memory, a structure read whose length comes from untrusted header fields was clamped only by the end of the file, so a crafted size made one read (and copy) of up to the rest of the file. Each such read now covers what the parser actually uses: - local heap names: read in growing pieces (64 bytes first, then 4x) up to the end of the data segment, instead of the rest of the segment per name (quadratic for a big symbol-table group); - fractal heap indirect blocks: the doubling-table geometry locates the entry covering the object, and the first read ends at that entry; only if it is unallocated does the walk read the rest of the block (it visits every entry then). One walk implementation serves both; - paged fixed/extensible array data blocks over 1 MiB: the prefix and page bitmap, then each page in use on its own (smaller blocks are still one read); - blocks under one checksum (non-paged array data blocks, extensible array index and super blocks): the bounds check that comes first (the checksum's; the page bitmap's for a super block) is made against the file length before reading (Window::check_extent), so a block claimed past the end of the file costs no read. With the checksum feature off the parser has no such first check and the old read stands. Other windows were already bounded (the superblock and object header prefixes, the fractal heap header by a u16, SOHM tables by u8/u16 counts) or are exact reads checked against the file length first. In memory nothing changes: the pieces are borrowed slices. Tests: CountingStorage over a crafted heap (16 MiB file, width and rows 0xFFFF: under 1 KiB read, 16.7 MB before), a heap segment claiming 64 MiB (one 64-byte read per short name), long names at every piece boundary, a fixed array block claimed past the end of a 16 MiB file (under 64 bytes read), and in the equivalence harness an h5py file with a 2.4 MB fixed array block and a >1 MiB extensible array block, whole and cut at 97 points: every chunk index agrees with the slice read and the largest takes 205 KB (2.4 MB and 1.2 MB when read whole). Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
2026-09-26 14:31:46 -05:00
parent e5359354b7
commit 76c97f6c94
6 changed files with 467 additions and 95 deletions
@@ -113,6 +113,10 @@ struct Tally {
    contiguous_required: usize,
    reads: u64,
    bytes: u64,
+    /// Chunk indexes (fixed and extensible arrays) read, and the most bytes
+    /// one of them took through the storage.
+    chunk_indexes: usize,
+    max_chunk_index_bytes: u64,
 }

 struct Walk<'a> {
@@ -152,6 +156,12 @@ impl Walk<'_> {
        );
    }

+    fn index_read(&mut self, bytes_before: u64) {
+        self.tally.chunk_indexes += 1;
+        let bytes = self.storage.bytes_read() - bytes_before;
+        self.tally.max_chunk_index_bytes = self.tally.max_chunk_index_bytes.max(bytes);
+    }
+
    fn st(&self) -> &dyn Storage {
        self.storage
    }
@@ -403,6 +413,7 @@ impl Walk<'_> {
                    let Ok(h) = h else { return };
                    let want =
                        read_fixed_array_chunks(slice, &h, &ds.dimensions, max, dims, es, os, ls);
+                    let before = self.storage.bytes_read();
                    let got = read_fixed_array_chunks_in(
                        self.st(),
                        &h,
@@ -413,6 +424,7 @@ impl Walk<'_> {
                        os,
                        ls,
                    );
+                    self.index_read(before);
                    self.same("fixed array chunks", &want, &got);
                } else {
                    let h = ExtensibleArrayHeader::parse(slice, *addr as usize, os, ls);
@@ -429,6 +441,7 @@ impl Walk<'_> {
                        os,
                        ls,
                    );
+                    let before = self.storage.bytes_read();
                    let got = read_extensible_array_chunks_in(
                        self.st(),
                        &h,
@@ -439,6 +452,7 @@ impl Walk<'_> {
                        os,
                        ls,
                    );
+                    self.index_read(before);
                    self.same("extensible array chunks", &want, &got);
                }
            }
@@ -451,7 +465,11 @@ fn check_file(path: &Path, tally: &mut Tally) {
    let Ok(bytes) = std::fs::read(path) else {
        return;
    };
-    let Ok((_, hdf5)) = split_user_block(&bytes) else {
+    check_bytes(&path.display().to_string(), &bytes, tally);
+}
+
+fn check_bytes(name: &str, bytes: &[u8], tally: &mut Tally) {
+    let Ok((_, hdf5)) = split_user_block(bytes) else {
        return;
    };
    let storage = CountingStorage::new(hdf5.to_vec());
@@ -459,7 +477,7 @@ fn check_file(path: &Path, tally: &mut Tally) {
    let mut walk = Walk {
        slice: hdf5,
        storage: &storage,
-        name: path.display().to_string(),
+        name: name.to_string(),
        tally,
    };
    walk.run();
@@ -473,7 +491,7 @@ fn check_file(path: &Path, tally: &mut Tally) {
            tally.checks - before.0,
            storage.reads(),
            storage.bytes_read(),
-            path.display()
+            name
        );
    }
 }
@@ -581,6 +599,16 @@ with h5py.File(p('v1_groups.h5'), 'w', libver='earliest', userblock_size=512) as
        g.attrs['n'] = i
    f.create_dataset('x', data=np.arange(10))

+# Paged chunk indexes whose data blocks are bigger than the storage reads
+# in one piece (1 MiB), with two chunks written: a fixed array of 300 000
+# chunks (a 2.4 MB data block) and an extensible array grown to 1.2e9 (its
+# last data block holds 131 072 chunks: over 1 MiB).
+with h5py.File(p('big_paged.h5'), 'w', libver='latest') as f:
+    d = f.create_dataset('fa', shape=(300000,), chunks=(1,), dtype='u1')
+    d[5] = 1; d[250000] = 2
+    e = f.create_dataset('ea', shape=(1,), maxshape=(None,), chunks=(1,), dtype='u1')
+    e.resize((1200000000,)); e[10] = 1; e[1100000000] = 3
+
 libs = glob.glob(os.path.join(os.path.dirname(h5py.__file__), '..', 'h5py.libs', 'libhdf5-*.so*'))
 if libs:
    lib = ctypes.CDLL(libs[0])
@@ -633,7 +661,7 @@ fn h5py_files_parse_identically_through_storage() {
        .iter()
        .map(|f| f.file_name().unwrap().to_string_lossy().into_owned())
        .collect();
-    for want in ["ea.h5", "v1_groups.h5"] {
+    for want in ["ea.h5", "v1_groups.h5", "big_paged.h5"] {
        assert!(names.iter().any(|n| n == want), "{names:?}");
    }
    if interop_required() {
@@ -641,6 +669,29 @@ fn h5py_files_parse_identically_through_storage() {
    }
    let mut tally = Tally::default();
    for f in &files {
+        if f.ends_with("big_paged.h5") {
+            // Only the pages in use are read, not the whole data blocks:
+            // read in one piece, the fixed array took 2.4 MB and the
+            // extensible array 1.2 MB (its super block's page bitmap and
+            // block addresses are most of what remains).
+            let mut big = Tally::default();
+            check_file(f, &mut big);
+            eprintln!("big paged blocks: {big:?}");
+            assert_eq!(big.chunk_indexes, 2, "{big:?}");
+            assert!(big.max_chunk_index_bytes < 256 << 10, "{big:?}");
+            // Truncated anywhere, the page-by-page reads still agree with
+            // the slice reads (errors included).
+            let bytes = std::fs::read(f).unwrap();
+            for cut in (0..bytes.len()).step_by(bytes.len() / 97) {
+                check_bytes(
+                    &format!("big_paged.h5 cut at {cut}"),
+                    &bytes[..cut],
+                    &mut big,
+                );
+            }
+            eprintln!("big paged blocks, truncated: {big:?}");
+            assert!(big.chunk_indexes > 100, "{big:?}");
+        }
        check_file(f, &mut tally);
    }
    eprintln!("h5py files: {tally:?}");