From efc2dc53c969ef57b56a29de5469161016b1d8d8 Mon Sep 17 00:00:00 2001 From: osobh Date: Fri, 25 Sep 2026 21:59:54 -0500 Subject: [PATCH 1/4] fix(format): read array members of version-1 compound datatypes HDF5 1.6 encoded a compound member that is a fixed-size array through legacy per-member fields (dimensionality, permutation, four dimension sizes) that the v1 decoder skipped, so a [4] i32 member read as one i32 with the wrong size. Build the array type from those fields as libhdf5 does (ignoring the permutation) and refuse more than four dimensions. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 6 ++ crates/clawhdf5-format/src/datatype.rs | 91 +++++++++++++++++++++++++- docs/known-issues.md | 2 + 3 files changed, 98 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b87415..ac26b3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -273,6 +273,12 @@ - CI keeps zlib-ng building and tested; the arm64 job no longer needs cmake. ### Correctness +- `clawhdf5-format` reader: array members of version-1 compound datatypes + (HDF5 1.6-era files, e.g. libhdf5's `tcompound.h5`) were read as a single + element: a `[4] i32` member came back as one `i32`, with the wrong size. + The legacy per-member dimension fields are now decoded into an array type, + as libhdf5 does; more than four dimensions, or a zero-sized one, is an + error. - `clawhdf5-format` reader — **values returned wrong with no error:** - Fixed Array and Extensible Array chunk indexes were laid out by the dataset's current shape instead of its max shape (23 libhdf5 test files, diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index 436a6cf..ba85afa 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -423,13 +423,44 @@ impl Datatype { ensure_len(data, pos, 4)?; let byte_offset = LittleEndian::read_u32(&data[pos..pos + 4]) as u64; pos += 4; + // v1 members can be fixed-size arrays of the member + // type (libhdf5 builds an array type from these + // fields; the permutation is ignored, as libhdf5 + // does). Skipping them read a `[4] i32` member as + // one `i32`. + let mut array_dims = Vec::new(); if version == 1 { ensure_len(data, pos, 28)?; + let ndims = data[pos] as usize; + // libhdf5 refuses more than four dimensions and, + // when building the array type, a zero-sized one. + let zero_dim = (0..ndims.min(4)).any(|j| { + let at = pos + 12 + 4 * j; + LittleEndian::read_u32(&data[at..at + 4]) == 0 + }); + if ndims > 4 || zero_dim { + return Err(FormatError::InvalidDatatypeVersion { + class: class_id, + version, + }); + } + array_dims = (0..ndims) + .map(|j| { + let at = pos + 12 + 4 * j; + LittleEndian::read_u32(&data[at..at + 4]) + }) + .collect(); pos += 28; } - let (member_dt, consumed) = + let (mut member_dt, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?; pos += consumed; + if !array_dims.is_empty() { + member_dt = Datatype::Array { + base_type: Box::new(member_dt), + dimensions: array_dims, + }; + } members.push(CompoundMember { name, byte_offset, @@ -1336,6 +1367,64 @@ mod tests { assert_xyid_compound(dt); } + #[test] + fn test_compound_v1_member_array_fields() { + // HDF5 1.6 wrote array members of a v1 compound through the legacy + // per-member fields (as in libhdf5's tools/test/testfiles/ + // tcompound.h5 `type2`: `int_array` [4] i32, `float_array` [5][6] + // f32). They used to be skipped, reading each member as a scalar. + let i32le: [u8; 12] = [ + 0x10, 0x08, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, + ]; + let mut b = vec![0x16, 0x02, 0x00, 0x00, 0x88, 0x00, 0x00, 0x00]; + for (name, offset, dims) in [ + (&b"int_array"[..], 0u32, &[4u32][..]), + (&b"xy"[..], 16, &[5u32, 6][..]), + ] { + let mut padded = name.to_vec(); + padded.resize((name.len() + 1 + 7) & !7, 0); + b.extend_from_slice(&padded); + b.extend_from_slice(&offset.to_le_bytes()); + b.push(dims.len() as u8); + b.extend_from_slice(&[0u8; 3 + 4 + 4]); // reserved, permutation, reserved + for j in 0..4 { + b.extend_from_slice(&dims.get(j).copied().unwrap_or(0).to_le_bytes()); + } + b.extend_from_slice(&i32le); + } + let (dt, consumed) = Datatype::parse(&b).unwrap(); + assert_eq!(consumed, b.len()); + let Datatype::Compound { members, .. } = dt else { + panic!("expected Compound, got {dt:?}"); + }; + let got: Vec<(&str, u64, u32, Option>)> = members + .iter() + .map(|m| { + let dims = match &m.datatype { + Datatype::Array { dimensions, .. } => Some(dimensions.clone()), + _ => None, + }; + (m.name.as_str(), m.byte_offset, m.datatype.type_size(), dims) + }) + .collect(); + assert_eq!( + got, + vec![ + ("int_array", 0, 16, Some(vec![4])), + ("xy", 16, 120, Some(vec![5, 6])), + ] + ); + + // More than four dimensions cannot be encoded, and libhdf5 refuses a + // zero-sized dimension (a fuzzed tcompound.h5, cve-2024-32616.h5). + let mut bad = b.clone(); + bad[8 + 16 + 4] = 5; + assert!(Datatype::parse(&bad).is_err()); + let mut bad = b.clone(); + bad[8 + 16 + 4] = 2; // [4, 0] + assert!(Datatype::parse(&bad).is_err()); + } + #[test] fn test_compound_v1_truncated_is_error_not_panic() { let bytes = compound_v1_bytes(); diff --git a/docs/known-issues.md b/docs/known-issues.md index 6dfa369..660dab0 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -74,6 +74,8 @@ the VDS item, which is marked. - Hyperslab selection versions 1 and 2 are refused. - **Files with a user block:** the base address is not applied. - **Old-style shared messages (version 1)** read the wrong address. +- **Array members of version-1 compound datatypes** (found while fixing the + item above) were read as one element. **Fixed 2026-09-25.** - **Groups and links:** - Groups with a user-defined link type (e.g. 187) cannot be listed. - Dense groups with more than about 22 000 links cannot be listed. From 055579485041e96862386ae7a268d4f481b05482 Mon Sep 17 00:00:00 2001 From: osobh Date: Fri, 25 Sep 2026 22:01:22 -0500 Subject: [PATCH 2/4] fix(format): read version-1 shared message addresses after the heap offset A version-1 shared message (HDF5 1.6) embeds the target as a symbol-table entry: after six reserved bytes comes a length-sized local-heap offset, then the object header address. We read the heap offset as the address, so datasets using a committed datatype in 1.6-era files (tcompound.h5, tcompound2.h5) failed with InvalidObjectHeaderVersion. parse_shared_ref now takes length_size and skips the offset, as libhdf5 does. Resolving a reference also no longer falls back to the first message of any type in the target header: a missing target message is SharedMessageTargetMissing instead of garbage. Fixture: tcompound.h5 from libhdf5's tools/test/testfiles (8 KiB). Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 9 ++ crates/clawhdf5-format/src/attribute.rs | 5 +- crates/clawhdf5-format/src/error.rs | 8 ++ crates/clawhdf5-format/src/shared_message.rs | 67 +++++----- .../tests/fixtures/tcompound.h5 | Bin 0 -> 8192 bytes crates/clawhdf5/tests/shared_message_v1.rs | 121 ++++++++++++++++++ docs/known-issues.md | 3 + 7 files changed, 178 insertions(+), 35 deletions(-) create mode 100644 crates/clawhdf5-format/tests/fixtures/tcompound.h5 create mode 100644 crates/clawhdf5/tests/shared_message_v1.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ac26b3b..6b054b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -273,6 +273,15 @@ - CI keeps zlib-ng building and tested; the arm64 job no longer needs cmake. ### Correctness +- `clawhdf5-format` reader: version-1 shared messages (HDF5 1.6-era files, + e.g. a dataset using a committed datatype in libhdf5's `tcompound.h5`) + read the heap-offset field of the embedded symbol-table entry as the + target address and failed with `InvalidObjectHeaderVersion`. The address + is now read after it, as libhdf5 does. **Breaking (format crate):** + `shared_message::parse_shared_ref` takes `length_size`. A reference whose + target header has no message of the referenced type is now + `FormatError::SharedMessageTargetMissing` instead of returning the first + other message found there (which decoded as garbage). - `clawhdf5-format` reader: array members of version-1 compound datatypes (HDF5 1.6-era files, e.g. libhdf5's `tcompound.h5`) were read as a single element: a `[4] i32` member came back as one `i32`, with the wrong size. diff --git a/crates/clawhdf5-format/src/attribute.rs b/crates/clawhdf5-format/src/attribute.rs index bb96bd5..06132bb 100644 --- a/crates/clawhdf5-format/src/attribute.rs +++ b/crates/clawhdf5-format/src/attribute.rs @@ -97,7 +97,7 @@ impl AttributeMessage { return Ok(Cow::Borrowed(bytes)); } let (file_data, offset_size) = file.ok_or(FormatError::UnresolvedSharedMessage)?; - let shared_ref = shared_message::parse_shared_ref(bytes, offset_size)?; + let shared_ref = shared_message::parse_shared_ref(bytes, offset_size, length_size)?; shared_message::resolve_shared_message( file_data, &shared_ref, @@ -407,7 +407,8 @@ pub fn extract_attributes_full( if msg.msg_type == MessageType::Attribute { if shared_message::is_shared(msg.flags) { // Shared attribute: resolve the reference to get actual attribute data - let shared_ref = shared_message::parse_shared_ref(&msg.data, offset_size)?; + let shared_ref = + shared_message::parse_shared_ref(&msg.data, offset_size, length_size)?; let resolved_data = shared_message::resolve_shared_message( file_data, &shared_ref, diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index 6d10c3d..7ae57f4 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -117,6 +117,9 @@ pub enum FormatError { /// A message is marked shared but was parsed without access to the file, /// so the reference to the real message could not be followed. UnresolvedSharedMessage, + /// A shared-message reference points at an object header that holds no + /// (unshared) message of the referenced type (raw message type id). + SharedMessageTargetMissing(u16), /// A selection does not fit the dataset it was applied to (wrong rank, or /// it reaches past a dimension's extent). SelectionOutOfBounds(String), @@ -339,6 +342,11 @@ impl fmt::Display for FormatError { FormatError::SelectionOutOfBounds(msg) => { write!(f, "selection out of bounds: {msg}") } + FormatError::SharedMessageTargetMissing(t) => write!( + f, + "shared message reference points at an object header with no message of type \ + {t:#06x}" + ), FormatError::UnresolvedSharedMessage => write!( f, "message is shared but no file data was available to resolve it" diff --git a/crates/clawhdf5-format/src/shared_message.rs b/crates/clawhdf5-format/src/shared_message.rs index 33334fa..a77edc1 100644 --- a/crates/clawhdf5-format/src/shared_message.rs +++ b/crates/clawhdf5-format/src/shared_message.rs @@ -154,13 +154,24 @@ pub fn is_shared(msg_flags: u8) -> bool { /// /// When the shared flag is set on a message, the data contains a reference /// instead of the actual message content. -pub fn parse_shared_ref(data: &[u8], offset_size: u8) -> Result { +/// +/// `length_size` is needed for version 1 references, which embed a +/// symbol-table-entry-shaped pointer whose first field is a length-sized +/// heap offset. +pub fn parse_shared_ref( + data: &[u8], + offset_size: u8, + length_size: u8, +) -> Result { ensure_len(data, 0, 2)?; let version = data[0]; let ref_type = data[1]; // Layouts (HDF5 spec IV.A.2 "Shared Message", and libhdf5's decoder): - // v1: version, type, reserved(6), address — always "committed" + // v1: version, type, reserved(6), then the HDF5 1.6 "symbol table + // entry" encoding of the target: a length-sized local-heap name + // offset (unused, skipped by libhdf5) followed by the object + // header address — always "committed" // v2: version, type, address — always "committed" // v3: version, type, then a fractal-heap ID if type == SOHM, otherwise // an address @@ -177,7 +188,7 @@ pub fn parse_shared_ref(data: &[u8], offset_size: u8) -> Result address_at(2 + 6), + 1 => address_at(2 + 6 + length_size as usize), 2 => address_at(2), 3 if ref_type == SHARE_TYPE_SOHM => { ensure_len(data, 2, FHEAP_ID_LEN)?; @@ -434,7 +445,7 @@ pub fn message_data_with_sohm<'a>( if !is_shared(msg.flags) { return Ok(Cow::Borrowed(&msg.data)); } - let shared_ref = parse_shared_ref(&msg.data, offset_size)?; + let shared_ref = parse_shared_ref(&msg.data, offset_size, length_size)?; let table = if shared_ref.heap_id.is_some() { load_sohm_table(file_data, offset_size, length_size)? } else { @@ -514,7 +525,7 @@ pub fn message_data<'a>( if !is_shared(msg.flags) { return Ok(Cow::Borrowed(&msg.data)); } - let shared_ref = parse_shared_ref(&msg.data, offset_size)?; + let shared_ref = parse_shared_ref(&msg.data, offset_size, length_size)?; resolve_shared_message( file_data, &shared_ref, @@ -571,24 +582,13 @@ pub fn resolve_shared_message_with_sohm( return Ok(msg.data.clone()); } } - // The message at that OH address is the message itself - // In many cases with type 1, the entire OH at that address IS the shared message - // Try returning the first message of any type that isn't Nil - for msg in &target_header.messages { - if msg.msg_type == target_msg_type { - return Ok(msg.data.clone()); - } - } - // Fall back to first non-nil message - for msg in &target_header.messages { - if msg.msg_type != MessageType::Nil { - return Ok(msg.data.clone()); - } - } - Err(FormatError::UnexpectedEof { - expected: 1, - available: 0, - }) + // The referenced header has no (unshared) message of the wanted + // type: the reference is wrong or the file is damaged. Handing + // back some other message's bytes — or another reference's — + // would decode as garbage, so refuse. + Err(FormatError::SharedMessageTargetMissing( + target_msg_type.to_u16(), + )) } (None, Some(heap_id)) => { let table = sohm_table.ok_or(FormatError::InvalidSharedMessageVersion(2))?; @@ -627,7 +627,7 @@ mod tests { data.push(SHARE_TYPE_COMMITTED); // message lives in another object header data.extend_from_slice(&0x1234u64.to_le_bytes()); // address - let shared = parse_shared_ref(&data, 8).unwrap(); + let shared = parse_shared_ref(&data, 8, 8).unwrap(); assert_eq!(shared.version, 3); assert_eq!(shared.ref_type, SHARE_TYPE_COMMITTED); assert_eq!(shared.object_header_address, Some(0x1234)); @@ -641,7 +641,7 @@ mod tests { data.push(SHARE_TYPE_HERE); // stored here but sharable: an address data.extend_from_slice(&0xABCDu64.to_le_bytes()); - let shared = parse_shared_ref(&data, 8).unwrap(); + let shared = parse_shared_ref(&data, 8, 8).unwrap(); assert_eq!(shared.version, 3); assert_eq!(shared.ref_type, 3); assert_eq!(shared.object_header_address, Some(0xABCD)); @@ -653,9 +653,10 @@ mod tests { data.push(1); // version data.push(0); // type data.extend_from_slice(&[0u8; 6]); // reserved + data.extend_from_slice(&0x10u64.to_le_bytes()); // heap name offset data.extend_from_slice(&0x5678u64.to_le_bytes()); - let shared = parse_shared_ref(&data, 8).unwrap(); + let shared = parse_shared_ref(&data, 8, 8).unwrap(); assert_eq!(shared.version, 1); assert_eq!(shared.object_header_address, Some(0x5678)); } @@ -668,7 +669,7 @@ mod tests { data.push(SHARE_TYPE_COMMITTED); data.extend_from_slice(&0x9000u32.to_le_bytes()); - let shared = parse_shared_ref(&data, 4).unwrap(); + let shared = parse_shared_ref(&data, 4, 4).unwrap(); assert_eq!(shared.version, 2); assert_eq!(shared.object_header_address, Some(0x9000)); } @@ -679,7 +680,7 @@ mod tests { // as written by h5py 3.16 / HDF5 2.0 (libver='latest'): header flags // 0x03 (shared), payload `02 02 <8-byte object header address>`. let data = [0x02, 0x02, 0xb3, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; - let shared = parse_shared_ref(&data, 8).unwrap(); + let shared = parse_shared_ref(&data, 8, 8).unwrap(); assert_eq!(shared.object_header_address, Some(0xb3)); assert!(shared.heap_id.is_none()); } @@ -691,7 +692,7 @@ mod tests { data.push(SHARE_TYPE_SOHM); // message lives in the SOHM fractal heap data.extend_from_slice(&[0xAA, 0xBB, 0xCC, 0xDD, 0x11, 0x22, 0x33, 0x44]); - let shared = parse_shared_ref(&data, 8).unwrap(); + let shared = parse_shared_ref(&data, 8, 8).unwrap(); assert_eq!(shared.version, 3); assert_eq!(shared.ref_type, SHARE_TYPE_SOHM); assert_eq!(shared.object_header_address, None); @@ -708,21 +709,21 @@ mod tests { data.push(SHARE_TYPE_SOHM); data.extend_from_slice(&[0xAA, 0xBB]); // only 2 bytes, need 8 - let err = parse_shared_ref(&data, 8).unwrap_err(); + let err = parse_shared_ref(&data, 8, 8).unwrap_err(); assert!(matches!(err, FormatError::UnexpectedEof { .. })); } #[test] fn invalid_version() { let data = vec![99, 0]; - let err = parse_shared_ref(&data, 8).unwrap_err(); + let err = parse_shared_ref(&data, 8, 8).unwrap_err(); assert_eq!(err, FormatError::InvalidSharedMessageVersion(99)); } #[test] fn truncated_data() { let data = vec![3u8]; // too short - let err = parse_shared_ref(&data, 8).unwrap_err(); + let err = parse_shared_ref(&data, 8, 8).unwrap_err(); assert!(matches!(err, FormatError::UnexpectedEof { .. })); } @@ -733,7 +734,7 @@ mod tests { data.push(SHARE_TYPE_COMMITTED); data.extend_from_slice(&0x1000u32.to_le_bytes()); - let shared = parse_shared_ref(&data, 4).unwrap(); + let shared = parse_shared_ref(&data, 4, 4).unwrap(); assert_eq!(shared.object_header_address, Some(0x1000)); } diff --git a/crates/clawhdf5-format/tests/fixtures/tcompound.h5 b/crates/clawhdf5-format/tests/fixtures/tcompound.h5 new file mode 100644 index 0000000000000000000000000000000000000000..d1ec6504cafee27eeda2e6ea97f95bd9bfc8c97d GIT binary patch literal 8192 zcmeHMJ8u&~5T5g0f(Z{f1xa`x9i>SEazlJ45tIT!#G}Rr!b5^23PFMjmmpE31giXu zl(a}eN`pk{G9@J)d~-8%an84P3`B{*Bkj)Y?CkB_?9A-m-rJcgSC0&x7$SyZkpe1_ zpERWUsX$?-tuku`B^+pGI-cdOn)a6!ubooDfo|WNo+k3h<~MBOGl5W{G5YwwvVcbg zcn6tV(lGp%+wav1HN}QJ8ch17BKY`PLXN=MOAxBxov%NeGif(29VEmELrC{@jJl$8 z(D1pl>6pKf|~<^&kLfp<3gC+`%!7v6!RJ~tmrstwb!AtHYMY=3+yq+gJ-ho zBGtpzc~;kXCDkuXsZK;T|C&9U`aIXzZuxf=all~fD6M||zgWPPf3xvx_Tb!*#I6Rg zPvzvCVe!1v`1Lfuc{@K6@@3NqlO`Yn%>)U#+R?iKc`@7BLy@o!PkzYfK(eevs=!!jX{=do{e{;iXD%GgbUmEkw>`79BEh*0r4 zB3ecHghD8&w(*+zyqIMhD61%+P?|){<8w$JFAmYk9}TJla1f!|&H1u=#Ub=7W6~BK zKo8{U@H_A4ny=M1aVR>(5oYG2t{ho8+d(t zK>&|)4xqqi8i3C^1rTtqwEGvNwFJygvuBmzK;7 String { + std::env::var("CLAWHDF5_PYTHON").unwrap_or_else(|_| "python3".to_string()) +} + +fn interop_required() -> bool { + std::env::var("CLAWHDF5_REQUIRE_INTEROP").is_ok_and(|v| v == "1") +} + +fn python_available() -> bool { + Command::new(python()) + .args(["-c", "import h5py, numpy"]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +fn compound(fields: &[(&str, DType)]) -> DType { + DType::Compound( + fields + .iter() + .map(|(n, t)| (n.to_string(), t.clone())) + .collect(), + ) +} + +fn expected() -> Vec<(&'static str, DType)> { + let int_float = |i: &str, f: &str| compound(&[(i, DType::I32), (f, DType::F32)]); + vec![ + ("group1/dset2", int_float("int_name", "float_name")), + ( + "group1/dset3", + compound(&[ + ("int_array", DType::Array(Box::new(DType::I32), vec![4])), + ( + "float_array", + DType::Array(Box::new(DType::F32), vec![5, 6]), + ), + ]), + ), + ("group1/dset4", int_float("int", "float")), + ("group2/dset5", int_float("int", "float")), + ] +} + +#[test] +fn v1_shared_datatype_resolves_to_the_committed_type() { + // Reading the heap offset as the address used to land on the superblock + // and fail with InvalidObjectHeaderVersion. + let file = File::from_bytes(FIXTURE.to_vec()).unwrap(); + for (path, dtype) in expected() { + assert_eq!( + file.dataset(path).unwrap().dtype().unwrap(), + dtype, + "{path}" + ); + } +} + +#[test] +fn v1_shared_datatype_field_names_match_h5py() { + if !python_available() { + assert!( + !interop_required(), + "CLAWHDF5_REQUIRE_INTEROP=1 but python3 with h5py is not available" + ); + eprintln!("SKIP: python3 with h5py not available"); + return; + } + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("tcompound.h5"); + std::fs::write(&path, FIXTURE).unwrap(); + let script = format!( + r#" +import h5py +with h5py.File("{path}", "r") as f: + for p in ("group1/dset2", "group1/dset3", "group1/dset4", "group2/dset5"): + print(p, *f[p].dtype.names) +"#, + path = path.display() + ); + let out = Command::new(python()) + .args(["-c", &script]) + .output() + .unwrap(); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + let theirs: Vec<&str> = stdout.lines().collect(); + let ours: Vec = expected() + .into_iter() + .map(|(p, t)| match t { + DType::Compound(fields) => { + let names: Vec = fields.into_iter().map(|(n, _)| n).collect(); + format!("{p} {}", names.join(" ")) + } + other => panic!("{other:?}"), + }) + .collect(); + assert_eq!(theirs, ours); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index 660dab0..0783e91 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -74,6 +74,9 @@ the VDS item, which is marked. - Hyperslab selection versions 1 and 2 are refused. - **Files with a user block:** the base address is not applied. - **Old-style shared messages (version 1)** read the wrong address. + **Fixed 2026-09-25:** the address follows a length-sized heap offset + (`tcompound.h5`, `tcompound2.h5`; their datasets now stop at the layout + v1 gap above). - **Array members of version-1 compound datatypes** (found while fixing the item above) were read as one element. **Fixed 2026-09-25.** - **Groups and links:** From a6e90f3ee30b8b7a39000814b4a54688280ca058 Mon Sep 17 00:00:00 2001 From: osobh Date: Fri, 25 Sep 2026 22:02:47 -0500 Subject: [PATCH 3/4] fix(format): apply the base address of files with a user block A file may start with a user block (h5py userblock_size, h5jam), putting the superblock at 512, 1024, ...; every address in the file is then relative to the superblock. The signature search found it, but every reader passed the whole file to the parsers, so addresses landed userblock bytes early and the root group failed with InvalidObjectHeaderVersion (twithub.h5, twithub513.h5, h5clear_fsm_persist_user_*.h5). Readers now view the file from the superblock on, taking the signature's position as the base address as libhdf5 does: File (mmap, buffered, from_bytes), MmapFile, LazyFile, AsyncHDF5File, the VOL and MPI VOL readers, the HNSW loader and external VDS source files. File, MmapFile and LazyFile gain user_block_size(). The new signature::split_user_block returns the two parts, and Superblock::parse refuses a non-zero offset (UserBlockNotStripped) so a format-level caller cannot silently apply superblock-relative addresses to the whole file. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 13 + crates/clawhdf5-ann/src/hnsw.rs | 7 +- crates/clawhdf5-format/src/data_read.rs | 8 +- crates/clawhdf5-format/src/error.rs | 10 + crates/clawhdf5-format/src/lib.rs | 11 +- crates/clawhdf5-format/src/shared_message.rs | 4 +- crates/clawhdf5-format/src/signature.rs | 36 +++ crates/clawhdf5-format/src/superblock.rs | 23 +- crates/clawhdf5-io/src/async_read.rs | 22 +- crates/clawhdf5-io/src/mpi_vol.rs | 9 +- crates/clawhdf5-io/src/vol.rs | 7 +- crates/clawhdf5/src/lazy.rs | 42 ++- crates/clawhdf5/src/mmap_file.rs | 50 ++- crates/clawhdf5/src/reader.rs | 60 +++- crates/clawhdf5/tests/userblock_interop.rs | 314 +++++++++++++++++++ docs/known-issues.md | 3 + 16 files changed, 540 insertions(+), 79 deletions(-) create mode 100644 crates/clawhdf5/tests/userblock_interop.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b054b1..5a5283b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -273,6 +273,19 @@ - CI keeps zlib-ng building and tested; the arm64 job no longer needs cmake. ### Correctness +- **Files with a user block** (`h5py.File(..., userblock_size=N)`, `h5jam`; + the superblock at 512, 1024, …) could not be read: every address in the + file is relative to the superblock, but it was applied from byte 0 + (`InvalidObjectHeaderVersion` on the root group). `File` (mmap, buffered, + `from_bytes`), `MmapFile`, `LazyFile`, `AsyncHDF5File`, the VOL readers, + the HNSW loader and external VDS sources now view the file from the + superblock on, using the signature's position as the base address as + libhdf5 does; `user_block_size()` reports the user block (h5py's + `userblock_size`), and `as_bytes()` returns the bytes from the superblock + on. **Breaking (format crate):** `Superblock::parse` refuses a non-zero + signature offset with `FormatError::UserBlockNotStripped`, since the + addresses it returns would be applied to the wrong bytes; pass the slice + from `signature::split_user_block` (new) and parse at offset 0. - `clawhdf5-format` reader: version-1 shared messages (HDF5 1.6-era files, e.g. a dataset using a committed datatype in libhdf5's `tcompound.h5`) read the heap-offset field of the embedded symbol-table entry as the diff --git a/crates/clawhdf5-ann/src/hnsw.rs b/crates/clawhdf5-ann/src/hnsw.rs index a526bee..a725b3f 100644 --- a/crates/clawhdf5-ann/src/hnsw.rs +++ b/crates/clawhdf5-ann/src/hnsw.rs @@ -13,7 +13,7 @@ use clawhdf5_format::filter_pipeline::FilterPipeline; use clawhdf5_format::group_v2::resolve_path_any; use clawhdf5_format::message_type::MessageType; use clawhdf5_format::object_header::ObjectHeader; -use clawhdf5_format::signature::find_signature; +use clawhdf5_format::signature::split_user_block; use clawhdf5_format::superblock::Superblock; use clawhdf5_io::FileWriter as IoFileWriter; @@ -861,8 +861,9 @@ impl HnswIndex { /// The HDF5 data must contain the `/ann/vectors`, `/ann/graph_layer_*`, /// and `/ann/config` datasets as produced by [`to_hdf5_bytes`]. pub fn load_from_hdf5(data: &[u8]) -> Result { - let sig_offset = find_signature(data)?; - let sb = Superblock::parse(data, sig_offset)?; + // Addresses are relative to the superblock: skip any user block. + let (_, data) = split_user_block(data)?; + let sb = Superblock::parse(data, 0)?; // Read config dataset and its attributes let config_attrs = read_dataset_attrs(data, &sb, "ann/config")?; diff --git a/crates/clawhdf5-format/src/data_read.rs b/crates/clawhdf5-format/src/data_read.rs index 82e6544..8d3b16e 100644 --- a/crates/clawhdf5-format/src/data_read.rs +++ b/crates/clawhdf5-format/src/data_read.rs @@ -575,11 +575,13 @@ fn read_named_dataset_raw( use crate::group_v2::resolve_path_any; use crate::message_type::MessageType; use crate::object_header::ObjectHeader; - use crate::signature::find_signature; + use crate::signature::split_user_block; use crate::superblock::Superblock; - let sig = find_signature(file_data)?; - let sb = Superblock::parse(file_data, sig)?; + // An external source file is handed over whole, user block included; + // its addresses are relative to its superblock. + let (_, file_data) = split_user_block(file_data)?; + let sb = Superblock::parse(file_data, 0)?; let addr = resolve_path_any(file_data, &sb, path)?; let hdr = ObjectHeader::parse(file_data, addr as usize, sb.offset_size, sb.length_size)?; diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index 7ae57f4..a54ca72 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -120,6 +120,11 @@ pub enum FormatError { /// A shared-message reference points at an object header that holds no /// (unshared) message of the referenced type (raw message type id). SharedMessageTargetMissing(u16), + /// A superblock was parsed at a non-zero offset of the buffer (the file + /// has a user block of this many bytes). HDF5 addresses are relative to + /// the superblock, so the buffer must start there: see + /// `signature::split_user_block`. + UserBlockNotStripped(u64), /// A selection does not fit the dataset it was applied to (wrong rank, or /// it reaches past a dimension's extent). SelectionOutOfBounds(String), @@ -342,6 +347,11 @@ impl fmt::Display for FormatError { FormatError::SelectionOutOfBounds(msg) => { write!(f, "selection out of bounds: {msg}") } + FormatError::UserBlockNotStripped(n) => write!( + f, + "file has a {n}-byte user block: parse the bytes from the superblock on \ + (signature::split_user_block)" + ), FormatError::SharedMessageTargetMissing(t) => write!( f, "shared message reference points at an object header with no message of type \ diff --git a/crates/clawhdf5-format/src/lib.rs b/crates/clawhdf5-format/src/lib.rs index 4a6c2a1..ffed10f 100644 --- a/crates/clawhdf5-format/src/lib.rs +++ b/crates/clawhdf5-format/src/lib.rs @@ -26,12 +26,13 @@ //! use clawhdf5_format::{signature, superblock, object_header, group_v2, //! datatype, dataspace, data_layout, data_read, message_type::MessageType}; //! -//! let file_data = std::fs::read("output.h5").unwrap(); -//! let sig = signature::find_signature(&file_data).unwrap(); -//! let sb = superblock::Superblock::parse(&file_data, sig).unwrap(); -//! let addr = group_v2::resolve_path_any(&file_data, &sb, "data").unwrap(); +//! let bytes = std::fs::read("output.h5").unwrap(); +//! // Addresses are relative to the superblock: skip any user block. +//! let (_user_block, file_data) = signature::split_user_block(&bytes).unwrap(); +//! let sb = superblock::Superblock::parse(file_data, 0).unwrap(); +//! let addr = group_v2::resolve_path_any(file_data, &sb, "data").unwrap(); //! let hdr = object_header::ObjectHeader::parse( -//! &file_data, addr as usize, sb.offset_size, sb.length_size).unwrap(); +//! file_data, addr as usize, sb.offset_size, sb.length_size).unwrap(); //! ``` //! //! # Features diff --git a/crates/clawhdf5-format/src/shared_message.rs b/crates/clawhdf5-format/src/shared_message.rs index a77edc1..f760021 100644 --- a/crates/clawhdf5-format/src/shared_message.rs +++ b/crates/clawhdf5-format/src/shared_message.rs @@ -408,8 +408,8 @@ pub fn load_sohm_table( offset_size: u8, length_size: u8, ) -> Result, FormatError> { - let sig = crate::signature::find_signature(file_data)?; - let sb = crate::superblock::Superblock::parse(file_data, sig)?; + // `file_data` starts at the superblock (see `signature::split_user_block`). + let sb = crate::superblock::Superblock::parse(file_data, 0)?; let Some(ext_addr) = sb .superblock_extension_address .filter(|&a| !is_undefined(a, offset_size)) diff --git a/crates/clawhdf5-format/src/signature.rs b/crates/clawhdf5-format/src/signature.rs index 27d5e75..600b650 100644 --- a/crates/clawhdf5-format/src/signature.rs +++ b/crates/clawhdf5-format/src/signature.rs @@ -11,6 +11,16 @@ pub const HDF5_SIGNATURE: [u8; 8] = [0x89, b'H', b'D', b'F', b'\r', b'\n', 0x1A, /// (powers of two starting at 512, plus offset 0). /// /// Returns the byte offset where the signature was found. +/// +/// A non-zero offset means the file starts with a *user block*, and every +/// address inside the file is relative to the superblock's position, not to +/// byte 0 (libhdf5 uses the signature's position as the base address even +/// when the stored base-address field disagrees). The parsers in this crate +/// take addresses as indices into `file_data`, so they must be handed the +/// bytes from the signature on — use [`split_user_block`]. [`Superblock::parse`] +/// refuses a non-zero offset for this reason. +/// +/// [`Superblock::parse`]: crate::superblock::Superblock::parse pub fn find_signature(data: &[u8]) -> Result { // Check offset 0 if data.len() >= 8 && data[..8] == HDF5_SIGNATURE { @@ -29,6 +39,17 @@ pub fn find_signature(data: &[u8]) -> Result { Err(FormatError::SignatureNotFound) } +/// Split a file into its user block and its HDF5 bytes. +/// +/// Returns `(user_block, hdf5)`: `user_block` is everything before the +/// superblock signature (empty for most files) and `hdf5` is the rest, in +/// which every HDF5 address is a plain index. Pass `hdf5` as `file_data` to +/// every parser in this crate, and parse the superblock at offset 0 of it. +pub fn split_user_block(data: &[u8]) -> Result<(&[u8], &[u8]), FormatError> { + let offset = find_signature(data)?; + Ok(data.split_at(offset)) +} + #[cfg(test)] mod tests { use super::*; @@ -88,6 +109,21 @@ mod tests { assert_eq!(find_signature(&data), Err(FormatError::SignatureNotFound)); } + #[test] + fn split_user_block_rebases_at_the_signature() { + let mut data = vec![7u8; 1024]; + data[512..520].copy_from_slice(&HDF5_SIGNATURE); + let (ub, hdf5) = split_user_block(&data).unwrap(); + assert_eq!(ub.len(), 512); + assert_eq!(hdf5.len(), 512); + assert_eq!(&hdf5[..8], &HDF5_SIGNATURE); + + data[..8].copy_from_slice(&HDF5_SIGNATURE); + let (ub, hdf5) = split_user_block(&data).unwrap(); + assert!(ub.is_empty()); + assert_eq!(hdf5.len(), 1024); + } + #[test] fn signature_prefers_earliest() { // Signature at both 0 and 512, should return 0 diff --git a/crates/clawhdf5-format/src/superblock.rs b/crates/clawhdf5-format/src/superblock.rs index e971495..d2ec4d6 100644 --- a/crates/clawhdf5-format/src/superblock.rs +++ b/crates/clawhdf5-format/src/superblock.rs @@ -174,8 +174,18 @@ impl Superblock { /// Parse a superblock from `data` starting at `signature_offset`. /// - /// The signature must be present at the given offset. + /// The signature must be present at the given offset, and that offset + /// must be 0: every address in an HDF5 file is relative to the + /// superblock, so when a file has a user block (signature at 512, 1024, + /// …) the caller must pass the bytes from the signature on — see + /// [`crate::signature::split_user_block`] — and use that slice as + /// `file_data` everywhere. A non-zero offset is refused with + /// [`FormatError::UserBlockNotStripped`] because the addresses in the + /// returned superblock would otherwise be applied to the wrong bytes. pub fn parse(data: &[u8], signature_offset: usize) -> Result { + if signature_offset != 0 { + return Err(FormatError::UserBlockNotStripped(signature_offset as u64)); + } let d = data .get(signature_offset..) .ok_or(FormatError::UnexpectedEof { @@ -676,7 +686,16 @@ mod tests { let mut data = vec![0u8; 1024]; let v0 = build_v0_bytes(8); data[512..512 + v0.len()].copy_from_slice(&v0); - let sb = Superblock::parse(&data, 512).unwrap(); + // Addresses are relative to the superblock, so parsing in place + // (where they would be applied to the whole buffer) is refused... + assert_eq!( + Superblock::parse(&data, 512), + Err(FormatError::UserBlockNotStripped(512)) + ); + // ...and the caller parses the bytes from the signature on. + let (ub, hdf5) = crate::signature::split_user_block(&data).unwrap(); + assert_eq!(ub.len(), 512); + let sb = Superblock::parse(hdf5, 0).unwrap(); assert_eq!(sb.version, 0); assert_eq!(sb.root_group_address, 96); } diff --git a/crates/clawhdf5-io/src/async_read.rs b/crates/clawhdf5-io/src/async_read.rs index 94afff0..d9e2ffe 100644 --- a/crates/clawhdf5-io/src/async_read.rs +++ b/crates/clawhdf5-io/src/async_read.rs @@ -268,28 +268,26 @@ impl AsyncHDF5File { /// /// Reads the entire file into memory, then parses the superblock. pub async fn open(reader: &R) -> Result { - let data = reader.read_all().await?; - let sig_offset = find_signature(&data)?; - let superblock = Superblock::parse(&data, sig_offset)?; - Ok(Self { data, superblock }) + Self::from_bytes(reader.read_all().await?) } /// Open an HDF5 file asynchronously from a file path. pub async fn open_path>(path: P) -> Result { - let data = tokio::fs::read(path).await?; - let sig_offset = find_signature(&data)?; - let superblock = Superblock::parse(&data, sig_offset)?; - Ok(Self { data, superblock }) + Self::from_bytes(tokio::fs::read(path).await?) } /// Open an HDF5 file from bytes already in memory. - pub fn from_bytes(data: Vec) -> Result { - let sig_offset = find_signature(&data)?; - let superblock = Superblock::parse(&data, sig_offset)?; + pub fn from_bytes(mut data: Vec) -> Result { + // HDF5 addresses are relative to the superblock: drop any user block + // so they index `data` directly. + let user_block = find_signature(&data)?; + data.drain(..user_block); + let superblock = Superblock::parse(&data, 0)?; Ok(Self { data, superblock }) } - /// Access the raw file bytes. + /// Access the file bytes from the superblock on (any user block is + /// dropped on open). pub fn as_bytes(&self) -> &[u8] { &self.data } diff --git a/crates/clawhdf5-io/src/mpi_vol.rs b/crates/clawhdf5-io/src/mpi_vol.rs index a1ddf72..7dac39e 100644 --- a/crates/clawhdf5-io/src/mpi_vol.rs +++ b/crates/clawhdf5-io/src/mpi_vol.rs @@ -180,7 +180,7 @@ fn mpi_collective_read(vol: &MpiVol, location: &str, path: &str) -> Result Result { reader: R, + /// Offset of the superblock in the file (the user-block size); every + /// HDF5 address is relative to it. + base: usize, superblock: Superblock, root_header: ObjectHeader, /// Cache of parsed object headers, keyed by address. @@ -73,9 +76,9 @@ impl LazyFile { /// /// Parses only the superblock and root group object header. pub fn open(reader: R) -> Result { - let data = reader.as_bytes(); - let sig_offset = signature::find_signature(data)?; - let superblock = Superblock::parse(data, sig_offset)?; + let (user_block, data) = signature::split_user_block(reader.as_bytes())?; + let base = user_block.len(); + let superblock = Superblock::parse(data, 0)?; let root_header = ObjectHeader::parse( data, superblock.root_group_address as usize, @@ -84,15 +87,26 @@ impl LazyFile { )?; Ok(Self { reader, + base, superblock, root_header, header_cache: RefCell::new(HashMap::new()), }) } - /// Returns the raw file bytes. + /// Returns the file's bytes from the superblock on (after any user + /// block), which is the space every HDF5 address in the file indexes. pub fn as_bytes(&self) -> &[u8] { - self.reader.as_bytes() + self.hdf5_bytes() + } + + /// Size of the user block before the superblock (0 for most files). + pub fn user_block_size(&self) -> u64 { + self.base as u64 + } + + fn hdf5_bytes(&self) -> &[u8] { + &self.reader.as_bytes()[self.base..] } /// Returns a reference to the parsed superblock. @@ -110,7 +124,7 @@ impl LazyFile { /// Resolve a path and return a `LazyDataset` handle. pub fn dataset(&self, path: &str) -> Result, Error> { - let data = self.reader.as_bytes(); + let data = self.hdf5_bytes(); let addr = group_v2::resolve_path_any(data, &self.superblock, path)?; let hdr = self.get_or_parse_header(addr)?; if !has_message(&hdr, MessageType::DataLayout) { @@ -124,7 +138,7 @@ impl LazyFile { /// Resolve a path and return a `LazyGroup` handle. pub fn group(&self, path: &str) -> Result, Error> { - let data = self.reader.as_bytes(); + let data = self.hdf5_bytes(); let addr = group_v2::resolve_path_any(data, &self.superblock, path)?; Ok(LazyGroup { file: self, @@ -163,7 +177,7 @@ impl LazyFile { } // Parse and cache - let data = self.reader.as_bytes(); + let data = self.hdf5_bytes(); let hdr = ObjectHeader::parse( data, address as usize, @@ -187,7 +201,7 @@ impl LazyFile { impl std::fmt::Debug for LazyFile { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("LazyFile") - .field("size", &self.reader.as_bytes().len()) + .field("size", &self.hdf5_bytes().len()) .field("superblock_version", &self.superblock.version) .field("cached_headers", &self.header_cache.borrow().len()) .finish() @@ -234,7 +248,7 @@ impl<'f, R: HDF5Read> LazyGroup<'f, R> { /// Read all attributes of this group. pub fn attrs(&self) -> Result, Error> { let hdr = self.file.get_or_parse_header(self.address)?; - let data = self.file.reader.as_bytes(); + let data = self.file.hdf5_bytes(); let attr_msgs = extract_attributes_full(data, &hdr, self.file.offset_size(), self.file.length_size())?; Ok(attrs_to_map( @@ -277,7 +291,7 @@ impl<'f, R: HDF5Read> LazyGroup<'f, R> { fn children(&self) -> Result, Error> { let hdr = self.file.get_or_parse_header(self.address)?; - let data = self.file.reader.as_bytes(); + let data = self.file.hdf5_bytes(); let os = self.file.offset_size(); let ls = self.file.length_size(); resolve_group_entries(data, &hdr, os, ls).map_err(Error::Format) @@ -360,7 +374,7 @@ impl<'f, R: HDF5Read> LazyDataset<'f, R> { let dl = self.data_layout()?; let ds = self.dataspace()?; let dt = self.datatype()?; - let slice = data_read::read_raw_data_zerocopy(self.file.reader.as_bytes(), &dl, &ds, &dt)?; + let slice = data_read::read_raw_data_zerocopy(self.file.hdf5_bytes(), &dl, &ds, &dt)?; Ok(slice) } @@ -401,7 +415,7 @@ impl<'f, R: HDF5Read> LazyDataset<'f, R> { /// Read all attributes of this dataset. pub fn attrs(&self) -> Result, Error> { - let data = self.file.reader.as_bytes(); + let data = self.file.hdf5_bytes(); let attr_msgs = extract_attributes_full( data, &self.header, @@ -479,7 +493,7 @@ impl<'f, R: HDF5Read> LazyDataset<'f, R> { let ds = self.dataspace()?; let dl = self.data_layout()?; let pipeline = self.filter_pipeline()?; - let data = self.file.reader.as_bytes(); + let data = self.file.hdf5_bytes(); // Unallocated storage reads as the dataset's fill value. clawhdf5_format::fill_value::read_full_with_fill( &self.header.messages, diff --git a/crates/clawhdf5/src/mmap_file.rs b/crates/clawhdf5/src/mmap_file.rs index 9119cdf..b7251ac 100644 --- a/crates/clawhdf5/src/mmap_file.rs +++ b/crates/clawhdf5/src/mmap_file.rs @@ -34,6 +34,9 @@ use crate::types::{AttrValue, DType, attrs_to_map, classify_datatype}; /// `&[u8]` slice via [`MmapDataset::read_raw_slice`]. pub struct MmapFile { reader: MmapReader, + /// Offset of the superblock in the mapped file (the user-block size); + /// every HDF5 address is relative to it. + base: usize, superblock: Superblock, } @@ -41,10 +44,25 @@ impl MmapFile { /// Open an HDF5 file using memory-mapped I/O. pub fn open>(path: P) -> Result { let reader = MmapReader::open(path).map_err(Error::Io)?; - let data = reader.as_bytes(); - let sig_offset = signature::find_signature(data)?; - let superblock = Superblock::parse(data, sig_offset)?; - Ok(Self { reader, superblock }) + let (user_block, data) = signature::split_user_block(reader.as_bytes())?; + let base = user_block.len(); + let superblock = Superblock::parse(data, 0)?; + Ok(Self { + reader, + base, + superblock, + }) + } + + /// The file's bytes from the superblock on — the space HDF5 addresses + /// index into. + fn hdf5_bytes(&self) -> &[u8] { + &self.reader.as_bytes()[self.base..] + } + + /// Size of the user block before the superblock (0 for most files). + pub fn user_block_size(&self) -> u64 { + self.base as u64 } /// Returns a handle to the root group. @@ -57,7 +75,7 @@ impl MmapFile { /// Resolve a path and return a `MmapDataset` handle. pub fn dataset(&self, path: &str) -> Result, Error> { - let data = self.reader.as_bytes(); + let data = self.hdf5_bytes(); let addr = group_v2::resolve_path_any(data, &self.superblock, path)?; let hdr = self.parse_header(addr)?; if !has_message(&hdr, MessageType::DataLayout) { @@ -71,7 +89,7 @@ impl MmapFile { /// Resolve a path and return a `MmapGroup` handle. pub fn group(&self, path: &str) -> Result, Error> { - let data = self.reader.as_bytes(); + let data = self.hdf5_bytes(); let addr = group_v2::resolve_path_any(data, &self.superblock, path)?; Ok(MmapGroup { file: self, @@ -79,9 +97,11 @@ impl MmapFile { }) } - /// Returns the raw file bytes (zero-copy from mmap). + /// Returns the file's bytes from the superblock on (after any user + /// block), zero-copy from the mmap. Every HDF5 address in the file + /// indexes this slice. pub fn as_bytes(&self) -> &[u8] { - self.reader.as_bytes() + self.hdf5_bytes() } /// Returns a reference to the parsed superblock. @@ -91,7 +111,7 @@ impl MmapFile { fn parse_header(&self, address: u64) -> Result { ObjectHeader::parse( - self.reader.as_bytes(), + self.hdf5_bytes(), address as usize, self.superblock.offset_size, self.superblock.length_size, @@ -155,7 +175,7 @@ impl<'f> MmapGroup<'f> { /// Read all attributes of this group. pub fn attrs(&self) -> Result, Error> { - let data = self.file.reader.as_bytes(); + let data = self.file.hdf5_bytes(); let hdr = self.file.parse_header(self.address)?; let attr_msgs = extract_attributes_full(data, &hdr, self.file.offset_size(), self.file.length_size())?; @@ -198,7 +218,7 @@ impl<'f> MmapGroup<'f> { } fn children(&self) -> Result, Error> { - let data = self.file.reader.as_bytes(); + let data = self.file.hdf5_bytes(); let hdr = self.file.parse_header(self.address)?; let os = self.file.offset_size(); let ls = self.file.length_size(); @@ -326,7 +346,7 @@ impl<'f> MmapDataset<'f> { actual: sz, })); } - let data = self.file.reader.as_bytes(); + let data = self.file.hdf5_bytes(); let a = addr as usize; if a + sz > data.len() { return Err(Error::Format(FormatError::UnexpectedEof { @@ -342,7 +362,7 @@ impl<'f> MmapDataset<'f> { /// Read all attributes of this dataset. pub fn attrs(&self) -> Result, Error> { - let data = self.file.reader.as_bytes(); + let data = self.file.hdf5_bytes(); let attr_msgs = extract_attributes_full( data, &self.header, @@ -423,7 +443,7 @@ impl<'f> MmapDataset<'f> { // Unallocated storage reads as the dataset's fill value. clawhdf5_format::fill_value::read_full_with_fill( &self.header.messages, - self.file.reader.as_bytes(), + self.file.hdf5_bytes(), &dl, &ds, dt.type_size() as usize, @@ -431,7 +451,7 @@ impl<'f> MmapDataset<'f> { self.file.length_size(), || { Ok(data_read::read_raw_data_full( - self.file.reader.as_bytes(), + self.file.hdf5_bytes(), &dl, &ds, &dt, diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index bb6a8fd..b3d7338 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -31,20 +31,43 @@ use crate::types::{AttrValue, DType, attrs_to_map, classify_datatype}; // --------------------------------------------------------------------------- /// Internal storage: either an owned `Vec` or a memory-mapped region. -enum FileData { +enum Backing { Owned(Vec), #[cfg(feature = "mmap")] Mmap(clawhdf5_io::MmapReader), } -impl FileData { - fn as_bytes(&self) -> &[u8] { +impl Backing { + fn whole_file(&self) -> &[u8] { match self { - FileData::Owned(v) => v, + Backing::Owned(v) => v, #[cfg(feature = "mmap")] - FileData::Mmap(r) => r.as_bytes(), + Backing::Mmap(r) => r.as_bytes(), } } +} + +/// The file's bytes, viewed from the superblock on. A file may start with a +/// user block (the superblock at 512, 1024, …); every HDF5 address is +/// relative to the superblock, so all parsing goes through [`Self::as_bytes`]. +struct FileData { + backing: Backing, + /// Offset of the superblock in the file (the user-block size). + base: usize, +} + +impl FileData { + /// Locate the superblock and parse it. + fn new(backing: Backing) -> Result<(Self, Superblock), Error> { + let (user_block, hdf5) = signature::split_user_block(backing.whole_file())?; + let base = user_block.len(); + let superblock = Superblock::parse(hdf5, 0)?; + Ok((Self { backing, base }, superblock)) + } + + fn as_bytes(&self) -> &[u8] { + &self.backing.whole_file()[self.base..] + } fn len(&self) -> usize { self.as_bytes().len() @@ -81,11 +104,9 @@ impl File { #[cfg(feature = "mmap")] { let reader = clawhdf5_io::MmapReader::open(path).map_err(Error::Io)?; - let data_ref = reader.as_bytes(); - let sig_offset = signature::find_signature(data_ref)?; - let superblock = Superblock::parse(data_ref, sig_offset)?; + let (data, superblock) = FileData::new(Backing::Mmap(reader))?; Ok(Self { - data: FileData::Mmap(reader), + data, superblock, chunk_cache: ChunkCache::new(), base_dir, @@ -116,10 +137,9 @@ impl File { /// In-memory files have no directory, so external Virtual Dataset sources /// cannot be resolved automatically (same-file VDS still works). pub fn from_bytes(data: Vec) -> Result { - let sig_offset = signature::find_signature(&data)?; - let superblock = Superblock::parse(&data, sig_offset)?; + let (data, superblock) = FileData::new(Backing::Owned(data))?; Ok(Self { - data: FileData::Owned(data), + data, superblock, chunk_cache: ChunkCache::new(), base_dir: None, @@ -209,11 +229,19 @@ impl File { Ok(results.into_iter().map(|(_, data)| data).collect()) } - /// Returns the raw file bytes. + /// Returns the file's bytes from the superblock on (after any user + /// block). Every HDF5 address in the file indexes this slice, so it is + /// what the `clawhdf5_format` parsers expect as `file_data`. pub fn as_bytes(&self) -> &[u8] { self.data.as_bytes() } + /// Size of the user block before the superblock (0 for most files). + /// Matches h5py's `File.userblock_size`. + pub fn user_block_size(&self) -> u64 { + self.data.base as u64 + } + /// Returns a reference to the parsed superblock. pub fn superblock(&self) -> &Superblock { &self.superblock @@ -221,10 +249,10 @@ impl File { /// Returns `true` when the file is backed by memory-mapped I/O. pub fn is_mmap(&self) -> bool { - match &self.data { - FileData::Owned(_) => false, + match &self.data.backing { + Backing::Owned(_) => false, #[cfg(feature = "mmap")] - FileData::Mmap(_) => true, + Backing::Mmap(_) => true, } } diff --git a/crates/clawhdf5/tests/userblock_interop.rs b/crates/clawhdf5/tests/userblock_interop.rs new file mode 100644 index 0000000..c9130b9 --- /dev/null +++ b/crates/clawhdf5/tests/userblock_interop.rs @@ -0,0 +1,314 @@ +//! Files that start with a user block (`h5py.File(..., userblock_size=N)`, +//! `h5jam`): the superblock sits at 512, 1024, ... and every address in the +//! file is relative to it. Each reader (buffered, mmap, `MmapFile`, +//! `LazyFile`) must apply that base, and read the same values h5py does. +//! +//! h5py writes the files; skipped when python3 with h5py is unavailable, +//! unless `CLAWHDF5_REQUIRE_INTEROP=1`. + +use std::collections::HashMap; +use std::path::Path; +use std::process::Command; + +use clawhdf5::{AttrValue, File, LazyFile, MmapFile}; + +fn python() -> String { + std::env::var("CLAWHDF5_PYTHON").unwrap_or_else(|_| "python3".to_string()) +} + +fn interop_required() -> bool { + std::env::var("CLAWHDF5_REQUIRE_INTEROP").is_ok_and(|v| v == "1") +} + +fn python_available() -> bool { + Command::new(python()) + .args(["-c", "import h5py, numpy"]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +macro_rules! skip_if_no_python { + () => { + if !python_available() { + assert!( + !interop_required(), + "CLAWHDF5_REQUIRE_INTEROP=1 but python3 with h5py is not available" + ); + eprintln!("SKIP: python3 with h5py not available"); + return; + } + }; +} + +/// Run `script` and return its stdout as `key -> values` (one +/// `key v1 v2 ...` line per key). +fn run_python(script: &str) -> HashMap> { + let output = Command::new(python()) + .args(["-c", script]) + .output() + .expect("failed to run python"); + assert!( + output.status.success(), + "python failed:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout) + .lines() + .filter_map(|line| { + let mut words = line.split_whitespace().map(str::to_string); + Some((words.next()?, words.collect())) + }) + .collect() +} + +fn parse(values: &[String]) -> Vec +where + T::Err: std::fmt::Debug, +{ + values.iter().map(|v| v.parse().unwrap()).collect() +} + +/// Write a file with a user block of `userblock` bytes holding contiguous, +/// chunked (deflate), compact and committed-type datasets, nested groups, +/// and attributes (compact and, under `latest`, dense). Prints what h5py +/// reads back. +fn write_file(path: &Path, userblock: u32, libver: &str) -> HashMap> { + let script = format!( + r#" +import h5py, numpy as np +path = "{path}" +with h5py.File(path, "w", userblock_size={userblock}, libver={libver}) as f: + f.attrs["title"] = "user block" + f.attrs["answer"] = np.int64(42) + f.create_dataset("contig", data=np.arange(12, dtype=", key: &str) -> String { + match map.get(key) { + Some(AttrValue::I64(v)) => format!("i64 {v}"), + Some(AttrValue::F64(v)) => format!("f64 {v}"), + Some(AttrValue::String(v)) => format!("str {v}"), + other => format!("{other:?}"), + } +} + +fn i64s(v: &[String]) -> Vec { + parse(v) +} + +/// Everything read through the `File` API must match h5py. +fn check_file(file: &File, expected: &HashMap>, label: &str) { + let ub: u64 = expected["userblock"][0].parse().unwrap(); + assert_eq!(file.user_block_size(), ub, "{label}: user block size"); + assert_eq!( + file.dataset("contig").unwrap().read_f64().unwrap(), + parse::(&expected["contig"]), + "{label}: contiguous" + ); + assert_eq!( + file.dataset("chunked") + .unwrap() + .read_i32() + .unwrap() + .iter() + .map(|&v| v as i64) + .collect::>(), + i64s(&expected["chunked"]), + "{label}: chunked" + ); + assert_eq!( + file.dataset("compact").unwrap().read_i64().unwrap(), + i64s(&expected["compact"]), + "{label}: compact" + ); + assert_eq!( + file.dataset("committed").unwrap().read_f32().unwrap(), + parse::(&expected["committed"]), + "{label}: committed datatype" + ); + assert_eq!( + file.dataset("a/b/deep").unwrap().read_i64().unwrap(), + i64s(&expected["deep"]), + "{label}: nested group" + ); + let many: Vec = (0..20) + .map(|i| { + file.dataset(&format!("many/d{i:02}")) + .unwrap() + .read_i32() + .unwrap()[0] as i64 + }) + .collect(); + assert_eq!(many, i64s(&expected["many"]), "{label}: many links"); + + let root = file.root().attrs().unwrap(); + assert_eq!(attr(&root, "title"), "str user block", "{label}"); + assert_eq!(attr(&root, "answer"), "i64 42", "{label}"); + let a = file.group("a").unwrap().attrs().unwrap(); + let k: Vec = (0..12) + .map(|i| match &a[&format!("k{i:02}")] { + AttrValue::I64(v) => *v, + _ => panic!("{label}: k{i:02} is not an i64"), + }) + .collect(); + assert_eq!(k, i64s(&expected["k"]), "{label}: attributes"); + assert_eq!( + attr(&file.group("a/b").unwrap().attrs().unwrap(), "scale"), + "f64 2.5", + "{label}" + ); + assert_eq!( + attr(&file.dataset("contig").unwrap().attrs().unwrap(), "units"), + "str m", + "{label}" + ); +} + +fn check_all_readers(path: &Path, expected: &HashMap>, label: &str) { + check_file( + &File::open(path).unwrap(), + expected, + &format!("{label} File::open"), + ); + check_file( + &File::open_buffered(path).unwrap(), + expected, + &format!("{label} File::open_buffered"), + ); + check_file( + &File::from_bytes(std::fs::read(path).unwrap()).unwrap(), + expected, + &format!("{label} File::from_bytes"), + ); + + let ub: u64 = expected["userblock"][0].parse().unwrap(); + + let mm = MmapFile::open(path).unwrap(); + assert_eq!(mm.user_block_size(), ub, "{label} MmapFile"); + assert_eq!( + mm.dataset("contig").unwrap().read_f64().unwrap(), + parse::(&expected["contig"]), + "{label} MmapFile contiguous" + ); + assert_eq!( + mm.dataset("compact").unwrap().read_i64().unwrap(), + i64s(&expected["compact"]), + "{label} MmapFile compact" + ); + assert_eq!( + mm.dataset("committed").unwrap().read_f32().unwrap(), + parse::(&expected["committed"]), + "{label} MmapFile committed" + ); + assert_eq!( + mm.dataset("a/b/deep").unwrap().read_i64().unwrap(), + i64s(&expected["deep"]), + "{label} MmapFile nested" + ); + assert_eq!( + attr(&mm.root().attrs().unwrap(), "answer"), + "i64 42", + "{label} MmapFile attrs" + ); + + let lazy = LazyFile::open_mmap(path).unwrap(); + assert_eq!(lazy.user_block_size(), ub, "{label} LazyFile"); + assert_eq!( + lazy.dataset("contig").unwrap().read_f64().unwrap(), + parse::(&expected["contig"]), + "{label} LazyFile contiguous" + ); + assert_eq!( + lazy.dataset("chunked") + .unwrap() + .read_i32() + .unwrap() + .iter() + .map(|&v| v as i64) + .collect::>(), + i64s(&expected["chunked"]), + "{label} LazyFile chunked" + ); + assert_eq!( + lazy.dataset("committed").unwrap().read_f32().unwrap(), + parse::(&expected["committed"]), + "{label} LazyFile committed" + ); + assert_eq!( + lazy.dataset("a/b/deep").unwrap().read_i64().unwrap(), + i64s(&expected["deep"]), + "{label} LazyFile nested" + ); + assert_eq!( + attr(&lazy.root().attrs().unwrap(), "answer"), + "i64 42", + "{label} LazyFile attrs" + ); +} + +#[test] +fn user_block_files_read_like_h5py() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + for userblock in [512u32, 4096] { + for libver in ["default", "latest"] { + let label = format!("userblock={userblock} libver={libver}"); + let path = dir.path().join(format!("ub_{userblock}_{libver}.h5")); + let expected = write_file(&path, userblock, libver); + assert_eq!(expected["userblock"], [userblock.to_string()], "{label}"); + check_all_readers(&path, &expected, &label); + } + } +} + +#[test] +fn file_without_user_block_reports_zero() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("no_ub.h5"); + let expected = write_file(&path, 0, "default"); + assert_eq!(expected["userblock"], ["0"]); + check_all_readers(&path, &expected, "userblock=0"); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index 0783e91..6f84c6a 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -73,6 +73,9 @@ the VDS item, which is marked. - `%b` printf-style source names are not expanded. - Hyperslab selection versions 1 and 2 are refused. - **Files with a user block:** the base address is not applied. + **Fixed 2026-09-25:** every reader views the file from the superblock on + (`twithub.h5`, `twithub513.h5`, `h5clear_fsm_persist_user_*.h5`; the + `twithub` files still stop at the user-defined link type below). - **Old-style shared messages (version 1)** read the wrong address. **Fixed 2026-09-25:** the address follows a length-sized heap offset (`tcompound.h5`, `tcompound2.h5`; their datasets now stop at the layout From 90e050944f1ab7d6335e031f665af866b632ee53 Mon Sep 17 00:00:00 2001 From: osobh Date: Fri, 25 Sep 2026 22:07:27 -0500 Subject: [PATCH 4/4] fix(format): refuse a local heap whose free list leaves the heap libhdf5 walks a local heap's free list when it loads the heap's data and refuses the heap ("bad heap free list") when a free block starts or ends outside the data segment, or links to offset 0. We never looked at the free list, so a damaged old-style group listed names read from the broken heap: once the user block of cve-2021-36977.h5 was applied, its root listed eight garbage names where libhdf5 fails. LocalHeap::validate_free_list (new) mirrors H5HL__fl_deserialize, with a cycle bound, and accepts H5HL_FREE_NULL (1) or an all-ones head as the end of the list. Like libhdf5 it runs when the first name is needed, not on parse, so an empty group with a damaged heap still lists as empty (cve-2018-13871.h5, cve-2024-29166.h5, gh-4431-poc-03.h5 keep matching h5py). Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 7 ++ crates/clawhdf5-format/src/error.rs | 6 + crates/clawhdf5-format/src/group_v1.rs | 12 ++ crates/clawhdf5-format/src/local_heap.rs | 99 ++++++++++++++- crates/clawhdf5/tests/local_heap_interop.rs | 131 ++++++++++++++++++++ docs/known-issues.md | 5 + 6 files changed, 258 insertions(+), 2 deletions(-) create mode 100644 crates/clawhdf5/tests/local_heap_interop.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a5283b..a4a5fa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -273,6 +273,13 @@ - CI keeps zlib-ng building and tested; the arm64 job no longer needs cmake. ### Correctness +- `clawhdf5-format` reader: an old-style group whose local heap has a free + list pointing outside the heap was listed with names read from the broken + heap (garbage names on `cve-2021-36977.h5` once its user block was + applied). libhdf5 refuses such a heap ("bad heap free list"); so do we now, + with `FormatError::InvalidLocalHeapFreeList`. As in libhdf5 the free list + is checked when the first name is read (`LocalHeap::validate_free_list`, + new), so an empty group with a damaged heap still lists as empty. - **Files with a user block** (`h5py.File(..., userblock_size=N)`, `h5jam`; the superblock at 512, 1024, …) could not be read: every address in the file is relative to the superblock, but it was applied from byte 0 diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index a54ca72..bb81939 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -80,6 +80,9 @@ pub enum FormatError { InvalidLocalHeapSignature, /// Invalid local heap version. InvalidLocalHeapVersion(u8), + /// A local heap's free list points outside its data segment (libhdf5: + /// "bad heap free list"). + InvalidLocalHeapFreeList, /// Invalid B-tree v1 signature. InvalidBTreeSignature, /// Invalid B-tree node type. @@ -278,6 +281,9 @@ impl fmt::Display for FormatError { FormatError::InvalidLocalHeapSignature => { write!(f, "invalid local heap signature") } + FormatError::InvalidLocalHeapFreeList => { + write!(f, "bad local heap free list") + } FormatError::InvalidLocalHeapVersion(v) => { write!(f, "invalid local heap version: {v}") } diff --git a/crates/clawhdf5-format/src/group_v1.rs b/crates/clawhdf5-format/src/group_v1.rs index 989f826..e55ad16 100644 --- a/crates/clawhdf5-format/src/group_v1.rs +++ b/crates/clawhdf5-format/src/group_v1.rs @@ -45,9 +45,16 @@ pub fn resolve_v1_group_entries( )?; let mut entries = Vec::new(); + let mut heap_checked = false; for snod_addr in snod_addrs { let snod = SymbolTableNode::parse(file_data, snod_addr as usize, offset_size)?; for entry in &snod.entries { + // Like libhdf5, look at the heap's free list only once a name is + // needed: an empty group with a damaged heap still lists. + if !heap_checked { + heap.validate_free_list(file_data, length_size)?; + heap_checked = true; + } let name = heap.read_string(file_data, entry.link_name_offset)?; entries.push(GroupEntry { name, @@ -85,12 +92,17 @@ pub fn find_v1_soft_link( offset_size, length_size, )?; + let mut heap_checked = false; for snod_addr in snod_addrs { let snod = SymbolTableNode::parse(file_data, snod_addr as usize, offset_size)?; for entry in &snod.entries { if entry.cache_type != CACHE_TYPE_SOFT_LINK { continue; } + if !heap_checked { + heap.validate_free_list(file_data, length_size)?; + heap_checked = true; + } if heap.read_string(file_data, entry.link_name_offset)? != name { continue; } diff --git a/crates/clawhdf5-format/src/local_heap.rs b/crates/clawhdf5-format/src/local_heap.rs index 33e3433..39e9b26 100644 --- a/crates/clawhdf5-format/src/local_heap.rs +++ b/crates/clawhdf5-format/src/local_heap.rs @@ -87,6 +87,57 @@ impl LocalHeap { }) } + /// Walk the free list the way libhdf5 does when it loads a heap's data + /// (`H5HL__fl_deserialize`), rejecting a heap whose free list points + /// outside the data segment. libhdf5 refuses such a heap ("bad heap free + /// list"), and names read from it would be garbage. + /// + /// libhdf5 only loads a heap when it needs a name from it (an empty + /// group's broken heap goes unnoticed), so call this before the first + /// [`Self::read_string`], not on parse. + /// + /// The end of the list is `H5HL_FREE_NULL` (1); an all-ones value (the + /// undefined address) is accepted as "no free list" too. + pub fn validate_free_list(&self, file_data: &[u8], length_size: u8) -> Result<(), FormatError> { + const FREE_NULL: u64 = 1; + let ls = length_size as usize; + let undefined = if ls >= 8 { + u64::MAX + } else { + (1u64 << (8 * ls)) - 1 + }; + let size = self.data_segment_size; + let seg = self.data_segment_address; + let mut next = self.free_list_head_offset; + // Each free block holds two lengths, so a list longer than this + // revisits a block: a cycle. + let max_blocks = size / (2 * ls as u64) + 1; + let mut walked = 0u64; + while next != FREE_NULL && next != undefined { + if next >= size || walked >= max_blocks { + return Err(FormatError::InvalidLocalHeapFreeList); + } + walked += 1; + let at = seg + .checked_add(next) + .and_then(|a| usize::try_from(a).ok()) + .ok_or(FormatError::InvalidLocalHeapFreeList)?; + let block_offset = next; + next = read_offset(file_data, at, length_size)?; + if next == 0 { + return Err(FormatError::InvalidLocalHeapFreeList); + } + let block_size = read_offset(file_data, at + ls, length_size)?; + if block_offset + .checked_add(block_size) + .is_none_or(|end| end > size) + { + return Err(FormatError::InvalidLocalHeapFreeList); + } + } + Ok(()) + } + /// Read a null-terminated string from the heap's data segment at the given byte offset. pub fn read_string(&self, file_data: &[u8], string_offset: u64) -> Result { let seg_addr = self.data_segment_address as usize; @@ -162,8 +213,8 @@ mod tests { // data_segment_size write_val(&mut file, pos, data_seg_size as u64, length_size); pos += length_size as usize; - // free_list_head_offset - write_val(&mut file, pos, 0xFFFFFFFF, length_size); + // free_list_head_offset: H5HL_FREE_NULL (no free space) + write_val(&mut file, pos, 1, length_size); pos += length_size as usize; // data_segment_address write_val(&mut file, pos, data_seg_offset as u64, offset_size); @@ -243,6 +294,50 @@ mod tests { assert_eq!(s, "test"); } + /// Heap with data segment `[a, b, c, 0-padding]` whose free list starts + /// at `head` and has one block `(next, size)` at offset 8. + fn heap_with_free_block(head: u64, next: u64, size: u64) -> Vec { + let mut file = build_heap_file(0, 100, &["abcdefg"], 8, 8); + file.resize(200, 0); + write_val(&mut file, 8, 32, 8); // data segment size + write_val(&mut file, 16, head, 8); + write_val(&mut file, 108, next, 8); + write_val(&mut file, 116, size, 8); + file + } + + #[test] + fn free_list_inside_the_segment_is_accepted() { + let file = heap_with_free_block(8, 1, 24); + let heap = LocalHeap::parse(&file, 0, 8, 8).unwrap(); + heap.validate_free_list(&file, 8).unwrap(); + assert_eq!(heap.read_string(&file, 0).unwrap(), "abcdefg"); + // An all-ones head is "no free list" too. + let file = heap_with_free_block(u64::MAX, 0, 0); + let heap = LocalHeap::parse(&file, 0, 8, 8).unwrap(); + assert!(heap.validate_free_list(&file, 8).is_ok()); + } + + #[test] + fn bad_free_list_is_rejected_like_libhdf5() { + for (head, next, size, why) in [ + (40, 1, 8, "head past the segment"), + (8, 1, 25, "block runs past the segment"), + (8, 0, 8, "next offset of zero"), + (8, 8, 8, "cycle"), + (8, 999, 8, "next past the segment"), + ] { + let file = heap_with_free_block(head, next, size); + // The header itself parses; the free list is checked on use. + let heap = LocalHeap::parse(&file, 0, 8, 8).unwrap(); + assert_eq!( + heap.validate_free_list(&file, 8).unwrap_err(), + FormatError::InvalidLocalHeapFreeList, + "{why}" + ); + } + } + #[test] fn invalid_version() { let mut file = build_heap_file(0, 100, &["x"], 8, 8); diff --git a/crates/clawhdf5/tests/local_heap_interop.rs b/crates/clawhdf5/tests/local_heap_interop.rs new file mode 100644 index 0000000..02aa6e0 --- /dev/null +++ b/crates/clawhdf5/tests/local_heap_interop.rs @@ -0,0 +1,131 @@ +//! Old-style (symbol-table) groups keep link names in a local heap. libhdf5 +//! validates the heap's free list when it loads the heap and refuses the +//! group ("bad heap free list") when the list points outside the heap; we +//! must refuse too instead of listing names read from a broken heap. Like +//! libhdf5, the check happens when a name is needed, so an empty group with +//! a broken heap still lists. +//! +//! h5py writes the files; skipped when python3 with h5py is unavailable, +//! unless `CLAWHDF5_REQUIRE_INTEROP=1`. + +use std::process::Command; + +use clawhdf5::File; + +fn python() -> String { + std::env::var("CLAWHDF5_PYTHON").unwrap_or_else(|_| "python3".to_string()) +} + +fn interop_required() -> bool { + std::env::var("CLAWHDF5_REQUIRE_INTEROP").is_ok_and(|v| v == "1") +} + +fn python_available() -> bool { + Command::new(python()) + .args(["-c", "import h5py, numpy"]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +macro_rules! skip_if_no_python { + () => { + if !python_available() { + assert!( + !interop_required(), + "CLAWHDF5_REQUIRE_INTEROP=1 but python3 with h5py is not available" + ); + eprintln!("SKIP: python3 with h5py not available"); + return; + } + }; +} + +fn run_python(script: &str) -> String { + let output = Command::new(python()) + .args(["-c", script]) + .output() + .expect("failed to run python"); + assert!( + output.status.success(), + "python failed:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).into_owned() +} + +#[test] +fn local_heap_free_list_checked_like_libhdf5() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let good = dir.path().join("good.h5"); + // Writes `good.h5` (a deleted link leaves a real free block in the root + // group's heap) and two copies whose root heap free list is broken; for + // each prints what h5py lists, or `ERROR`. + let script = format!( + r#" +import h5py, struct +good = "{good}" +with h5py.File(good, "w", libver="earliest") as f: + for name in ("alpha", "beta", "gamma"): + f.create_group(name) + del f["beta"] +data = bytearray(open(good, "rb").read()) +heap = data.find(b"HEAP") # the root group's heap is written first +size, head, seg = struct.unpack_from(" = out.lines().collect(); + assert_eq!( + lines, + [ + "good alpha gamma", + "bad_head ERROR", + "bad_block ERROR", + "bad_empty" + ], + "h5py's view changed" + ); + + let file = File::open(&good).unwrap(); + let mut groups = file.root().groups().unwrap(); + groups.sort(); + assert_eq!(groups, ["alpha", "gamma"]); + + for name in ["bad_head", "bad_block"] { + let file = File::open(dir.path().join(format!("{name}.h5"))).unwrap(); + let listed = file.root().groups(); + assert!( + listed.is_err(), + "{name}: listed {listed:?} from a heap libhdf5 rejects" + ); + } + + let file = File::open(dir.path().join("bad_empty.h5")).unwrap(); + assert_eq!(file.root().groups().unwrap(), Vec::::new()); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index 6f84c6a..4d57750 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -86,6 +86,11 @@ the VDS item, which is marked. - Groups with a user-defined link type (e.g. 187) cannot be listed. - Dense groups with more than about 22 000 links cannot be listed. - Soft links are left out of `datasets()`. + - **Wrong data (found while fixing user blocks):** an old-style group whose + local-heap free list points outside the heap listed garbage names where + libhdf5 refuses the heap. **Fixed 2026-09-25** + (`InvalidLocalHeapFreeList`, checked when a name is first read, as + libhdf5 does). - **Dense attributes:** a large attribute stored as a fractal-heap "huge" object makes every attribute on the object fail. This affects real NetCDF files (`issue671.nc`).