format, clawhdf5: cut every Storage read to the range asked for

ExtentBytes and read_exact_at/read_upto rejected short results but passed
longer-than-asked ones through, and FileData forwarded them too, so a
Storage that broke read_at's contract by returning extra bytes had them
decoded or returned as data (a contiguous dataset read gained 37 junk
bytes). gather_storage alone trimmed.

- storage::exact_len (new, pub): a read of len bytes as exactly len — cut
  when longer, an error when short. read_exact_at, read_upto and
  ExtentBytes (so chunk fetches and selection gathers) go through it.
- FileData cuts a backend's answer to what it asked for before laying the
  cache image over it.
- Tests: over a storage that appends 37 junk bytes to every read, every
  format-crate fixture reads exactly as from the slice
  (overlong_reads_are_cut_to_the_range_asked_for), and every facade
  fixture opens and reads through File::open_storage as through File::open
  (overlong_storage_reads_identically). Both failed before.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

crates/clawhdf5-format/src/storage.rs

@@ -43,6 +43,9 @@ pub trait Storage {
     /// end of the storage (and empty when `offset` is at or past the end);
     /// a backend that cannot serve a range returns an error instead of a
     /// short read.
+    /// It is never longer than `len`; the parsers cut a longer result to
+    /// `len` (see [`exact_len`]) rather than read bytes from outside the
+    /// range.
     fn read_at(&self, offset: u64, len: usize) -> Result<Cow<'_, [u8]>, FormatError>;
 
     /// Current length of the storage in bytes.
@@ -218,13 +221,30 @@ pub fn read_exact_at<S: Storage + ?Sized>(
         Some(end) if end <= file.len() => {}
         _ => return Err(eof()),
     }
-    let bytes = file.read_at(offset, len)?;
-    if bytes.len() < len {
-        // The storage shrank or the backend served a short read inside the
-        // file: never parse a partial structure.
-        return Err(short_read());
+    // A short read (the storage shrank, or the backend served less inside
+    // the file) is an error: never parse a partial structure.
+    exact_len(file.read_at(offset, len)?, len)
+}
+
+/// `bytes`, the result of asking a [`Storage`] for `len` bytes, as exactly
+/// `len` bytes: a longer result (a backend that broke
+/// [`Storage::read_at`]'s contract) is cut to `len`, so bytes from outside
+/// the range asked for are never parsed or returned; a shorter one is an
+/// error (the storage shrank, or the backend failed), never a partial
+/// structure.
+#[inline]
+pub fn exact_len(bytes: Cow<'_, [u8]>, len: usize) -> Result<Cow<'_, [u8]>, FormatError> {
+    match bytes.len().cmp(&len) {
+        core::cmp::Ordering::Equal => Ok(bytes),
+        core::cmp::Ordering::Less => Err(short_read()),
+        core::cmp::Ordering::Greater => Ok(match bytes {
+            Cow::Borrowed(b) => Cow::Borrowed(&b[..len]),
+            Cow::Owned(mut v) => {
+                v.truncate(len);
+                Cow::Owned(v)
+            }
+        }),
     }
-    Ok(bytes)
 }
 
 #[cold]
@@ -329,11 +349,7 @@ pub fn read_upto<S: Storage + ?Sized>(
     }
     let avail = file.len().saturating_sub(offset);
     let len = usize::try_from(avail).map_or(max, |a| a.min(max));
-    let bytes = file.read_at(offset, len)?;
-    if bytes.len() < len {
-        return Err(short_read());
-    }
-    Ok(bytes)
+    exact_len(file.read_at(offset, len)?, len)
 }
 
 /// Most stored bytes fetched by one [`Storage::read_ranges`] call when a
@@ -461,10 +477,7 @@ impl<'a> ExtentBytes<'a> {
                 ));
             }
             for ((slot, bytes), r) in slots.into_iter().zip(got).zip(&ranges) {
-                if (bytes.len() as u64) < r.end - r.start {
-                    return Err(short_read());
-                }
-                out[slot] = Extent::Bytes(bytes);
+                out[slot] = Extent::Bytes(exact_len(bytes, (r.end - r.start) as usize)?);
             }
         }
         Ok(ExtentBytes::Fetched { base, extents: out })

