From b8492bd28d997c464da794cc278e55c7966ba5c7 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:22:27 -0500 Subject: [PATCH] fix(tools): h5rs check --data follows VL data into the global heap The README said check skips only "global heap collections other than those a value read touches", but read_dataset returns the raw heap IDs, so no collection was ever read: a file whose global heap collection claims a 4 GiB object passed `check --data` with no problems, while h5dump (and h5rs dump/diff) fail on it. With --data, every variable-length element (strings and sequences, also inside compounds, arrays and nested sequences) of every dataset and attribute is followed into its collection. A collection that does not parse, a missing heap object, or a sequence longer than its heap object is a problem at the collection's address, once per object; the summary counts the collections read. Measured on tank, 2026-09-26: the 418 fully-read conformance ok files still pass (scripts/h5rs-check-ok-files.sh --data, 0 flagged), and `check --data` now flags 152 of the 180 CVE-corpus files (was 147); of the 28 it passes, h5dump 1.14.6 rejects 21 (was 26 of 33). Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 6 +- crates/clawhdf5-tools/README.md | 13 +- crates/clawhdf5-tools/src/check.rs | 180 +++++++++++++++++++- crates/clawhdf5-tools/tests/h5rs_interop.rs | 38 +++++ docs/known-issues.md | 4 +- 5 files changed, 228 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff031e6..a8203d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -151,8 +151,10 @@ printed with its address; exit 1 when there are any. libhdf5's h5check reads only the 1.8 format. On the conformance corpus it passes all 418 files that both clawhdf5 and h5py read in full, and `check --data` flags - 147 of the 180 files of the CVE corpus (tank, 2026-09-26). It inherits - the library's tolerance, though: 26 of the 33 it passes are files + 152 of the 180 files of the CVE corpus (tank, 2026-09-26). `--data` also + follows variable-length data into its global heap collections and + reports a damaged one at its address. It inherits the library's + tolerance, though: 21 of the 28 it passes are files h5dump 1.14.6 rejects (see `docs/known-issues.md`, header checks). - Values over `--max-bytes` (default 1 GiB) are reported instead of read; a panic is caught and reported as an internal error (exit 3). diff --git a/crates/clawhdf5-tools/README.md b/crates/clawhdf5-tools/README.md index 4f6460c..8b94845 100644 --- a/crates/clawhdf5-tools/README.md +++ b/crates/clawhdf5-tools/README.md @@ -216,7 +216,12 @@ extension) and checks: no two pieces overlap. `--data` also reads every dataset, decoding every chunk through its filters -(which catches corrupt compressed data and Fletcher-32 mismatches). Data the +(which catches corrupt compressed data and Fletcher-32 mismatches), and +follows every variable-length element (strings and sequences, also inside +compounds and arrays) of every dataset and attribute into its global heap +collection: a collection that does not parse, a missing heap object, or a +sequence longer than its heap object is a problem at the collection's +address. Data the tool cannot decode (a filter it does not implement, such as szip, or a dataset over `--max-bytes`) is a `note:`, not a problem. Every problem is printed with the address of the structure involved; the exit status is 0 @@ -228,11 +233,11 @@ also covers the structures HDF5 1.10+ writes (fixed/extensible array and v2 B-tree chunk indexes, and version 3 superblocks). What it does not check: free-space manager and shared-message (SOHM) table -checksums, global heap collections other than those a value read touches, -and objects reachable only by external links. It validates with +checksums, global heap collections no variable-length value points into (and +none at all without `--data`), and objects reachable only by external links. It validates with clawhdf5's parsers, so it accepts what they accept: some header damage that libhdf5 refuses goes unreported (of the 180 files of the HDF Group's CVE -corpus, `check --data` passes 33, and h5dump 1.14.6 rejects 26 of those; +corpus, `check --data` passes 28, and h5dump 1.14.6 rejects 21 of those; tank, 2026-09-26). ## Robustness diff --git a/crates/clawhdf5-tools/src/check.rs b/crates/clawhdf5-tools/src/check.rs index 635b234..b5dd3c5 100644 --- a/crates/clawhdf5-tools/src/check.rs +++ b/crates/clawhdf5-tools/src/check.rs @@ -7,7 +7,7 @@ //! data lies inside the file without overlapping other raw data. Every //! problem is reported with the address of the structure involved. -use std::collections::HashSet; +use std::collections::{BTreeMap, HashSet}; use std::panic::{self, AssertUnwindSafe}; use clawhdf5_format::attribute_info::AttributeInfoMessage; @@ -39,7 +39,8 @@ problem is printed with the address of the structure involved. --data also read every dataset, decoding every chunk through its filters (catches corrupt compressed data and Fletcher-32 - mismatches) + mismatches), and follow every variable-length element of + datasets and attributes into its global heap collection -q, --quiet print only the problems, not the summary --max-bytes N largest dataset read by --data (default 1 GiB) @@ -48,6 +49,21 @@ found, 3 internal error."; const MAX_CHUNKS_CHECKED: usize = 10_000_000; +/// Whether values of `dt` hold variable-length data (in the global heap). +fn has_vl(dt: &Datatype, depth: u32) -> bool { + if depth > 32 { + return false; + } + match dt { + Datatype::VariableLength { .. } => true, + Datatype::Compound { members, .. } => { + members.iter().any(|m| has_vl(&m.datatype, depth + 1)) + } + Datatype::Array { base_type, .. } => has_vl(base_type, depth + 1), + _ => false, + } +} + #[derive(Default)] struct Counts { objects: u64, @@ -57,6 +73,9 @@ struct Counts { messages: u64, chunks: u64, datasets_read: u64, + /// Global heap collections that variable-length data points into, + /// parsed without error (with --data). + global_heaps: u64, sb_checksum: u64, ohdr_v2: u64, btree_v2: u64, @@ -83,6 +102,8 @@ struct Checker<'a> { extents: Vec<(u64, u64, String)>, heaps_seen: HashSet, btrees_seen: HashSet, + /// Global heap collections already read (with --data). + gcols_seen: HashSet, panicked: bool, } @@ -135,6 +156,7 @@ pub fn run(args: &mut Args, out: &mut Out) -> std::io::Result { extents: Vec::new(), heaps_seen: HashSet::new(), btrees_seen: HashSet::new(), + gcols_seen: HashSet::new(), panicked: false, }; c.superblock(); @@ -318,10 +340,16 @@ impl Checker<'_> { } self.messages(addr, path, h); match self.h5.attributes(h) { - Ok((_, errs)) => { + Ok((attrs, errs)) => { for e in errs { self.problem(addr, path, format!("attribute: {e}")); } + if self.read_data { + for a in &attrs { + let what = format!("attribute \"{}\": ", a.name); + self.vl_data(addr, path, &what, &a.datatype, &a.dataspace, &a.raw_data); + } + } } Err(e) => self.err(addr, path, &e), } @@ -598,7 +626,10 @@ impl Checker<'_> { } if self.read_data { match self.h5.read_dataset(path, dt, ds) { - Ok(_) => self.counts.datasets_read += 1, + Ok(raw) => { + self.counts.datasets_read += 1; + self.vl_data(addr, path, "", dt, ds, &raw); + } // Valid data this tool cannot decode is not a problem with // the file. Err(e) if e.kind != ErrorKind::Corrupt => self.notes.push(Problem { @@ -611,6 +642,141 @@ impl Checker<'_> { } } + /// With --data: follow every variable-length element of `raw` (the + /// values of a dataset or attribute) into the global heap, so a damaged + /// collection, a missing heap object or a sequence longer than its heap + /// object is reported at the collection's address. Each bad collection + /// is reported once per object. + fn vl_data( + &mut self, + addr: u64, + path: &str, + what: &str, + dt: &Datatype, + ds: &Dataspace, + raw: &[u8], + ) { + if !has_vl(dt, 0) { + return; + } + let n = crate::h5::num_elements(ds).unwrap_or(0); + let size = dt.type_size() as usize; + let mut bad: BTreeMap = BTreeMap::new(); + for i in 0..n { + let Some(b) = usize::try_from(i) + .ok() + .and_then(|i| i.checked_mul(size)) + .and_then(|s| raw.get(s..s.checked_add(size)?)) + else { + self.problem( + addr, + path, + format!("{what}element {i} is past the data read"), + ); + break; + }; + self.vl_element(dt, b, 0, &mut bad); + if bad.len() >= 100 { + break; + } + } + for (a, msg) in bad { + self.problem(a, path, format!("{what}variable-length data: {msg}")); + } + } + + fn vl_element(&mut self, dt: &Datatype, b: &[u8], depth: u32, bad: &mut BTreeMap) { + if depth > 32 { + return; + } + match dt { + Datatype::VariableLength { + is_string, + base_type, + .. + } => { + let os = usize::from(self.h5.os()); + let (Some(lenb), Some(addrb), Some(idxb)) = + (b.get(..4), b.get(4..4 + os), b.get(4 + os..8 + os)) + else { + return; + }; + let le = |x: &[u8]| { + x.iter() + .enumerate() + .fold(0u64, |a, (i, &v)| a | (u64::from(v) << (8 * i))) + }; + let (len, gcol, idx) = (le(lenb), le(addrb), le(idxb)); + let undef = if os >= 8 { + u64::MAX + } else { + (1u64 << (8 * os)) - 1 + }; + if len == 0 || gcol == 0 || gcol == undef || bad.contains_key(&gcol) { + return; + } + let obj = match self.h5.heap_object(gcol, idx as u32) { + Ok(o) => o, + Err(e) => { + bad.insert(e.addr.unwrap_or(gcol), e.msg); + return; + } + }; + if self.gcols_seen.insert(gcol) { + self.counts.global_heaps += 1; + } + let bs = if *is_string { + 1 + } else { + u64::from(base_type.type_size()) + }; + if len + .checked_mul(bs) + .is_none_or(|need| need > obj.len() as u64) + { + bad.insert( + gcol, + format!( + "global heap object {idx} holds {} bytes; the element needs {len} x {bs}", + obj.len() + ), + ); + return; + } + if !*is_string && bs > 0 && has_vl(base_type, depth + 1) { + let bs = bs as usize; + for k in 0..len as usize { + self.vl_element(base_type, &obj[k * bs..(k + 1) * bs], depth + 1, bad); + } + } + } + Datatype::Compound { members, .. } => { + for m in members { + if let Some(mb) = usize::try_from(m.byte_offset).ok().and_then(|o| b.get(o..)) { + self.vl_element(&m.datatype, mb, depth + 1, bad); + } + } + } + Datatype::Array { + base_type, + dimensions, + } => { + let bs = base_type.type_size() as usize; + let n = dimensions + .iter() + .try_fold(1usize, |a, &d| a.checked_mul(d as usize)) + .unwrap_or(usize::MAX); + for k in 0..n { + match b.get(k * bs..(k + 1) * bs) { + Some(eb) => self.vl_element(base_type, eb, depth + 1, bad), + None => break, + } + } + } + _ => {} + } + } + fn chunked( &mut self, addr: u64, @@ -814,7 +980,11 @@ impl Checker<'_> { c.chunk_index_checksummed )?; if self.read_data { - writeln!(out.o, "datasets read: {}", c.datasets_read)?; + writeln!( + out.o, + "datasets read: {}, global heap collections read: {}", + c.datasets_read, c.global_heaps + )?; } match self.problems.len() { 0 => writeln!(out.o, "no problems found"), diff --git a/crates/clawhdf5-tools/tests/h5rs_interop.rs b/crates/clawhdf5-tools/tests/h5rs_interop.rs index b82eb65..c325533 100644 --- a/crates/clawhdf5-tools/tests/h5rs_interop.rs +++ b/crates/clawhdf5-tools/tests/h5rs_interop.rs @@ -639,6 +639,44 @@ fn check_flags_every_corrupted_checksum() { } } +/// `check --data` follows variable-length elements into the global heap: +/// a damaged collection is reported at its address, for the dataset and +/// the attribute that point into it. Without --data it is not read. +#[test] +fn check_data_follows_vl_data_into_the_global_heap() { + let Some(f) = generate() else { return }; + for name in LIBVERS { + let mut data = std::fs::read(f.path(name)).unwrap(); + let gcols: Vec = (0..data.len() - 4) + .filter(|&i| &data[i..i + 4] == b"GCOL") + .collect(); + assert!(!gcols.is_empty(), "{name}: no global heap"); + // "GCOL" version(1) reserved(3) size(8), then heap object 1: + // index(2) refcount(2) reserved(4) size(8) — claim 4 GiB. + let at = gcols[0]; + data[at + 24..at + 28].fill(0xff); + let bad = f.path(&format!("bad-gcol-{name}")); + std::fs::write(&bad, &data).unwrap(); + let bad = bad.to_string_lossy().into_owned(); + let o = h5rs(&["check", "--data", &bad]); + let s = stdout(&o); + assert_eq!(code(&o), 1, "{name}: {s}"); + let want = format!("problem: {at:#x} /vlstr: variable-length data: global heap"); + assert!(s.contains(&want), "{name}: no {want:?} in\n{s}"); + // h5dump refuses the file too. + if tool_available("h5dump") { + assert_ne!(code(&run("h5dump", &[&bad])), 0, "{name}: h5dump read it"); + } + assert_eq!(code(&h5rs(&["check", &bad])), 0, "{name}: without --data"); + let ok = h5rs(&["check", "--data", &f.p(name)]); + assert!( + stdout(&ok).contains(&format!("global heap collections read: {}", gcols.len())), + "{name}: {}", + stdout(&ok) + ); + } +} + #[test] fn check_flags_a_truncated_file() { let Some(f) = generate() else { return }; diff --git a/docs/known-issues.md b/docs/known-issues.md index 077dbfd..1500a63 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -137,8 +137,8 @@ fill-value item that did is fixed). - **Header checks:** on 12 CVE datasets libhdf5 rejects a corrupt header and we read data anyway. We need stricter header checks. The same gap shows in `h5rs check`, which validates with the library's parsers: of the 180 - files of the CVE corpus, `check --data` passes 33, and h5dump 1.14.6 - rejects 26 of those (measured on tank, 2026-09-26, with + files of the CVE corpus, `check --data` passes 28, and h5dump 1.14.6 + rejects 21 of those (measured on tank, 2026-09-26, with `h5rs check --data F` and `h5dump F` per file). - **Writer:** - Nested groups beyond one level: path-like names are now refused, not