From a6ed3a5c7d287e444358d307bc8dc545c6aca448 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 11:36:10 -0500 Subject: [PATCH] fix(format): resolve cache-image flush-dependency parents as libhdf5 does The review suggested libhdf5 loads every image entry and resolves flush-dependency parents afterwards. It does not: H5C__reconstruct_cache_contents (HDF5 1.14.6 and 2.0.0, and develop) inserts each entry and then searches the cache index for its parents in the same loop, failing with "fd parent not in cache?!?" when one is missing. So a parent must be an earlier image entry, as before, or metadata cached before the image loads: the superblock (address 0) and the superblock extension's object header, which libhdf5 reads to find the image. Those two were refused as parents; they are now accepted. A parent listed after its child is still refused, as libhdf5 refuses it, and so is an entry that is its own parent ("Child entry flush dependency parent can't be itself"). apply_cache_image takes the superblock to know the extension address. Test: superblock_ext::tests::flush_dependency_parents_must_already_be_cached (parent-first loads, child-first refused, extension header accepted, self-parent refused); the extension-header case fails without the fix. Co-Authored-By: Claude Opus 5.5 (1M context) --- conformance/probe/src/main.rs | 2 +- crates/clawhdf5-format/src/superblock_ext.rs | 91 +++++++++++++++++--- 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/conformance/probe/src/main.rs b/conformance/probe/src/main.rs index 9fa9929..fe619a0 100644 --- a/conformance/probe/src/main.rs +++ b/conformance/probe/src/main.rs @@ -731,7 +731,7 @@ fn main() { let view = match ext.and_then(|x| x.cache_image) { None => None, Some(loc) => match guarded(|| { - superblock_ext::apply_cache_image(hdf5, loc, sb.offset_size, sb.length_size) + superblock_ext::apply_cache_image(hdf5, loc, &sb) .map_err(e) }) { Ok(v) => Some(v), diff --git a/crates/clawhdf5-format/src/superblock_ext.rs b/crates/clawhdf5-format/src/superblock_ext.rs index d7ec3cc..2db3d8b 100644 --- a/crates/clawhdf5-format/src/superblock_ext.rs +++ b/crates/clawhdf5-format/src/superblock_ext.rs @@ -297,16 +297,16 @@ struct ImageEntry { /// (`H5C__decode_cache_image_header`, `H5C__reconstruct_cache_entry`): /// signature and version, the image length it records, entry types, rings /// and ages in range, entry addresses inside the file and not repeated, -/// flush-dependency parents that are earlier entries. +/// flush-dependency parents already in the cache. /// /// libhdf5 does not verify the block's trailing checksum when it loads an /// image, so neither does this. pub fn apply_cache_image( data: &[u8], location: CacheImageLocation, - offset_size: u8, - length_size: u8, + sb: &Superblock, ) -> Result, FormatError> { + let (offset_size, length_size) = (sb.offset_size, sb.length_size); let bad = FormatError::InvalidCacheImage; let start = usize::try_from(location.address).map_err(|_| bad("address out of range"))?; let len = usize::try_from(location.length).map_err(|_| bad("length out of range"))?; @@ -336,6 +336,17 @@ pub fn apply_cache_image( } let mut entries = Vec::new(); + // What is in libhdf5's cache when it loads the image: the superblock + // and the superblock extension's object header (read to find the image). + // Each entry's flush-dependency parents are looked up in the cache as + // the entry is inserted (`H5C__reconstruct_cache_contents` searches the + // index inside the loop that inserts the entries, in HDF5 1.14.6 and + // 2.0.0 alike), so a parent must be one of those or an earlier entry. + let mut cached = BTreeSet::new(); + cached.insert(0); + if let Some(ext) = sb.superblock_extension_address { + cached.insert(ext); + } let mut seen = BTreeSet::new(); for _ in 0..n_entries { let type_id = c.u8()?; @@ -374,8 +385,8 @@ pub fn apply_cache_image( let parent = c .addr(offset_size)? .ok_or(bad("invalid flush dependency parent offset"))?; - if !seen.contains(&parent) { - return Err(bad("flush dependency parent not in the image")); + if !seen.contains(&parent) && !cached.contains(&parent) { + return Err(bad("fd parent not in cache")); } } let len = usize::try_from(size).map_err(|_| bad(RAN_OFF))?; @@ -415,7 +426,7 @@ pub fn metadata_view(data: &[u8], sb: &Superblock) -> Result>, Fo Some(SuperblockExtension { cache_image: Some(location), .. - }) => apply_cache_image(data, location, sb.offset_size, sb.length_size).map(Some), + }) => apply_cache_image(data, location, sb).map(Some), _ => Ok(None), } } @@ -562,18 +573,37 @@ mod tests { /// A cache image block with `entries` of (address, bytes). fn image(entries: &[(u64, &[u8])]) -> Vec { + let with_deps: Vec<_> = entries.iter().map(|&(a, b)| (a, b, 0, None)).collect(); + image_with_deps(&with_deps) + } + + /// A cache image block with `entries` of (address, bytes, flush + /// dependency children, flush dependency parent). + fn image_with_deps(entries: &[(u64, &[u8], u16, Option)]) -> Vec { let mut b = Vec::new(); b.extend_from_slice(MDCI_SIGNATURE); b.push(0); b.push(0); b.extend_from_slice(&0u64.to_le_bytes()); // length, patched below b.extend_from_slice(&(entries.len() as u32).to_le_bytes()); - for (addr, bytes) in entries { - b.extend_from_slice(&[5, 0x02, 1, 0]); // type, flags (in LRU), ring, age - b.extend_from_slice(&[0; 6]); // children, dirty children, parents + for &(addr, bytes, children, parent) in entries { + let mut flags = 0x02; // in LRU + if children > 0 { + flags |= MDCI_ENTRY_IS_FD_PARENT; + } + if parent.is_some() { + flags |= MDCI_ENTRY_IS_FD_CHILD; + } + b.extend_from_slice(&[5, flags, 1, 0]); // type, flags, ring, age + b.extend_from_slice(&children.to_le_bytes()); + b.extend_from_slice(&0u16.to_le_bytes()); // dirty children + b.extend_from_slice(&u16::from(parent.is_some()).to_le_bytes()); b.extend_from_slice(&0i32.to_le_bytes()); b.extend_from_slice(&addr.to_le_bytes()); b.extend_from_slice(&(bytes.len() as u64).to_le_bytes()); + if let Some(p) = parent { + b.extend_from_slice(&p.to_le_bytes()); + } b.extend_from_slice(bytes); } b.extend_from_slice(&[0; 4]); // checksum (not verified, as in libhdf5) @@ -592,7 +622,7 @@ mod tests { address: at, length: img.len() as u64, }; - let out = apply_cache_image(&f, loc, 8, 8).unwrap(); + let out = apply_cache_image(&f, loc, &sb_v2(u64::MAX)).unwrap(); assert_eq!(out.len(), f.len()); assert_eq!(&out[16..22], b"HEADER"); assert_eq!(&out[40..44], b"NODE"); @@ -605,7 +635,7 @@ mod tests { address: 64, length: img.len() as u64, }; - apply_cache_image(&f, loc, 8, 8).unwrap_err() + apply_cache_image(&f, loc, &sb_v2(u64::MAX)).unwrap_err() }; let mut sig = image(&[(16, b"x")]); sig[0] = b'X'; @@ -627,4 +657,43 @@ mod tests { cut[6..14].copy_from_slice(&n.to_le_bytes()); assert!(matches!(bad(cut), FormatError::InvalidCacheImage(_))); } + + /// libhdf5 resolves an entry's flush-dependency parents as it inserts + /// the entry (`H5C__reconstruct_cache_contents`): a parent must be an + /// earlier entry, or the superblock or its extension's object header, + /// which are cached before the image loads. A parent listed after its + /// child fails ("fd parent not in cache?!?"). + #[test] + fn flush_dependency_parents_must_already_be_cached() { + let load = |img: Vec| { + let mut f = vec![0u8; 64]; + f.extend_from_slice(&img); + let loc = CacheImageLocation { + address: 64, + length: img.len() as u64, + }; + apply_cache_image(&f, loc, &sb_v2(48)) + }; + // Parent first, as libhdf5 writes images. + assert!( + load(image_with_deps(&[ + (16, b"P", 1, None), + (40, b"C", 0, Some(16)) + ])) + .is_ok() + ); + // Child first: libhdf5 does not find the parent. + assert_eq!( + load(image_with_deps(&[ + (40, b"C", 0, Some(16)), + (16, b"P", 1, None) + ])) + .unwrap_err(), + FormatError::InvalidCacheImage("fd parent not in cache") + ); + // The superblock extension's header (at 48 here) is in the cache. + assert!(load(image_with_deps(&[(40, b"C", 0, Some(48))])).is_ok()); + // An entry cannot be its own parent. + assert!(load(image_with_deps(&[(40, b"C", 1, Some(40))])).is_err()); + } }