crates/clawhdf5-format/tests/storage_equivalence.rs

@@ -753,6 +753,73 @@ fn corpus_parses_identically_through_storage() {
     assert!(tally.files > 0);
 }
 
+/// The whole read of every object reachable from the root group of `sb`'s
+/// file — each dataset whole (fill-aware) and half of it through a
+/// selection, and each group's listing — as one result to compare.
+fn read_everything(file: &dyn Storage, sb: &Superblock) -> Result<String, FormatError> {
+    let (os, ls) = (sb.offset_size, sb.length_size);
+    let mut out = String::new();
+    let mut queue = VecDeque::from([sb.root_group_address]);
+    let mut seen = HashSet::new();
+    while let Some(addr) = queue.pop_front() {
+        if seen.len() > 200 || !seen.insert(addr) {
+            continue;
+        }
+        let header = ObjectHeader::parse_in(file, addr, os, ls)?;
+        let find = |t: MessageType| {
+            header
+                .messages
+                .iter()
+                .find(|m| m.msg_type == t)
+                .map(|m| message_data_with_sohm_in(file, m, os, ls))
+                .transpose()
+        };
+        if let (Some(dt), Some(ds), Some(dl)) = (
+            find(MessageType::Datatype)?,
+            find(MessageType::Dataspace)?,
+            find(MessageType::DataLayout)?,
+        ) {
+            let dt = Datatype::parse(&dt)?.0;
+            let ds = Dataspace::parse(&ds, ls)?;
+            let dl = DataLayout::parse(&dl, os, ls)?;
+            let pl = find(MessageType::FilterPipeline)?
+                .map(|p| FilterPipeline::parse(&p))
+                .transpose()?;
+            let data = read_full_with_fill_in(
+                &header.messages,
+                file,
+                &dl,
+                &ds,
+                dt.type_size() as usize,
+                os,
+                ls,
+                || read_raw_data_full_in(file, &dl, &ds, &dt, pl.as_ref(), os, ls),
+            );
+            out.push_str(&format!("{addr}: {data:?}\n"));
+            if let Some(&d0) = ds.dimensions.first() {
+                let rank = ds.dimensions.len();
+                let sel = Selection::Hyperslab {
+                    start: vec![0; rank],
+                    stride: vec![1; rank],
+                    count: std::iter::once(d0.div_ceil(2))
+                        .chain(ds.dimensions[1..].iter().copied())
+                        .collect(),
+                    block: vec![1; rank],
+                };
+                let part =
+                    read_raw_data_selection_in(file, &dl, &ds, &dt, pl.as_ref(), os, ls, &sel);
+                out.push_str(&format!("{addr} half: {part:?}\n"));
+            }
+        }
+        let children = group_v2::resolve_group_children_in(file, sb, addr);
+        out.push_str(&format!("{addr} children: {children:?}\n"));
+        if let Ok(c) = children {
+            queue.extend(c.iter().map(|c| c.object_header_address));
+        }
+    }
+    Ok(out)
+}
+
 /// A storage that misbehaves: fails its `fail_at`-th read (1-based, `0`
 /// never), and, with `short`, serves one byte less than asked for inside
 /// the file (a truncated response).
