From 75444950f3cd0dd30ac313df0ed9a06e4d4c4e91 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 19:04:56 -0500 Subject: [PATCH] clawhdf5: storage harness compares errors, not just failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 10 +- crates/clawhdf5/tests/storage_equivalence.rs | 136 ++++++++++++++++--- 2 files changed, 126 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbd2eb0..c74077f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/crates/clawhdf5/tests/storage_equivalence.rs b/crates/clawhdf5/tests/storage_equivalence.rs index 71768d2..9a5ace8 100644 --- a/crates/clawhdf5/tests/storage_equivalence.rs +++ b/crates/clawhdf5/tests/storage_equivalence.rs @@ -47,15 +47,15 @@ fn digest(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(r: &Result) -> String { +/// in hash-map order, so which failing chunk it reports varies. +fn value(r: &Result) -> 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 { + 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 = (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, Vec) = ( + 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()); +}