From 92c8285549c5c556cabe4d9506274b3a593495e7 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 14:07:01 -0500 Subject: [PATCH] format: verify B-tree v2 internal node checksums Only leaves and the header were checked. Harmless while every lookup read the whole tree, but the indexed lookup prunes children by the keys stored in internal nodes, so one corrupted byte there could route a name to the wrong child and report it missing with no error. A BTIN whose lookup3 checksum does not match is now ChecksumMismatch on every read (lookups and full traversals), as in libhdf5. Test: one byte of the root BTIN of the 35 001-link h5py group's name index changed -> lookups, paths and listings through File, MmapFile and LazyFile all fail with ChecksumMismatch, and h5py refuses both. Before, lookups returned Ok. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/btree_v2.rs | 19 +++++ .../clawhdf5/tests/indexed_lookup_interop.rs | 84 +++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/crates/clawhdf5-format/src/btree_v2.rs b/crates/clawhdf5-format/src/btree_v2.rs index 7582c05..d3f6250 100644 --- a/crates/clawhdf5-format/src/btree_v2.rs +++ b/crates/clawhdf5-format/src/btree_v2.rs @@ -355,6 +355,23 @@ fn read_internal_node( pos += total_nrec_width; // skip total records in subtree children.push((addr, child_nrec)); } + + // The checksum follows the child pointers and covers the node up to it. + // Lookups prune children by the keys in this node, so an unverified + // internal node could hide a record without any error: libhdf5 refuses + // a mismatch here, and so does this. + #[cfg(feature = "checksum")] + { + ensure_len(file_data, pos, 4)?; + let stored = LittleEndian::read_u32(&file_data[pos..pos + 4]); + let computed = crate::checksum::jenkins_lookup3(&file_data[offset..pos]); + if computed != stored { + return Err(FormatError::ChecksumMismatch { + expected: stored, + computed, + }); + } + } Ok((records_start, children)) } @@ -733,6 +750,8 @@ mod tests { buf.extend_from_slice(&child_nrec.to_le_bytes()[..nrec_width]); buf.resize(buf.len() + total_width, 0); } + let sum = crate::checksum::jenkins_lookup3(&buf); + buf.extend_from_slice(&sum.to_le_bytes()); buf } diff --git a/crates/clawhdf5/tests/indexed_lookup_interop.rs b/crates/clawhdf5/tests/indexed_lookup_interop.rs index 30adb0c..71f48f0 100644 --- a/crates/clawhdf5/tests/indexed_lookup_interop.rs +++ b/crates/clawhdf5/tests/indexed_lookup_interop.rs @@ -411,3 +411,87 @@ fn links_of_every_kind_resolve_by_name_as_in_h5py() { } } } + +/// The link name index (v2 B-tree, record type 5) of the big group: its +/// depth and root node address, read from the one type-5 `BTHD` in the file. +fn name_index_root(bytes: &[u8]) -> (u16, usize) { + let headers: Vec = bytes + .windows(4) + .enumerate() + .filter(|(i, w)| *w == b"BTHD" && bytes.get(i + 5) == Some(&5)) + .map(|(i, _)| i) + .collect(); + assert_eq!(headers.len(), 1, "type-5 B-tree headers at {headers:?}"); + let h = headers[0]; + // signature, version, type, node size (4), record size (2), depth (2), + // split and merge percent, root address (8). + let depth = u16::from_le_bytes([bytes[h + 12], bytes[h + 13]]); + let root = u64::from_le_bytes(bytes[h + 16..h + 24].try_into().unwrap()); + (depth, usize::try_from(root).unwrap()) +} + +/// One byte changed in a key of the name index's root (an internal node) +/// must be an error, not a name quietly routed to the wrong child and +/// reported missing: lookups prune children by those keys. libhdf5 checks +/// the internal node's checksum and refuses the group; so must we, for a +/// lookup and for a listing. +#[test] +fn a_corrupt_internal_index_node_is_an_error_not_a_missing_name() { + skip_if_no_python!(); + let fx = fixture(); + let mut bytes = std::fs::read(&fx.path).unwrap(); + let (depth, root) = name_index_root(&bytes); + assert!(depth >= 2, "want a deep index, got depth {depth}"); + assert_eq!(&bytes[root..root + 4], b"BTIN"); + // Signature, version, type, then record 0: its name hash comes first. + bytes[root + 6] ^= 0x5a; + let dir = tempfile::tempdir().unwrap(); + let bad = dir.path().join("bad.h5"); + std::fs::write(&bad, &bytes).unwrap(); + let bad = bad.display().to_string(); + + let is_checksum = |e: &clawhdf5::Error| { + matches!( + e, + clawhdf5::Error::Format(FormatError::ChecksumMismatch { .. }) + ) + }; + let f = File::open(&bad).unwrap(); + let g = f.group("g").unwrap(); + // Every name, present or not, goes through the root. + for name in fx.links.keys().step_by(97).chain(&fx.missing_links) { + let err = g.dataset(name).map(|_| ()).unwrap_err(); + assert!(is_checksum(&err), "dataset({name:?}): {err:?}"); + } + let err = f.dataset("/g/n0").map(|_| ()).unwrap_err(); + assert!(is_checksum(&err), "path: {err:?}"); + let err = g.datasets().unwrap_err(); + assert!(is_checksum(&err), "listing: {err:?}"); + let err = g.entries().unwrap_err(); + assert!(is_checksum(&err), "entries: {err:?}"); + + let m = MmapFile::open(&bad).unwrap(); + let mg = m.group("g").unwrap(); + assert!(mg.dataset("n0").is_err_and(|e| is_checksum(&e))); + assert!(mg.datasets().is_err_and(|e| is_checksum(&e))); + let l = LazyFile::open_mmap(&bad).unwrap(); + let lg = l.group("g").unwrap(); + assert!(lg.dataset("n0").is_err_and(|e| is_checksum(&e))); + assert!(lg.datasets().is_err_and(|e| is_checksum(&e))); + + // libhdf5 refuses both too. + let out = run_python(&format!( + "import h5py\n\ + r = []\n\ + with h5py.File(r'{bad}', 'r') as f:\n\ + \x20 g = f['g']\n\ + \x20 for op in (lambda: g['n0'], lambda: list(g)):\n\ + \x20 try:\n\ + \x20 op()\n\ + \x20 r.append('ok')\n\ + \x20 except Exception as e:\n\ + \x20 r.append('checksum' if 'checksum' in str(e) else repr(e))\n\ + print(' '.join(r))", + )); + assert_eq!(out, "checksum checksum"); +}