clawhdf5: storage harness compares errors, not just failures

The facade equivalence harness turned every data-read error into "Err", so
it could not see File::open_storage failing differently from File::open
(a Storage or ContiguousStorageRequired error where the mmap path gives a
decode error, say).

- value() keeps the whole error. The only allowance is for a line on which
  File::open itself varies between opens — the chunk cache lists a damaged
  dataset's chunks in hash-map order, so which failing chunk a full read
  reports varies (cve-2025-2310.h5, the one corpus file where this shows):
  both sides must fail there, and a fresh File::open (up to 64) must
  reproduce the storage's exact error. Open errors were already compared
  in full; they still agree.
- The storage transcript may not contain ContiguousStorageRequired.
- More selections: a strided hyperslab (every third row) through
  read_f64_selection, and out-of-order points through read_selection and
  read_i64_selection.
- harness_compares_errors_not_just_failures checks the harness itself:
  two different errors are different values, and a difference File::open
  does not produce is reported.

With full errors the harness passes on the 61 fixtures and on the corpus
(701 files, 621 open).

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

CHANGELOG.md

@@ -98,7 +98,15 @@
     that open) through `File::open` and through `File::open_storage` over
     `CountingStorage`: the tree, every attribute (all, and each by name),
     every dataset's shape, types and values (all bytes, `f64`, `f32`,
