From 81e82940484309f5b3e7bfef50eb7f5fcdfa70c1 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 19 Sep 2026 06:31:17 -0700 Subject: [PATCH 1/6] fix(format): read datasets and attributes that use committed datatypes A dataset created from a committed (named) datatype stores only a shared- message reference to it. The facade parsed those reference bytes as the datatype itself, producing `Time { size: 0 }` and unreadable data, and an attribute using a committed datatype was silently dropped. - shared_message::parse_shared_ref had the encoding wrong: it skipped six reserved bytes for version 2 (only version 1 has them) and had the version 3 types inverted (1 is the SOHM heap, 2 is "committed, in another object header"). Verified against h5py 3.16 / HDF5 2.0, which writes `02 02
` under both default and latest libver bounds. Resolution now dispatches on which field the reference carries. - New shared_message::message_data resolves a header message through the indirection; the reader, lazy and mmap facades use it for datatype, dataspace and filter-pipeline messages. - AttributeMessage honours the v2/v3 flags (bit 0 datatype shared, bit 1 dataspace shared) via the new parse_in_file, used everywhere file data is available. Parsing a shared attribute without file access is now FormatError::UnresolvedSharedMessage instead of a garbage datatype. - h5py interop test covering both libver settings. Co-Authored-By: Claude Fable 5.1 --- crates/clawhdf5-format/src/attribute.rs | 124 ++++++++++++-- crates/clawhdf5-format/src/error.rs | 7 + crates/clawhdf5-format/src/shared_message.rs | 168 ++++++++++++------- crates/clawhdf5/src/lazy.rs | 43 ++++- crates/clawhdf5/src/mmap_file.rs | 43 ++++- crates/clawhdf5/src/reader.rs | 43 ++++- crates/clawhdf5/tests/h5py_interop_tests.rs | 49 ++++++ 7 files changed, 374 insertions(+), 103 deletions(-) diff --git a/crates/clawhdf5-format/src/attribute.rs b/crates/clawhdf5-format/src/attribute.rs index 2c8eaca..bb96bd5 100644 --- a/crates/clawhdf5-format/src/attribute.rs +++ b/crates/clawhdf5-format/src/attribute.rs @@ -1,7 +1,9 @@ //! HDF5 Attribute message parsing (message type 0x000C). #[cfg(not(feature = "std"))] -use alloc::{string::String, vec::Vec}; +use alloc::{borrow::Cow, string::String, vec::Vec}; +#[cfg(feature = "std")] +use std::borrow::Cow; use crate::attribute_info::AttributeInfoMessage; use crate::btree_v2::{BTreeV2Header, collect_btree_v2_records}; @@ -48,17 +50,64 @@ impl AttributeMessage { /// /// `length_size` is needed for dataspace dimension parsing. pub fn parse(data: &[u8], length_size: u8) -> Result { + Self::parse_impl(data, length_size, None) + } + + /// [`AttributeMessage::parse`] with access to the rest of the file, which + /// is needed when the attribute's datatype or dataspace is *shared* (v2/v3 + /// flag bits 0/1) — e.g. an attribute created with a committed datatype. + /// In that case the embedded bytes are a reference to the real message, + /// not the message. Without file access such an attribute is an error + /// rather than a garbage datatype. + pub fn parse_in_file( + data: &[u8], + file_data: &[u8], + offset_size: u8, + length_size: u8, + ) -> Result { + Self::parse_impl(data, length_size, Some((file_data, offset_size))) + } + + fn parse_impl( + data: &[u8], + length_size: u8, + file: Option<(&[u8], u8)>, + ) -> Result { ensure_len(data, 0, 2)?; let version = data[0]; match version { 1 => Self::parse_v1(data, length_size), - 2 => Self::parse_v2(data, length_size), - 3 => Self::parse_v3(data, length_size), + 2 => Self::parse_v2(data, length_size, file), + 3 => Self::parse_v3(data, length_size, file), _ => Err(FormatError::InvalidAttributeVersion(version)), } } + /// The bytes of an embedded datatype/dataspace message, following the + /// shared-message reference when `shared` is set. + fn embedded_message<'a>( + bytes: &'a [u8], + shared: bool, + msg_type: MessageType, + length_size: u8, + file: Option<(&[u8], u8)>, + ) -> Result, FormatError> { + if !shared { + 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)?; + shared_message::resolve_shared_message( + file_data, + &shared_ref, + msg_type, + offset_size, + length_size, + ) + .map(Cow::Owned) + } + fn parse_v1(data: &[u8], length_size: u8) -> Result { // version(1) + reserved(1) + name_size(2) + datatype_size(2) + dataspace_size(2) = 8 ensure_len(data, 0, 8)?; @@ -94,7 +143,13 @@ impl AttributeMessage { }) } - fn parse_v2(data: &[u8], length_size: u8) -> Result { + fn parse_v2( + data: &[u8], + length_size: u8, + file: Option<(&[u8], u8)>, + ) -> Result { + // Flags: bit 0 = datatype is shared, bit 1 = dataspace is shared. + let flags = data.get(1).copied().unwrap_or(0); // version(1) + flags(1) + name_size(2) + datatype_size(2) + dataspace_size(2) = 8 ensure_len(data, 0, 8)?; let name_size = u16::from_le_bytes([data[2], data[3]]) as usize; @@ -110,12 +165,26 @@ impl AttributeMessage { // Datatype (NO padding) ensure_len(data, pos, datatype_size)?; - let (datatype, _) = Datatype::parse(&data[pos..pos + datatype_size])?; + let dt_bytes = Self::embedded_message( + &data[pos..pos + datatype_size], + flags & 0x01 != 0, + MessageType::Datatype, + length_size, + file, + )?; + let (datatype, _) = Datatype::parse(&dt_bytes)?; pos += datatype_size; // Dataspace (NO padding) ensure_len(data, pos, dataspace_size)?; - let dataspace = Dataspace::parse(&data[pos..pos + dataspace_size], length_size)?; + let ds_bytes = Self::embedded_message( + &data[pos..pos + dataspace_size], + flags & 0x02 != 0, + MessageType::Dataspace, + length_size, + file, + )?; + let dataspace = Dataspace::parse(&ds_bytes, length_size)?; pos += dataspace_size; let raw_data = compute_raw_data(data, pos, &dataspace, &datatype); @@ -128,7 +197,13 @@ impl AttributeMessage { }) } - fn parse_v3(data: &[u8], length_size: u8) -> Result { + fn parse_v3( + data: &[u8], + length_size: u8, + file: Option<(&[u8], u8)>, + ) -> Result { + // Flags: bit 0 = datatype is shared, bit 1 = dataspace is shared. + let flags = data.get(1).copied().unwrap_or(0); // version(1) + flags(1) + name_size(2) + datatype_size(2) + dataspace_size(2) + encoding(1) = 9 ensure_len(data, 0, 9)?; let name_size = u16::from_le_bytes([data[2], data[3]]) as usize; @@ -145,12 +220,26 @@ impl AttributeMessage { // Datatype (NO padding) ensure_len(data, pos, datatype_size)?; - let (datatype, _) = Datatype::parse(&data[pos..pos + datatype_size])?; + let dt_bytes = Self::embedded_message( + &data[pos..pos + datatype_size], + flags & 0x01 != 0, + MessageType::Datatype, + length_size, + file, + )?; + let (datatype, _) = Datatype::parse(&dt_bytes)?; pos += datatype_size; // Dataspace (NO padding) ensure_len(data, pos, dataspace_size)?; - let dataspace = Dataspace::parse(&data[pos..pos + dataspace_size], length_size)?; + let ds_bytes = Self::embedded_message( + &data[pos..pos + dataspace_size], + flags & 0x02 != 0, + MessageType::Dataspace, + length_size, + file, + )?; + let dataspace = Dataspace::parse(&ds_bytes, length_size)?; pos += dataspace_size; let raw_data = compute_raw_data(data, pos, &dataspace, &datatype); @@ -326,10 +415,20 @@ pub fn extract_attributes_full( offset_size, length_size, )?; - let attr = AttributeMessage::parse(&resolved_data, length_size)?; + let attr = AttributeMessage::parse_in_file( + &resolved_data, + file_data, + offset_size, + length_size, + )?; attrs.push(attr); } else { - let attr = AttributeMessage::parse(&msg.data, length_size)?; + let attr = AttributeMessage::parse_in_file( + &msg.data, + file_data, + offset_size, + length_size, + )?; attrs.push(attr); } } @@ -399,7 +498,8 @@ fn extract_dense_attributes( let attr_data = fh.read_managed_object(file_data, id_bytes, offset_size)?; // The data in the heap is a complete attribute message - let attr = AttributeMessage::parse(&attr_data, length_size)?; + let attr = + AttributeMessage::parse_in_file(&attr_data, file_data, offset_size, length_size)?; attrs.push(attr); } diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index 183eb12..43a1cae 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -114,6 +114,9 @@ pub enum FormatError { InvalidAttributeInfoVersion(u8), /// Invalid shared message version. InvalidSharedMessageVersion(u8), + /// 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, /// Invalid SOHM table version. InvalidSohmTableVersion(u8), /// Invalid SOHM table signature (expected "SMTB"). @@ -307,6 +310,10 @@ impl fmt::Display for FormatError { FormatError::InvalidSharedMessageVersion(v) => { write!(f, "invalid shared message version: {v}") } + FormatError::UnresolvedSharedMessage => write!( + f, + "message is shared but no file data was available to resolve it" + ), FormatError::InvalidSohmTableVersion(v) => { write!(f, "invalid SOHM table version: {v}") } diff --git a/crates/clawhdf5-format/src/shared_message.rs b/crates/clawhdf5-format/src/shared_message.rs index 6ba121d..954618d 100644 --- a/crates/clawhdf5-format/src/shared_message.rs +++ b/crates/clawhdf5-format/src/shared_message.rs @@ -16,8 +16,12 @@ //! - SMLI list structure: simple list of shared message entries //! - B-tree v2 type 7: indexed shared message entries +#[cfg(not(feature = "std"))] +use alloc::borrow::Cow; #[cfg(not(feature = "std"))] use alloc::vec::Vec; +#[cfg(feature = "std")] +use std::borrow::Cow; use crate::btree_v2::{BTreeV2Header, collect_btree_v2_records}; use crate::error::FormatError; @@ -28,6 +32,14 @@ use crate::object_header::ObjectHeader; /// Fractal heap ID length for SOHM entries (fixed at 8 bytes). const FHEAP_ID_LEN: usize = 8; +/// Shared-message `type` values (version 3 encoding). +/// The message is in the file's shared-message (SOHM) fractal heap. +const SHARE_TYPE_SOHM: u8 = 1; +/// The message is in another object's header (a committed/named datatype). +const SHARE_TYPE_COMMITTED: u8 = 2; +/// The message is stored here but is sharable. +const SHARE_TYPE_HERE: u8 = 3; + /// A resolved shared message reference. #[derive(Debug, Clone)] pub struct SharedMessageRef { @@ -35,9 +47,10 @@ pub struct SharedMessageRef { pub ref_type: u8, /// Version of the shared message encoding. pub version: u8, - /// Address of the object header containing the shared message (type 1, 3). + /// Address of the object header holding the message (committed). Set for + /// every v1/v2 reference and for v3 types 2 and 3. pub object_header_address: Option, - /// Fractal heap ID for type 2 (SOHM) references. + /// Fractal heap ID for a v3 SOHM (type 1) reference. pub heap_id: Option<[u8; FHEAP_ID_LEN]>, } @@ -146,48 +159,39 @@ pub fn parse_shared_ref(data: &[u8], offset_size: u8) -> Result` + // for a dataset using a committed datatype under both default and + // `latest` libver bounds. + let address_at = |pos: usize| -> Result { + ensure_len(data, pos, offset_size as usize)?; + Ok(SharedMessageRef { + ref_type, + version, + object_header_address: Some(read_offset(data, pos, offset_size)?), + heap_id: None, + }) + }; match version { - 1 | 2 => { - // v1/v2: reserved(6) + address(offset_size) - let pos = 2 + 6; // skip reserved bytes - ensure_len(data, pos, offset_size as usize)?; - let addr = read_offset(data, pos, offset_size)?; + 1 => address_at(2 + 6), + 2 => address_at(2), + 3 if ref_type == SHARE_TYPE_SOHM => { + ensure_len(data, 2, FHEAP_ID_LEN)?; + let mut id = [0u8; FHEAP_ID_LEN]; + id.copy_from_slice(&data[2..2 + FHEAP_ID_LEN]); Ok(SharedMessageRef { ref_type, version, - object_header_address: Some(addr), - heap_id: None, + object_header_address: None, + heap_id: Some(id), }) } - 3 => { - match ref_type { - 1 | 3 => { - // type 1/3: message in another object header - // v3 layout: version(1) + type(1) + address(offset_size) - ensure_len(data, 2, offset_size as usize)?; - let addr = read_offset(data, 2, offset_size)?; - Ok(SharedMessageRef { - ref_type, - version, - object_header_address: Some(addr), - heap_id: None, - }) - } - 2 => { - // type 2: SOHM table (fractal heap ID) - ensure_len(data, 2, FHEAP_ID_LEN)?; - let mut id = [0u8; FHEAP_ID_LEN]; - id.copy_from_slice(&data[2..2 + FHEAP_ID_LEN]); - Ok(SharedMessageRef { - ref_type, - version, - object_header_address: None, - heap_id: Some(id), - }) - } - _ => Err(FormatError::InvalidSharedMessageVersion(ref_type)), - } - } + 3 if ref_type == SHARE_TYPE_COMMITTED || ref_type == SHARE_TYPE_HERE => address_at(2), + 3 => Err(FormatError::InvalidSharedMessageVersion(ref_type)), _ => Err(FormatError::InvalidSharedMessageVersion(version)), } } @@ -422,6 +426,35 @@ pub fn resolve_sohm_message( fh_header.read_managed_object(file_data, heap_id, offset_size) } +/// The payload of an object-header message, following the indirection if the +/// message is *shared* (header flag bit 1). +/// +/// A shared message's bytes are not the message itself but a reference to +/// where it lives — e.g. a dataset created with a committed (named) datatype +/// stores only a pointer to that datatype's object header. Every reader of a +/// message that may be shared (datatype, dataspace, fill value, filter +/// pipeline, attribute) must go through this; parsing the reference bytes as +/// the message yields garbage rather than an error. +pub fn message_data<'a>( + file_data: &[u8], + msg: &'a crate::object_header::HeaderMessage, + offset_size: u8, + length_size: u8, +) -> Result, FormatError> { + if !is_shared(msg.flags) { + return Ok(Cow::Borrowed(&msg.data)); + } + let shared_ref = parse_shared_ref(&msg.data, offset_size)?; + resolve_shared_message( + file_data, + &shared_ref, + msg.msg_type, + offset_size, + length_size, + ) + .map(Cow::Owned) +} + /// Resolve a shared message to its actual message data. /// /// For type 1/3 (shared in another object header), reads the target object header @@ -453,14 +486,14 @@ pub fn resolve_shared_message_with_sohm( length_size: u8, sohm_table: Option<&SohmTable>, ) -> Result, FormatError> { - match shared_ref.ref_type { - 1 | 3 => { - let addr = shared_ref - .object_header_address - .ok_or(FormatError::UnexpectedEof { - expected: 1, - available: 0, - })?; + // Dispatch on what the reference carries rather than on `ref_type`: v1/v2 + // references are always an object-header address whatever their type + // byte says. + match ( + shared_ref.object_header_address, + shared_ref.heap_id.as_ref(), + ) { + (Some(addr), _) => { let target_header = ObjectHeader::parse(file_data, addr as usize, offset_size, length_size)?; for msg in &target_header.messages { @@ -487,11 +520,7 @@ pub fn resolve_shared_message_with_sohm( available: 0, }) } - 2 => { - let heap_id = shared_ref - .heap_id - .as_ref() - .ok_or(FormatError::InvalidSharedMessageVersion(2))?; + (None, Some(heap_id)) => { let table = sohm_table.ok_or(FormatError::InvalidSharedMessageVersion(2))?; resolve_sohm_message( file_data, @@ -502,7 +531,7 @@ pub fn resolve_shared_message_with_sohm( length_size, ) } - _ => Err(FormatError::InvalidSharedMessageVersion( + (None, None) => Err(FormatError::InvalidSharedMessageVersion( shared_ref.ref_type, )), } @@ -522,15 +551,15 @@ mod tests { } #[test] - fn parse_v3_type1_ref() { + fn parse_v3_committed_ref() { let mut data = Vec::new(); data.push(3); // version - data.push(1); // type 1 = shared in another OH + 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(); assert_eq!(shared.version, 3); - assert_eq!(shared.ref_type, 1); + assert_eq!(shared.ref_type, SHARE_TYPE_COMMITTED); assert_eq!(shared.object_header_address, Some(0x1234)); assert!(shared.heap_id.is_none()); } @@ -539,7 +568,7 @@ mod tests { fn parse_v3_type3_ref() { let mut data = Vec::new(); data.push(3); // version - data.push(3); // type 3 = shared in another OH (v3 encoding) + 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(); @@ -563,10 +592,10 @@ mod tests { #[test] fn parse_v2_ref() { + // v2 dropped v1's six reserved bytes: the address follows the type. let mut data = Vec::new(); data.push(2); // version - data.push(0); // type - data.extend_from_slice(&[0u8; 6]); // reserved + data.push(SHARE_TYPE_COMMITTED); data.extend_from_slice(&0x9000u32.to_le_bytes()); let shared = parse_shared_ref(&data, 4).unwrap(); @@ -575,15 +604,26 @@ mod tests { } #[test] - fn parse_v3_type2_sohm() { + fn parse_v2_ref_from_hdf5_2_0() { + // Datatype message of a dataset created with a committed datatype, + // 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(); + assert_eq!(shared.object_header_address, Some(0xb3)); + assert!(shared.heap_id.is_none()); + } + + #[test] + fn parse_v3_sohm_ref() { let mut data = Vec::new(); data.push(3); // version - data.push(2); // type 2 = SOHM heap + 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(); assert_eq!(shared.version, 3); - assert_eq!(shared.ref_type, 2); + assert_eq!(shared.ref_type, SHARE_TYPE_SOHM); assert_eq!(shared.object_header_address, None); assert_eq!( shared.heap_id, @@ -592,10 +632,10 @@ mod tests { } #[test] - fn parse_v3_type2_too_short() { + fn parse_v3_sohm_too_short() { let mut data = Vec::new(); data.push(3); // version - data.push(2); // type 2 = SOHM heap + 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(); @@ -620,7 +660,7 @@ mod tests { fn parse_four_byte_offsets() { let mut data = Vec::new(); data.push(3); // version - data.push(1); // type 1 + data.push(SHARE_TYPE_COMMITTED); data.extend_from_slice(&0x1000u32.to_le_bytes()); let shared = parse_shared_ref(&data, 4).unwrap(); diff --git a/crates/clawhdf5/src/lazy.rs b/crates/clawhdf5/src/lazy.rs index 192a05c..4484a4c 100644 --- a/crates/clawhdf5/src/lazy.rs +++ b/crates/clawhdf5/src/lazy.rs @@ -416,15 +416,43 @@ impl<'f, R: HDF5Read> LazyDataset<'f, R> { )) } + /// A header message's payload, resolved through the shared-message + /// indirection when needed (e.g. a committed datatype). See + /// [`clawhdf5_format::shared_message::message_data`]. + fn message_payload( + &self, + msg_type: MessageType, + ) -> Result>, Error> { + self.header + .messages + .iter() + .find(|m| m.msg_type == msg_type) + .map(|msg| { + clawhdf5_format::shared_message::message_data( + self.file.as_bytes(), + msg, + self.file.offset_size(), + self.file.length_size(), + ) + .map_err(Error::Format) + }) + .transpose() + } + + fn required_payload(&self, msg_type: MessageType) -> Result, Error> { + self.message_payload(msg_type)? + .ok_or(Error::MissingMessage(msg_type)) + } + fn datatype(&self) -> Result { - let msg = find_message(&self.header, MessageType::Datatype)?; - let (dt, _) = Datatype::parse(&msg.data)?; + let data = self.required_payload(MessageType::Datatype)?; + let (dt, _) = Datatype::parse(&data)?; Ok(dt) } fn dataspace(&self) -> Result { - let msg = find_message(&self.header, MessageType::Dataspace)?; - Ok(Dataspace::parse(&msg.data, self.file.length_size())?) + let data = self.required_payload(MessageType::Dataspace)?; + Ok(Dataspace::parse(&data, self.file.length_size())?) } fn data_layout(&self) -> Result { @@ -441,11 +469,8 @@ impl<'f, R: HDF5Read> LazyDataset<'f, R> { /// filters" would hand the caller the still-compressed bytes as if they /// were the data. fn filter_pipeline(&self) -> Result, Error> { - self.header - .messages - .iter() - .find(|m| m.msg_type == MessageType::FilterPipeline) - .map(|msg| FilterPipeline::parse(&msg.data).map_err(Error::Format)) + self.message_payload(MessageType::FilterPipeline)? + .map(|data| FilterPipeline::parse(&data).map_err(Error::Format)) .transpose() } diff --git a/crates/clawhdf5/src/mmap_file.rs b/crates/clawhdf5/src/mmap_file.rs index 1573780..c1886a4 100644 --- a/crates/clawhdf5/src/mmap_file.rs +++ b/crates/clawhdf5/src/mmap_file.rs @@ -357,15 +357,43 @@ impl<'f> MmapDataset<'f> { )) } + /// A header message's payload, resolved through the shared-message + /// indirection when needed (e.g. a committed datatype). See + /// [`clawhdf5_format::shared_message::message_data`]. + fn message_payload( + &self, + msg_type: MessageType, + ) -> Result>, Error> { + self.header + .messages + .iter() + .find(|m| m.msg_type == msg_type) + .map(|msg| { + clawhdf5_format::shared_message::message_data( + self.file.as_bytes(), + msg, + self.file.offset_size(), + self.file.length_size(), + ) + .map_err(Error::Format) + }) + .transpose() + } + + fn required_payload(&self, msg_type: MessageType) -> Result, Error> { + self.message_payload(msg_type)? + .ok_or(Error::MissingMessage(msg_type)) + } + fn datatype(&self) -> Result { - let msg = find_message(&self.header, MessageType::Datatype)?; - let (dt, _) = Datatype::parse(&msg.data)?; + let data = self.required_payload(MessageType::Datatype)?; + let (dt, _) = Datatype::parse(&data)?; Ok(dt) } fn dataspace(&self) -> Result { - let msg = find_message(&self.header, MessageType::Dataspace)?; - Ok(Dataspace::parse(&msg.data, self.file.length_size())?) + let data = self.required_payload(MessageType::Dataspace)?; + Ok(Dataspace::parse(&data, self.file.length_size())?) } fn data_layout(&self) -> Result { @@ -382,11 +410,8 @@ impl<'f> MmapDataset<'f> { /// filters" would hand the caller the still-compressed bytes as if they /// were the data. fn filter_pipeline(&self) -> Result, Error> { - self.header - .messages - .iter() - .find(|m| m.msg_type == MessageType::FilterPipeline) - .map(|msg| FilterPipeline::parse(&msg.data).map_err(Error::Format)) + self.message_payload(MessageType::FilterPipeline)? + .map(|data| FilterPipeline::parse(&data).map_err(Error::Format)) .transpose() } diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index fee9594..4bbe0cd 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -723,15 +723,43 @@ impl<'f> Dataset<'f> { )?) } + /// A header message's payload, resolved through the shared-message + /// indirection when needed (e.g. a committed datatype). See + /// [`clawhdf5_format::shared_message::message_data`]. + fn message_payload( + &self, + msg_type: MessageType, + ) -> Result>, Error> { + self.header + .messages + .iter() + .find(|m| m.msg_type == msg_type) + .map(|msg| { + clawhdf5_format::shared_message::message_data( + self.file.as_bytes(), + msg, + self.file.offset_size(), + self.file.length_size(), + ) + .map_err(Error::Format) + }) + .transpose() + } + + fn required_payload(&self, msg_type: MessageType) -> Result, Error> { + self.message_payload(msg_type)? + .ok_or(Error::MissingMessage(msg_type)) + } + fn datatype(&self) -> Result { - let msg = find_message(&self.header, MessageType::Datatype)?; - let (dt, _) = Datatype::parse(&msg.data)?; + let data = self.required_payload(MessageType::Datatype)?; + let (dt, _) = Datatype::parse(&data)?; Ok(dt) } fn dataspace(&self) -> Result { - let msg = find_message(&self.header, MessageType::Dataspace)?; - Ok(Dataspace::parse(&msg.data, self.file.length_size())?) + let data = self.required_payload(MessageType::Dataspace)?; + Ok(Dataspace::parse(&data, self.file.length_size())?) } fn data_layout(&self) -> Result { @@ -748,11 +776,8 @@ impl<'f> Dataset<'f> { /// filters" would hand the caller the still-compressed bytes as if they /// were the data. fn filter_pipeline(&self) -> Result, Error> { - self.header - .messages - .iter() - .find(|m| m.msg_type == MessageType::FilterPipeline) - .map(|msg| FilterPipeline::parse(&msg.data).map_err(Error::Format)) + self.message_payload(MessageType::FilterPipeline)? + .map(|data| FilterPipeline::parse(&data).map_err(Error::Format)) .transpose() } diff --git a/crates/clawhdf5/tests/h5py_interop_tests.rs b/crates/clawhdf5/tests/h5py_interop_tests.rs index dc6121a..a7154fd 100644 --- a/crates/clawhdf5/tests/h5py_interop_tests.rs +++ b/crates/clawhdf5/tests/h5py_interop_tests.rs @@ -565,3 +565,52 @@ print("OK") ); assert_eq!(run_python_output(&script), "OK"); } + +// --------------------------------------------------------------------------- +// h5py uses committed (named) datatypes -> clawhdf5 reads +// --------------------------------------------------------------------------- + +/// A dataset or attribute created from a committed datatype stores only a +/// *shared message* reference to it. These used to be parsed as the datatype +/// itself (yielding `Time { size: 0 }` and unreadable data) and the attribute +/// was silently dropped. +#[test] +fn h5py_committed_datatypes_clawhdf5_reads() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + for (tag, kwargs) in [("default", ""), ("latest", ", libver='latest'")] { + let path = dir.path().join(format!("committed_{tag}.h5")); + let path_str = path.display().to_string(); + let script = format!( + r#" +import h5py, numpy as np +with h5py.File("{path_str}", "w"{kwargs}) as f: + f["f8type"] = np.dtype(" Date: Sat, 19 Sep 2026 06:35:47 -0700 Subject: [PATCH 2/6] feat(format): apply fill values to unallocated storage on read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HDF5 allocates lazily: a chunk nobody wrote doesn't exist in the file, and a dataset nobody wrote has no data address. Such regions must read as the dataset's fill value. There was no Fill Value message parser at all, so: - a sparse chunked dataset read its holes as zeros — silently wrong whenever the fill value isn't zero (h5py `fillvalue=-1` came back as 0); - a dataset that was created but never written failed with NoDataAllocated / "no address for chunked layout" where h5py returns a filled array. New clawhdf5_format::fill_value: parses Fill Value messages v1-v3 and the old 0x0004 message (validated against HDF5 2.0 output under default and latest libver), builds a fully filled dataset when there is no storage, and writes the fill value into exactly the chunk-grid cells absent from the chunk index — never mistaking a stored zero for a hole, clipping edge chunks, any rank. It is skipped entirely for the default (zero) fill value. The chunk index dispatch is extracted from read_chunked_data into a reusable list_chunks. The reader, lazy and mmap facades apply it on full reads; selection reads go through a fill-aware full read when the fill value matters. h5py interop test compares against h5py's own readback, including a sparse 2-D dataset and a hyperslab straddling allocated and unallocated chunks. Co-Authored-By: Claude Fable 5.1 --- crates/clawhdf5-format/src/chunked_read.rs | 36 +- crates/clawhdf5-format/src/data_read.rs | 2 +- crates/clawhdf5-format/src/fill_value.rs | 398 ++++++++++++++++++++ crates/clawhdf5-format/src/lib.rs | 1 + crates/clawhdf5/src/lazy.rs | 20 +- crates/clawhdf5/src/mmap_file.rs | 20 +- crates/clawhdf5/src/reader.rs | 40 +- crates/clawhdf5/tests/h5py_interop_tests.rs | 85 +++++ 8 files changed, 582 insertions(+), 20 deletions(-) create mode 100644 crates/clawhdf5-format/src/fill_value.rs diff --git a/crates/clawhdf5-format/src/chunked_read.rs b/crates/clawhdf5-format/src/chunked_read.rs index edb1705..8ba9332 100644 --- a/crates/clawhdf5-format/src/chunked_read.rs +++ b/crates/clawhdf5-format/src/chunked_read.rs @@ -362,15 +362,17 @@ pub fn generate_implicit_chunks( } /// Read a chunked dataset, decompressing chunks as needed. -pub fn read_chunked_data( +/// Every allocated chunk of a chunked dataset, for any supported chunk index, +/// plus the spatial chunk dimensions. Chunks the file never allocated (sparse +/// datasets) are simply absent from the list. +pub fn list_chunks( file_data: &[u8], layout: &DataLayout, dataspace: &Dataspace, - datatype: &Datatype, - pipeline: Option<&FilterPipeline>, + elem_size: usize, offset_size: u8, length_size: u8, -) -> Result, FormatError> { +) -> Result<(Vec, Vec), FormatError> { let ( chunk_dimensions, version, @@ -404,8 +406,6 @@ pub fn read_chunked_data( let addr = addr_opt .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; - let elem_size = datatype.type_size() as usize; - // Both v3 and v4 include element size as last dim (rank+1) let ndims = chunk_dimensions.len(); let rank = ndims @@ -494,6 +494,30 @@ pub fn read_chunked_data( } }; + Ok((chunks, chunk_dims)) +} + +pub fn read_chunked_data( + file_data: &[u8], + layout: &DataLayout, + dataspace: &Dataspace, + datatype: &Datatype, + pipeline: Option<&FilterPipeline>, + offset_size: u8, + length_size: u8, +) -> Result, FormatError> { + let elem_size = datatype.type_size() as usize; + let (chunks, chunk_dims) = list_chunks( + file_data, + layout, + dataspace, + elem_size, + offset_size, + length_size, + )?; + let rank = chunk_dims.len(); + let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); + // Assemble output let total_bytes = checked_byte_len(dataspace.checked_num_elements()?, elem_size)?; if total_bytes == 0 { diff --git a/crates/clawhdf5-format/src/data_read.rs b/crates/clawhdf5-format/src/data_read.rs index 9f33175..e6c4271 100644 --- a/crates/clawhdf5-format/src/data_read.rs +++ b/crates/clawhdf5-format/src/data_read.rs @@ -600,7 +600,7 @@ fn read_named_dataset_raw( } /// Extract selected elements from a full dataset buffer. -fn extract_selection_from_buffer( +pub fn extract_selection_from_buffer( full_data: &[u8], dims: &[u64], elem_size: usize, diff --git a/crates/clawhdf5-format/src/fill_value.rs b/crates/clawhdf5-format/src/fill_value.rs new file mode 100644 index 0000000..f1b0c3b --- /dev/null +++ b/crates/clawhdf5-format/src/fill_value.rs @@ -0,0 +1,398 @@ +//! Fill Value messages (0x0005, and the old 0x0004) and applying them on read. +//! +//! HDF5 allocates storage lazily: a chunk nobody wrote to does not exist in the +//! file, and a contiguous dataset nobody wrote to has no data address at all. +//! Reading such a region must yield the dataset's *fill value* (zeros unless +//! the creator chose otherwise). The readers in [`crate::chunked_read`] leave +//! those regions zeroed; [`apply_to_unallocated_chunks`] then overwrites exactly +//! the chunk-grid cells that are absent from the chunk index — so it can never +//! mistake a stored zero for a hole — and is skipped entirely in the common +//! case of a zero fill value. + +#[cfg(not(feature = "std"))] +use alloc::{format, vec, vec::Vec}; + +use crate::chunked_read::{alloc_output, checked_byte_len, list_chunks}; +use crate::data_layout::DataLayout; +use crate::dataspace::Dataspace; +use crate::error::FormatError; +use crate::message_type::MessageType; +use crate::object_header::HeaderMessage; + +/// Largest fill value accepted. A fill value is one element of the dataset's +/// datatype; this only bounds the allocation driven by the message's size field. +const MAX_FILL_VALUE_SIZE: usize = 1 << 20; + +/// Parse a Fill Value message, returning the user-defined fill value bytes, or +/// `None` when the dataset uses the default (all zeros) or has the fill value +/// explicitly undefined. +pub fn parse_fill_value(msg: &HeaderMessage) -> Result>, FormatError> { + let data = msg.data.as_slice(); + let value_at = |pos: usize| -> Result>, FormatError> { + let size_bytes = data.get(pos..pos + 4).ok_or(FormatError::UnexpectedEof { + expected: pos + 4, + available: data.len(), + })?; + let size = u32::from_le_bytes([size_bytes[0], size_bytes[1], size_bytes[2], size_bytes[3]]) + as usize; + if size == 0 { + return Ok(None); + } + if size > MAX_FILL_VALUE_SIZE { + return Err(FormatError::Overflow(format!( + "fill value of {size} bytes exceeds the {MAX_FILL_VALUE_SIZE}-byte limit" + ))); + } + let start = pos + 4; + let value = + data.get(start..start.saturating_add(size)) + .ok_or(FormatError::UnexpectedEof { + expected: start.saturating_add(size), + available: data.len(), + })?; + Ok(Some(value.to_vec())) + }; + + match msg.msg_type { + // Old fill value message: size(4), value. + MessageType::FillValueOld => value_at(0), + MessageType::FillValue => { + let version = *data.first().ok_or(FormatError::UnexpectedEof { + expected: 1, + available: 0, + })?; + match version { + // version, alloc time, write time, defined, [size, value] + 1 | 2 => { + let defined = *data.get(3).ok_or(FormatError::UnexpectedEof { + expected: 4, + available: data.len(), + })?; + if version == 2 && defined == 0 { + Ok(None) + } else if data.len() < 8 && version == 1 { + // v1 always carries a size, but tolerate its absence. + Ok(None) + } else { + value_at(4) + } + } + // version, flags (bit 4 = undefined, bit 5 = defined), [size, value] + 3 => { + let flags = *data.get(1).ok_or(FormatError::UnexpectedEof { + expected: 2, + available: data.len(), + })?; + if flags & 0x10 != 0 || flags & 0x20 == 0 { + Ok(None) + } else { + value_at(2) + } + } + v => Err(FormatError::UnsupportedVersion(v)), + } + } + _ => Ok(None), + } +} + +/// The fill value that applies to a dataset given its header messages. The new +/// message wins over the old one when both are present. +pub fn dataset_fill_value(messages: &[HeaderMessage]) -> Result>, FormatError> { + for wanted in [MessageType::FillValue, MessageType::FillValueOld] { + if let Some(msg) = messages.iter().find(|m| m.msg_type == wanted) { + if crate::shared_message::is_shared(msg.flags) { + // A shared fill value is legal but vanishingly rare; treat it + // as the default rather than misparsing the reference. + return Ok(None); + } + if let Some(value) = parse_fill_value(msg)? { + return Ok(Some(value)); + } + } + } + Ok(None) +} + +/// `true` when a fill value is absent or all zeros, i.e. identical to what the +/// readers already produce for unallocated storage. +pub fn is_default(fill: Option<&[u8]>) -> bool { + fill.is_none_or(|f| f.iter().all(|&b| b == 0)) +} + +/// A whole dataset's worth of fill value: what reading a dataset with no +/// allocated storage at all must return. +pub fn filled_dataset( + dataspace: &Dataspace, + elem_size: usize, + fill: Option<&[u8]>, +) -> Result, FormatError> { + let total = checked_byte_len(dataspace.checked_num_elements()?, elem_size)?; + let mut out = alloc_output(total)?; + if let Some(fill) = fill.filter(|f| f.len() == elem_size && !is_default(Some(f))) { + for element in out.chunks_exact_mut(elem_size) { + element.copy_from_slice(fill); + } + } + Ok(out) +} + +/// Whether the layout has any storage in the file at all. A dataset that was +/// created but never written to has none. +pub fn has_storage(layout: &DataLayout) -> bool { + !matches!( + layout, + DataLayout::Contiguous { address: None, .. } + | DataLayout::Chunked { + btree_address: None, + .. + } + ) +} + +/// Run a full-dataset `read`, giving unallocated storage its fill value: a +/// dataset with no storage at all reads as entirely fill value (instead of +/// failing), and a chunked dataset has the fill value written into every +/// chunk the file never allocated. +#[allow(clippy::too_many_arguments)] +pub fn read_full_with_fill>( + messages: &[HeaderMessage], + file_data: &[u8], + layout: &DataLayout, + dataspace: &Dataspace, + elem_size: usize, + offset_size: u8, + length_size: u8, + read: impl FnOnce() -> Result, E>, +) -> Result, E> { + let fill = dataset_fill_value(messages)?; + if !has_storage(layout) { + return Ok(filled_dataset(dataspace, elem_size, fill.as_deref())?); + } + let mut output = read()?; + apply_to_unallocated_chunks( + &mut output, + file_data, + layout, + dataspace, + elem_size, + fill.as_deref(), + offset_size, + length_size, + )?; + Ok(output) +} + +/// Overwrite, in a fully read chunked dataset `output`, every region whose +/// chunk was never allocated with `fill`. No-op for non-chunked layouts, a +/// default fill value, or a fill value whose size doesn't match the element. +#[allow(clippy::too_many_arguments)] +pub fn apply_to_unallocated_chunks( + output: &mut [u8], + file_data: &[u8], + layout: &DataLayout, + dataspace: &Dataspace, + elem_size: usize, + fill: Option<&[u8]>, + offset_size: u8, + length_size: u8, +) -> Result<(), FormatError> { + let Some(fill) = fill.filter(|f| f.len() == elem_size && !is_default(Some(f))) else { + return Ok(()); + }; + if !matches!(layout, DataLayout::Chunked { .. }) || elem_size == 0 { + return Ok(()); + } + let (chunks, chunk_dims) = list_chunks( + file_data, + layout, + dataspace, + elem_size, + offset_size, + length_size, + )?; + let rank = chunk_dims.len(); + let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); + if rank == 0 || ds_dims.len() != rank || chunk_dims.contains(&0) { + return Ok(()); + } + + // Row-major strides over the dataset and over the chunk grid. + let mut ds_strides = vec![1usize; rank]; + for i in (0..rank - 1).rev() { + ds_strides[i] = ds_strides[i + 1].saturating_mul(ds_dims[i + 1]); + } + let grid: Vec = ds_dims + .iter() + .zip(&chunk_dims) + .map(|(&d, &c)| d.div_ceil(c)) + .collect(); + let cells = grid + .iter() + .try_fold(1usize, |acc, &g| acc.checked_mul(g)) + .ok_or_else(|| FormatError::Overflow("chunk grid size overflows".into()))?; + if cells == 0 { + return Ok(()); + } + + let mut allocated = vec![false; cells]; + for chunk in &chunks { + // Undefined address: the index has a slot for the chunk but no storage. + if chunk.address == u64::MAX || chunk.offsets.len() < rank { + continue; + } + let mut cell = 0usize; + let mut in_range = true; + for d in 0..rank { + let coord = chunk.offsets[d] as usize / chunk_dims[d]; + if coord >= grid[d] { + in_range = false; + break; + } + cell = cell * grid[d] + coord; + } + if in_range { + allocated[cell] = true; + } + } + + let mut coord = vec![0usize; rank]; + for (cell, is_allocated) in allocated.iter().enumerate() { + if *is_allocated { + continue; + } + // Decode the cell index into grid coordinates. + let mut rem = cell; + for d in (0..rank).rev() { + coord[d] = rem % grid[d]; + rem /= grid[d]; + } + fill_cell( + output, + &coord, + &chunk_dims, + &ds_dims, + &ds_strides, + elem_size, + fill, + ); + } + Ok(()) +} + +/// Fill the part of chunk-grid cell `coord` that lies inside the dataset. +fn fill_cell( + output: &mut [u8], + coord: &[usize], + chunk_dims: &[usize], + ds_dims: &[usize], + ds_strides: &[usize], + elem_size: usize, + fill: &[u8], +) { + let rank = coord.len(); + let start: Vec = (0..rank).map(|d| coord[d] * chunk_dims[d]).collect(); + let end: Vec = (0..rank) + .map(|d| (start[d] + chunk_dims[d]).min(ds_dims[d])) + .collect(); + if (0..rank).any(|d| start[d] >= end[d]) { + return; + } + // Walk every row (all dims but the last) and fill the run along the last. + let run = end[rank - 1] - start[rank - 1]; + let mut idx = start.clone(); + loop { + let first: usize = (0..rank).map(|d| idx[d] * ds_strides[d]).sum(); + let from = first * elem_size; + let to = from + run * elem_size; + if let Some(region) = output.get_mut(from..to) { + for element in region.chunks_exact_mut(elem_size) { + element.copy_from_slice(fill); + } + } + // Advance the odometer over dims 0..rank-1. + let mut d = rank - 1; + loop { + if d == 0 { + return; + } + d -= 1; + idx[d] += 1; + if idx[d] < end[d] { + break; + } + idx[d] = start[d]; + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn msg(msg_type: MessageType, data: &[u8]) -> HeaderMessage { + HeaderMessage { + msg_type, + size: data.len(), + flags: 0, + creation_order: None, + data: data.to_vec(), + } + } + + #[test] + fn parses_v3_defined_undefined_and_default() { + // Real message for h5py `fillvalue=-1` on an i4 dataset (HDF5 2.0). + let defined = msg( + MessageType::FillValue, + &[3, 0x2b, 4, 0, 0, 0, 0xff, 0xff, 0xff, 0xff], + ); + assert_eq!(parse_fill_value(&defined).unwrap(), Some(vec![0xff; 4])); + let default = msg(MessageType::FillValue, &[3, 0x0a]); + assert_eq!(parse_fill_value(&default).unwrap(), None); + let undefined = msg(MessageType::FillValue, &[3, 0x19]); + assert_eq!(parse_fill_value(&undefined).unwrap(), None); + } + + #[test] + fn parses_v2_and_old_messages() { + let v2 = msg(MessageType::FillValue, &[2, 2, 2, 1, 2, 0, 0, 0, 7, 0]); + assert_eq!(parse_fill_value(&v2).unwrap(), Some(vec![7, 0])); + let v2_undefined = msg(MessageType::FillValue, &[2, 2, 2, 0]); + assert_eq!(parse_fill_value(&v2_undefined).unwrap(), None); + let old = msg(MessageType::FillValueOld, &[2, 0, 0, 0, 9, 9]); + assert_eq!(parse_fill_value(&old).unwrap(), Some(vec![9, 9])); + } + + #[test] + fn truncated_or_oversized_fill_is_an_error() { + let short = msg(MessageType::FillValue, &[3, 0x29, 4, 0, 0, 0, 0xff]); + assert!(parse_fill_value(&short).is_err()); + let huge = msg(MessageType::FillValue, &[3, 0x29, 0xff, 0xff, 0xff, 0x7f]); + assert!(matches!( + parse_fill_value(&huge), + Err(FormatError::Overflow(_)) + )); + } + + #[test] + fn fill_cell_clips_edge_chunks_in_2d() { + // 3x5 dataset, 2x2 chunks; fill grid cell (1, 2): rows 2..3, cols 4..5. + let mut out = vec![0u8; 15]; + fill_cell(&mut out, &[1, 2], &[2, 2], &[3, 5], &[5, 1], 1, &[9]); + let mut expected = vec![0u8; 15]; + expected[2 * 5 + 4] = 9; + assert_eq!(out, expected); + + // Interior cell (0, 1): rows 0..2, cols 2..4. + let mut out = vec![0u8; 15]; + fill_cell(&mut out, &[0, 1], &[2, 2], &[3, 5], &[5, 1], 1, &[7]); + let filled: Vec = out + .iter() + .enumerate() + .filter(|(_, b)| **b == 7) + .map(|(i, _)| i) + .collect(); + assert_eq!(filled, [2, 3, 7, 8]); + } +} diff --git a/crates/clawhdf5-format/src/lib.rs b/crates/clawhdf5-format/src/lib.rs index ae5623d..dae44b6 100644 --- a/crates/clawhdf5-format/src/lib.rs +++ b/crates/clawhdf5-format/src/lib.rs @@ -67,6 +67,7 @@ pub mod ea_writer; pub mod error; pub mod extensible_array; pub mod file_writer; +pub mod fill_value; pub mod filter_pipeline; pub mod filters; mod filters_szip; diff --git a/crates/clawhdf5/src/lazy.rs b/crates/clawhdf5/src/lazy.rs index 4484a4c..4a7421e 100644 --- a/crates/clawhdf5/src/lazy.rs +++ b/crates/clawhdf5/src/lazy.rs @@ -480,15 +480,27 @@ impl<'f, R: HDF5Read> LazyDataset<'f, R> { let dl = self.data_layout()?; let pipeline = self.filter_pipeline()?; let data = self.file.reader.as_bytes(); - Ok(data_read::read_raw_data_full( + // Unallocated storage reads as the dataset's fill value. + clawhdf5_format::fill_value::read_full_with_fill( + &self.header.messages, data, &dl, &ds, - &dt, - pipeline.as_ref(), + dt.type_size() as usize, self.file.offset_size(), self.file.length_size(), - )?) + || { + Ok(data_read::read_raw_data_full( + data, + &dl, + &ds, + &dt, + pipeline.as_ref(), + self.file.offset_size(), + self.file.length_size(), + )?) + }, + ) } } diff --git a/crates/clawhdf5/src/mmap_file.rs b/crates/clawhdf5/src/mmap_file.rs index c1886a4..9119cdf 100644 --- a/crates/clawhdf5/src/mmap_file.rs +++ b/crates/clawhdf5/src/mmap_file.rs @@ -420,15 +420,27 @@ impl<'f> MmapDataset<'f> { let ds = self.dataspace()?; let dl = self.data_layout()?; let pipeline = self.filter_pipeline()?; - Ok(data_read::read_raw_data_full( + // 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(), &dl, &ds, - &dt, - pipeline.as_ref(), + dt.type_size() as usize, self.file.offset_size(), self.file.length_size(), - )?) + || { + Ok(data_read::read_raw_data_full( + self.file.reader.as_bytes(), + &dl, + &ds, + &dt, + pipeline.as_ref(), + self.file.offset_size(), + self.file.length_size(), + )?) + }, + ) } } diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 4bbe0cd..4c9da19 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -448,6 +448,24 @@ impl<'f> Dataset<'f> { let ds = self.dataspace()?; let dl = self.data_layout()?; let pipeline = self.filter_pipeline()?; + // The selection reader knows nothing about fill values. When they + // matter — no storage at all, or a non-zero fill on a chunked (possibly + // sparse) dataset — select from a fill-aware full read instead. (The + // selection reader currently decodes the full dataset too, so this + // costs nothing extra.) + let fill = clawhdf5_format::fill_value::dataset_fill_value(&self.header.messages)?; + let fill_matters = !clawhdf5_format::fill_value::has_storage(&dl) + || (matches!(dl, DataLayout::Chunked { .. }) + && !clawhdf5_format::fill_value::is_default(fill.as_deref())); + if fill_matters { + let full = self.read_raw()?; + return Ok(data_read::extract_selection_from_buffer( + &full, + &ds.dimensions, + dt.type_size() as usize, + selection, + )?); + } Ok(data_read::read_raw_data_selection( self.file.data.as_bytes(), &dl, @@ -808,16 +826,28 @@ impl<'f> Dataset<'f> { )?); } - Ok(data_read::read_raw_data_cached( + // Unallocated storage reads as the dataset's fill value. + clawhdf5_format::fill_value::read_full_with_fill( + &self.header.messages, self.file.data.as_bytes(), &dl, &ds, - &dt, - pipeline.as_ref(), + dt.type_size() as usize, self.file.offset_size(), self.file.length_size(), - &self.file.chunk_cache, - )?) + || { + Ok(data_read::read_raw_data_cached( + self.file.data.as_bytes(), + &dl, + &ds, + &dt, + pipeline.as_ref(), + self.file.offset_size(), + self.file.length_size(), + &self.file.chunk_cache, + )?) + }, + ) } } diff --git a/crates/clawhdf5/tests/h5py_interop_tests.rs b/crates/clawhdf5/tests/h5py_interop_tests.rs index a7154fd..ef6d01a 100644 --- a/crates/clawhdf5/tests/h5py_interop_tests.rs +++ b/crates/clawhdf5/tests/h5py_interop_tests.rs @@ -614,3 +614,88 @@ with h5py.File("{path_str}", "w"{kwargs}) as f: ); } } + +// --------------------------------------------------------------------------- +// h5py writes sparse / never-written datasets -> clawhdf5 applies fill values +// --------------------------------------------------------------------------- + +/// Parse h5py's `print(arr.ravel().tolist())` output for integer data. +fn parse_int_list(s: &str) -> Vec { + s.trim() + .trim_matches(|c| c == '[' || c == ']') + .split(',') + .filter(|t| !t.trim().is_empty()) + .map(|t| t.trim().parse().unwrap()) + .collect() +} + +/// Storage HDF5 never allocated must read as the dataset's fill value. These +/// used to read as zeros (silently wrong for a non-zero fill value) or fail +/// outright (`NoDataAllocated`) for a dataset that was never written. +#[test] +fn h5py_fill_values_clawhdf5_reads() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + for (tag, kwargs) in [("default", ""), ("latest", ", libver='latest'")] { + let path = dir.path().join(format!("fill_{tag}.h5")); + let path_str = path.display().to_string(); + let script = format!( + r#" +import h5py, numpy as np +with h5py.File("{path_str}", "w"{kwargs}) as f: + d = f.create_dataset("partial", shape=(20,), dtype="> = run_python_output(&script) + .lines() + .map(|line| { + let (name, list) = line.split_once(' ').unwrap(); + (name.to_string(), parse_int_list(list)) + }) + .collect(); + + let file = File::open(&path).unwrap(); + for name in [ + "partial", + "never", + "never_chunked", + "default_fill", + "gz", + "sparse2d", + ] { + assert_eq!( + file.dataset(name).unwrap().read_i32().unwrap(), + expected[name], + "{tag}/{name}" + ); + } + // A hyperslab straddling allocated and unallocated chunks. + let slab = clawhdf5_format::selection::Selection::Hyperslab { + start: vec![1, 2], + stride: vec![1, 1], + count: vec![4, 5], + block: vec![1, 1], + }; + assert_eq!( + file.dataset("sparse2d") + .unwrap() + .read_i32_selection(&slab) + .unwrap(), + expected["slab"], + "{tag}/sparse2d hyperslab" + ); + } +} From e38c8133bcda52c0d4b6f670d674b5d501269e40 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 19 Sep 2026 06:39:01 -0700 Subject: [PATCH 3/6] feat(format): follow soft links; explicit errors for external links and external raw data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Path resolution follows soft links in both old-style (symbol table, cache type 2) and new-style (compact and dense Link message) groups: absolute and relative targets, links to groups, links through links, with a depth limit so a link cycle is NestingDepthExceeded rather than a hang. A dangling link reports the target it could not find. Previously every soft link was PathNotFound. - An external link is FormatError::ExternalLinkUnsupported { filename, object_path } instead of a misleading PathNotFound. - Message 0x0007 (External Data Files) is now a known MessageType, and a dataset carrying it is FormatError::ExternalDataFilesUnsupported. Such a dataset has no data address in this file, so it would otherwise be read as "never written" and answered with fill values — wrong data, no error. - Dense link iteration is shared between hard-link listing and the new symbolic-link lookup; entry listing behaviour is unchanged. - h5py interop test for both libver settings. Co-Authored-By: Claude Fable 5.1 --- crates/clawhdf5-format/src/error.rs | 23 +++ crates/clawhdf5-format/src/fill_value.rs | 9 ++ crates/clawhdf5-format/src/group_v1.rs | 48 ++++++ crates/clawhdf5-format/src/group_v2.rs | 153 +++++++++++++++++--- crates/clawhdf5-format/src/message_type.rs | 17 ++- crates/clawhdf5/tests/h5py_interop_tests.rs | 85 +++++++++++ 6 files changed, 314 insertions(+), 21 deletions(-) diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index 43a1cae..a8fa0ec 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -117,6 +117,17 @@ 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, + /// The dataset's raw data is stored in external files (External Data + /// Files message), which this reader does not follow. + ExternalDataFilesUnsupported, + /// The path goes through an external link (a link into another file), + /// which this reader does not follow. + ExternalLinkUnsupported { + /// The file the link points into. + filename: String, + /// The object path within that file. + object_path: String, + }, /// Invalid SOHM table version. InvalidSohmTableVersion(u8), /// Invalid SOHM table signature (expected "SMTB"). @@ -310,6 +321,18 @@ impl fmt::Display for FormatError { FormatError::InvalidSharedMessageVersion(v) => { write!(f, "invalid shared message version: {v}") } + FormatError::ExternalLinkUnsupported { + filename, + object_path, + } => write!( + f, + "path goes through an external link to {object_path} in {filename}, which is \ + not supported" + ), + FormatError::ExternalDataFilesUnsupported => write!( + f, + "dataset raw data is stored in external file(s), which is not supported" + ), FormatError::UnresolvedSharedMessage => write!( f, "message is shared but no file data was available to resolve it" diff --git a/crates/clawhdf5-format/src/fill_value.rs b/crates/clawhdf5-format/src/fill_value.rs index f1b0c3b..d10ba47 100644 --- a/crates/clawhdf5-format/src/fill_value.rs +++ b/crates/clawhdf5-format/src/fill_value.rs @@ -165,6 +165,15 @@ pub fn read_full_with_fill>( length_size: u8, read: impl FnOnce() -> Result, E>, ) -> Result, E> { + // A dataset with external raw data also has no data address in this + // file. It is NOT unallocated — its values live elsewhere — so it must + // never be answered with the fill value. + if messages + .iter() + .any(|m| m.msg_type == MessageType::ExternalDataFiles) + { + return Err(FormatError::ExternalDataFilesUnsupported.into()); + } let fill = dataset_fill_value(messages)?; if !has_storage(layout) { return Ok(filled_dataset(dataspace, elem_size, fill.as_deref())?); diff --git a/crates/clawhdf5-format/src/group_v1.rs b/crates/clawhdf5-format/src/group_v1.rs index f295039..989f826 100644 --- a/crates/clawhdf5-format/src/group_v1.rs +++ b/crates/clawhdf5-format/src/group_v1.rs @@ -60,6 +60,54 @@ pub fn resolve_v1_group_entries( Ok(entries) } +/// Symbol table cache type for a soft link: the scratch pad's first four bytes +/// are the local-heap offset of the link's target path, and the entry's object +/// header address is undefined. +const CACHE_TYPE_SOFT_LINK: u32 = 2; + +/// The target path of the soft link called `name` in a v1 group, if any. +pub fn find_v1_soft_link( + file_data: &[u8], + sym_table_msg: &SymbolTableMessage, + name: &str, + offset_size: u8, + length_size: u8, +) -> Result, FormatError> { + let heap = LocalHeap::parse( + file_data, + sym_table_msg.local_heap_address as usize, + offset_size, + length_size, + )?; + let snod_addrs = collect_symbol_table_nodes( + file_data, + sym_table_msg.btree_address, + offset_size, + length_size, + )?; + 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.read_string(file_data, entry.link_name_offset)? != name { + continue; + } + let value_offset = u32::from_le_bytes([ + entry.scratch_pad[0], + entry.scratch_pad[1], + entry.scratch_pad[2], + entry.scratch_pad[3], + ]); + return heap + .read_string(file_data, u64::from(value_offset)) + .map(Some); + } + } + Ok(None) +} + /// Extract the SymbolTableMessage from an object header's messages. fn find_symbol_table_message( obj_header: &ObjectHeader, diff --git a/crates/clawhdf5-format/src/group_v2.rs b/crates/clawhdf5-format/src/group_v2.rs index 87f1d65..903c7d6 100644 --- a/crates/clawhdf5-format/src/group_v2.rs +++ b/crates/clawhdf5-format/src/group_v2.rs @@ -63,14 +63,15 @@ fn resolve_compact_entries( Ok(entries) } -/// Resolve entries from dense storage (fractal heap + B-tree v2). -fn resolve_dense_entries( +/// Visit every link in dense storage (fractal heap + B-tree v2 name index). +fn for_each_dense_link( file_data: &[u8], link_info: &LinkInfoMessage, fh_addr: u64, offset_size: u8, length_size: u8, -) -> Result, FormatError> { + mut visit: impl FnMut(LinkMessage), +) -> Result<(), FormatError> { // Parse fractal heap let fh = FractalHeapHeader::parse(file_data, fh_addr as usize, offset_size, length_size)?; @@ -81,7 +82,6 @@ fn resolve_dense_entries( let btree_hdr = BTreeV2Header::parse(file_data, btree_addr as usize, offset_size, length_size)?; let records = collect_btree_v2_records(file_data, &btree_hdr, offset_size, length_size)?; - let mut entries = Vec::new(); for record in &records { // For type 5 (name index): hash(4) + heap_id(heap_id_length) // For type 6 (creation order): creation_order(8) + heap_id(heap_id_length) @@ -98,22 +98,94 @@ fn resolve_dense_entries( // Read managed object from fractal heap let link_data = fh.read_managed_object(file_data, id_bytes, offset_size)?; + visit(LinkMessage::parse(&link_data, offset_size)?); + } + Ok(()) +} - // Parse as Link message - let link = LinkMessage::parse(&link_data, offset_size)?; - if let LinkTarget::Hard { - object_header_address, - } = link.link_target - { - entries.push(GroupEntry { - name: link.name, +/// Resolve entries from dense storage (fractal heap + B-tree v2). +fn resolve_dense_entries( + file_data: &[u8], + link_info: &LinkInfoMessage, + fh_addr: u64, + offset_size: u8, + length_size: u8, +) -> Result, FormatError> { + let mut entries = Vec::new(); + for_each_dense_link( + file_data, + link_info, + fh_addr, + offset_size, + length_size, + |link| { + if let LinkTarget::Hard { object_header_address, - cache_type: 0, - }); + } = link.link_target + { + entries.push(GroupEntry { + name: link.name, + object_header_address, + cache_type: 0, + }); + } + }, + )?; + Ok(entries) +} + +/// The soft or external link called `name` in this group, if there is one. +/// Hard links are what `resolve_group_entries` returns; this is consulted only +/// when a path component isn't among them. +fn find_symbolic_link( + file_data: &[u8], + object_header: &ObjectHeader, + name: &str, + offset_size: u8, + length_size: u8, +) -> Result, FormatError> { + if is_v1_group(object_header) { + let Some(sym_msg) = object_header + .messages + .iter() + .find(|m| m.msg_type == MessageType::SymbolTable) + else { + return Ok(None); + }; + let stm = SymbolTableMessage::parse(&sym_msg.data, offset_size)?; + return group_v1::find_v1_soft_link(file_data, &stm, name, offset_size, length_size) + .map(|target| target.map(|target_path| LinkTarget::Soft { target_path })); + } + if !is_v2_group(object_header) { + return Ok(None); + } + let is_symbolic = |t: &LinkTarget| !matches!(t, LinkTarget::Hard { .. }); + let link_info = find_link_info(object_header, offset_size)?; + let mut found = None; + if let Some(fh_addr) = link_info.fractal_heap_address { + for_each_dense_link( + file_data, + &link_info, + fh_addr, + offset_size, + length_size, + |link| { + if link.name == name && is_symbolic(&link.link_target) { + found = Some(link.link_target); + } + }, + )?; + } else { + for msg in &object_header.messages { + if msg.msg_type == MessageType::Link { + let link = LinkMessage::parse(&msg.data, offset_size)?; + if link.name == name && is_symbolic(&link.link_target) { + found = Some(link.link_target); + } + } } } - - Ok(entries) + Ok(found) } /// Find and parse the Link Info message from an object header. @@ -158,6 +230,19 @@ pub fn resolve_path_any( file_data: &[u8], superblock: &Superblock, path: &str, +) -> Result { + resolve_path_following_links(file_data, superblock, path, 0) +} + +/// Soft links followed while resolving one path. Guards against link cycles +/// (`a -> b -> a`), which are legal to create. +const MAX_SOFT_LINK_DEPTH: u8 = 16; + +fn resolve_path_following_links( + file_data: &[u8], + superblock: &Superblock, + path: &str, + depth: u8, ) -> Result { let components: Vec<&str> = path.split('/').filter(|s| !s.is_empty()).collect(); if components.is_empty() { @@ -176,7 +261,9 @@ pub fn resolve_path_any( for (i, component) in components.iter().enumerate() { let entries = resolve_group_entries(file_data, ¤t_header, os, ls)?; - let found = entries.iter().find(|e| e.name == *component); + let found = entries + .iter() + .find(|e| e.name == *component && e.object_header_address != u64::MAX); match found { Some(entry) => { if i == components.len() - 1 { @@ -186,7 +273,37 @@ pub fn resolve_path_any( current_header = ObjectHeader::parse(file_data, current_addr as usize, os, ls)?; } None => { - return Err(FormatError::PathNotFound(String::from(*component))); + return match find_symbolic_link(file_data, ¤t_header, component, os, ls)? { + Some(LinkTarget::Soft { target_path }) => { + if depth >= MAX_SOFT_LINK_DEPTH { + return Err(FormatError::NestingDepthExceeded); + } + // A relative target is relative to the group holding + // the link; then the rest of the original path. + let mut full = String::new(); + if !target_path.starts_with('/') { + for parent in &components[..i] { + full.push('/'); + full.push_str(parent); + } + } + full.push('/'); + full.push_str(&target_path); + for rest in &components[i + 1..] { + full.push('/'); + full.push_str(rest); + } + resolve_path_following_links(file_data, superblock, &full, depth + 1) + } + Some(LinkTarget::External { + filename, + object_path, + }) => Err(FormatError::ExternalLinkUnsupported { + filename, + object_path, + }), + _ => Err(FormatError::PathNotFound(String::from(*component))), + }; } } } diff --git a/crates/clawhdf5-format/src/message_type.rs b/crates/clawhdf5-format/src/message_type.rs index 6b66a9a..dce4d24 100644 --- a/crates/clawhdf5-format/src/message_type.rs +++ b/crates/clawhdf5-format/src/message_type.rs @@ -9,6 +9,9 @@ pub enum MessageType { Datatype, FillValueOld, FillValue, + /// External Data Files (0x0007): the dataset's raw data lives in other + /// files, listed by this message. + ExternalDataFiles, Link, DataLayout, GroupInfo, @@ -36,6 +39,7 @@ impl MessageType { 0x0004 => MessageType::FillValueOld, 0x0005 => MessageType::FillValue, 0x0006 => MessageType::Link, + 0x0007 => MessageType::ExternalDataFiles, 0x0008 => MessageType::DataLayout, 0x000A => MessageType::GroupInfo, 0x000B => MessageType::FilterPipeline, @@ -60,6 +64,7 @@ impl MessageType { MessageType::Datatype => 0x0003, MessageType::FillValueOld => 0x0004, MessageType::FillValue => 0x0005, + MessageType::ExternalDataFiles => 0x0007, MessageType::Link => 0x0006, MessageType::DataLayout => 0x0008, MessageType::GroupInfo => 0x000A, @@ -90,6 +95,7 @@ mod tests { (0x0003, MessageType::Datatype), (0x0004, MessageType::FillValueOld), (0x0005, MessageType::FillValue), + (0x0007, MessageType::ExternalDataFiles), (0x0006, MessageType::Link), (0x0008, MessageType::DataLayout), (0x000A, MessageType::GroupInfo), @@ -119,8 +125,13 @@ mod tests { #[test] fn unknown_type_zero_gap() { - // 0x0007 is not a defined type - let mt = MessageType::from_u16(0x0007); - assert_eq!(mt, MessageType::Unknown(0x0007)); + // 0x0009 is reserved for the library's own testing; no file uses it. + let mt = MessageType::from_u16(0x0009); + assert_eq!(mt, MessageType::Unknown(0x0009)); + // 0x0007 used to be treated as unknown: it is External Data Files. + assert_eq!( + MessageType::from_u16(0x0007), + MessageType::ExternalDataFiles + ); } } diff --git a/crates/clawhdf5/tests/h5py_interop_tests.rs b/crates/clawhdf5/tests/h5py_interop_tests.rs index ef6d01a..83becfb 100644 --- a/crates/clawhdf5/tests/h5py_interop_tests.rs +++ b/crates/clawhdf5/tests/h5py_interop_tests.rs @@ -699,3 +699,88 @@ with h5py.File("{path_str}", "r") as f: ); } } + +// --------------------------------------------------------------------------- +// h5py writes soft / external links and external raw data -> clawhdf5 +// --------------------------------------------------------------------------- + +/// Soft links are followed (absolute, relative, through groups, with a cycle +/// guard). Things this reader does not follow — external links, and datasets +/// whose raw data lives in another file — are explicit errors. They used to +/// surface as a misleading `PathNotFound`, and external raw data could read +/// back as fill values. +#[test] +fn h5py_links_clawhdf5_resolves_or_refuses() { + use clawhdf5::Error; + use clawhdf5_format::error::FormatError; + + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let dir_str = dir.path().display().to_string(); + for (tag, kwargs) in [("default", ""), ("latest", ", libver='latest'")] { + let script = format!( + r#" +import h5py, numpy as np, os +os.chdir("{dir_str}") +with h5py.File("other_{tag}.h5", "w"{kwargs}) as o: + o.create_dataset("remote", data=np.arange(3, dtype=" Date: Sat, 19 Sep 2026 06:39:30 -0700 Subject: [PATCH 4/6] security(clawhdf5): confine virtual-dataset source files to the base directory The VDS resolver joined the source file name stored in the HDF5 file straight onto the opened file's directory. That name is untrusted: an absolute path replaces the base directory outright and `..` components climb out of it, so a crafted file could make the reader open any path the process can reach. Only plain relative paths of normal components are accepted now; anything else resolves to "source not found". Co-Authored-By: Claude Fable 5.1 --- crates/clawhdf5/src/reader.rs | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 4c9da19..fb17557 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -812,7 +812,7 @@ impl<'f> Dataset<'f> { let base_dir = self.file.base_dir.clone(); let resolver = move |name: &str| -> Option> { let dir = base_dir.as_ref()?; - std::fs::read(dir.join(name)).ok() + std::fs::read(dir.join(sibling_file_name(name)?)).ok() }; return Ok(data_read::read_raw_data_full_with_resolver( self.file.data.as_bytes(), @@ -888,6 +888,23 @@ fn datatype_byte_order(dt: &Datatype) -> DatatypeByteOrder { } } +/// A source-file name taken from inside an HDF5 file, accepted only if it +/// stays within the directory of the file that named it. +/// +/// The name is untrusted input. Joining it blindly lets a crafted file make +/// the reader open any path the process can reach — an absolute path replaces +/// the base directory entirely, and `..` components climb out of it. Only +/// plain relative paths made of normal components are allowed. +fn sibling_file_name(name: &str) -> Option<&std::path::Path> { + use std::path::Component; + let path = std::path::Path::new(name); + let mut components = path.components().peekable(); + components.peek()?; + components + .all(|c| matches!(c, Component::Normal(_) | Component::CurDir)) + .then_some(path) +} + fn find_message( header: &ObjectHeader, msg_type: MessageType, @@ -941,3 +958,24 @@ fn resolve_group_entries( Ok(Vec::new()) } } + +#[cfg(test)] +mod sibling_file_name_tests { + use super::sibling_file_name; + + #[test] + fn only_paths_inside_the_base_directory_are_accepted() { + for ok in ["source.h5", "./source.h5", "sub/dir/source.h5"] { + assert!(sibling_file_name(ok).is_some(), "{ok}"); + } + for bad in [ + "", + "/etc/passwd", + "../secret.h5", + "sub/../../secret.h5", + "sub/../ok.h5", + ] { + assert!(sibling_file_name(bad).is_none(), "{bad}"); + } + } +} From 24afcdc70f1b7933be2b9911da659aa9955afe24 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 19 Sep 2026 06:43:34 -0700 Subject: [PATCH 5/6] test(agent): WAL property tests, crash-recovery matrix, WAL fuzz target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - tests/wal_properties.rs — deterministic generator, reproducible by seed: everything appended is read back intact (300 cases), and after ANY damage to the file (bit flips, truncation, inserted/deleted bytes, duplicated or rotated regions, overwritten ranges; 1500 cases) reading never panics and yields an exact prefix of what was written — the guarantee the chained CRC exists to give. Opening for append then repairs the tail and a new entry lands right behind the surviving prefix. - tests/crash_recovery.rs — builds the on-disk images a process crash can leave and reopens each against a model of what was acknowledged: an image after every operation (random saves, in-place updates, checkpoints, small wal_max_entries), the checkpoint window (new .h5 + not-yet-truncated WAL) over several rounds, and the WAL torn at every byte length, which must recover the checkpoint plus a prefix of the operations logged since. - fuzz/fuzz_wal_replay — arbitrary bytes as a WAL: read and open-for-append must not panic, and open() must not change what is replayable. Based on the target from the clawmates mission branch (4aee2fa), with the repair property added. The CI fuzz step now covers both fuzz crates. Co-Authored-By: Claude Fable 5.1 --- crates/clawhdf5-agent/fuzz/.gitignore | 3 + crates/clawhdf5-agent/fuzz/Cargo.toml | 23 ++ .../fuzz/fuzz_targets/fuzz_wal_replay.rs | 36 +++ crates/clawhdf5-agent/tests/crash_recovery.rs | 187 +++++++++++++++ crates/clawhdf5-agent/tests/wal_properties.rs | 213 ++++++++++++++++++ scripts/ci-test.sh | 14 +- 6 files changed, 470 insertions(+), 6 deletions(-) create mode 100644 crates/clawhdf5-agent/fuzz/.gitignore create mode 100644 crates/clawhdf5-agent/fuzz/Cargo.toml create mode 100644 crates/clawhdf5-agent/fuzz/fuzz_targets/fuzz_wal_replay.rs create mode 100644 crates/clawhdf5-agent/tests/crash_recovery.rs create mode 100644 crates/clawhdf5-agent/tests/wal_properties.rs diff --git a/crates/clawhdf5-agent/fuzz/.gitignore b/crates/clawhdf5-agent/fuzz/.gitignore new file mode 100644 index 0000000..c937f05 --- /dev/null +++ b/crates/clawhdf5-agent/fuzz/.gitignore @@ -0,0 +1,3 @@ +target/ +artifacts/ +coverage/ diff --git a/crates/clawhdf5-agent/fuzz/Cargo.toml b/crates/clawhdf5-agent/fuzz/Cargo.toml new file mode 100644 index 0000000..6f8bc01 --- /dev/null +++ b/crates/clawhdf5-agent/fuzz/Cargo.toml @@ -0,0 +1,23 @@ +[package] +name = "clawhdf5-agent-fuzz" +version = "0.0.0" +publish = false +edition = "2024" + +[package.metadata] +cargo-fuzz = true + +[dependencies] +libfuzzer-sys = "0.4" +tempfile = "3" + +[dependencies.clawhdf5-agent] +path = ".." + +[workspace] +members = ["."] + +[[bin]] +name = "fuzz_wal_replay" +path = "fuzz_targets/fuzz_wal_replay.rs" +doc = false diff --git a/crates/clawhdf5-agent/fuzz/fuzz_targets/fuzz_wal_replay.rs b/crates/clawhdf5-agent/fuzz/fuzz_targets/fuzz_wal_replay.rs new file mode 100644 index 0000000..a4a81a9 --- /dev/null +++ b/crates/clawhdf5-agent/fuzz/fuzz_targets/fuzz_wal_replay.rs @@ -0,0 +1,36 @@ +#![no_main] +//! Arbitrary bytes as a WAL file. Reading, and opening for append (which scans +//! the chain and truncates an unverifiable tail), must never panic, hang, or +//! allocate without bound — and after `open` repairs the file, everything +//! `read_entries` returned before must still be returned. +//! +//! The deterministic counterpart that runs in ordinary CI is +//! `tests/wal_properties.rs`; this target explores inputs it cannot reach. + +use std::io::Write as _; + +use clawhdf5_agent::wal::WalFile; +use libfuzzer_sys::fuzz_target; + +fuzz_target!(|data: &[u8]| { + let Ok(mut tmp) = tempfile::NamedTempFile::new() else { + return; + }; + if tmp.write_all(data).and_then(|()| tmp.flush()).is_err() { + return; + } + let before = WalFile::read_entries(tmp.path()).map(|e| e.len()); + // Only the chained formats (header versions 3 and 4) are repaired in + // place. `open` deliberately recreates a legacy-format file from scratch: + // `HDF5Memory::open` has already replayed its entries by then. + let chained = matches!(data.get(4), Some(3 | 4)); + let opened = WalFile::open(tmp.path()); + if !chained { + return; + } + if let (Ok(before), Ok(wal)) = (before, opened) { + drop(wal); + let after = WalFile::read_entries(tmp.path()).map(|e| e.len()); + assert_eq!(after.ok(), Some(before), "open() changed what is replayable"); + } +}); diff --git a/crates/clawhdf5-agent/tests/crash_recovery.rs b/crates/clawhdf5-agent/tests/crash_recovery.rs new file mode 100644 index 0000000..bd91179 --- /dev/null +++ b/crates/clawhdf5-agent/tests/crash_recovery.rs @@ -0,0 +1,187 @@ +//! Crash-recovery matrix for `HDF5Memory`. +//! +//! A process crash leaves whatever reached the OS on disk. These tests build +//! the on-disk images such a crash can leave behind — after every operation, +//! inside the checkpoint window (new `.h5` in place, WAL not yet truncated), +//! and with the WAL torn at every possible length — then reopen each image +//! and check the recovered store against a model of what was acknowledged. +//! +//! Invariants: +//! * never a duplicated or invented record; +//! * an image taken between operations recovers *exactly* the acknowledged +//! state; +//! * a torn WAL recovers the last checkpoint plus a prefix of the operations +//! logged since. + +use std::path::{Path, PathBuf}; + +use clawhdf5_agent::{AgentMemory, HDF5Memory, MemoryConfig, MemoryEntry}; +use tempfile::TempDir; + +struct Rng(u64); + +impl Rng { + fn next(&mut self) -> u64 { + self.0 = self.0.wrapping_add(0x9E37_79B9_7F4A_7C15); + let mut z = self.0; + z = (z ^ (z >> 30)).wrapping_mul(0xBF58_476D_1CE4_E5B9); + z = (z ^ (z >> 27)).wrapping_mul(0x94D0_49BB_1331_11EB); + z ^ (z >> 31) + } + fn below(&mut self, n: usize) -> usize { + (self.next() % n.max(1) as u64) as usize + } +} + +fn entry(chunk: &str, tags: &str) -> MemoryEntry { + MemoryEntry { + chunk: chunk.to_string(), + embedding: vec![1.0, 0.0, 0.0, 0.0], + source_channel: "test".into(), + timestamp: 1.0, + session_id: "s".into(), + tags: tags.to_string(), + } +} + +fn wal_path(h5: &Path) -> PathBuf { + h5.with_extension("h5.wal") +} + +/// Copy the store (`.h5` + WAL) into a fresh directory, as a crash image. +fn image(h5: &Path, into: &TempDir, name: &str) -> PathBuf { + let dest = into.path().join(format!("{name}.h5")); + std::fs::copy(h5, &dest).unwrap(); + if wal_path(h5).exists() { + std::fs::copy(wal_path(h5), wal_path(&dest)).unwrap(); + } + dest +} + +fn recovered(h5: &Path) -> Vec { + // Read-only: the image must not be modified, and no lock is needed. + HDF5Memory::open_read_only(h5).unwrap().cache.chunks.clone() +} + +/// Apply one random operation to the store and to the model. +fn step(mem: &mut HDF5Memory, model: &mut Vec, rng: &mut Rng, n: usize) { + match rng.below(6) { + 0 => mem.flush_wal().unwrap(), + 1 if !model.is_empty() => { + // Update an existing record in place, addressed by its tag. + let idx = rng.below(model.len()); + let chunk = format!("u{n}"); + assert_eq!( + mem.save_or_update(entry(&chunk, &format!("tag{idx}"))) + .unwrap(), + idx + ); + model[idx] = chunk; + } + _ => { + let chunk = format!("c{n}"); + mem.save(entry(&chunk, &format!("tag{}", model.len()))) + .unwrap(); + model.push(chunk); + } + } +} + +#[test] +fn image_after_every_operation_recovers_the_acknowledged_state() { + for seed in 0..40u64 { + let mut rng = Rng(seed); + let dir = TempDir::new().unwrap(); + let images = TempDir::new().unwrap(); + let mut config = MemoryConfig::new(dir.path().join("store.h5"), "agent", 4); + config.wal_enabled = true; + config.wal_max_entries = 1 + rng.below(6); // force frequent checkpoints + let h5 = config.path.clone(); + let mut mem = HDF5Memory::create(config).unwrap(); + let mut model = Vec::new(); + + for n in 0..30 { + step(&mut mem, &mut model, &mut rng, n); + let img = image(&h5, &images, &format!("s{seed}-{n}")); + assert_eq!(recovered(&img), model, "seed {seed}, after op {n}"); + } + } +} + +#[test] +fn crash_inside_the_checkpoint_window_never_duplicates() { + for seed in 0..40u64 { + let mut rng = Rng(seed ^ 0xABCD); + let dir = TempDir::new().unwrap(); + let images = TempDir::new().unwrap(); + let mut config = MemoryConfig::new(dir.path().join("store.h5"), "agent", 4); + config.wal_enabled = true; + config.wal_max_entries = 1000; // checkpoints only when we ask + let h5 = config.path.clone(); + let mut mem = HDF5Memory::create(config).unwrap(); + let mut model = Vec::new(); + + for round in 0..4 { + for n in 0..(1 + rng.below(6)) { + step(&mut mem, &mut model, &mut rng, round * 100 + n); + } + // The WAL as it is just before the checkpoint... + let stale_wal = images.path().join(format!("stale-{seed}-{round}.wal")); + if wal_path(&h5).exists() { + std::fs::copy(wal_path(&h5), &stale_wal).unwrap(); + } + mem.flush_wal().unwrap(); + // ...put back next to the NEW .h5: the crash-in-the-window image. + let img = image(&h5, &images, &format!("w{seed}-{round}")); + if stale_wal.exists() { + std::fs::copy(&stale_wal, wal_path(&img)).unwrap(); + } + assert_eq!(recovered(&img), model, "seed {seed}, round {round}"); + } + } +} + +#[test] +fn torn_wal_recovers_checkpoint_plus_a_prefix() { + let dir = TempDir::new().unwrap(); + let images = TempDir::new().unwrap(); + let mut config = MemoryConfig::new(dir.path().join("store.h5"), "agent", 4); + config.wal_enabled = true; + config.wal_max_entries = 1000; + let h5 = config.path.clone(); + let mut mem = HDF5Memory::create(config).unwrap(); + + for name in ["a", "b"] { + mem.save(entry(name, name)).unwrap(); + } + mem.flush_wal().unwrap(); + let checkpointed = vec!["a".to_string(), "b".to_string()]; + + // States the store passes through as each later op is logged. + let mut states = vec![checkpointed.clone()]; + let mut model = checkpointed.clone(); + mem.save(entry("c", "c")).unwrap(); + model.push("c".into()); + states.push(model.clone()); + mem.save_or_update(entry("a2", "a")).unwrap(); + model[0] = "a2".into(); + states.push(model.clone()); + mem.save(entry("d", "d")).unwrap(); + model.push("d".into()); + states.push(model.clone()); + + let full_wal = std::fs::read(wal_path(&h5)).unwrap(); + let mut seen = std::collections::BTreeSet::new(); + for len in 0..=full_wal.len() { + let img = image(&h5, &images, &format!("t{len}")); + std::fs::write(wal_path(&img), &full_wal[..len]).unwrap(); + let got = recovered(&img); + let which = states + .iter() + .position(|s| *s == got) + .unwrap_or_else(|| panic!("WAL torn at {len} bytes recovered {got:?}")); + seen.insert(which); + } + // Every intermediate state is reachable, and the full WAL gives the last. + assert_eq!(seen.into_iter().collect::>(), [0, 1, 2, 3]); +} diff --git a/crates/clawhdf5-agent/tests/wal_properties.rs b/crates/clawhdf5-agent/tests/wal_properties.rs new file mode 100644 index 0000000..ad714d5 --- /dev/null +++ b/crates/clawhdf5-agent/tests/wal_properties.rs @@ -0,0 +1,213 @@ +//! Property tests for the write-ahead log. +//! +//! A deterministic generator (no external crates, reproducible from the seed +//! printed on failure) drives thousands of cases through two properties: +//! +//! 1. **Round trip** — whatever was appended is read back, in order, intact. +//! 2. **Prefix under corruption** — after *any* damage to the file (bit flips, +//! truncation, inserted or deleted bytes, duplicated or reordered regions), +//! reading never panics and yields an exact *prefix* of what was written. +//! This is the guarantee the chained CRC exists to provide: replay may stop +//! early, but it never returns a corrupted, reordered, or invented entry. + +use clawhdf5_agent::wal::{WalEntry, WalEntryType, WalFile}; + +/// SplitMix64: tiny, well-distributed, and fully determined by its seed. +struct Rng(u64); + +impl Rng { + fn next(&mut self) -> u64 { + self.0 = self.0.wrapping_add(0x9E37_79B9_7F4A_7C15); + let mut z = self.0; + z = (z ^ (z >> 30)).wrapping_mul(0xBF58_476D_1CE4_E5B9); + z = (z ^ (z >> 27)).wrapping_mul(0x94D0_49BB_1331_11EB); + z ^ (z >> 31) + } + + fn below(&mut self, n: usize) -> usize { + (self.next() % n.max(1) as u64) as usize + } + + fn string(&mut self, max_len: usize) -> String { + const ALPHABET: &[char] = &['a', 'Z', '0', ' ', '\n', '\0', 'é', '漢', '🦀', '"']; + (0..self.below(max_len + 1)) + .map(|_| ALPHABET[self.below(ALPHABET.len())]) + .collect() + } +} + +/// What a test appended, in a form comparable with what is read back. +#[derive(Debug, Clone, PartialEq)] +enum Logged { + Save(String, Vec, String, String, String, u64), + Update(usize, String, Vec, u64), + Tombstone(usize, u64), +} + +fn logged(entry: &WalEntry) -> Logged { + // Compare floats by bit pattern so NaN payloads and -0.0 count as intact. + let bits: Vec = entry.embedding.iter().map(|f| f.to_bits()).collect(); + let ts = entry.timestamp.to_bits(); + match entry.entry_type { + WalEntryType::Save => Logged::Save( + entry.chunk.clone(), + bits, + entry.source_channel.clone(), + entry.session_id.clone(), + entry.tags.clone(), + ts, + ), + WalEntryType::Update => { + Logged::Update(entry.update_index.unwrap(), entry.chunk.clone(), bits, ts) + } + WalEntryType::Tombstone => Logged::Tombstone(entry.tombstone_index.unwrap(), ts), + WalEntryType::ActivationUpdate => unreachable!("never written by these tests"), + } +} + +/// Append a random mix of records; return what was written. +fn write_random_wal(path: &std::path::Path, rng: &mut Rng) -> Vec { + let mut wal = WalFile::open(path).unwrap(); + let mut written = Vec::new(); + for _ in 0..rng.below(12) { + let timestamp = f64::from_bits(rng.next()); + if rng.below(5) == 0 { + let index = rng.below(1000); + wal.append_tombstone(index, timestamp).unwrap(); + written.push(Logged::Tombstone(index, timestamp.to_bits())); + continue; + } + let update_index = (rng.below(4) == 0).then(|| rng.below(1000)); + let entry = WalEntry { + entry_type: if update_index.is_some() { + WalEntryType::Update + } else { + WalEntryType::Save + }, + timestamp, + chunk: rng.string(40), + embedding: (0..rng.below(9)) + .map(|_| f32::from_bits(rng.next() as u32)) + .collect(), + source_channel: rng.string(8), + session_id: rng.string(8), + tags: rng.string(8), + tombstone_index: None, + update_index, + }; + wal.append_save(&entry).unwrap(); + written.push(logged(&entry)); + } + written +} + +fn read_back(path: &std::path::Path) -> Option> { + WalFile::read_entries(path) + .ok() + .map(|entries| entries.iter().map(logged).collect()) +} + +#[test] +fn everything_appended_is_read_back_intact() { + let dir = tempfile::TempDir::new().unwrap(); + for seed in 0..300u64 { + let path = dir.path().join(format!("rt-{seed}.wal")); + let written = write_random_wal(&path, &mut Rng(seed)); + assert_eq!(read_back(&path).unwrap(), written, "seed {seed}"); + // Reopening (which scans and repositions) must not disturb anything. + drop(WalFile::open(&path).unwrap()); + assert_eq!( + read_back(&path).unwrap(), + written, + "seed {seed} after reopen" + ); + } +} + +/// Damage `bytes` in one of several ways. +fn corrupt(bytes: &mut Vec, rng: &mut Rng) { + if bytes.is_empty() { + return; + } + match rng.below(7) { + 0 => { + let i = rng.below(bytes.len()); + bytes[i] ^= 1 << rng.below(8); + } + 1 => bytes.truncate(rng.below(bytes.len())), + 2 => { + let i = rng.below(bytes.len() + 1); + bytes.insert(i, rng.next() as u8); + } + 3 => { + let i = rng.below(bytes.len()); + bytes.remove(i); + } + 4 => { + // Duplicate a region in place (a replayed/duplicated entry). + let a = rng.below(bytes.len()); + let b = a + rng.below(bytes.len() - a); + let region = bytes[a..b].to_vec(); + let at = rng.below(bytes.len() + 1); + bytes.splice(at..at, region); + } + 5 => { + // Swap two regions (reordered entries). + let mid = rng.below(bytes.len()); + bytes.rotate_left(mid); + } + _ => { + let i = rng.below(bytes.len()); + let n = rng.below(bytes.len() - i + 1); + for b in &mut bytes[i..i + n] { + *b = rng.next() as u8; + } + } + } +} + +#[test] +fn any_corruption_yields_a_prefix_never_a_wrong_entry() { + let dir = tempfile::TempDir::new().unwrap(); + let mut shortened = 0u32; + for seed in 0..1500u64 { + let mut rng = Rng(seed ^ 0xC0FF_EE00); + let path = dir.path().join("c.wal"); + let _ = std::fs::remove_file(&path); + let written = write_random_wal(&path, &mut rng); + + let mut bytes = std::fs::read(&path).unwrap(); + for _ in 0..=rng.below(3) { + corrupt(&mut bytes, &mut rng); + } + std::fs::write(&path, &bytes).unwrap(); + + // An unreadable header is a clean error; anything else is a prefix. + if let Some(read) = read_back(&path) { + assert!( + read.len() <= written.len() && read[..] == written[..read.len()], + "seed {seed}: read {read:?}\nis not a prefix of {written:?}" + ); + if read.len() < written.len() { + shortened += 1; + } + // Opening for append repairs the tail; what was readable stays so, + // and a new entry lands right after it. + if let Ok(mut wal) = WalFile::open(&path) { + wal.append_tombstone(7, 1.0).unwrap(); + drop(wal); + let mut expected = read.clone(); + expected.push(Logged::Tombstone(7, 1.0f64.to_bits())); + assert_eq!( + read_back(&path).unwrap(), + expected, + "seed {seed} after repair" + ); + } + } + } + assert!( + shortened > 100, + "corruption rarely took effect: {shortened}" + ); +} diff --git a/scripts/ci-test.sh b/scripts/ci-test.sh index 65dc9ea..aa7146d 100755 --- a/scripts/ci-test.sh +++ b/scripts/ci-test.sh @@ -95,12 +95,14 @@ run_step "check-nostd.sh" "$SCRIPT_DIR/check-nostd.sh" # 8. Optional fuzz smoke run if [ -n "${CLAWHDF5_FUZZ_SECONDS:-}" ]; then fuzz_smoke() { - local target - cd "$SCRIPT_DIR/../crates/clawhdf5-format" || return 1 - for target in $(cargo +nightly fuzz list); do - echo "--- fuzz: $target" - cargo +nightly fuzz run "$target" -- \ - -max_total_time="$CLAWHDF5_FUZZ_SECONDS" || return 1 + local crate target + for crate in clawhdf5-format clawhdf5-agent; do + cd "$SCRIPT_DIR/../crates/$crate" || return 1 + for target in $(cargo +nightly fuzz list); do + echo "--- fuzz: $crate/$target" + cargo +nightly fuzz run "$target" -- \ + -max_total_time="$CLAWHDF5_FUZZ_SECONDS" || return 1 + done done } run_step "fuzz smoke (${CLAWHDF5_FUZZ_SECONDS}s/target)" fuzz_smoke From a0ff8ef32c0442fc8ce96019d6fb58a13b6b28ea Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 19 Sep 2026 06:43:54 -0700 Subject: [PATCH 6/6] docs: changelog and known issues for the format robustness work Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 36 ++++++++++++++++++++++++++++++++++++ docs/known-issues.md | 24 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a4eeb6..50f6397 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,37 @@ - `clawhdf5-agent`: `benches/bench.rs` and `benches/memory_bench.rs` no longer compiled against the current `strategy`/`consolidation` APIs. +### HDF5 Compatibility +- `clawhdf5-format`/`clawhdf5`: datasets and attributes that use a **committed + (named) datatype** now read correctly. They store a shared-message reference; + the facade parsed the reference bytes as the datatype (`Time { size: 0 }`, + unreadable data) and silently dropped such attributes. The shared-reference + parser itself was wrong for real files: version 2 has no reserved bytes, and + the version 3 types were inverted (1 = SOHM heap, 2 = committed). +- **Fill values are applied on read.** There was no Fill Value message parser: + the holes of a sparse chunked dataset read as zeros even when the fill value + was not zero (silently wrong data), and a dataset that was created but never + written failed with `NoDataAllocated` where h5py returns a filled array. + Messages v1–v3 and the old 0x0004 form are parsed; the fill value is written + into exactly the chunk-grid cells missing from the chunk index. +- **Soft links are followed** during path resolution, in old- and new-style + groups (absolute/relative targets, links to groups, links through links), + with a depth limit so a link cycle is an error rather than a hang. A dangling + link reports the target it could not find. +- Things the reader does not follow are now explicit errors instead of wrong + answers: an external link is `ExternalLinkUnsupported { filename, + object_path }` (was `PathNotFound`), and a dataset whose raw data lives in + external files (message 0x0007, now a known `MessageType`) is + `ExternalDataFilesUnsupported` (it would otherwise read as fill values). +- All of the above are covered by h5py interop tests under both default and + `libver='latest'` bounds, compared against h5py's own readback. + +### Security +- `clawhdf5`: virtual-dataset source file names are untrusted input but were + joined straight onto the opened file's directory, so a crafted file could + make the reader open any path the process can reach (absolute path, or `..` + components). Only plain relative paths inside that directory are accepted. + ### Durability & Integrity - `clawhdf5-agent`: a crash between writing a checkpoint and truncating the WAL no longer **duplicates every pending entry** on the next open. Each @@ -79,6 +110,11 @@ The `#[ignore]`d `writer_h5py_tests` suite is run explicitly. - h5py-generated-file tests now cover default libver bounds as well as `libver='latest'` (HDF5 2.0 raised the default low bound to 1.8). +- `clawhdf5-agent`: WAL property tests (round trip; after any corruption the + entries read back are an exact prefix of what was written — 1500 seeded + cases), a crash-recovery matrix (an on-disk image after every operation, the + checkpoint window, and the WAL torn at every byte length, each reopened and + checked against a model), and a WAL fuzz target. - Optional fuzz smoke run (`CLAWHDF5_FUZZ_SECONDS=N scripts/ci-test.sh`); new datatype corpus seeds for v1 compound and native complex messages. diff --git a/docs/known-issues.md b/docs/known-issues.md index d078acc..37fe945 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -107,3 +107,27 @@ only attributes convertible to `AttrValue`. An attribute with, e.g., a compound datatype is omitted from the map with no error or indication that it exists. Planned: surface these as an explicit `AttrValue` variant (raw bytes + datatype) or an error, as part of the "no silent skips" robustness work. + +## B-tree v2 chunk index (layout v4, index type 5) is not supported + +**Status:** open. + +**Summary:** a chunked dataset with **two or more unlimited dimensions** written +with `libver='latest'` indexes its chunks with a version-2 B-tree. Reading it +fails with `ChunkedReadError("unsupported chunked layout version=4, +index_type=Some(5)")`. Single-chunk, implicit, fixed-array and +extensible-array indexes (and the v3 B-tree v1) are supported. + +**Repro:** `f.create_dataset("d", shape=(5, 7), chunks=(2, 3), maxshape=(None, None))` +with `h5py.File(..., libver='latest')`. + +## External links and external raw data are not followed + +**Status:** open (by design for now); both are explicit errors. + +**Summary:** a path through an external link returns +`FormatError::ExternalLinkUnsupported { filename, object_path }`, and a dataset +created with `external=[...]` storage returns +`FormatError::ExternalDataFilesUnsupported`. Neither is resolved. If support is +added, file names must be confined to the opened file's directory, as the +virtual-dataset resolver now does.