@@ -808,78 +875,7 @@ fn misbehaving_storage_never_returns_wrong_data() {
         let Ok(sb) = Superblock::parse(hdf5, 0) else {
             continue;
         };
-        // The whole read of every object, as one result to compare.
-        let everything = |file: &dyn Storage| -> Result<String, FormatError> {
-            let (os, ls) = (sb.offset_size, sb.length_size);
-            let mut out = String::new();
-            let mut queue = VecDeque::from([sb.root_group_address]);
-            let mut seen = HashSet::new();
-            while let Some(addr) = queue.pop_front() {
-                if seen.len() > 200 || !seen.insert(addr) {
-                    continue;
-                }
-                let header = ObjectHeader::parse_in(file, addr, os, ls)?;
-                let find = |t: MessageType| {
-                    header
-                        .messages
-                        .iter()
-                        .find(|m| m.msg_type == t)
-                        .map(|m| message_data_with_sohm_in(file, m, os, ls))
-                        .transpose()
-                };
-                if let (Some(dt), Some(ds), Some(dl)) = (
-                    find(MessageType::Datatype)?,
-                    find(MessageType::Dataspace)?,
-                    find(MessageType::DataLayout)?,
-                ) {
-                    let dt = Datatype::parse(&dt)?.0;
-                    let ds = Dataspace::parse(&ds, ls)?;
-                    let dl = DataLayout::parse(&dl, os, ls)?;
-                    let pl = find(MessageType::FilterPipeline)?
-                        .map(|p| FilterPipeline::parse(&p))
-                        .transpose()?;
-                    let data = read_full_with_fill_in(
-                        &header.messages,
-                        file,
-                        &dl,
-                        &ds,
-                        dt.type_size() as usize,
-                        os,
-                        ls,
-                        || read_raw_data_full_in(file, &dl, &ds, &dt, pl.as_ref(), os, ls),
-                    );
-                    out.push_str(&format!("{addr}: {data:?}\n"));
-                    if let Some(&d0) = ds.dimensions.first() {
-                        let rank = ds.dimensions.len();
-                        let sel = Selection::Hyperslab {
-                            start: vec![0; rank],
-                            stride: vec![1; rank],
-                            count: std::iter::once(d0.div_ceil(2))
-                                .chain(ds.dimensions[1..].iter().copied())
-                                .collect(),
-                            block: vec![1; rank],
-                        };
-                        let part = read_raw_data_selection_in(
-                            file,
-                            &dl,
-                            &ds,
-                            &dt,
-                            pl.as_ref(),
-                            os,
-                            ls,
-                            &sel,
-                        );
-                        out.push_str(&format!("{addr} half: {part:?}\n"));
-                    }
-                }
-                let children = group_v2::resolve_group_children_in(file, &sb, addr);
-                out.push_str(&format!("{addr} children: {children:?}\n"));
-                if let Ok(c) = children {
-                    queue.extend(c.iter().map(|c| c.object_header_address));
-                }
-            }
-            Ok(out)
-        };
+        let everything = |file: &dyn Storage| read_everything(file, &sb);
         let want = everything(&hdf5);
         let counting = CountingStorage::new(hdf5.to_vec());
         assert_eq!(
@@ -947,6 +943,61 @@ fn misbehaving_storage_never_returns_wrong_data() {
     );
 }
 
+/// A storage that breaks `read_at`'s contract the other way: every read
+/// comes back with 37 bytes more than asked for (junk past the range).
+struct Overlong {
+    data: Vec<u8>,
+}
+
+impl Storage for Overlong {
+    fn read_at(&self, offset: u64, len: usize) -> Result<std::borrow::Cow<'_, [u8]>, FormatError> {
+        let mut v = self.data.as_slice().read_at(offset, len)?.into_owned();
+        v.extend(std::iter::repeat_n(0xa5, 37));
+        Ok(std::borrow::Cow::Owned(v))
+    }
+
+    fn len(&self) -> u64 {
+        self.data.len() as u64
+    }
+}
+
+/// Bytes past the range asked for are never used: every read through a
+/// storage that returns more than asked gives exactly the in-memory result.
+#[test]
+fn overlong_reads_are_cut_to_the_range_asked_for() {
+    let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures");
+    let mut files = Vec::new();
+    hdf5_files(&dir, &mut files);
+    files.sort();
+    let mut compared = 0;
+    for path in &files {
+        let Ok(bytes) = std::fs::read(path) else {
+            continue;
+        };
+        let Ok((_, hdf5)) = split_user_block(&bytes) else {
+            continue;
+        };
+        let Ok(sb) = Superblock::parse(hdf5, 0) else {
+            continue;
+        };
+        let want = read_everything(&hdf5, &sb);
+        let got = read_everything(
+            &Overlong {
+                data: hdf5.to_vec(),
+            },
+            &sb,
+        );
+        assert_eq!(
+            format!("{got:?}"),
+            format!("{want:?}"),
+            "{}",
+            path.display()
+        );
+        compared += 1;
+    }
+    assert!(compared > 40, "{compared}");
+}
+
 /// A read_at-only storage that also counts `read_ranges` calls and ranges.
 struct BatchCounting {
     inner: CountingStorage,