-    `i64`, a hyperslab, strings, VL sequences) must be identical, and are.
+    `i64`, a box hyperslab, a strided one, out-of-order points, strings, VL
+    sequences) must be identical, and are — errors included, in full (open
+    errors and every read's). The one allowance is a line on which
+    `File::open` itself varies between two opens (the chunk cache lists a
+    damaged dataset's chunks in hash-map order, so which failing chunk a
+    full read reports varies: `cve-2025-2310.h5`), and then only if a fresh
+    `File::open` reproduces the storage's error. No storage read may answer
+    `ContiguousStorageRequired`. A storage that returns more bytes than
+    asked reads every fixture identically too.
     It also counts what one pass — open, list, read every attribute and
     every dataset once — asks of a storage with no cache: 176 092 `read_at`
     calls and 208 MB for the 621 corpus files (254 MB of files); the most

crates/clawhdf5/tests/storage_equivalence.rs

@@ -47,15 +47,15 @@ fn digest<T: std::fmt::Debug>(v: &T) -> String {
     format!("{}…[{} bytes, fnv {h:016x}]", &s[..80], s.len())
 }
 
-/// A data read's result: its value, or just `Err` — a full read goes
+/// A data read's result: its value, or its error in full (the two paths
+/// must fail the same way, not just both fail). One case is known to vary
+/// between two `File`s and is allowed for in [`check`]: a full read goes
 /// through the file's chunk cache, which lists a damaged dataset's chunks
-/// in hash-map order, so which failing chunk it reports varies from one
-/// `File` to the next (the format crate's harness compares these errors on
-/// the uncached path).
-fn value<T: std::fmt::Debug, E>(r: &Result<T, E>) -> String {
+/// in hash-map order, so which failing chunk it reports varies.
+fn value<T: std::fmt::Debug, E: std::fmt::Debug>(r: &Result<T, E>) -> String {
     match r {
         Ok(v) => digest(v),
-        Err(_) => "Err".into(),
+        Err(e) => format!("Err({e:?})"),
     }
 }
 
@@ -173,6 +173,43 @@ fn dataset(out: &mut String, path: &str, ds: &clawhdf5::Dataset<'_>) {
                 value(&ds.read_selection(&sel))
             )
             .unwrap();
+            // Every third row (a strided hyperslab), and a few points out
+            // of order: the last element, the first, one in the middle.
+            let strided = Selection::Hyperslab {
+                start: vec![0; rank],
+                stride: std::iter::once(3)
+                    .chain(std::iter::repeat_n(1, rank - 1))
+                    .collect(),
+                count: std::iter::once(d0.div_ceil(3))
+                    .chain(shape[1..].iter().copied())
+                    .collect(),
+                block: vec![1; rank],
+            };
+            writeln!(
+                out,
+                "{path} f64 strided {}",
+                value(&ds.read_f64_selection(&strided))
+            )
+            .unwrap();
+            if shape.iter().all(|&d| d > 0) {
+                let points = Selection::Points(vec![
+                    shape.iter().map(|&d| d - 1).collect(),
+                    vec![0; rank],
+                    shape.iter().map(|&d| d / 2).collect(),
+                ]);
+                writeln!(
+                    out,
+                    "{path} bytes points {}",
+                    value(&ds.read_selection(&points))
+                )
+                .unwrap();
+                writeln!(
+                    out,
+                    "{path} i64 points {}",
+                    value(&ds.read_i64_selection(&points))
+                )
+                .unwrap();
+            }
         }
     }
     match &raw_dt {
@@ -296,20 +333,16 @@ fn check(path: &Path, totals: &mut Totals) {
     assert_eq!(local.user_block_size(), remote.user_block_size(), "{name}");
     let want = transcript(&local);
     let got = transcript(&remote);
+    // Every read path works over a storage without the file in memory.
+    assert!(
+        !got.contains("ContiguousStorageRequired"),
+        "{name}: a read needed the file in memory"
+    );
     if want != got {
-        let first = want
-            .lines()
-            .zip(got.lines())
-            .find(|(w, g)| w != g)
-            .map(|(w, g)| format!("\n local:   {w}\n storage: {g}"))
-            .unwrap_or_else(|| {
-                format!(
-                    "\n {} vs {} lines",
-                    want.lines().count(),
-                    got.lines().count()
-                )
-            });
-        panic!("{name}: File::open_storage differs from File::open{first}");
+        let first = unexplained_difference(path, &want, &got);
+        if let Some(first) = first {
+            panic!("{name}: File::open_storage differs from File::open{first}");
+        }
     }
     totals.reads += storage.reads();
     totals.bytes += storage.bytes_read();
@@ -334,6 +367,40 @@ fn check(path: &Path, totals: &mut Totals) {
         .push((reads, read_bytes, bytes.len() as u64, name));
 }
 
+/// Why the storage transcript `got` differs from the `File::open` one
+/// `want`, or `None` when every line that differs is one `File::open` can
+/// give too: a line that varies between two `File`s (the chunk cache's
+/// hash-map order picks which failing chunk a damaged dataset's full read
+/// reports) and whose storage value some fresh `File::open` reproduces.
+/// Nothing else is allowed to differ.
+fn unexplained_difference(path: &Path, want: &str, got: &str) -> Option<String> {
+    let (want, got): (Vec<&str>, Vec<&str>) = (want.lines().collect(), got.lines().collect());
+    if want.len() != got.len() {
+        return Some(format!("\n {} vs {} lines", want.len(), got.len()));
+    }
+    let mut open: Vec<usize> = (0..want.len()).filter(|&i| want[i] != got[i]).collect();
+    // Only reads that fail on both sides may vary.
+    if let Some(&i) = open
+        .iter()
+        .find(|&&i| !(want[i].contains(" Err(") && got[i].contains(" Err(")))
+    {
+        return Some(format!("\n local:   {}\n storage: {}", want[i], got[i]));
+    }
+    for _ in 0..64 {
+        let again = transcript(&File::open(path).unwrap());
+        let again: Vec<&str> = again.lines().collect();
+        open.retain(|&i| again.get(i) != Some(&got[i]));
+        if open.is_empty() {
+            return None;
+        }
+    }
+    let i = open[0];
+    Some(format!(
+        "\n local:   {}\n storage: {}\n (no File::open of 64 gave the storage's result)",
+        want[i], got[i]
+    ))
+}
+
 fn report(what: &str, totals: &mut Totals) {
     eprintln!(
         "{what}: {} files ({} open, {} bytes); comparison: {} read_at calls, {} bytes; \
@@ -479,3 +546,34 @@ fn overlong_storage_reads_identically() {
     }
     assert!(compared >= 40, "{compared}");
 }
+
+/// The harness tells failures apart: two different errors are two
+/// different transcripts, and only a difference `File::open` itself
+/// produces between two opens is let through.
+#[test]
+fn harness_compares_errors_not_just_failures() {
+    let a: Result<(), FormatError> = Err(FormatError::ContiguousStorageRequired("x"));
+    let b: Result<(), FormatError> = Err(FormatError::Storage("x".into()));
+    assert_ne!(value(&a), value(&b));
+    let path = Path::new(env!("CARGO_MANIFEST_DIR"))
+        .join("../clawhdf5-format/tests/fixtures/chunked_2d.h5");
+    let want = transcript(&File::open(&path).unwrap());
+    assert_eq!(unexplained_difference(&path, &want, &want), None);
+    // A read that fails differently through the storage.
+    let line = want.lines().position(|l| l.contains(" all ")).unwrap();
+    let (mut w, mut g): (Vec<String>, Vec<String>) = (
+        want.lines().map(String::from).collect(),
+        want.lines().map(String::from).collect(),
+    );
+    w[line] = format!(
+        "{} Err(Format(DataSizeMismatch))",
+        &w[line][..w[line].find(" all ").unwrap() + 4]
+    );
+    g[line] = format!(
+        "{} Err(Format(Storage(\"injected\")))",
+        &g[line][..g[line].find(" all ").unwrap() + 4]
+    );
+    assert!(unexplained_difference(&path, &w.join("\n"), &g.join("\n")).is_some());
+    // A value against an error is never let through.
+    assert!(unexplained_difference(&path, &want, &g.join("\n")).is_some());
+}