From f4dee1cd084a3e08a16dffc3ba1568bbe16c8fb3 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 00:02:49 -0500 Subject: [PATCH 01/17] fix(format): refuse object headers libhdf5 refuses to load ObjectHeader::parse now checks each header message the way libhdf5's H5O__chunk_deserialize does, and fails with InvalidObjectHeader (libhdf5's own error text) instead of reading objects out of a corrupt header: - v1: every message in chunk 0 is read (not just the prefix's count) and more messages than the prefix claims is "bad object header message count"; message sizes must be multiples of 8; leftover bytes are a gap, which only v2 allows; the prefix's chunk size must fit its count. - v1 and v2: a message running past its chunk is an error (it used to end the chunk quietly, dropping it and everything after); contradictory message flags; a message of a class that cannot be shared flagged shared/shareable; a reference-count message in a v1 header; malformed continuation, reference-count and modification-time messages (libhdf5 decodes these while loading the header). - v2: unknown header status flags, max_compact < min_dense, a chunk 0 smaller than a message header, a gap in a chunk that has NIL messages. Conformance (cached corpus, tank): 569 -> 570 ok (h5stat_err_refcount.h5). Objects libhdf5 refuses that clawhdf5 used to read: cve-2016-4332-mtime (/dataset), cve-2016-4332-mtime-new, cve-2018-11204, cve-2018-13873, cve-2024-32619, cve-2024-33873, cve-2024-33874, gh-4433-poc-08; seven more CVE objects that already failed now fail with libhdf5's reason. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/error.rs | 8 + crates/clawhdf5-format/src/object_header.rs | 626 ++++++++++++++++---- 2 files changed, 504 insertions(+), 130 deletions(-) diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index bb81939..6d038f5 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -201,6 +201,11 @@ pub enum FormatError { DuplicateDatasetName(String), /// Integer overflow in size computation (malformed data protection). Overflow(String), + /// An object header that libhdf5 refuses to load (the reason is + /// libhdf5's own error text): a misaligned or overrunning message, a + /// wrong message count, contradictory message flags, a message of a + /// class that cannot be shared flagged shareable, … + InvalidObjectHeader(&'static str), } impl fmt::Display for FormatError { @@ -445,6 +450,9 @@ impl fmt::Display for FormatError { FormatError::Overflow(msg) => { write!(f, "integer overflow: {msg}") } + FormatError::InvalidObjectHeader(why) => { + write!(f, "corrupt object header: {why}") + } } } } diff --git a/crates/clawhdf5-format/src/object_header.rs b/crates/clawhdf5-format/src/object_header.rs index 4b8067f..b28fdad 100644 --- a/crates/clawhdf5-format/src/object_header.rs +++ b/crates/clawhdf5-format/src/object_header.rs @@ -108,10 +108,20 @@ impl ObjectHeader { return Err(FormatError::InvalidObjectHeaderVersion(version)); } - let num_messages = LittleEndian::read_u16(&data[offset + 2..offset + 4]); + let num_messages = LittleEndian::read_u16(&data[offset + 2..offset + 4]) as usize; let reference_count = LittleEndian::read_u32(&data[offset + 4..offset + 8]); let header_data_size = LittleEndian::read_u32(&data[offset + 8..offset + 12]) as usize; + // libhdf5 (H5O__prefix_deserialize): a header with messages needs room + // for at least one message header, and one without has an empty chunk. + if (num_messages > 0 && header_data_size < V1_MSG_HEADER_SIZE) + || (num_messages == 0 && header_data_size > 0) + { + return Err(FormatError::InvalidObjectHeader( + "bad object header chunk size", + )); + } + // Pad to 8-byte alignment: header prefix is 12 bytes, pad to 16 let padding = 4; // pad 12-byte prefix to 16-byte alignment let msg_start = offset @@ -124,64 +134,23 @@ impl ObjectHeader { ensure_len(data, msg_start, header_data_size)?; let mut messages = Vec::new(); - let mut pos = msg_start; - let msg_end = - msg_start - .checked_add(header_data_size) - .ok_or(FormatError::UnexpectedEof { - expected: usize::MAX, - available: data.len(), - })?; - - for _ in 0..num_messages { - if pos + 8 > msg_end { - break; - } - let msg_type_raw = LittleEndian::read_u16(&data[pos..pos + 2]); - let msg_data_size = LittleEndian::read_u16(&data[pos + 2..pos + 4]) as usize; - let msg_flags = data[pos + 4]; - // reserved(3) at pos+5..pos+8 - pos += 8; - - ensure_len(data, pos, msg_data_size)?; - let msg_type = MessageType::from_u16(msg_type_raw); - - check_unknown_message(msg_type, msg_flags)?; - - if msg_type != MessageType::Nil { - messages.push(HeaderMessage { - msg_type, - size: msg_data_size, - flags: msg_flags, - creation_order: None, - data: data[pos..pos + msg_data_size].to_vec(), - }); - } - - pos += msg_data_size; - - // Follow continuations - if msg_type == MessageType::ObjectHeaderContinuation { - let cont_msg_data = &messages - .last() - .ok_or(FormatError::InvalidObjectHeaderSignature)? - .data; - if cont_msg_data.len() >= (offset_size as usize + length_size as usize) { - let cont_offset = read_offset(cont_msg_data, 0, offset_size)? as usize; - let cont_length = - read_offset(cont_msg_data, offset_size as usize, length_size)? as usize; - // Parse continuation block (v1: just raw messages, no signature) - let cont_msgs = Self::parse_v1_continuation( - data, - cont_offset, - cont_length, - offset_size, - length_size, - 32, // max continuation depth - )?; - messages.extend(cont_msgs); - } - } + let chunk0_count = Self::parse_v1_chunk( + data, + msg_start, + header_data_size, + offset_size, + length_size, + MAX_V1_CONTINUATION_DEPTH, + &mut messages, + )?; + // libhdf5 reads every message in the first chunk and refuses a header + // whose prefix claims fewer than that (continuation chunks are read + // later and not held to the count). Stopping after the claimed number + // silently dropped the rest. + if chunk0_count > num_messages { + return Err(FormatError::InvalidObjectHeader( + "bad object header message count", + )); } Ok(ObjectHeader { @@ -196,72 +165,87 @@ impl ObjectHeader { }) } - fn parse_v1_continuation( + /// Parse the messages of one version-1 chunk (`length` bytes at + /// `offset`, no signature), following continuation messages as they are + /// met. Returns how many messages (NIL ones included) this chunk itself + /// holds. + /// + /// A version-1 chunk is filled with messages whose sizes are multiples of + /// 8; libhdf5 refuses a message that is not aligned, that runs past the + /// end of the chunk, or leftover bytes too few for a message header (a + /// "gap", which only version 2 allows). + #[allow(clippy::too_many_arguments)] + fn parse_v1_chunk( data: &[u8], offset: usize, length: usize, offset_size: u8, length_size: u8, depth_remaining: u16, - ) -> Result, FormatError> { + messages: &mut Vec, + ) -> Result { if depth_remaining == 0 { return Err(FormatError::NestingDepthExceeded); } ensure_len(data, offset, length)?; - let mut messages = Vec::new(); + let end = offset + length; let mut pos = offset; - let end = offset.saturating_add(length); + let mut count = 0usize; - while pos + 8 <= end { + while pos < end { + if end - pos < V1_MSG_HEADER_SIZE { + return Err(FormatError::InvalidObjectHeader( + "gap found in early version of file format", + )); + } let msg_type_raw = LittleEndian::read_u16(&data[pos..pos + 2]); let msg_data_size = LittleEndian::read_u16(&data[pos + 2..pos + 4]) as usize; let msg_flags = data[pos + 4]; - pos += 8; + // reserved(3) at pos+5..pos+8 + pos += V1_MSG_HEADER_SIZE; - if pos + msg_data_size > end { - break; + if !msg_data_size.is_multiple_of(8) { + return Err(FormatError::InvalidObjectHeader("message not aligned")); } + if msg_data_size > end - pos { + return Err(FormatError::InvalidObjectHeader( + "message size exceeds buffer end", + )); + } + let body = &data[pos..pos + msg_data_size]; + check_message(1, msg_type_raw, msg_flags, body, offset_size, length_size)?; + count += 1; let msg_type = MessageType::from_u16(msg_type_raw); - - check_unknown_message(msg_type, msg_flags)?; - if msg_type != MessageType::Nil { messages.push(HeaderMessage { msg_type, size: msg_data_size, flags: msg_flags, creation_order: None, - data: data[pos..pos + msg_data_size].to_vec(), + data: body.to_vec(), }); } - pos += msg_data_size; - // Recursive continuations + // Follow continuations (v1 continuation chunks are just raw + // messages, no signature); check_message has checked the body. if msg_type == MessageType::ObjectHeaderContinuation { - let cont_msg_data = &messages - .last() - .ok_or(FormatError::InvalidObjectHeaderSignature)? - .data; - if cont_msg_data.len() >= (offset_size as usize + length_size as usize) { - let cont_offset = read_offset(cont_msg_data, 0, offset_size)? as usize; - let cont_length = - read_offset(cont_msg_data, offset_size as usize, length_size)? as usize; - let cont_msgs = Self::parse_v1_continuation( - data, - cont_offset, - cont_length, - offset_size, - length_size, - depth_remaining - 1, - )?; - messages.extend(cont_msgs); - } + let cont_offset = read_offset(body, 0, offset_size)? as usize; + let cont_length = read_offset(body, offset_size as usize, length_size)? as usize; + Self::parse_v1_chunk( + data, + cont_offset, + cont_length, + offset_size, + length_size, + depth_remaining - 1, + messages, + )?; } } - Ok(messages) + Ok(count) } fn parse_v2( @@ -278,6 +262,11 @@ impl ObjectHeader { return Err(FormatError::InvalidObjectHeaderVersion(version)); } let flags = data[offset + 5]; + if flags & !V2_HDR_ALL_FLAGS != 0 { + return Err(FormatError::InvalidObjectHeader( + "unknown object header status flag(s)", + )); + } let mut pos = offset + 6; @@ -297,7 +286,14 @@ impl ObjectHeader { // Optional attribute storage thresholds (flags bit 4) if flags & 0x10 != 0 { ensure_len(data, pos, 4)?; - // max_compact_attrs(2) + min_dense_attrs(2) — read but don't store for now + // max_compact_attrs(2) + min_dense_attrs(2) — checked, not stored + let max_compact = LittleEndian::read_u16(&data[pos..pos + 2]); + let min_dense = LittleEndian::read_u16(&data[pos + 2..pos + 4]); + if max_compact < min_dense { + return Err(FormatError::InvalidObjectHeader( + "bad object header attribute phase change values", + )); + } pos += 4; } @@ -312,6 +308,14 @@ impl ObjectHeader { ensure_len(data, pos, chunk_size_width as usize)?; let chunk0_size = read_offset(data, pos, chunk_size_width)? as usize; pos += chunk_size_width as usize; + // Bit 2: attribute creation order tracked → messages include creation order field + let has_creation_order = flags & 0x04 != 0; + let msg_header_size = if has_creation_order { 6 } else { 4 }; + if chunk0_size > 0 && chunk0_size < msg_header_size { + return Err(FormatError::InvalidObjectHeader( + "bad object header chunk size", + )); + } let chunk0_msg_start = pos; let chunk0_msg_end = pos @@ -335,9 +339,6 @@ impl ObjectHeader { } } - // Bit 2: attribute creation order tracked → messages include creation order field - let has_creation_order = flags & 0x04 != 0; - // Parse messages from chunk0 let mut messages = Vec::new(); let mut continuations = Vec::new(); @@ -396,8 +397,20 @@ impl ObjectHeader { ) -> Result<(), FormatError> { let msg_header_size = if has_creation_order { 6 } else { 4 }; let mut pos = start; + let mut null_count = 0usize; - while pos + msg_header_size <= end { + while pos < end { + // Leftover bytes too few for a message header are a gap, which + // libhdf5 allows only in a chunk without NIL messages (a writer + // that leaves a gap had no NIL message to put the space in). + if end - pos < msg_header_size { + if null_count != 0 { + return Err(FormatError::InvalidObjectHeader( + "gap in chunk with no null messages", + )); + } + break; + } let msg_type_raw = data[pos] as u16; let msg_data_size = LittleEndian::read_u16(&data[pos + 1..pos + 3]) as usize; let msg_flags = data[pos + 3]; @@ -408,32 +421,29 @@ impl ObjectHeader { }; pos += msg_header_size; - if pos + msg_data_size > end { - // Could be padding at end of chunk - break; + if msg_data_size > end - pos { + return Err(FormatError::InvalidObjectHeader( + "message size exceeds buffer end", + )); } + let body = &data[pos..pos + msg_data_size]; + check_message(2, msg_type_raw, msg_flags, body, offset_size, length_size)?; let msg_type = MessageType::from_u16(msg_type_raw); - - check_unknown_message(msg_type, msg_flags)?; - - let msg_data = data[pos..pos + msg_data_size].to_vec(); - if msg_type == MessageType::ObjectHeaderContinuation { - // Parse continuation offset/length from message data - if msg_data.len() >= (offset_size as usize + length_size as usize) { - let cont_off = read_offset(&msg_data, 0, offset_size)? as usize; - let cont_len = - read_offset(&msg_data, offset_size as usize, length_size)? as usize; - continuations.push((cont_off, cont_len)); - } - } else if msg_type != MessageType::Nil { + // check_message has checked the body holds both fields. + let cont_off = read_offset(body, 0, offset_size)? as usize; + let cont_len = read_offset(body, offset_size as usize, length_size)? as usize; + continuations.push((cont_off, cont_len)); + } else if msg_type == MessageType::Nil { + null_count += 1; + } else { messages.push(HeaderMessage { msg_type, size: msg_data_size, flags: msg_flags, creation_order, - data: msg_data, + data: body.to_vec(), }); } @@ -496,22 +506,149 @@ impl ObjectHeader { } } -/// Header message flag bit 7: fail if the message is unknown, always. +/// Size of a version-1 message header: type(2) + size(2) + flags(1) + reserved(3). +const V1_MSG_HEADER_SIZE: usize = 8; + +/// How deep version-1 continuation chunks may chain (malformed-data guard). +const MAX_V1_CONTINUATION_DEPTH: u16 = 32; + +/// Every defined version-2 object header status flag (libhdf5 +/// `H5O_HDR_ALL_FLAGS`): chunk-0 size width (bits 0-1), attribute creation +/// order tracked/indexed, attribute phase-change values, times stored. +const V2_HDR_ALL_FLAGS: u8 = 0x3F; + +// Header message flag bits (libhdf5 `H5O_MSG_FLAG_*`). Bit 0 (constant) needs +// no check. Bit 3 (fail if unknown and the file is opened for writing) never +// fails a read: the parser only ever reads, as libhdf5 ignores it for a +// read-only open. +const MSG_FLAG_SHARED: u8 = 0x02; +const MSG_FLAG_DONTSHARE: u8 = 0x04; +const MSG_FLAG_FAIL_IF_UNKNOWN_AND_OPEN_FOR_WRITE: u8 = 0x08; +const MSG_FLAG_MARK_IF_UNKNOWN: u8 = 0x10; +const MSG_FLAG_WAS_UNKNOWN: u8 = 0x20; +const MSG_FLAG_SHAREABLE: u8 = 0x40; +/// Fail if the message is unknown, whatever the access mode. const MSG_FLAG_FAIL_IF_UNKNOWN_ALWAYS: u8 = 0x80; -/// Refuse an unknown message the file says no reader may skip. +/// Message type ids libhdf5 has a class for (`H5O_msg_class_g`): 0x00-0x18 +/// except 0x09 (a test-only "bogus" message). Anything else is an unknown +/// message. +fn is_known_message(id: u16) -> bool { + id <= 0x18 && id != 0x09 +} + +/// Message classes that may be shared (`H5O_SHARE_IS_SHARABLE`): dataspace, +/// datatype, the two fill-value messages, filter pipeline and attribute. +fn is_shareable_message(id: u16) -> bool { + matches!(id, 0x01 | 0x03 | 0x04 | 0x05 | 0x0B | 0x0C) +} + +/// Check one header message the way libhdf5 does while it loads an object +/// header (`H5O__chunk_deserialize`), so an object libhdf5 refuses to open is +/// refused here too instead of being read from a corrupt header: /// -/// The parser only ever reads, so bit 3 (fail only when opened for writing) -/// is ignored, as libhdf5 ignores it for a read-only open; bit 7 fails -/// regardless of access mode. This had the two the wrong way round, failing -/// objects libhdf5 reads and reading ones it refuses (`tbogus.h5`). -fn check_unknown_message(msg_type: MessageType, msg_flags: u8) -> Result<(), FormatError> { - match msg_type { - MessageType::Unknown(id) if msg_flags & MSG_FLAG_FAIL_IF_UNKNOWN_ALWAYS != 0 => { - Err(FormatError::UnsupportedMessage(id)) - } - _ => Ok(()), +/// - contradictory flag combinations; +/// - an unknown message the file says no reader may skip (bit 7). This had +/// bits 3 and 7 the wrong way round once, failing objects libhdf5 reads +/// and reading ones it refuses (`tbogus.h5`); +/// - a known message whose class cannot be shared, flagged shared or +/// shareable (`cve-2016-4332`); +/// - the messages libhdf5 decodes while loading the header, whose decode +/// errors fail the load: continuation, reference count (which a version-1 +/// header cannot hold), and both modification-time messages. +fn check_message( + header_version: u8, + id: u16, + flags: u8, + body: &[u8], + offset_size: u8, + length_size: u8, +) -> Result<(), FormatError> { + let bad_flags = FormatError::InvalidObjectHeader("bad flag combination for message"); + if flags & MSG_FLAG_SHARED != 0 && flags & MSG_FLAG_DONTSHARE != 0 { + return Err(bad_flags); } + if flags & MSG_FLAG_WAS_UNKNOWN != 0 + && (flags & MSG_FLAG_FAIL_IF_UNKNOWN_AND_OPEN_FOR_WRITE != 0 + || flags & MSG_FLAG_MARK_IF_UNKNOWN == 0) + { + return Err(bad_flags); + } + + if !is_known_message(id) { + if flags & MSG_FLAG_FAIL_IF_UNKNOWN_ALWAYS != 0 { + return Err(FormatError::UnsupportedMessage(id)); + } + return Ok(()); + } + if flags & (MSG_FLAG_SHARED | MSG_FLAG_SHAREABLE) != 0 && !is_shareable_message(id) { + return Err(FormatError::InvalidObjectHeader( + "message of unshareable class flagged as shareable", + )); + } + + let overrun = FormatError::InvalidObjectHeader("ran off end of input buffer while decoding"); + match id { + // Continuation: address + length, and the chunk cannot be empty. + 0x10 => { + if body.len() < offset_size as usize + length_size as usize { + return Err(overrun); + } + if read_offset(body, offset_size as usize, length_size)? == 0 { + return Err(FormatError::InvalidObjectHeader( + "invalid continuation chunk size (0)", + )); + } + } + // Reference count: version-2 headers only; version 0 then a u32. + 0x16 => { + if header_version == 1 { + return Err(FormatError::InvalidObjectHeader( + "object header version does not support reference count message", + )); + } + match body.first() { + None => return Err(overrun), + Some(0) => {} + Some(_) => { + return Err(FormatError::InvalidObjectHeader( + "bad version number for reference count message", + )); + } + } + if body.len() < 5 { + return Err(overrun); + } + } + // Old modification time: "YYYYMMDDhhmmss" and 2 reserved bytes. + 0x0E => { + if body.len() < 16 { + return Err(overrun); + } + if !body[..14].iter().all(u8::is_ascii_digit) { + return Err(FormatError::InvalidObjectHeader( + "badly formatted modification time message", + )); + } + } + // New modification time: version 1, 3 reserved bytes, u32 seconds. + 0x12 => { + match body.first() { + None => return Err(overrun), + Some(1) => {} + Some(_) => { + return Err(FormatError::InvalidObjectHeader( + "bad version number for mtime message", + )); + } + } + if body.len() < 8 { + return Err(overrun); + } + } + _ => {} + } + Ok(()) } #[cfg(test)] @@ -528,11 +665,14 @@ mod tests { // Calculate total header message data size let mut msg_bytes = Vec::new(); for (mtype, mdata, mflags) in messages { + // v1 message sizes are multiples of 8 (the data is zero-padded). + let padded = mdata.len().div_ceil(8) * 8; msg_bytes.extend_from_slice(&mtype.to_le_bytes()); // type(2) - msg_bytes.extend_from_slice(&(mdata.len() as u16).to_le_bytes()); // size(2) + msg_bytes.extend_from_slice(&(padded as u16).to_le_bytes()); // size(2) msg_bytes.push(*mflags); // flags(1) msg_bytes.extend_from_slice(&[0u8; 3]); // reserved(3) msg_bytes.extend_from_slice(mdata); // data + msg_bytes.resize(msg_bytes.len() + padded - mdata.len(), 0); } let mut buf = Vec::new(); @@ -622,9 +762,10 @@ mod tests { let hdr = ObjectHeader::parse(&data, 0, 8, 8).unwrap(); assert_eq!(hdr.messages.len(), 2); assert_eq!(hdr.messages[0].msg_type, MessageType::Dataspace); - assert_eq!(hdr.messages[0].data, vec![1, 2, 3, 4]); + // v1 message data is padded to a multiple of 8 bytes. + assert_eq!(hdr.messages[0].data, vec![1, 2, 3, 4, 0, 0, 0, 0]); assert_eq!(hdr.messages[1].msg_type, MessageType::DataLayout); - assert_eq!(hdr.messages[1].data, vec![5, 6]); + assert_eq!(hdr.messages[1].data[..2], [5, 6]); } #[test] @@ -650,7 +791,7 @@ mod tests { // Bit 3 = fail if unknown *and the file is opened for writing*. This // parser only reads, so libhdf5 (read-only) opens such an object and // so must we. Bits 4/5 (mark if unknown / was unknown) never fail. - for flags in [0x08u8, 0x10, 0x20, 0x38] { + for flags in [0x08u8, 0x10, 0x30] { let messages = [(0x00FFu16, &[0xAA][..], flags)]; let data = build_v1_header(&messages, 8, 8); let hdr = ObjectHeader::parse(&data, 0, 8, 8).unwrap(); @@ -658,6 +799,231 @@ mod tests { } } + #[test] + fn contradictory_message_flags_are_refused() { + // libhdf5: "bad flag combination for message" for shared + don't + // share, was-unknown without mark-if-unknown, and was-unknown with + // fail-if-unknown-on-write. + for flags in [0x06u8, 0x20, 0x38] { + let data = build_v1_header(&[(0x00FFu16, &[0xAA][..], flags)], 8, 8); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("bad flag combination for message"), + "flags {flags:#x}" + ); + let data = build_v2_header(0x00, &[(0xF0, &[1, 2], flags)], None); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("bad flag combination for message"), + "flags {flags:#x}" + ); + } + } + + #[test] + fn unshareable_message_flagged_shareable_is_refused() { + // A layout (0x08) or modification time (0x12) message cannot be + // shared; bit 1 (shared) or bit 6 (shareable) on one is corruption + // (cve-2016-4332). A datatype (0x03) may be shareable. + let mtime = [1u8, 0, 0, 0, 0x10, 0x20, 0x30, 0x40]; + for (id, flags) in [(0x08u16, 0x40u8), (0x08, 0x02), (0x12, 0x40)] { + let data = build_v1_header(&[(id, &mtime[..], flags)], 8, 8); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader( + "message of unshareable class flagged as shareable" + ), + "id {id:#x} flags {flags:#x}" + ); + } + let data = build_v1_header(&[(0x03, &[0u8; 8][..], 0x40)], 8, 8); + assert!(ObjectHeader::parse(&data, 0, 8, 8).is_ok()); + // An unknown message is never checked for shareability. + let data = build_v1_header(&[(0x00FF, &[0u8; 8][..], 0x40)], 8, 8); + assert!(ObjectHeader::parse(&data, 0, 8, 8).is_ok()); + } + + #[test] + fn v1_message_must_be_aligned() { + // cve-2018-13873: a v1 message whose size is not a multiple of 8. + let mut data = build_v1_header(&[(0x01, &[0u8; 8][..], 0)], 8, 8); + data[16 + 2] = 7; // size field of the only message + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("message not aligned") + ); + } + + #[test] + fn message_overrunning_its_chunk_is_refused() { + // It used to end the chunk quietly, dropping this message and any + // after it. + let mut data = build_v1_header(&[(0x01, &[0u8; 8][..], 0)], 8, 8); + data[16 + 2] = 16; + data.resize(data.len() + 64, 0); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("message size exceeds buffer end") + ); + let mut data = build_v2_header(0x00, &[(0x01, &[1, 2], 0)], None); + data[7 + 1] = 9; // size of the only message (after OHDR, ver, flags, chunk size) + let chk = crate::checksum::jenkins_lookup3(&data[..data.len() - 4]); + let n = data.len(); + data[n - 4..].copy_from_slice(&chk.to_le_bytes()); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("message size exceeds buffer end") + ); + } + + #[test] + fn v1_gap_after_last_message_is_refused() { + // Fewer than 8 bytes left over: a gap, which only version 2 allows. + let mut data = build_v1_header(&[(0x01, &[0u8; 8][..], 0)], 8, 8); + data[8] += 4; // header_data_size + data.extend_from_slice(&[0u8; 4]); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("gap found in early version of file format") + ); + } + + #[test] + fn v1_chunk_holding_more_messages_than_the_prefix_says_is_refused() { + // cve-2024-32619: the prefix says 1 message, the chunk holds 2. The + // second used to be dropped silently. + let mut data = build_v1_header(&[(0x01, &[0u8; 8][..], 0), (0x03, &[0u8; 8][..], 0)], 8, 8); + data[2] = 1; + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("bad object header message count") + ); + // Fewer in the chunk than the prefix says is fine (the rest may be in + // continuation chunks; libhdf5 only enforces that with strict checks). + data[2] = 3; + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap().messages.len(), + 2 + ); + } + + #[test] + fn v1_prefix_chunk_size_must_fit_the_message_count() { + let mut data = build_v1_header(&[], 8, 8); + data[8] = 8; // no messages but a non-empty chunk + data.extend_from_slice(&[0u8; 8]); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("bad object header chunk size") + ); + } + + #[test] + fn v1_header_cannot_hold_a_reference_count_message() { + // cve-2018-11204. + let data = build_v1_header(&[(0x16, &[0, 2, 0, 0, 0][..], 0)], 8, 8); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader( + "object header version does not support reference count message" + ) + ); + let data = build_v2_header(0x00, &[(0x16, &[0, 2, 0, 0, 0], 0)], None); + assert!(ObjectHeader::parse(&data, 0, 8, 8).is_ok()); + } + + #[test] + fn modification_time_messages_are_decoded_with_the_header() { + // cve-2024-33873 (version 0) and cve-2024-33874 (empty message). + for (body, why) in [ + ( + &[0u8, 0, 0, 0, 1, 2, 3, 4][..], + "bad version number for mtime message", + ), + (&[][..], "ran off end of input buffer while decoding"), + ] { + let data = build_v2_header(0x00, &[(0x12, body, 0)], None); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader(why) + ); + } + let data = build_v2_header(0x00, &[(0x12, &[1, 0, 0, 0, 1, 2, 3, 4], 0)], None); + assert!(ObjectHeader::parse(&data, 0, 8, 8).is_ok()); + // The old (0x0E) message is 14 ASCII digits and 2 reserved bytes. + let data = build_v1_header(&[(0x0E, &b"20110414214255\0\0"[..], 0)], 8, 8); + assert!(ObjectHeader::parse(&data, 0, 8, 8).is_ok()); + let data = build_v1_header(&[(0x0E, &b"2011041421425x\0\0"[..], 0)], 8, 8); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("badly formatted modification time message") + ); + } + + #[test] + fn continuation_message_must_hold_a_nonempty_chunk() { + let mut cont = [0u8; 16]; + cont[..8].copy_from_slice(&64u64.to_le_bytes()); + let data = build_v2_header(0x00, &[(0x10, &cont, 0)], None); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("invalid continuation chunk size (0)") + ); + let data = build_v2_header(0x00, &[(0x10, &cont[..8], 0)], None); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("ran off end of input buffer while decoding") + ); + } + + #[test] + fn v2_prefix_is_checked() { + let data = build_v2_header(0x40, &[(0x01, &[1], 0)], None); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("unknown object header status flag(s)") + ); + // build_v2_header writes max_compact 8, min_dense 6; swap them. + let mut data = build_v2_header(0x10, &[(0x01, &[1], 0)], None); + data[6] = 6; + data[8] = 8; + let chk = crate::checksum::jenkins_lookup3(&data[..data.len() - 4]); + let n = data.len(); + data[n - 4..].copy_from_slice(&chk.to_le_bytes()); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("bad object header attribute phase change values") + ); + } + + #[test] + fn v2_gap_is_allowed_only_without_nil_messages() { + // Three bytes after the last message: a gap (a message header is 4). + let mut data = build_v2_header(0x00, &[(0x01, &[1, 2, 3], 0), (0x03, &[], 0)], None); + // Turn the empty datatype message (4 header bytes) into a 3-byte gap + // by shrinking the chunk. + let n = data.len(); + data.truncate(n - 5); + data[6] -= 1; + let chk = crate::checksum::jenkins_lookup3(&data); + data.extend_from_slice(&chk.to_le_bytes()); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap().messages.len(), + 1 + ); + + let mut data = build_v2_header(0x00, &[(0x00, &[1, 2, 3], 0), (0x03, &[], 0)], None); + let n = data.len(); + data.truncate(n - 5); + data[6] -= 1; + let chk = crate::checksum::jenkins_lookup3(&data); + data.extend_from_slice(&chk.to_le_bytes()); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::InvalidObjectHeader("gap in chunk with no null messages") + ); + } + #[test] fn parse_v2_unknown_message_flags() { let data = build_v2_header(0x00, &[(0xF0, &[1, 2], 0x08)], None); From 3cf8cd86f22bfbcca713443c41f177848643c7a1 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 00:08:58 -0500 Subject: [PATCH 02/17] fix(format): refuse datatypes libhdf5 refuses to decode Datatype::parse now makes the checks of libhdf5's H5O__dtype_decode_helper and fails with InvalidDatatype (libhdf5's own error text) instead of decoding a corrupt type: - size 0 ("invalid datatype size"), for every class; - integer bit offset/precision outside the type, or precision 0; - float sign/exponent/mantissa outside the type, empty, or overlapping; normalization 3; bit 6 without bit 0 from version 3; - compound with no members, a member outside the compound, a duplicate name, or a member overlapping an earlier one; - enum whose size differs from its base type's, or an empty member name; - array of more than 32 dimensions or with a zero-sized one (v1 compound array members now say so rather than InvalidDatatypeVersion); - opaque tag length that is not a multiple of 8. Bit 6 of a version-1/2 float's class bits used to be read as VAX order, byte-swapping values; libhdf5 ignores it before version 3, and so does this now. Only checks HDF5 2.0 (h5py 3.16) makes are added: newer libhdf5 also checks bit fields, the variable-length kind and array sizes, but h5py opens files that fail those, so they are left out. Each check was confirmed against h5py by corrupting a file it wrote. The conformance probe now decodes committed datatypes, as h5py's f[name] does. Conformance (cached corpus, tank): 570 ok, unchanged. Objects libhdf5 refuses that clawhdf5 used to read: cve-2016-4332-mtime (/cmpnd), cve-2017-17508, cve-2024-32616 (/type1), cve-2024-32618, cve-2026-34734, bad_compound.h5 (/cmpnd, /dataset); eight more that already failed now fail with libhdf5's reason (e.g. cve-2024-29163 "mantissa range out of bounds"). Co-Authored-By: Claude Opus 5.5 (1M context) --- conformance/probe/src/main.rs | 15 + crates/clawhdf5-format/src/attribute.rs | 3 +- crates/clawhdf5-format/src/datatype.rs | 461 +++++++++++++++++++++++- crates/clawhdf5-format/src/error.rs | 7 + 4 files changed, 473 insertions(+), 13 deletions(-) diff --git a/conformance/probe/src/main.rs b/conformance/probe/src/main.rs index 1298cf0..c4cc012 100644 --- a/conformance/probe/src/main.rs +++ b/conformance/probe/src/main.rs @@ -309,6 +309,14 @@ impl<'a> Ctx<'a> { } } + fn read_named_datatype(&self, h: &ObjectHeader) -> Result<(), String> { + let dtb = self + .payload(h, MessageType::Datatype)? + .ok_or("MissingMessage(Datatype)")?; + Datatype::parse(&dtb).map_err(e)?; + Ok(()) + } + fn read_dataset(&self, h: &ObjectHeader, rec: &mut Map) -> Result<(), String> { let dtb = self .payload(h, MessageType::Datatype)? @@ -778,6 +786,13 @@ fn main() { { rec.insert("error".into(), Value::String(msg)); } + // Opening a committed datatype decodes it (h5py's `f[name]` fails on + // one libhdf5 cannot decode), so decode it here too. + if kind == "datatype" + && let Err(msg) = guarded(|| ctx.read_named_datatype(&h)) + { + rec.insert("error".into(), Value::String(msg)); + } if kind != "datatype" { match guarded(|| ctx.attrs(&h)) { Ok(m) => { diff --git a/crates/clawhdf5-format/src/attribute.rs b/crates/clawhdf5-format/src/attribute.rs index c844c3c..df17f74 100644 --- a/crates/clawhdf5-format/src/attribute.rs +++ b/crates/clawhdf5-format/src/attribute.rs @@ -573,7 +573,8 @@ mod tests { /// Build an f64 LE datatype message. fn build_f64_dt() -> Vec { - let mut buf = build_dt_header(1, 1, [0x00, 0x00, 0x02], 8); + // Sign bit 63 (bits 8-15 of the class bits). + let mut buf = build_dt_header(1, 1, [0x20, 63, 0x00], 8); let mut props = [0u8; 12]; props[2..4].copy_from_slice(&64u16.to_le_bytes()); // bit_precision props[4] = 52; // exp_location diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index ba85afa..efc2afb 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -208,6 +208,36 @@ fn offset_bytes_for_size(compound_size: u32) -> usize { } /// Read an unsigned integer of 1, 2, 4, or 8 bytes (LE). +/// The size field of the datatype message at `pos`, as stored (a +/// variable-length type's stored size is not modelled in [`Datatype`]). +fn stored_type_size(data: &[u8], pos: usize) -> Result { + ensure_len(data, pos, 8)?; + Ok(LittleEndian::read_u32(&data[pos + 4..pos + 8])) +} + +/// A compound member's size in the compound, as libhdf5 counts it. +fn member_size(dt: &Datatype) -> u64 { + u64::from(dt.type_size()) +} + +/// libhdf5 refuses an array type of more than `H5S_MAX_RANK` (32) +/// dimensions. +fn check_array_rank(ndims: usize) -> Result<(), FormatError> { + if ndims > 32 { + return Err(invalid("too many dimensions for array datatype")); + } + Ok(()) +} + +/// A zero-sized array dimension makes a zero-sized type, which libhdf5 +/// cannot open ("unable to retrieve size of datatype"). +fn check_array_dims(dims: &[u32]) -> Result<(), FormatError> { + if dims.contains(&0) { + return Err(invalid("zero-sized dimension specified")); + } + Ok(()) +} + fn read_uint(data: &[u8], offset: usize, nbytes: usize) -> Result { ensure_len(data, offset, nbytes)?; let slice = &data[offset..offset + nbytes]; @@ -232,10 +262,101 @@ fn read_uint(data: &[u8], offset: usize, nbytes: usize) -> Result) -> FormatError { + FormatError::InvalidDatatype(why.into()) +} + +/// libhdf5's bounds checks on an integer type's bit offset and precision +/// (`H5O__dtype_decode_helper`): both must lie inside the type. (Newer +/// libhdf5 checks bit fields the same way; HDF5 2.0, which h5py 3.16 ships, +/// does not, and opens such a type.) +fn check_integer_bits(size: u32, bit_offset: u16, bit_precision: u16) -> Result<(), FormatError> { + let bits = u64::from(size) * 8; + if u64::from(bit_offset) >= bits { + return Err(invalid("integer offset out of bounds")); + } + if bit_precision == 0 { + return Err(invalid("precision is zero")); + } + if u64::from(bit_offset) + u64::from(bit_precision) > bits { + return Err(invalid("integer offset+precision out of bounds")); + } + Ok(()) +} + +/// Whether the closed bit ranges `[a0, a1]` and `[b0, b1]` share a bit. +fn ranges_overlap(a0: u64, a1: u64, b0: u64, b1: u64) -> bool { + a0 <= b1 && b0 <= a1 +} + +/// libhdf5's checks on a floating-point type's fields: sign, exponent and +/// mantissa must lie inside the type, be non-empty, and not overlap. +/// (libhdf5 does not check a float's bit offset and precision.) +fn check_float_fields( + size: u32, + sign: u8, + epos: u8, + esize: u8, + mpos: u8, + msize: u8, +) -> Result<(), FormatError> { + let bits = u64::from(size) * 8; + let (sign, epos, esize, mpos, msize) = ( + u64::from(sign), + u64::from(epos), + u64::from(esize), + u64::from(mpos), + u64::from(msize), + ); + if sign >= bits { + return Err(invalid("sign bit position out of bounds")); + } + if esize == 0 { + return Err(invalid("exponent size can't be zero")); + } + if epos >= bits { + return Err(invalid("exponent starting position out of bounds")); + } + if epos + esize > bits { + return Err(invalid("exponent range out of bounds")); + } + if msize == 0 { + return Err(invalid("mantissa size can't be zero")); + } + if mpos >= bits { + return Err(invalid("mantissa starting position out of bounds")); + } + if mpos + msize > bits { + return Err(invalid("mantissa range out of bounds")); + } + let (e_end, m_end) = (epos + esize - 1, mpos + msize - 1); + if ranges_overlap(sign, sign, epos, e_end) { + return Err(invalid("exponent and sign positions overlap")); + } + if ranges_overlap(sign, sign, mpos, m_end) { + return Err(invalid("mantissa and sign positions overlap")); + } + if ranges_overlap(epos, e_end, mpos, m_end) { + return Err(invalid("mantissa and exponent positions overlap")); + } + Ok(()) +} + impl Datatype { /// Parse a datatype message from raw bytes. /// /// Returns `(Datatype, bytes_consumed)` for recursive parsing. + /// + /// A type libhdf5 refuses to decode is refused here too, with + /// [`FormatError::InvalidDatatype`] carrying libhdf5's reason: size 0, + /// integer/bit-field/float bit fields outside the type or overlapping, + /// a compound with no members, a member outside its compound, a + /// duplicate or overlapping member, an enum whose size differs from its + /// base type's or with an empty name, an array of more than 32 + /// dimensions or a zero-sized one, an unaligned opaque tag length. + /// Reading such a type used to return data from a corrupt file. Checks + /// newer libhdf5 releases add but HDF5 2.0 (h5py 3.16) lacks are left + /// out, so a file h5py opens still opens here. pub fn parse(data: &[u8]) -> Result<(Datatype, usize), FormatError> { Self::parse_with_depth(data, 0) } @@ -259,6 +380,9 @@ impl Datatype { let size = LittleEndian::read_u32(&data[4..8]); let mut pos = 8; + if size == 0 { + return Err(invalid("invalid datatype size")); + } match class_id { 0 => { @@ -272,6 +396,7 @@ impl Datatype { let signed = (bf0 >> 3) & 0x01 == 1; let bit_offset = LittleEndian::read_u16(&data[pos..pos + 2]); let bit_precision = LittleEndian::read_u16(&data[pos + 2..pos + 4]); + check_integer_bits(size, bit_offset, bit_precision)?; pos += 4; Ok(( Datatype::FixedPoint { @@ -289,13 +414,23 @@ impl Datatype { ensure_len(data, pos, 12)?; let bo_low = bf0 & 0x01; let bo_high = (bf0 >> 6) & 0x01; + // Bit 6 (with bit 0) is VAX order, defined by version 3; libhdf5 + // ignores bit 6 in older versions, which this read as VAX, + // byte-swapping a little-endian float. + let bo_high = if version >= 3 { bo_high } else { 0 }; let byte_order = match (bo_high, bo_low) { (0, 0) => DatatypeByteOrder::LittleEndian, (0, 1) => DatatypeByteOrder::BigEndian, - (1, 0) => DatatypeByteOrder::Vax, + (1, 0) => { + return Err(invalid("bad byte order for datatype message")); + } (1, 1) => DatatypeByteOrder::Vax, _ => unreachable!(), }; + // Bits 4-5: mantissa normalization; 3 is undefined. + if (bf0 >> 4) & 0x03 == 3 { + return Err(invalid("unknown floating-point normalization")); + } let bit_offset = LittleEndian::read_u16(&data[pos..pos + 2]); let bit_precision = LittleEndian::read_u16(&data[pos + 2..pos + 4]); let exponent_location = data[pos + 4]; @@ -303,6 +438,14 @@ impl Datatype { let mantissa_location = data[pos + 6]; let mantissa_size = data[pos + 7]; let exponent_bias = LittleEndian::read_u32(&data[pos + 8..pos + 12]); + check_float_fields( + size, + bf1, + exponent_location, + exponent_size, + mantissa_location, + mantissa_size, + )?; pos += 12; Ok(( Datatype::FloatingPoint { @@ -371,6 +514,10 @@ impl Datatype { 5 => { // Opaque let tag_len = bf0 as usize; + // libhdf5 writes the NUL-padded length, a multiple of 8. + if !tag_len.is_multiple_of(8) { + return Err(invalid("opaque flag field must be aligned")); + } ensure_len(data, pos, tag_len)?; // The stored tag is NUL-padded to a multiple of 8 bytes; the // tag itself ends at the first NUL (libhdf5 reads it with @@ -384,7 +531,40 @@ impl Datatype { 6 => { // Compound let num_members = (bf0 as u16) | ((bf1 as u16) << 8); - let mut members = Vec::with_capacity(num_members as usize); + if num_members == 0 { + return Err(invalid("invalid number of members: 0")); + } + let mut members: Vec = Vec::with_capacity(num_members as usize); + // libhdf5 checks each member as it is decoded: it must fit in + // the compound (by its own stored size, before a v1 member's + // array dimensions are applied), and must not repeat a name + // or overlap an earlier member (by its final size). + let check_member = |members: &[CompoundMember], + name: &str, + byte_offset: u64, + stored_size: u32, + final_size: u64| + -> Result<(), FormatError> { + if byte_offset + u64::from(stored_size) > u64::from(size) { + return Err(invalid( + "member type extends outside its parent compound type", + )); + } + if let Some(j) = members.iter().position(|m| m.name == name) { + return Err(invalid(format!( + "duplicated compound field name '{name}', for fields {j} and {}", + members.len() + ))); + } + let end = byte_offset + final_size; + if members.iter().any(|m| { + let m_end = m.byte_offset + member_size(&m.datatype); + byte_offset < m_end && m.byte_offset < end + }) { + return Err(invalid("member overlaps with previous member")); + } + Ok(()) + }; if (3..=5).contains(&version) { // v3, v4 and v5 share the compact member encoding (name, @@ -396,9 +576,17 @@ impl Datatype { pos += name_len; let byte_offset = read_uint(data, pos, ob)?; pos += ob; + let stored_size = stored_type_size(data, pos)?; let (member_dt, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?; pos += consumed; + check_member( + &members, + &name, + byte_offset, + stored_size, + member_size(&member_dt), + )?; members.push(CompoundMember { name, byte_offset, @@ -438,11 +626,11 @@ impl Datatype { 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, - }); + if ndims > 4 { + return Err(invalid("invalid number of dimensions for array")); + } + if zero_dim { + return Err(invalid("zero-sized dimension specified")); } array_dims = (0..ndims) .map(|j| { @@ -452,6 +640,7 @@ impl Datatype { .collect(); pos += 28; } + let stored_size = stored_type_size(data, pos)?; let (mut member_dt, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?; pos += consumed; @@ -461,6 +650,13 @@ impl Datatype { dimensions: array_dims, }; } + check_member( + &members, + &name, + byte_offset, + stored_size, + member_size(&member_dt), + )?; members.push(CompoundMember { name, byte_offset, @@ -499,6 +695,9 @@ impl Datatype { let (base_type, base_consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?; pos += base_consumed; let base_size = base_type.type_size(); + if base_size != size { + return Err(invalid("ENUM datatype size does not match parent")); + } let mut members = Vec::with_capacity(num_members as usize); // Enum layout: base_type, then all names (null-terminated), then all values // v1/v2: names are padded to 8-byte boundaries @@ -506,6 +705,9 @@ impl Datatype { let mut member_names = Vec::with_capacity(num_members as usize); for _ in 0..num_members { let (name, name_len) = read_null_terminated_string(data, pos)?; + if name.is_empty() { + return Err(invalid("0 length enum name")); + } if version < 3 { let padded = (name_len + 7) & !7; pos += padded; @@ -566,6 +768,7 @@ impl Datatype { if version == 2 { ensure_len(data, pos, 4)?; let ndims = data[pos] as usize; + check_array_rank(ndims)?; pos += 4; // ndims(1) + reserved(3) ensure_len(data, pos, ndims * 4 + ndims * 4)?; let mut dimensions = Vec::with_capacity(ndims); @@ -573,6 +776,7 @@ impl Datatype { dimensions.push(LittleEndian::read_u32(&data[pos..pos + 4])); pos += 4; } + check_array_dims(&dimensions)?; // skip permutation indices pos += ndims * 4; let (base_type, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?; @@ -589,6 +793,7 @@ impl Datatype { // type); HDF5 1.14+/2.0 with `libver=latest` emits v5. ensure_len(data, pos, 1)?; let ndims = data[pos] as usize; + check_array_rank(ndims)?; pos += 1; ensure_len(data, pos, ndims * 4)?; let mut dimensions = Vec::with_capacity(ndims); @@ -596,6 +801,7 @@ impl Datatype { dimensions.push(LittleEndian::read_u32(&data[pos..pos + 4])); pos += 4; } + check_array_dims(&dimensions)?; let (base_type, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?; pos += consumed; Ok(( @@ -985,7 +1191,8 @@ mod tests { ) -> Vec { // LE byte order: bo_low=0, bo_high=0 let bf0 = 0x00u8; - let bf1 = 0x00u8; + // Sign bit: the top bit. + let bf1 = (size * 8 - 1) as u8; // mantissa norm = 2 (MSB not stored) in bits 24-31... wait, that's bf2 let bf2 = 0x02u8; // norm = 2 let mut buf = build_dt_header(1, 1, [bf0, bf1, bf2], size); @@ -1012,7 +1219,7 @@ mod tests { let levels = MAX_DATATYPE_DEPTH as usize + 10; let mut data = Vec::new(); for _ in 0..levels { - data.extend_from_slice(&build_dt_header(9, 3, [0, 0, 0], 0)); + data.extend_from_slice(&build_dt_header(9, 3, [0, 0, 0], 16)); } data.extend_from_slice(&build_fixed_point(4, false, false, 0, 32)); @@ -1026,7 +1233,7 @@ mod tests { let levels = MAX_DATATYPE_DEPTH as usize - 1; let mut data = Vec::new(); for _ in 0..levels { - data.extend_from_slice(&build_dt_header(9, 3, [0, 0, 0], 0)); + data.extend_from_slice(&build_dt_header(9, 3, [0, 0, 0], 16)); } data.extend_from_slice(&build_fixed_point(4, false, false, 0, 32)); @@ -1176,8 +1383,8 @@ mod tests { #[test] fn test_opaque() { - // tag_len = 4, tag = "BLOB" - let mut buf = build_dt_header(5, 1, [4, 0, 0], 64); + // tag = "BLOB"; the stored length is the NUL-padded length, 8 + let mut buf = build_dt_header(5, 1, [8, 0, 0], 64); buf.extend_from_slice(b"BLOB"); // Pad to 8 bytes buf.extend_from_slice(&[0, 0, 0, 0]); @@ -2049,4 +2256,234 @@ mod tests { }; assert_eq!(dt.type_size(), 48); } + /// Every check here mirrors one in libhdf5's `H5O__dtype_decode_helper`; + /// the error text is libhdf5's. + fn invalid_reason(data: &[u8]) -> String { + match Datatype::parse(data) { + Err(FormatError::InvalidDatatype(why)) => why, + other => panic!("expected InvalidDatatype, got {other:?}"), + } + } + + #[test] + fn size_zero_is_refused() { + // cve-2017-17508: a variable-length string member of stored size 0. + let mut data = build_dt_header(9, 1, [1, 0, 0], 0); + data.extend_from_slice(&build_fixed_point(1, false, false, 0, 8)); + assert_eq!(invalid_reason(&data), "invalid datatype size"); + assert_eq!( + invalid_reason(&build_dt_header(3, 1, [0, 0, 0], 0)), + "invalid datatype size" + ); + assert_eq!( + invalid_reason(&build_fixed_point(0, false, false, 0, 0)), + "invalid datatype size" + ); + } + + #[test] + fn integer_bits_must_lie_inside_the_type() { + assert_eq!( + invalid_reason(&build_fixed_point(4, false, false, 32, 1)), + "integer offset out of bounds" + ); + assert_eq!( + invalid_reason(&build_fixed_point(4, false, false, 0, 0)), + "precision is zero" + ); + assert_eq!( + invalid_reason(&build_fixed_point(4, false, false, 8, 25)), + "integer offset+precision out of bounds" + ); + // A partial-precision integer inside its bytes is fine. + assert!(Datatype::parse(&build_fixed_point(4, false, false, 12, 8)).is_ok()); + } + + #[test] + fn float_fields_must_lie_inside_the_type_and_not_overlap() { + // (sign, epos, esize, mpos, msize) on an f32 + let f32_with = |sign: u8, epos: u8, esize: u8, mpos: u8, msize: u8| { + let mut data = build_dt_header(1, 1, [0x20, sign, 0], 4); + data.extend_from_slice(&0u16.to_le_bytes()); + data.extend_from_slice(&32u16.to_le_bytes()); + data.extend_from_slice(&[epos, esize, mpos, msize]); + data.extend_from_slice(&127u32.to_le_bytes()); + data + }; + assert!(Datatype::parse(&f32_with(31, 23, 8, 0, 23)).is_ok()); + for (fields, why) in [ + ((32, 23, 8, 0, 23), "sign bit position out of bounds"), + ((31, 23, 0, 0, 23), "exponent size can't be zero"), + ( + (31, 32, 8, 0, 23), + "exponent starting position out of bounds", + ), + ((31, 30, 8, 0, 23), "exponent range out of bounds"), + ((31, 23, 8, 0, 0), "mantissa size can't be zero"), + ( + (31, 23, 8, 40, 1), + "mantissa starting position out of bounds", + ), + // cve-2024-29163: a 128-bit mantissa in a 4-byte float. + ((31, 23, 8, 0, 128), "mantissa range out of bounds"), + ((23, 23, 8, 0, 23), "exponent and sign positions overlap"), + ((0, 23, 8, 0, 23), "mantissa and sign positions overlap"), + // cve-2026-34734. + ( + (31, 20, 8, 0, 23), + "mantissa and exponent positions overlap", + ), + ] { + let (sign, epos, esize, mpos, msize) = fields; + assert_eq!( + invalid_reason(&f32_with(sign, epos, esize, mpos, msize)), + why, + "{fields:?}" + ); + } + // Normalization 3 is undefined; bit 6 (VAX) needs bit 0 from v3. + let mut data = f32_with(31, 23, 8, 0, 23); + data[1] = 0x30; + assert_eq!( + invalid_reason(&data), + "unknown floating-point normalization" + ); + let mut data = f32_with(31, 23, 8, 0, 23); + data[0] = 0x31; // version 3 + data[1] = 0x60; + assert_eq!(invalid_reason(&data), "bad byte order for datatype message"); + } + + #[test] + fn float_bit_6_is_vax_order_only_from_version_3() { + // h5py opens a v1 float with bit 6 set as an ordinary little-endian + // float; it used to be read as VAX order. + let mut data = build_float(4, 23, 8, 0, 23, 127); + data[1] |= 0x40; + match Datatype::parse(&data).unwrap().0 { + Datatype::FloatingPoint { byte_order, .. } => { + assert_eq!(byte_order, DatatypeByteOrder::LittleEndian) + } + other => panic!("{other:?}"), + } + data[0] = 0x31; + data[1] |= 0x01; + match Datatype::parse(&data).unwrap().0 { + Datatype::FloatingPoint { byte_order, .. } => { + assert_eq!(byte_order, DatatypeByteOrder::Vax) + } + other => panic!("{other:?}"), + } + } + + #[test] + fn opaque_tag_length_must_be_padded() { + let mut data = build_dt_header(5, 1, [4, 0, 0], 4); + data.extend_from_slice(b"BLOB"); + assert_eq!(invalid_reason(&data), "opaque flag field must be aligned"); + } + + /// A v3 compound of `size` bytes with `(name, offset, member)` members. + fn compound_v3(size: u32, members: &[(&str, u8, Vec)]) -> Vec { + let n = members.len() as u8; + let mut data = build_dt_header(6, 3, [n, 0, 0], size); + for (name, off, dt) in members { + data.extend_from_slice(name.as_bytes()); + data.push(0); + data.push(*off); + data.extend_from_slice(dt); + } + data + } + + #[test] + fn compound_members_are_checked() { + let i4 = build_fixed_point(4, false, true, 0, 32); + // cve-2016-4332: no members. + assert_eq!( + invalid_reason(&compound_v3(8, &[])), + "invalid number of members: 0" + ); + assert_eq!( + invalid_reason(&compound_v3( + 8, + &[("a", 0, i4.clone()), ("b", 6, i4.clone())] + )), + "member type extends outside its parent compound type" + ); + assert_eq!( + invalid_reason(&compound_v3( + 8, + &[("a", 0, i4.clone()), ("a", 4, i4.clone())] + )), + "duplicated compound field name 'a', for fields 0 and 1" + ); + assert_eq!( + invalid_reason(&compound_v3( + 8, + &[("a", 0, i4.clone()), ("b", 2, i4.clone())] + )), + "member overlaps with previous member" + ); + assert_eq!( + invalid_reason(&compound_v3( + 8, + &[("b", 4, i4.clone()), ("a", 2, i4.clone())] + )), + "member overlaps with previous member" + ); + // Members out of offset order, and gaps, are fine. + assert!(Datatype::parse(&compound_v3(12, &[("b", 8, i4.clone()), ("a", 0, i4)])).is_ok()); + } + + #[test] + fn enum_is_checked() { + let base = build_fixed_point(4, false, true, 0, 32); + let enum_of = |size: u32, names: &[&str]| { + let mut data = build_dt_header(8, 3, [names.len() as u8, 0, 0], size); + data.extend_from_slice(&base); + for n in names { + data.extend_from_slice(n.as_bytes()); + data.push(0); + } + for i in 0..names.len() as u32 { + data.extend_from_slice(&i.to_le_bytes()); + } + data + }; + assert!(Datatype::parse(&enum_of(4, &["RED", "GREEN"])).is_ok()); + // cve-2024-32618. + assert_eq!( + invalid_reason(&enum_of(4, &["", "GREEN"])), + "0 length enum name" + ); + assert_eq!( + invalid_reason(&enum_of(2, &["RED"])), + "ENUM datatype size does not match parent" + ); + } + + #[test] + fn array_dimensions_are_checked() { + let base = build_fixed_point(4, false, true, 0, 32); + let array_v3 = |dims: &[u32]| { + let n = dims.iter().product::().max(1); + let mut data = build_dt_header(10, 3, [0, 0, 0], 4 * n); + data.push(dims.len() as u8); + for d in dims { + data.extend_from_slice(&d.to_le_bytes()); + } + data.extend_from_slice(&base); + data + }; + assert!(Datatype::parse(&array_v3(&[2, 3])).is_ok()); + assert_eq!( + invalid_reason(&array_v3(&[2, 0])), + "zero-sized dimension specified" + ); + assert_eq!( + invalid_reason(&array_v3(&[1; 33])), + "too many dimensions for array datatype" + ); + } } diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index 6d038f5..46f8123 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -206,6 +206,10 @@ pub enum FormatError { /// wrong message count, contradictory message flags, a message of a /// class that cannot be shared flagged shareable, … InvalidObjectHeader(&'static str), + /// A datatype message libhdf5 refuses to decode (the reason is + /// libhdf5's own error text): size 0, bit fields outside the type, + /// an empty enum name, a compound member outside its compound, … + InvalidDatatype(String), } impl fmt::Display for FormatError { @@ -453,6 +457,9 @@ impl fmt::Display for FormatError { FormatError::InvalidObjectHeader(why) => { write!(f, "corrupt object header: {why}") } + FormatError::InvalidDatatype(why) => { + write!(f, "invalid datatype: {why}") + } } } } From a5ca97001545753f7fe823796af5085a1069ca10 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 00:12:04 -0500 Subject: [PATCH 03/17] fix: refuse numeric types with unusually many unused bits in v1 headers libhdf5 1.14.4+ treats an integer, float or bit field wider than a byte whose precision and offset leave more than half its bits unused as corruption when the type sits in a header without a checksum (version 1), unless the file is opened with H5Pset_relax_file_integrity_checks (H5T_is_numeric_with_unusual_unused_bits). clawhdf5 read such types, e.g. a 3-bit integer in 4 bytes (cve-2024-29162) or a 32-bit float in 65525 bytes (cve-2024-32614, tmisc38a.h5). New Datatype::check_unused_bits (recursive) and Datatype::parse_in_header, which applies it for version-1 headers. Dataset datatypes (facade File, LazyFile, MmapFile; clawhdf5-io VOL, MPI VOL, async reader; the conformance probe) and compact attributes in version-1 headers use it. Conformance (cached corpus, tank): 570 ok, unchanged; cve-2024-29162, cve-2024-32614 and tmisc38a.h5 now refuse the object h5py refuses, and tmisc38b.h5 / unknown-1.h5 now fail with libhdf5's reason. Co-Authored-By: Claude Opus 5.5 (1M context) --- conformance/probe/src/main.rs | 4 +- crates/clawhdf5-format/src/attribute.rs | 15 +++- crates/clawhdf5-format/src/datatype.rs | 93 +++++++++++++++++++++++++ crates/clawhdf5-io/src/async_read.rs | 2 +- crates/clawhdf5-io/src/mpi_vol.rs | 4 +- crates/clawhdf5-io/src/vol.rs | 4 +- crates/clawhdf5/src/lazy.rs | 2 +- crates/clawhdf5/src/mmap_file.rs | 2 +- crates/clawhdf5/src/reader.rs | 2 +- 9 files changed, 117 insertions(+), 11 deletions(-) diff --git a/conformance/probe/src/main.rs b/conformance/probe/src/main.rs index c4cc012..9629dcc 100644 --- a/conformance/probe/src/main.rs +++ b/conformance/probe/src/main.rs @@ -313,7 +313,7 @@ impl<'a> Ctx<'a> { let dtb = self .payload(h, MessageType::Datatype)? .ok_or("MissingMessage(Datatype)")?; - Datatype::parse(&dtb).map_err(e)?; + Datatype::parse_in_header(&dtb, h.version).map_err(e)?; Ok(()) } @@ -321,7 +321,7 @@ impl<'a> Ctx<'a> { let dtb = self .payload(h, MessageType::Datatype)? .ok_or("MissingMessage(Datatype)")?; - let (dt, _) = Datatype::parse(&dtb).map_err(e)?; + let (dt, _) = Datatype::parse_in_header(&dtb, h.version).map_err(e)?; rec.insert("dtype".into(), Value::String(dtype_str(&dt))); let dsb = self .payload(h, MessageType::Dataspace)? diff --git a/crates/clawhdf5-format/src/attribute.rs b/crates/clawhdf5-format/src/attribute.rs index df17f74..11bde3e 100644 --- a/crates/clawhdf5-format/src/attribute.rs +++ b/crates/clawhdf5-format/src/attribute.rs @@ -362,6 +362,18 @@ fn extract_name(bytes: &[u8]) -> String { String::from_utf8_lossy(&bytes[..end]).into_owned() } +/// An attribute's datatype gets libhdf5's extra check for a header without +/// a checksum (see [`Datatype::check_unused_bits`]). +fn check_in_header( + attr: AttributeMessage, + header: &ObjectHeader, +) -> Result { + if header.version == 1 { + attr.datatype.check_unused_bits()?; + } + Ok(attr) +} + /// Extract all attribute messages from an object header. pub fn extract_attributes( header: &ObjectHeader, @@ -371,7 +383,7 @@ pub fn extract_attributes( for msg in &header.messages { if msg.msg_type == MessageType::Attribute { let attr = AttributeMessage::parse(&msg.data, length_size)?; - attrs.push(attr); + attrs.push(check_in_header(attr, header)?); } } Ok(attrs) @@ -465,6 +477,7 @@ fn extract_attributes_with( } else { AttributeMessage::parse_in_file(&msg.data, file_data, offset_size, length_size) }; + let attr = attr.and_then(|a| check_in_header(a, header)); match attr { Ok(attr) => attrs.push(attr), Err(e) => on_error(e)?, diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index efc2afb..d97a61d 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -858,6 +858,70 @@ impl Datatype { } } + /// [`Self::parse`] for the datatype message of an object whose header + /// has version `header_version`: a version-1 header, which has no + /// checksum, additionally gets [`Self::check_unused_bits`], as libhdf5 + /// does. Use this wherever the header is at hand. + pub fn parse_in_header( + data: &[u8], + header_version: u8, + ) -> Result<(Datatype, usize), FormatError> { + let parsed = Self::parse(data)?; + if header_version == 1 { + parsed.0.check_unused_bits()?; + } + Ok(parsed) + } + + /// libhdf5's guard against a corrupt numeric type in a header without + /// a checksum (`H5T_is_numeric_with_unusual_unused_bits`, HDF5 1.14.4+): + /// an integer, float or bit field wider than a byte whose precision and + /// offset leave more than half its bits unused is taken for corruption + /// (e.g. a 3-bit integer in 4 bytes, `cve-2024-29162`, or a 32-bit float + /// in 65525 bytes, `cve-2024-32614`), anywhere in the type. libhdf5 + /// skips the check for checksummed (version-2) headers and when the + /// file is opened with `H5Pset_relax_file_integrity_checks`; so does + /// [`Self::parse_in_header`], which has no such option. + pub fn check_unused_bits(&self) -> Result<(), FormatError> { + match self { + Datatype::FixedPoint { + size, + bit_offset, + bit_precision, + .. + } + | Datatype::FloatingPoint { + size, + bit_offset, + bit_precision, + .. + } + | Datatype::BitField { + size, + bit_offset, + bit_precision, + .. + } => { + let bits = u64::from(*size) * 8; + let prec = u64::from(*bit_precision); + if *size > 1 && prec < bits && bits > 2 * (prec + u64::from(*bit_offset)) { + return Err(invalid(format!( + "datatype has unusually large # of unused bits (prec = {prec} bits, \ + size = {size} bytes), possibly corrupted file" + ))); + } + Ok(()) + } + Datatype::Compound { members, .. } => members + .iter() + .try_for_each(|m| m.datatype.check_unused_bits()), + Datatype::Enumeration { base_type, .. } + | Datatype::VariableLength { base_type, .. } + | Datatype::Array { base_type, .. } => base_type.check_unused_bits(), + _ => Ok(()), + } + } + /// Serialize datatype to HDF5 message bytes. pub fn serialize(&self) -> Vec { match self { @@ -2354,6 +2418,35 @@ mod tests { assert_eq!(invalid_reason(&data), "bad byte order for datatype message"); } + #[test] + fn unusual_unused_bits_are_refused_in_version_1_headers_only() { + // cve-2024-29162: a 3-bit integer in 4 bytes. + let data = build_fixed_point(4, false, true, 0, 3); + assert!(Datatype::parse_in_header(&data, 2).is_ok()); + assert_eq!( + match Datatype::parse_in_header(&data, 1) { + Err(FormatError::InvalidDatatype(why)) => why, + other => panic!("{other:?}"), + }, + "datatype has unusually large # of unused bits (prec = 3 bits, size = 4 bytes), \ + possibly corrupted file" + ); + // Half the bits used (with the offset) is not unusual; nor is a + // 1-byte type; nor a full-precision one. + for (size, offset, prec) in [(4u32, 0u16, 16u16), (4, 8, 8), (1, 0, 1), (8, 0, 64)] { + let data = build_fixed_point(size, false, true, offset, prec); + assert!( + Datatype::parse_in_header(&data, 1).is_ok(), + "{size} {offset} {prec}" + ); + } + // Nested: a compound member's type is checked too. + let member = build_fixed_point(4, false, true, 0, 15); + let data = compound_v3(4, &[("a", 0, member)]); + assert!(Datatype::parse_in_header(&data, 2).is_ok()); + assert!(Datatype::parse_in_header(&data, 1).is_err()); + } + #[test] fn float_bit_6_is_vax_order_only_from_version_3() { // h5py opens a v1 float with bit 6 set as an ordinary little-endian diff --git a/crates/clawhdf5-io/src/async_read.rs b/crates/clawhdf5-io/src/async_read.rs index d9e2ffe..ddfce7b 100644 --- a/crates/clawhdf5-io/src/async_read.rs +++ b/crates/clawhdf5-io/src/async_read.rs @@ -312,7 +312,7 @@ impl AsyncHDF5File { let dt_msg = find_msg(&header, MessageType::Datatype).ok_or(FormatError::DatasetMissingData)?; - let (datatype, _) = Datatype::parse(&dt_msg.data)?; + let (datatype, _) = Datatype::parse_in_header(&dt_msg.data, header.version)?; let ds_msg = find_msg(&header, MessageType::Dataspace).ok_or(FormatError::DatasetMissingShape)?; diff --git a/crates/clawhdf5-io/src/mpi_vol.rs b/crates/clawhdf5-io/src/mpi_vol.rs index 7dac39e..fe39c4e 100644 --- a/crates/clawhdf5-io/src/mpi_vol.rs +++ b/crates/clawhdf5-io/src/mpi_vol.rs @@ -205,8 +205,8 @@ fn mpi_collective_read(vol: &MpiVol, location: &str, path: &str) -> Result LazyDataset<'f, R> { fn datatype(&self) -> Result { let data = self.required_payload(MessageType::Datatype)?; - let (dt, _) = Datatype::parse(&data)?; + let (dt, _) = Datatype::parse_in_header(&data, self.header.version)?; Ok(dt) } diff --git a/crates/clawhdf5/src/mmap_file.rs b/crates/clawhdf5/src/mmap_file.rs index eeb1efb..8c35163 100644 --- a/crates/clawhdf5/src/mmap_file.rs +++ b/crates/clawhdf5/src/mmap_file.rs @@ -426,7 +426,7 @@ impl<'f> MmapDataset<'f> { fn datatype(&self) -> Result { let data = self.required_payload(MessageType::Datatype)?; - let (dt, _) = Datatype::parse(&data)?; + let (dt, _) = Datatype::parse_in_header(&data, self.header.version)?; Ok(dt) } diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 65c0292..5f2a027 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -850,7 +850,7 @@ impl<'f> Dataset<'f> { fn datatype(&self) -> Result { let data = self.required_payload(MessageType::Datatype)?; - let (dt, _) = Datatype::parse(&data)?; + let (dt, _) = Datatype::parse_in_header(&data, self.header.version)?; Ok(dt) } From e73ac2af092fd3b7a85c6cedbe1955995fa12651 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 00:21:58 -0500 Subject: [PATCH 04/17] fix(format): validate chunk dimensions and chunk index offsets like libhdf5 A chunk dimension of 0 read a dataset as all fill values, 0x80000000 made an 8 GiB chunk, and a chunk dimension the chunk index's offsets are not multiples of read chunks at the wrong place (cve-2018-11205). libhdf5 refuses all of these; now so does clawhdf5: - DataLayout::parse (H5O__layout_decode): no chunk dimension 0 ("bad chunk dimension value"), at most 33 dimensions, and before layout v4 at least 2 ("bad dimensions for chunked storage"). New FormatError::InvalidChunkDimensions. - Reading a chunked dataset (H5D__chunk_init / H5D__chunk_set_sizes): the chunk rank must match the dataspace's and a chunk must be under 4 GiB. One chunked_read::chunk_geometry replaces the four copies of the rank check. - v1 B-tree chunk index (H5D__btree_decode_key): every key's offsets must be multiples of the chunk dimensions, including the keys that only bound a node, which is where cve-2018-11205's bad dimension shows. New chunked_read::collect_chunk_info_checked; the chunked read and selection paths use it. New interop test header_validation_interop.rs: h5py writes chunked files (layout v3 and v4), the script corrupts the chunk dimension, and clawhdf5 must read exactly the copies h5py reads. Conformance (cached corpus, tank): 570 ok, unchanged; cve-2018-11205 now refuses the dataset h5py refuses; six more objects that already failed now fail with libhdf5's reason. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/chunked_read.rs | 288 +++++++++++++----- crates/clawhdf5-format/src/data_layout.rs | 87 +++++- crates/clawhdf5-format/src/data_read.rs | 5 +- crates/clawhdf5-format/src/error.rs | 7 + .../tests/header_validation_interop.rs | 155 ++++++++++ 5 files changed, 455 insertions(+), 87 deletions(-) create mode 100644 crates/clawhdf5/tests/header_validation_interop.rs diff --git a/crates/clawhdf5-format/src/chunked_read.rs b/crates/clawhdf5-format/src/chunked_read.rs index 3a3dc05..beac35f 100644 --- a/crates/clawhdf5-format/src/chunked_read.rs +++ b/crates/clawhdf5-format/src/chunked_read.rs @@ -148,6 +148,46 @@ pub(crate) fn checked_byte_len(elements: u64, elem_size: usize) -> Result Result<(usize, Vec), FormatError> { + let rank = chunk_dimensions.len().checked_sub(1).ok_or_else(|| { + FormatError::InvalidChunkDimensions("chunked layout has no dimensions".into()) + })?; + if dataspace.dimensions.len() != rank { + return Err(FormatError::InvalidChunkDimensions(format!( + "dimensionality of chunks doesn't match the dataspace (chunk rank {rank}, \ + dataspace rank {})", + dataspace.dimensions.len() + ))); + } + let spatial = &chunk_dimensions[..rank]; + if let Some(d) = spatial.iter().position(|&c| c == 0) { + return Err(FormatError::InvalidChunkDimensions(format!( + "chunk size must be > 0, dim = {d}" + ))); + } + let bytes = spatial + .iter() + .fold(elem_size as u128, |acc, &c| acc * u128::from(c)); + if bytes > u128::from(u32::MAX) { + return Err(FormatError::InvalidChunkDimensions(format!( + "chunk size must be < 4GB (chunk {spatial:?} of {elem_size}-byte elements)" + ))); + } + Ok((rank, spatial.iter().map(|&c| c as usize).collect())) +} + /// Product of chunk dimensions times the element size, overflow-checked. pub(crate) fn checked_chunk_byte_len( chunk_dims: &[usize], @@ -222,7 +262,76 @@ pub fn collect_chunk_info( offset_size: u8, length_size: u8, ) -> Result, FormatError> { - collect_chunk_info_inner(file_data, btree_address, ndims, offset_size, length_size, 0) + collect_chunk_info_inner( + file_data, + btree_address, + ndims, + None, + offset_size, + length_size, + 0, + ) +} + +/// [`collect_chunk_info`] for a layout with these `chunk_dimensions` (the +/// layout message's list, element size last), checking every key of the +/// B-tree as libhdf5 does (`H5D__btree_decode_key`): each coordinate offset +/// must be a multiple of its chunk dimension. That includes the keys that +/// only bound a node (internal-node keys and each node's final key), which +/// is where a corrupt chunk dimension shows when the chunks themselves all +/// start at offset 0 in that dimension (`cve-2018-11205`). A key that fails +/// ("bad coordinate offset") means a corrupt index or chunk dimension; the +/// chunks were read at the wrong place, or the dataset read as fill values. +pub fn collect_chunk_info_checked( + file_data: &[u8], + btree_address: u64, + chunk_dimensions: &[u32], + offset_size: u8, + length_size: u8, +) -> Result, FormatError> { + collect_chunk_info_inner( + file_data, + btree_address, + chunk_dimensions.len(), + Some(chunk_dimensions), + offset_size, + length_size, + 0, + ) +} + +/// Check one v1 B-tree chunk key's offsets (see +/// [`collect_chunk_info_checked`]). +fn check_key_offsets(offsets: &[u64], chunk_dimensions: &[u32]) -> Result<(), FormatError> { + for (&offset, &dim) in offsets.iter().zip(chunk_dimensions) { + if dim == 0 || offset % u64::from(dim) != 0 { + return Err(FormatError::ChunkedReadError(format!( + "bad coordinate offset {offsets:?} for chunk dimensions {chunk_dimensions:?}" + ))); + } + } + Ok(()) +} + +/// Read the `ndims` 8-byte offsets of the chunk key at `pos` (after its +/// chunk size and filter mask) and check them when `chunk_dimensions` is +/// given. +fn read_key_offsets( + file_data: &[u8], + pos: usize, + ndims: usize, + chunk_dimensions: Option<&[u32]>, +) -> Result, FormatError> { + let mut offsets = Vec::with_capacity(ndims); + let mut kp = pos + 8; + for _ in 0..ndims { + offsets.push(read_offset(file_data, kp, CHUNK_KEY_OFFSET_SIZE)?); + kp += CHUNK_KEY_OFFSET_SIZE as usize; + } + if let Some(dims) = chunk_dimensions { + check_key_offsets(&offsets, dims)?; + } + Ok(offsets) } /// Width of each chunk offset in a v1 chunk B-tree key, independent of the @@ -237,6 +346,7 @@ fn collect_chunk_info_inner( file_data: &[u8], btree_address: u64, ndims: usize, + chunk_dimensions: Option<&[u32]>, offset_size: u8, _length_size: u8, depth: usize, @@ -296,12 +406,7 @@ fn collect_chunk_info_inner( file_data[pos + 6], file_data[pos + 7], ]); - let mut offsets = Vec::with_capacity(ndims); - let mut kp = pos + 8; - for _ in 0..ndims { - offsets.push(read_offset(file_data, kp, CHUNK_KEY_OFFSET_SIZE)?); - kp += CHUNK_KEY_OFFSET_SIZE as usize; - } + let offsets = read_key_offsets(file_data, pos, ndims, chunk_dimensions)?; pos += key_size; // Parse child address @@ -315,7 +420,8 @@ fn collect_chunk_info_inner( address, }); } - // Skip final key + // The final key only bounds the node; libhdf5 still checks it. + read_key_offsets(file_data, pos, ndims, chunk_dimensions)?; Ok(chunks) } else { // Internal node: recurse into children @@ -324,11 +430,13 @@ fn collect_chunk_info_inner( let mut child_addrs = Vec::with_capacity(entries_used); for _ in 0..entries_used { - pos += key_size; // skip key + read_key_offsets(file_data, pos, ndims, chunk_dimensions)?; + pos += key_size; let child_addr = read_offset(file_data, pos, offset_size)?; child_addrs.push(child_addr); pos += os; } + read_key_offsets(file_data, pos, ndims, chunk_dimensions)?; let mut all_chunks = Vec::new(); for child_addr in child_addrs { @@ -336,6 +444,7 @@ fn collect_chunk_info_inner( file_data, child_addr, ndims, + chunk_dimensions, offset_size, _length_size, depth + 1, @@ -549,30 +658,13 @@ pub fn list_chunks( .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; // Both v3 and v4 include element size as last dim (rank+1) - let ndims = chunk_dimensions.len(); - let rank = ndims - .checked_sub(1) - .ok_or_else(|| FormatError::ChunkedReadError("chunked layout has no dimensions".into()))?; - let chunk_dims: Vec = chunk_dimensions[..rank] - .iter() - .map(|&d| d as usize) - .collect(); - + let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); - if ds_dims.len() != rank { - return Err(FormatError::ChunkedReadError(format!( - "rank mismatch: dataspace has {} dims, layout has {} chunk dims (rank={})", - ds_dims.len(), - chunk_dimensions.len(), - rank - ))); - } // Collect chunks based on version and index type let mut chunks = match (version, chunk_index_type) { (3, _) => { - let ndims = chunk_dimensions.len(); // rank+1 - collect_chunk_info(file_data, addr, ndims, offset_size, length_size)? + collect_chunk_info_checked(file_data, addr, chunk_dimensions, offset_size, length_size)? } (4, Some(1)) => { // Single chunk — one chunk covering the entire dataset @@ -819,24 +911,8 @@ pub fn read_chunked_data_cached( .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; let elem_size = datatype.type_size() as usize; - let ndims = chunk_dimensions.len(); - let rank = ndims - .checked_sub(1) - .ok_or_else(|| FormatError::ChunkedReadError("chunked layout has no dimensions".into()))?; - let chunk_dims: Vec = chunk_dimensions[..rank] - .iter() - .map(|&d| d as usize) - .collect(); - + let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); - if ds_dims.len() != rank { - return Err(FormatError::ChunkedReadError(format!( - "rank mismatch: dataspace has {} dims, layout has {} chunk dims (rank={})", - ds_dims.len(), - chunk_dimensions.len(), - rank - ))); - } // The per-file cache is shared across datasets (and threads); every // lookup is keyed by this dataset's chunk-index address, so another @@ -1140,24 +1216,8 @@ pub fn read_chunked_data_sweep( .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; let elem_size = datatype.type_size() as usize; - let ndims = chunk_dimensions.len(); - let rank = ndims - .checked_sub(1) - .ok_or_else(|| FormatError::ChunkedReadError("chunked layout has no dimensions".into()))?; - let chunk_dims: Vec = chunk_dimensions[..rank] - .iter() - .map(|&d| d as usize) - .collect(); - + let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); - if ds_dims.len() != rank { - return Err(FormatError::ChunkedReadError(format!( - "rank mismatch: dataspace has {} dims, layout has {} chunk dims (rank={})", - ds_dims.len(), - chunk_dimensions.len(), - rank - ))); - } // The per-file cache is shared across datasets (and threads); every // lookup is keyed by this dataset's chunk-index address, so another @@ -1292,24 +1352,8 @@ pub fn read_chunked_data_indexed( .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; let elem_size = datatype.type_size() as usize; - let ndims = chunk_dimensions.len(); - let rank = ndims - .checked_sub(1) - .ok_or_else(|| FormatError::ChunkedReadError("chunked layout has no dimensions".into()))?; - let chunk_dims: Vec = chunk_dimensions[..rank] - .iter() - .map(|&d| d as usize) - .collect(); - + let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); - if ds_dims.len() != rank { - return Err(FormatError::ChunkedReadError(format!( - "rank mismatch: dataspace has {} dims, layout has {} chunk dims (rank={})", - ds_dims.len(), - chunk_dimensions.len(), - rank - ))); - } // Chunk index and assembly plan for this dataset, built on first access // and kept per dataset (keyed by chunk-index address) in the shared cache. @@ -1620,11 +1664,12 @@ mod tests { write_offset(&mut buf, chunk.address, offset_size); } - // Final key (dummy) + // Final key (its offsets must be on the chunk grid, as libhdf5 + // checks; 0 always is) buf.extend_from_slice(&0u32.to_le_bytes()); // chunk_size buf.extend_from_slice(&0u32.to_le_bytes()); // filter_mask for _ in 0..ndims { - write_offset(&mut buf, u64::MAX, 8); + write_offset(&mut buf, 0, 8); } buf @@ -1632,6 +1677,48 @@ mod tests { // --- ChunkInfo collection tests --- + #[test] + fn checked_collection_refuses_keys_off_the_chunk_grid() { + let chunk = |offsets: Vec, address| ChunkInfo { + chunk_size: 80, + filter_mask: 0, + offsets, + address, + }; + let good = + build_chunk_btree_leaf(&[chunk(vec![0, 0], 0x100), chunk(vec![10, 0], 0x200)], 2, 8); + assert_eq!( + collect_chunk_info_checked(&good, 0, &[10, 8], 8, 8) + .unwrap() + .len(), + 2 + ); + // A chunk key off the grid. + let bad = + build_chunk_btree_leaf(&[chunk(vec![0, 0], 0x100), chunk(vec![7, 0], 0x200)], 2, 8); + assert!(collect_chunk_info(&bad, 0, 2, 8, 8).is_ok()); + assert!(matches!( + collect_chunk_info_checked(&bad, 0, &[10, 8], 8, 8), + Err(FormatError::ChunkedReadError(m)) if m.starts_with("bad coordinate offset") + )); + // cve-2018-11205: the chunks all start at 0 in dimension 1, and only + // the node's final key shows the chunk dimension is wrong. + let mut two_d = build_chunk_btree_leaf( + &[chunk(vec![0, 0, 0], 0x100), chunk(vec![10, 0, 0], 0x200)], + 3, + 8, + ); + // Final key: (20, 20, 0), the end of a 20 x 20 dataset. + let final_key = two_d.len() - 24; + two_d[final_key..final_key + 8].copy_from_slice(&20u64.to_le_bytes()); + two_d[final_key + 8..final_key + 16].copy_from_slice(&20u64.to_le_bytes()); + assert!(collect_chunk_info_checked(&two_d, 0, &[10, 20, 4], 8, 8).is_ok()); + assert!(matches!( + collect_chunk_info_checked(&two_d, 0, &[10, 32788, 4], 8, 8), + Err(FormatError::ChunkedReadError(m)) if m.starts_with("bad coordinate offset [20, 20, 0]") + )); + } + #[test] fn collect_two_chunks_from_leaf() { let ndims = 2; // rank+1 for 1D dataset @@ -1750,6 +1837,41 @@ mod tests { use crate::dataspace::{Dataspace, DataspaceType}; use crate::datatype::{Datatype, DatatypeByteOrder}; + #[test] + fn chunk_geometry_matches_libhdf5_open_checks() { + let space = |dims: &[u64]| Dataspace { + space_type: DataspaceType::Simple, + rank: dims.len() as u8, + dimensions: dims.to_vec(), + max_dimensions: None, + }; + assert_eq!( + chunk_geometry(&[4, 5, 8], &space(&[10, 10]), 8).unwrap(), + (2, vec![4, 5]) + ); + // Rank mismatch. + assert!(matches!( + chunk_geometry(&[4, 8], &space(&[10, 10]), 8), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("doesn't match") + )); + // Zero dimension (a layout built in memory, bypassing the parser). + assert!(matches!( + chunk_geometry(&[4, 0, 8], &space(&[10, 10]), 8), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("must be > 0") + )); + // 0x80000000 x 4-byte elements is 8 GiB; the largest allowed chunk + // is 4 GiB - 1 bytes. These dims used to hang the reader. + assert!(matches!( + chunk_geometry(&[0x8000_0000, 4], &space(&[10]), 4), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("4GB") + )); + assert!(matches!( + chunk_geometry(&[0xFFFF_FFFF, 0xFFFF_FFFF, 1], &space(&[10, 10]), 1), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("4GB") + )); + assert!(chunk_geometry(&[0xFFFF_FFFF, 1], &space(&[10]), 1).is_ok()); + } + fn make_f64_type() -> Datatype { Datatype::FloatingPoint { size: 8, @@ -1863,8 +1985,8 @@ mod tests { let file_data = vec![0u8; 64]; let result = read_chunked_data(&file_data, &layout, &dataspace, &datatype, None, 8, 8); assert!( - matches!(result, Err(FormatError::ChunkedReadError(_))), - "expected a clean ChunkedReadError, got {result:?}" + matches!(result, Err(FormatError::InvalidChunkDimensions(_))), + "expected a clean InvalidChunkDimensions, got {result:?}" ); } diff --git a/crates/clawhdf5-format/src/data_layout.rs b/crates/clawhdf5-format/src/data_layout.rs index 60a8243..54122fc 100644 --- a/crates/clawhdf5-format/src/data_layout.rs +++ b/crates/clawhdf5-format/src/data_layout.rs @@ -24,6 +24,34 @@ pub struct VdsMapping { pub virtual_selection: Vec, } +/// Most dimensions a layout message can list (libhdf5 `H5O_LAYOUT_NDIMS`): +/// 32 dataspace dimensions plus the element size. +const MAX_LAYOUT_NDIMS: usize = 33; + +/// libhdf5's checks on a chunked layout message's dimensions +/// (`H5O__layout_decode`): at most [`MAX_LAYOUT_NDIMS`], no dimension 0, and +/// before version 4 at least one dataspace dimension plus the element size. +/// A zero chunk dimension used to read the dataset as all fill values. +fn check_chunk_dims(dims: Vec, layout_version: u8) -> Result, FormatError> { + if dims.len() > MAX_LAYOUT_NDIMS { + return Err(FormatError::InvalidChunkDimensions( + "dimensionality is too large".into(), + )); + } + if layout_version < 4 && dims.len() < 2 { + return Err(FormatError::InvalidChunkDimensions( + "bad dimensions for chunked storage".into(), + )); + } + if let Some(u) = dims.iter().position(|&d| d == 0) { + return Err(FormatError::InvalidChunkDimensions(format!( + "bad chunk dimension value when parsing layout message - chunk dimension must be \ + positive: mesg->u.chunk.dim[{u}] = 0" + ))); + } + Ok(dims) +} + /// Parsed HDF5 data layout message. #[derive(Debug, Clone, PartialEq)] pub enum DataLayout { @@ -394,7 +422,7 @@ impl DataLayout { Ok(DataLayout::Contiguous { address, size }) } _ => Ok(DataLayout::Chunked { - chunk_dimensions: dims, + chunk_dimensions: check_chunk_dims(dims, 2)?, btree_address: address, version: 3, chunk_index_type: None, @@ -457,7 +485,7 @@ impl DataLayout { p += 4; } Ok(DataLayout::Chunked { - chunk_dimensions, + chunk_dimensions: check_chunk_dims(chunk_dimensions, 3)?, btree_address, version: 3, chunk_index_type: None, @@ -506,6 +534,11 @@ impl DataLayout { let dimensionality = data[pos + 1] as usize; let dim_size_encoded_length = data[pos + 2] as usize; let mut p = pos + 3; + if dimensionality > MAX_LAYOUT_NDIMS { + return Err(FormatError::InvalidChunkDimensions( + "dimensionality is too large".into(), + )); + } // dimension sizes ensure_len(data, p, dimensionality * dim_size_encoded_length)?; @@ -547,6 +580,7 @@ impl DataLayout { chunk_dimensions.push(val); p += dim_size_encoded_length; } + let chunk_dimensions = check_chunk_dims(chunk_dimensions, 4)?; // chunk index type ensure_len(data, p, 1)?; @@ -755,6 +789,55 @@ mod tests { ); } + /// A v3 chunked layout message with these dims (element size last). + fn v3_chunked_msg(dims: &[u32]) -> Vec { + let mut buf = vec![3u8, 2, dims.len() as u8]; + buf.extend_from_slice(&0x1000u64.to_le_bytes()); + for d in dims { + buf.extend_from_slice(&d.to_le_bytes()); + } + buf + } + + #[test] + fn chunk_dimensions_are_checked_when_the_layout_is_parsed() { + assert!(DataLayout::parse(&v3_chunked_msg(&[4, 4, 8]), 8, 8).is_ok()); + // A zero chunk dimension used to read as all fill values. + let err = DataLayout::parse(&v3_chunked_msg(&[4, 0, 8]), 8, 8).unwrap_err(); + assert!( + matches!(&err, FormatError::InvalidChunkDimensions(m) if m.contains("dim[1] = 0")), + "{err:?}" + ); + // Only the element-size dimension: libhdf5 "bad dimensions". + assert_eq!( + DataLayout::parse(&v3_chunked_msg(&[8]), 8, 8).unwrap_err(), + FormatError::InvalidChunkDimensions("bad dimensions for chunked storage".into()) + ); + assert_eq!( + DataLayout::parse(&v3_chunked_msg(&[1; 34]), 8, 8).unwrap_err(), + FormatError::InvalidChunkDimensions("dimensionality is too large".into()) + ); + // v1/v2 and v4 messages get the zero check too. + let mut v1 = v1v2_header(1, 2, 2); + v1.extend_from_slice(&0x1000u64.to_le_bytes()); + v1.extend_from_slice(&0u32.to_le_bytes()); + v1.extend_from_slice(&8u32.to_le_bytes()); + assert!(matches!( + DataLayout::parse(&v1, 8, 8), + Err(FormatError::InvalidChunkDimensions(_)) + )); + let mut v4 = vec![4u8, 2, 0, 2, 4]; + v4.extend_from_slice(&0u32.to_le_bytes()); + v4.extend_from_slice(&8u32.to_le_bytes()); + v4.push(3); // fixed array index + v4.push(0); // page bits + v4.extend_from_slice(&0x1000u64.to_le_bytes()); + assert!(matches!( + DataLayout::parse(&v4, 8, 8), + Err(FormatError::InvalidChunkDimensions(_)) + )); + } + #[test] fn v1v2_rejects_bad_class_dimensionality_and_truncation() { assert_eq!( diff --git a/crates/clawhdf5-format/src/data_read.rs b/crates/clawhdf5-format/src/data_read.rs index 76c2b0e..91b739a 100644 --- a/crates/clawhdf5-format/src/data_read.rs +++ b/crates/clawhdf5-format/src/data_read.rs @@ -360,6 +360,7 @@ pub fn read_raw_data_selection( chunk_index_type, .. } => { + crate::chunked_read::chunk_geometry(chunk_dimensions, dataspace, elem_size)?; // For chunked data, only read chunks that intersect the selection let chunk_dims: Vec = chunk_dimensions.iter().map(|&d| d as u64).collect(); let rank = dims.len(); @@ -400,10 +401,10 @@ pub fn read_raw_data_selection( } else { // v3: B-tree v1 if let Some(addr) = btree_address { - crate::chunked_read::collect_chunk_info( + crate::chunked_read::collect_chunk_info_checked( file_data, *addr, - rank + 1, + chunk_dimensions, offset_size, length_size, )? diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index 46f8123..814a5a7 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -210,6 +210,10 @@ pub enum FormatError { /// libhdf5's own error text): size 0, bit fields outside the type, /// an empty enum name, a compound member outside its compound, … InvalidDatatype(String), + /// A chunked layout whose chunk dimensions libhdf5 refuses: a zero + /// dimension, a rank that does not match the dataspace, or a chunk of + /// 4 GiB or more. + InvalidChunkDimensions(String), } impl fmt::Display for FormatError { @@ -460,6 +464,9 @@ impl fmt::Display for FormatError { FormatError::InvalidDatatype(why) => { write!(f, "invalid datatype: {why}") } + FormatError::InvalidChunkDimensions(why) => { + write!(f, "invalid chunk dimensions: {why}") + } } } } diff --git a/crates/clawhdf5/tests/header_validation_interop.rs b/crates/clawhdf5/tests/header_validation_interop.rs new file mode 100644 index 0000000..a2f3e7a --- /dev/null +++ b/crates/clawhdf5/tests/header_validation_interop.rs @@ -0,0 +1,155 @@ +//! Corrupt files that libhdf5 refuses must be refused here too, not read. +//! +//! h5py writes a valid file, the script corrupts a copy the way a damaged or +//! malicious file would be, and records whether h5py (libhdf5) still opens +//! and reads the object. clawhdf5 must agree: read the valid file, refuse +//! each corrupt one. Skipped when python3 with h5py is unavailable, unless +//! `CLAWHDF5_REQUIRE_INTEROP=1`. + +use std::path::Path; +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; + } + }; +} + +/// Runs `body` (Python, with `h5py`, `numpy as np`, `struct` imported and +/// `d` the output directory) and then, for every `NAME.h5` it wrote, +/// prints `NAME ok` when h5py opens and reads dataset `d` and `NAME ERROR` +/// otherwise. Returns those lines, sorted. +fn h5py_verdicts(dir: &Path, body: &str) -> Vec { + let script = format!( + r#" +import h5py, numpy as np, struct, os, glob +d = "{dir}" +{body} +for path in sorted(glob.glob(os.path.join(d, "*.h5"))): + name = os.path.basename(path)[:-3] + try: + with h5py.File(path, "r") as f: + f["d"][()] + print(name, "ok") + except Exception: + print(name, "ERROR") +"#, + dir = dir.display() + ); + let out = Command::new(python()) + .args(["-c", &script]) + .output() + .expect("failed to run python"); + assert!( + out.status.success(), + "python failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let mut lines: Vec = String::from_utf8_lossy(&out.stdout) + .lines() + .map(str::to_owned) + .collect(); + lines.sort(); + lines +} + +/// Whether clawhdf5 opens and reads dataset `d` of `path` (as raw bytes of +/// whatever type it has). +fn clawhdf5_reads(path: &Path) -> Result<(), String> { + let file = File::open(path).map_err(|e| format!("open: {e}"))?; + let ds = file.dataset("d").map_err(|e| format!("dataset: {e}"))?; + ds.dtype().map_err(|e| format!("dtype: {e}"))?; + ds.shape().map_err(|e| format!("shape: {e}"))?; + ds.read_i32().map(|_| ()).map_err(|e| format!("read: {e}")) +} + +/// h5py's verdict for each file must be `expected`, and clawhdf5 must read +/// exactly the files h5py reads. +fn assert_agrees_with_h5py(dir: &Path, verdicts: &[String], expected: &[&str]) { + assert_eq!(verdicts, expected, "h5py's view changed"); + for line in verdicts { + let (name, verdict) = line.split_once(' ').unwrap(); + let ours = clawhdf5_reads(&dir.join(format!("{name}.h5"))); + match verdict { + "ok" => assert!(ours.is_ok(), "{name}: h5py reads it, we fail: {ours:?}"), + _ => assert!(ours.is_err(), "{name}: h5py refuses it, we read it"), + } + } +} + +#[test] +fn chunk_dimensions_libhdf5_refuses_are_refused() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + // A chunked int32 dataset (chunk 37, element size 4 after it in the + // layout message), with libver earliest (layout v3) and latest (v4). + // The corrupt copies set the chunk dimension to 0 (which read as all + // fill values), to 0x80000000 (an 8 GiB chunk) and to 38, which the + // chunk index's offsets 37 and 74 are not multiples of (libhdf5: "bad + // coordinate offset"; the chunks were read at the wrong place). A v4 + // layout indexes chunks by position, not offset, but both libraries + // refuse the changed chunk grid there too. + let verdicts = h5py_verdicts( + dir.path(), + r#" +for libver in ("earliest", "latest"): + good = os.path.join(d, f"{libver}_good.h5") + with h5py.File(good, "w", libver=libver) as f: + f.create_dataset("d", data=np.arange(100, dtype=" 0 + for name, value in (("zero", 0), ("huge", 0x80000000), ("offgrid", 38)): + bad = bytearray(data) + if value >= 1 << (8 * width): + continue + bad[at:at + width] = value.to_bytes(width, "little") + open(os.path.join(d, f"{libver}_{name}.h5"), "wb").write(bad) +"#, + ); + assert_agrees_with_h5py( + dir.path(), + &verdicts, + &[ + "earliest_good ok", + "earliest_huge ERROR", + "earliest_offgrid ERROR", + "earliest_zero ERROR", + "latest_good ok", + "latest_offgrid ERROR", + "latest_zero ERROR", + ], + ); +} From 7d7a7e75d406638b177d6cc4d57d82ae4f26609f Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 00:30:35 -0500 Subject: [PATCH 05/17] fix: refuse truncated files and read nothing past the recorded end of file The superblock records where the file's data ends. libhdf5 refuses to open a file shorter than that ("truncated file", H5F__super_read) and fails any read past it ("addr overflow" / "address plus size exceeds file eoa"). clawhdf5 read whatever was left of a truncated file, and read bytes after the recorded end as if they belonged to the file. New Superblock::data_end: FormatError::TruncatedFile for a file shorter than its recorded end, otherwise where the HDF5 data ends. As in libhdf5, the recorded end moves with the superblock when its recorded base address is not where it is (a user block added afterwards; cve-2021-36977, which h5py reads, depends on it), and the check is skipped for a v3 superblock still being written in SWMR mode. File, LazyFile, MmapFile and the conformance probe refuse a truncated file and parse only up to the end. Files clawhdf5 writes record their true length, and the v2.5.0 agent-store fixture and files written by v2.7.0 (plain, paged, user-block free) pass the check. Interop test: h5py writes a file; a copy missing its last 8 bytes must be refused by both, a copy with bytes appended and one moved behind a new 512-byte user block must read in both. Conformance (cached corpus, tank): 570 -> 571 ok (h5clear_fsm_persist_less.h5, whose data past the recorded end was being read); ten files h5py refuses as truncated (cve-2018-13874, cve-2018-13876, the family/multi/subfiling members, h5clear_fsm_persist_ greater/user_greater) are now refused at open instead of read. Co-Authored-By: Claude Opus 5.5 (1M context) --- conformance/probe/src/main.rs | 11 ++++ crates/clawhdf5-format/src/error.rs | 18 ++++++ crates/clawhdf5-format/src/superblock.rs | 60 +++++++++++++++++++ crates/clawhdf5/src/lazy.rs | 9 ++- crates/clawhdf5/src/mmap_file.rs | 8 ++- crates/clawhdf5/src/reader.rs | 22 ++++--- .../tests/header_validation_interop.rs | 45 ++++++++++++++ 7 files changed, 164 insertions(+), 9 deletions(-) diff --git a/conformance/probe/src/main.rs b/conformance/probe/src/main.rs index 9629dcc..87c660d 100644 --- a/conformance/probe/src/main.rs +++ b/conformance/probe/src/main.rs @@ -724,6 +724,17 @@ fn main() { return; } }; + // libhdf5 refuses a truncated file and reads nothing past the recorded + // end of file. + let base = (data.len() - hdf5.len()) as u64; + let hdf5 = match sb.data_end(base, data.len() as u64) { + Ok(end) => &hdf5[..end as usize], + Err(err) => { + top.insert("open_error".into(), Value::String(e(err))); + println!("{}", Value::Object(top)); + return; + } + }; top.insert("superblock_version".into(), json!(sb.version)); let ctx = Ctx { data: hdf5, diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index 814a5a7..b3cfd65 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -214,6 +214,14 @@ pub enum FormatError { /// dimension, a rank that does not match the dataspace, or a chunk of /// 4 GiB or more. InvalidChunkDimensions(String), + /// The superblock's end-of-file address lies past the end of the file: + /// the file was truncated (libhdf5 refuses to open it). + TruncatedFile { + /// End of file recorded in the superblock (relative to byte 0). + stored_eof: u64, + /// The file's actual length in bytes. + actual_len: u64, + }, } impl fmt::Display for FormatError { @@ -467,6 +475,16 @@ impl fmt::Display for FormatError { FormatError::InvalidChunkDimensions(why) => { write!(f, "invalid chunk dimensions: {why}") } + FormatError::TruncatedFile { + stored_eof, + actual_len, + } => { + write!( + f, + "truncated file: the superblock records end of file {stored_eof}, \ + but the file is {actual_len} bytes" + ) + } } } } diff --git a/crates/clawhdf5-format/src/superblock.rs b/crates/clawhdf5-format/src/superblock.rs index d2ec4d6..a565321 100644 --- a/crates/clawhdf5-format/src/superblock.rs +++ b/crates/clawhdf5-format/src/superblock.rs @@ -100,6 +100,42 @@ pub mod swmr_flags { } impl Superblock { + /// Where the HDF5 data ends, relative to the superblock, for a file of + /// `file_len` bytes whose superblock is at `user_block` (both counted + /// from the start of the file), with libhdf5's truncation check + /// (`H5F__super_read`). + /// + /// The superblock records the end of the file's data as an absolute + /// address. A file shorter than that was truncated, and libhdf5 refuses + /// to open it ("truncated file"); so does this, with + /// [`FormatError::TruncatedFile`]. Bytes past that address are not part + /// of the file: libhdf5 fails any read of them ("addr overflow" / + /// "address plus size exceeds file eoa"), so a reader should parse only + /// the data up to the returned end. As libhdf5 does for a SWMR reader, + /// the check is skipped for a version-3 superblock whose writer is still + /// writing it in SWMR mode (it extends the file as it goes); the data + /// then ends at the end of the file. + /// + /// When the superblock's recorded base address differs from where the + /// superblock actually is (a user block added or removed after the file + /// was written), libhdf5 moves the recorded end of file by the same + /// amount, and so does this. + pub fn data_end(&self, user_block: u64, file_len: u64) -> Result { + let eof = + i128::from(self.eof_address) - i128::from(self.base_address) + i128::from(user_block); + if eof < 0 || eof > i128::from(file_len) { + if self.version >= 3 && self.is_swmr_write() { + return Ok(file_len.saturating_sub(user_block)); + } + return Err(FormatError::TruncatedFile { + stored_eof: u64::try_from(eof).unwrap_or(self.eof_address), + actual_len: file_len, + }); + } + // 0 <= eof <= file_len, so it fits a u64. + Ok((eof as u64).saturating_sub(user_block)) + } + /// Whether the file was opened with write access when the superblock was written. pub fn is_write_access(&self) -> bool { self.consistency_flags & swmr_flags::WRITE_ACCESS != 0 @@ -537,6 +573,30 @@ mod tests { buf } + #[test] + fn data_end_refuses_truncated_files_like_libhdf5() { + // build_v2_bytes records base 0, end of file 2048. + let sb = Superblock::parse(&build_v2_bytes(8, 2), 0).unwrap(); + assert_eq!(sb.data_end(0, 2048), Ok(2048)); + // Bytes past the recorded end are not part of the file. + assert_eq!(sb.data_end(0, 4096), Ok(2048)); + assert_eq!( + sb.data_end(0, 2047), + Err(FormatError::TruncatedFile { + stored_eof: 2048, + actual_len: 2047 + }) + ); + // A user block added in front after the file was written (the + // recorded base address is still 0): the end moves with it. + assert_eq!(sb.data_end(512, 2560), Ok(2048)); + assert!(sb.data_end(512, 2559).is_err()); + // A v3 superblock still being written in SWMR mode is not checked. + let mut swmr = Superblock::parse(&build_v2_bytes(8, 3), 0).unwrap(); + swmr.consistency_flags = swmr_flags::WRITE_ACCESS | swmr_flags::SWMR_WRITE; + assert_eq!(swmr.data_end(0, 1000), Ok(1000)); + } + #[test] fn parse_v0_8byte_offsets() { let data = build_v0_bytes(8); diff --git a/crates/clawhdf5/src/lazy.rs b/crates/clawhdf5/src/lazy.rs index cf70f70..d893485 100644 --- a/crates/clawhdf5/src/lazy.rs +++ b/crates/clawhdf5/src/lazy.rs @@ -43,6 +43,8 @@ pub struct LazyFile { /// Offset of the superblock in the file (the user-block size); every /// HDF5 address is relative to it. base: usize, + /// End of the HDF5 data (`Superblock::data_end`, absolute). + end: usize, superblock: Superblock, root_header: ObjectHeader, /// Cache of parsed object headers, keyed by address. @@ -74,9 +76,13 @@ impl LazyFile { /// /// Parses only the superblock and root group object header. pub fn open(reader: R) -> Result { + let whole_len = reader.as_bytes().len() as u64; let (user_block, data) = signature::split_user_block(reader.as_bytes())?; let base = user_block.len(); let superblock = Superblock::parse(data, 0)?; + // Refuse a truncated file; read nothing past the recorded end of file. + let end = base + superblock.data_end(base as u64, whole_len)? as usize; + let data = &reader.as_bytes()[base..end]; let root_header = ObjectHeader::parse( data, superblock.root_group_address as usize, @@ -86,6 +92,7 @@ impl LazyFile { Ok(Self { reader, base, + end, superblock, root_header, header_cache: RefCell::new(HashMap::new()), @@ -104,7 +111,7 @@ impl LazyFile { } fn hdf5_bytes(&self) -> &[u8] { - &self.reader.as_bytes()[self.base..] + &self.reader.as_bytes()[self.base..self.end] } /// Returns a reference to the parsed superblock. diff --git a/crates/clawhdf5/src/mmap_file.rs b/crates/clawhdf5/src/mmap_file.rs index 8c35163..7f544ca 100644 --- a/crates/clawhdf5/src/mmap_file.rs +++ b/crates/clawhdf5/src/mmap_file.rs @@ -35,6 +35,8 @@ pub struct MmapFile { /// Offset of the superblock in the mapped file (the user-block size); /// every HDF5 address is relative to it. base: usize, + /// End of the HDF5 data (`Superblock::data_end`, absolute). + end: usize, superblock: Superblock, } @@ -42,12 +44,16 @@ 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 whole_len = reader.as_bytes().len() as u64; let (user_block, data) = signature::split_user_block(reader.as_bytes())?; let base = user_block.len(); let superblock = Superblock::parse(data, 0)?; + // Refuse a truncated file; read nothing past the recorded end of file. + let end = base + superblock.data_end(base as u64, whole_len)? as usize; Ok(Self { reader, base, + end, superblock, }) } @@ -55,7 +61,7 @@ impl MmapFile { /// 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..] + &self.reader.as_bytes()[self.base..self.end] } /// Size of the user block before the superblock (0 for most files). diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 5f2a027..6e506b4 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -45,26 +45,34 @@ impl Backing { } } -/// 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`]. +/// The file's bytes, viewed from the superblock on and up to the end of +/// file the superblock records. 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, + /// End of the HDF5 data in the file (`Superblock::data_end`, absolute). + end: usize, } impl FileData { - /// Locate the superblock and parse it. + /// Locate the superblock and parse it. A truncated file is refused, and + /// bytes past the recorded end of file are not read, as in libhdf5. fn new(backing: Backing) -> Result<(Self, Superblock), Error> { - let (user_block, hdf5) = signature::split_user_block(backing.whole_file())?; + let whole = backing.whole_file(); + let (user_block, hdf5) = signature::split_user_block(whole)?; let base = user_block.len(); let superblock = Superblock::parse(hdf5, 0)?; - Ok((Self { backing, base }, superblock)) + let end = superblock.data_end(base as u64, whole.len() as u64)?; + // data_end is at most the file length (less the user block). + let end = base + end as usize; + Ok((Self { backing, base, end }, superblock)) } fn as_bytes(&self) -> &[u8] { - &self.backing.whole_file()[self.base..] + &self.backing.whole_file()[self.base..self.end] } fn len(&self) -> usize { diff --git a/crates/clawhdf5/tests/header_validation_interop.rs b/crates/clawhdf5/tests/header_validation_interop.rs index a2f3e7a..23509e1 100644 --- a/crates/clawhdf5/tests/header_validation_interop.rs +++ b/crates/clawhdf5/tests/header_validation_interop.rs @@ -153,3 +153,48 @@ for libver in ("earliest", "latest"): ], ); } + +#[test] +fn truncated_files_are_refused_and_nothing_past_the_end_of_file_is_read() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + // The superblock records where the file's data ends. A copy missing its + // last bytes is truncated: libhdf5 refuses to open it (this read what + // was left). Bytes appended after the end are not part of the file, and + // a superblock moved by prepending a user block (its recorded base + // address now wrong) has its end of file moved with it; both still read. + let verdicts = h5py_verdicts( + dir.path(), + r#" +for libver in ("earliest", "latest"): + good = os.path.join(d, f"{libver}_good.h5") + with h5py.File(good, "w", libver=libver) as f: + f.create_dataset("d", data=np.arange(100, dtype=" Date: Sat, 26 Sep 2026 00:34:44 -0500 Subject: [PATCH 06/17] fix(format): keep reading the floats and empty strings clawhdf5 v2.7.0 wrote Two of the datatype checks added on this branch refused files clawhdf5 itself wrote up to v2.7.0: it put the sign bit of every float at position 63 (so every f32 it wrote failed "sign bit position out of bounds", including every agent store's embeddings), and wrote an empty-string attribute with a size-0 string type ("invalid datatype size", failing every attribute of the object). libhdf5 refuses both, but neither decodes to wrong values (an IEEE float's sign position is not used; a size-0 string is empty), so this reader keeps accepting them. New fixtures written by clawhdf5 v2.7.0 (FileBuilder with every datatype, layout and attribute kind it could write, and a FileWriter paged file) and legacy_writer_files.rs, which reads every object of them. The agent's v2.5.0 store fixture (float16_store) passes again. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/datatype.rs | 42 ++++++--- .../tests/fixtures/written_by_v2_7_0.h5 | Bin 0 -> 12264 bytes .../tests/fixtures/written_by_v2_7_0_paged.h5 | Bin 0 -> 192 bytes crates/clawhdf5/tests/legacy_writer_files.rs | 83 ++++++++++++++++++ 4 files changed, 113 insertions(+), 12 deletions(-) create mode 100644 crates/clawhdf5/tests/fixtures/written_by_v2_7_0.h5 create mode 100644 crates/clawhdf5/tests/fixtures/written_by_v2_7_0_paged.h5 create mode 100644 crates/clawhdf5/tests/legacy_writer_files.rs diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index d97a61d..980575d 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -289,9 +289,15 @@ fn ranges_overlap(a0: u64, a1: u64, b0: u64, b1: u64) -> bool { a0 <= b1 && b0 <= a1 } -/// libhdf5's checks on a floating-point type's fields: sign, exponent and -/// mantissa must lie inside the type, be non-empty, and not overlap. -/// (libhdf5 does not check a float's bit offset and precision.) +/// libhdf5's checks on a floating-point type's fields: exponent and mantissa +/// must lie inside the type, be non-empty, and not overlap each other or the +/// sign bit. (libhdf5 does not check a float's bit offset and precision.) +/// +/// One libhdf5 check is left out on purpose: a sign bit position outside the +/// type ("sign bit position out of bounds"). clawhdf5 up to v2.7.0 wrote 63 +/// there for every float, so every `f32` it wrote (every agent store's +/// embeddings) would stop opening. The position is not used to decode an +/// IEEE float, so reading such a type returns the right values. fn check_float_fields( size: u32, sign: u8, @@ -308,9 +314,6 @@ fn check_float_fields( u64::from(mpos), u64::from(msize), ); - if sign >= bits { - return Err(invalid("sign bit position out of bounds")); - } if esize == 0 { return Err(invalid("exponent size can't be zero")); } @@ -380,7 +383,12 @@ impl Datatype { let size = LittleEndian::read_u32(&data[4..8]); let mut pos = 8; - if size == 0 { + // libhdf5 refuses size 0 for every class. A fixed-length string is + // exempt: clawhdf5 up to v2.7.0 wrote an empty-string attribute + // with a size-0 string type, and refusing it would fail every + // attribute of such objects, while reading it (an empty string) is + // harmless. + if size == 0 && class_id != 3 { return Err(invalid("invalid datatype size")); } @@ -2335,10 +2343,9 @@ mod tests { let mut data = build_dt_header(9, 1, [1, 0, 0], 0); data.extend_from_slice(&build_fixed_point(1, false, false, 0, 8)); assert_eq!(invalid_reason(&data), "invalid datatype size"); - assert_eq!( - invalid_reason(&build_dt_header(3, 1, [0, 0, 0], 0)), - "invalid datatype size" - ); + // Except a fixed-length string, which clawhdf5 <= v2.7.0 wrote for an + // empty-string attribute. + assert!(Datatype::parse(&build_dt_header(3, 1, [0, 0, 0], 0)).is_ok()); assert_eq!( invalid_reason(&build_fixed_point(0, false, false, 0, 0)), "invalid datatype size" @@ -2376,7 +2383,6 @@ mod tests { }; assert!(Datatype::parse(&f32_with(31, 23, 8, 0, 23)).is_ok()); for (fields, why) in [ - ((32, 23, 8, 0, 23), "sign bit position out of bounds"), ((31, 23, 0, 0, 23), "exponent size can't be zero"), ( (31, 32, 8, 0, 23), @@ -2447,6 +2453,18 @@ mod tests { assert!(Datatype::parse_in_header(&data, 1).is_err()); } + #[test] + fn f32_written_by_clawhdf5_up_to_2_7_0_still_parses() { + // Those versions put the sign bit at 63 whatever the float's size; + // libhdf5 refuses it ("sign bit position out of bounds"). + let mut data = build_dt_header(1, 1, [0x20, 63, 0], 4); + data.extend_from_slice(&0u16.to_le_bytes()); + data.extend_from_slice(&32u16.to_le_bytes()); + data.extend_from_slice(&[23, 8, 0, 23]); + data.extend_from_slice(&127u32.to_le_bytes()); + assert!(Datatype::parse(&data).is_ok()); + } + #[test] fn float_bit_6_is_vax_order_only_from_version_3() { // h5py opens a v1 float with bit 6 set as an ordinary little-endian diff --git a/crates/clawhdf5/tests/fixtures/written_by_v2_7_0.h5 b/crates/clawhdf5/tests/fixtures/written_by_v2_7_0.h5 new file mode 100644 index 0000000000000000000000000000000000000000..ebb7cf1a07589c25375a9a8337696101066acb61 GIT binary patch literal 12264 zcmeI&2~-rvx(DzY24sXmMnxrx01gU&X)+Qni~sI-kdQbR)@)$ zF-wY18XLd3=>jJUkCM2cm3h<420l71eGW;iJlaeOJCytu*x&jH4 z@QQef(*2E&j+VFQ8yGPw>@SYhx9f>bc~UW@Ia9tud0wrMp7)e@<9R7c%UN7h?ZvT| z*PHMSANQo~Rq~d?&g9rxJX2s#;?DRMFX^Po-NaIImdq4bQ(a20Zt)b;`D1mPXWaIS z2nc7U*ud!iE7~W5Sn?Vz67$tZ_S@z%obNxCaK);(6k1`&B&>P$vUrJ?qhElJrurK5 zT6cwmlAm~@zaW_{FQx}IeMLfwO-LQfqjpq4O!We-HEIfe%50JUeBkn zW++gPb??3(+2$z9I)-O+$8-)pjht-{YnY}F&% zBkUM{He`3zkIyhyUD=hE_0O@ny0RayJbaaf=$1~M6rPSB|D?Zn26R&b^A=_Vad;AA z9XY>OBr$8~zGWxZDnmEG6#l>Kj zpxQAB{kEj5K}n%eeDeHlNjHO%;*edlWM_kt;`~{&q|%_Icy4NzbT=qzLWfkdq=!LC zajdLavWr1U@ha3T>1j|>m|G3cl9xfry0y2{)$kgb{Z2{4Yec=b<~4FPyhgYSbg9M> zxf)(0nmY~ZG`vPMSsIiyyhb!<8k97=Ml?ejlr+3XG%p&IG`vQ1=NptXyhaV_Ed7@R z-Sj8p(;q+B7zYhq??$?rhc|M~kPb$==}*Qk@!JrOqrB+ydr?Nrncu-!z&o>hN~8&E z!*4$<;73`1%y=1lMJ0IWxc~FvM(lH1zXW6qxsmUMZ`tMTxog>U zs)OG%jrXN~!nWZDFTsa}fLW7rSsK;BdzRzs&v!CMTEM$+qtk~E=pF_5CVY8hVcH=U zMu%^O1tZ_zme2aqCcGaz4vxFRq_kT;P`q;_=mGnR7Vz_4r>n~olWA#7@$Kf_2yO=ceYV*42(b`d4lwRVS_sy_cjF zstXo7f!I+UVb{mipH{Ix?yb!;mrFj3ruQsoBN1 zgzC&~&;lvH>m<64z|WiTOW4-keBd|YIjPxSsPX*f<*9`b^v?<2tR<%A*xjBs!pdje z1}lW>!ou4kCPgn16(?Z>Vk)UB7t4uyu-dAdWMEo6LppLM%wyRM-Kpf=@|ed(}*X znNYkzYio5eRVuEF93amYTK$JNsJMy#Nw`hb2|bRn^*x-&=nhWz=k-S&VVO+Dn3lg; zBC0@W_^;@W;&>q(2xEL1Pc@f|ZKz&uB>v*H>DR4QF}4o!;WjWeP8HaJ3#cF*M#2Qp z!W{Sjmcu&u6t=@Y$cIyK5sKk9+=IvP9Qc_x1{ugf0ajoQHed&>!2z7W1(e_k-k<_} z24Vpa4B^lZ2EtGn2{AAl#%fZAM~}4rX5I9HPofH@o63#M>eQ3eHL+;kFwSYnS>BJG@v%=CG)7EYf#bHR-Hv%^#YKEawGzfg6cZ%D7cy@&WmhLvUn z_T6}Xx%;S+bl={~atG$c-I^6LWY3Sqd810F_wrx3#xuF>=FHxaTW{@4E-#tkAGY>j z%G0u2v%^Lldinfmx$TUDQftp|S*WrO^G}br)@IjRf-~}lB(z_dTXpWxtzDN+hu3ZY zbl-;ETfWTRx%t4xJzGmswG#H(F10#ha96uGG`5yiDZ`H!%#@^Kjm!HLu0c&VG&5fM zFfrwP=VEp{?)E{oB7LN_I{e8m$6RdM$(C$zSQ%%%?C!p6^UF=tR&4^8HS+E69&*j% zsM7iDp5`Z$OUD;hyeurQdigTlPQKn@;fJk5b~zQ!&vyEJ^VU@x$9=K&(d;OjP4R0# ziXFChT<<@4i;t!R6Tv(YfXwSqwwm5p9lX1%}i;nOL_RaH~nuuf*Lku^X0&d5wx`;71)$LP#8<*5@d>{n<49CIRq z;&by{PYk`5JSZV|(uLGVX)^npm1!rfqk|mpuAbzWb9Hb+LhjR&D(mQTUgd)memq!N z9yM+MkoW@s@n_c8a!&rrBDDWJ`XAcgXVEOjjg|F0wk)~iHaN@i#fGXwcT(PZ{$44j zoeLG-I5YMcE+>{NG+XZgkCRA#>+E5b2nchT3@(OdG0yvo0w;l@kzaw`1+P)>W}X-we7k7%lc&g(jE#wS~erI|SIJ)mbli)Ag7{nQx8CF0XyI6cc{3mYrw%NdQ27b%-5to=au7ReZ8 z!!XzR4bGJ)&{FelbfX|csd4#{?@@HfHrz!r7%pWyv z!4B-9CA5Opz@IeRLOXB(Z|Dv^fTnY%#%~Su0w3@NKNt-Bdo{n{!(cd!fRPXdZ$mW1 z03P31DolZ?@E)WAKmKW;h3SwE%itqe4l7_Ktb*0B2G+vIunu-Z9_)d=un+db0r&#G zgoAJh^b>y}=1*VzZJWP7TLORF;*XmAosz$u@XO0DB7bIX4;{b>oS`G|2PAY;-P=Eg z1lAe+Apim)2zo;>gaCgw41;j!1F;YXqhK_|!x%__u`mu2VLZ%$444VCU^dKwxiAmr zLnbVMPaqrC!v@HKjj#zmh0kC!d=7^p9}3_I9ED?W98SPVI0dIcKk?qo`z-IMymvN* zW?%!&K|d*xaR(3R0-oRnU7;K3rwC;FLO=a4ps01Uc#^N3i#kD0VCjF z&5XeW-h!sU|LM#InnMe)1v{{Zme2~ifG2oCSLg=b&>ea}Pf&px`ojR=9~TG0AQ%io zU?>cO;V=RwK@udxWOx@+AQh&-RCo{4U@>mct5I39CRq-9zSk_yO+2 z1Nadh!Xwa6b+D}|$e=EmfjP**0`yaB2!4Lc)00Kcj zjYK{Q-iBz1fmn!xQJ|mRM}8V;VLGJ449I|)pr2MFzXsOA$FL4Qfoxb0zn3;(T@Gx7 zP4Fpv2Akn?*aEq*6^_7BI0nbz1e}CZa2n3QSvUu`;T!lC?!b3&7s{XStz*DG;_M3q@$iV{Yfh8!QJ~V)a&<-4cf2D5^9l!~kp(Atx7jOk%@B@Dc zfItX>-Vh8S5DH=Nchdp1{|opM4#FWg4Eaz1`l%TC>+m()fSYg&N}v?<(=+5N;W_*Q z|AH!b0WU#6S)u)npfNN7Yj_KqLNm}$Zpe2ACAfnJbOHV?%?tEXIP!fU0{B01^n?B| z03zXkBI%y;nar#%Rd%_J@WXFNQMB$UpNSt~C-X-eq&@uc2I(1p#6j|CA;ihHLfm5~ z#Ao(G^k^l-$*qMbPwmeiK=e{=-_ZDKg-R%7MbVm~4MjVOEh)C9*p{LL#r71PD0Zai zLeZ6?8^z8Pl@#46dQj{_(UYPV#jX^)QS_$RonjA)Jt?Y$sHMo%l&8ovl&8pgQJy06 zp*%(AOL>aSkMb0mKjkU10LoKjft07nf+$as^`<;U7EF1HEQInDSt#WxvM|b1WZ^nUm}=2K)5w4S1tVm?LIm)29%Qp~5w`q6rdT8jA;S$|qjQA;tOA{#*K zDQYR^Q)H2}o}!jwK1DW=)>G6{%%{lybIF&#+cTM+vBKD_=3d0XZ5gdFrD#R*iKFnN zc*Od#zUn{?=6G7-)K*xtg`!hC%2RC7LC6c=z5oCG9lfZeu`mB(QhlcQd2n)|vqRB| z&f@>pc^Qyhwo7P%`1hZ2SU3~^jKjL~@Xt7`67P)vR1}}S#4$F!ldz|>$;%H2Pha9c vDetDSQ|Tm>*iBLCOnHjRj+Cc3$~-$)Sn^l?@;N}92gJ7FJyM1DLe;+kWu-@h literal 0 HcmV?d00001 diff --git a/crates/clawhdf5/tests/fixtures/written_by_v2_7_0_paged.h5 b/crates/clawhdf5/tests/fixtures/written_by_v2_7_0_paged.h5 new file mode 100644 index 0000000000000000000000000000000000000000..8f837143813386424b68eec7b23ba444ba8ce8fa GIT binary patch literal 192 zcmeD5aB<`1lHy|F;9!7(|4?uMDqsSW5MW^Ndoc5@zlTc@6N4I)5J()R7)G=4FfcGO zFs7tJHC@zNCIHdk!YsnTD5zl1zyg+1U=ZhE5U&Ta8G%}ufO?pqT3DGF7@4^^fX1^h V?tqHHTvz|U=paO>!5&CE008~VD9r!> literal 0 HcmV?d00001 diff --git a/crates/clawhdf5/tests/legacy_writer_files.rs b/crates/clawhdf5/tests/legacy_writer_files.rs new file mode 100644 index 0000000..3e8a3b9 --- /dev/null +++ b/crates/clawhdf5/tests/legacy_writer_files.rs @@ -0,0 +1,83 @@ +//! Files written by older clawhdf5 releases must keep opening, even where +//! libhdf5 refuses them: stricter validation of corrupt files must not lock +//! users out of their own data. +//! +//! `fixtures/written_by_v2_7_0.h5` and `written_by_v2_7_0_paged.h5` were +//! written by clawhdf5 v2.7.0 (`FileBuilder` / `FileWriter` with every +//! datatype, layout and attribute kind it could write). v2.7.0 wrote the sign +//! bit of every float at position 63 and a size-0 string type for an empty +//! string attribute; libhdf5 refuses both, this reader must not. + +use std::path::PathBuf; + +use clawhdf5::{AttrValue, File}; + +fn fixture(name: &str) -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures") + .join(name) +} + +#[test] +fn every_object_of_a_v2_7_0_file_reads() { + let file = File::open(fixture("written_by_v2_7_0.h5")).unwrap(); + + let (attrs, errors) = file.root().attrs_with_errors().unwrap(); + assert!(errors.is_empty(), "{errors:?}"); + // (It reads as no strings, as it did before.) + assert!( + matches!(&attrs["empty"], AttrValue::StringArray(v) if v.iter().all(String::is_empty)), + "{:?}", + attrs["empty"] + ); + assert!(matches!(&attrs["title"], AttrValue::String(s) if s == "old")); + + let f32s = |name: &str| file.dataset(name).unwrap().read_f32().unwrap(); + assert_eq!(f32s("f32"), [1.0, 2.0, 3.0]); + assert_eq!( + f32s("f32_2d"), + (0..60).map(|x| x as f32).collect::>() + ); + assert_eq!( + f32s("chunked"), + (0..1000).map(|x| x as f32).collect::>() + ); + assert!(f32s("empty").is_empty()); + assert_eq!( + file.dataset("f64").unwrap().read_f64().unwrap(), + [1.0, 2.0, 3.0] + ); + assert_eq!(file.dataset("i32").unwrap().read_i32().unwrap(), [1, -2, 3]); + assert_eq!(file.dataset("i64").unwrap().read_i64().unwrap(), [1, -2, 3]); + assert_eq!(file.dataset("u64").unwrap().read_u64().unwrap(), [1, 2, 3]); + assert_eq!( + file.dataset("chunked_2d").unwrap().read_i32().unwrap(), + (0..600).collect::>() + ); + for name in ["unlimited", "maxshape"] { + assert_eq!( + file.dataset(name).unwrap().read_f64().unwrap(), + (0..100).map(|x| x as f64).collect::>(), + "{name}" + ); + } + assert_eq!( + file.dataset("compact").unwrap().read_i32().unwrap(), + [7, 8, 9] + ); + for name in ["u8", "compound", "enum", "enum8"] { + file.dataset(name) + .unwrap() + .dtype() + .unwrap_or_else(|e| panic!("{name}: {e}")); + } + + let grp = file.group("grp").unwrap(); + let (attrs, errors) = grp.attrs_with_errors().unwrap(); + assert!(errors.is_empty(), "{errors:?}"); + assert_eq!(attrs.len(), 21); + assert_eq!(grp.dataset("d").unwrap().read_f32().unwrap(), [4.0, 5.0]); + + let paged = File::open(fixture("written_by_v2_7_0_paged.h5")).unwrap(); + assert_eq!(paged.dataset("d").unwrap().read_f32().unwrap(), [1.0, 2.0]); +} From a59d83d47d06ef838c0e70227ac70730837abaa0 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 00:35:25 -0500 Subject: [PATCH 07/17] test: compare header and datatype damage with what h5py refuses h5py writes a dataset (libver earliest, so version-1 object headers) and the script damages one field of a copy: a layout message flagged shareable, a message size that is not a multiple of 8, a compound field that repeats an earlier name or overlaps it, an empty enum member name, a float exponent overlapping the mantissa. h5py refuses every damaged copy, and clawhdf5 must refuse exactly those and read the valid files. All of them but the unaligned one (then an UnexpectedEof) read before this branch. The helper now reads any datatype (File::read_multi) rather than only integers. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/header_validation_interop.rs | 73 ++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/crates/clawhdf5/tests/header_validation_interop.rs b/crates/clawhdf5/tests/header_validation_interop.rs index 23509e1..14dc9d7 100644 --- a/crates/clawhdf5/tests/header_validation_interop.rs +++ b/crates/clawhdf5/tests/header_validation_interop.rs @@ -85,7 +85,9 @@ fn clawhdf5_reads(path: &Path) -> Result<(), String> { let ds = file.dataset("d").map_err(|e| format!("dataset: {e}"))?; ds.dtype().map_err(|e| format!("dtype: {e}"))?; ds.shape().map_err(|e| format!("shape: {e}"))?; - ds.read_i32().map(|_| ()).map_err(|e| format!("read: {e}")) + file.read_multi(&["d"]) + .map(|_| ()) + .map_err(|e| format!("read: {e}")) } /// h5py's verdict for each file must be `expected`, and clawhdf5 must read @@ -198,3 +200,72 @@ for libver in ("earliest", "latest"): assert!(err.to_string().contains("truncated file"), "{name}: {err}"); } } + +#[test] +fn header_and_datatype_damage_libhdf5_refuses_is_refused() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + // Each case damages one field of a valid file h5py wrote (libver + // earliest, so version-1 object headers); all of these read before. + let verdicts = h5py_verdicts( + dir.path(), + r#" +def write(name, make): + path = os.path.join(d, name + ".h5") + with h5py.File(path, "w", libver="earliest") as f: + make(f) + return bytearray(open(path, "rb").read()) + +def save(name, data): + open(os.path.join(d, name + ".h5"), "wb").write(data) + +# A layout message (type 8, 24 bytes, v1 header) flagged shareable. +data = write("layout_good", lambda f: f.create_dataset("d", data=np.arange(4, dtype=" 0 +bad = bytearray(data); bad[at + 4] |= 0x40 +save("layout_shareable", bad) +# A message size that is not a multiple of 8 in a v1 header. +bad = bytearray(data); bad[at + 2] = 23 +save("layout_unaligned", bad) + +# A compound whose second field repeats the first's name, or overlaps it. +dt = np.dtype([("aa", " Date: Sat, 26 Sep 2026 00:36:49 -0500 Subject: [PATCH 08/17] docs: changelog and known issues for the header hardening CHANGELOG (Correctness): the new header, datatype, chunk and truncation checks, what is left out on purpose (checks HDF5 2.0 lacks; the two v2.7.0 writer quirks), the conformance numbers and the new FormatError variants. known-issues: the "Header checks" audit gap is fixed, with the one CVE object and two CVE files libhdf5 still refuses and we read. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 51 ++++++++++++++++++++++++++++++++++++++++++++ docs/known-issues.md | 26 ++++++++++++++++++++-- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fc429d..93cf91e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -296,6 +296,57 @@ - CI keeps zlib-ng building and tested; the arm64 job no longer needs cmake. ### Correctness +- **Corrupt files libhdf5 refuses are now refused instead of read.** On the + HDF Group's CVE reproducers, 18 objects that libhdf5 (HDF5 2.0, through + h5py) refuses to open were read by clawhdf5, some as wrong data (a chunk + dimension of 0 read as all fill values; chunks read at offsets off the + chunk grid). The + parser now makes libhdf5's checks, with libhdf5's error text: + - object headers (`FormatError::InvalidObjectHeader`): every message of a + v1 chunk is read and more than the prefix's count is refused (the rest + used to be dropped); v1 message sizes must be multiples of 8 and a v1 + chunk cannot end in a gap; a message running past its chunk is an error + (it used to end the chunk quietly); contradictory message flags; a + message of a class that cannot be shared flagged shareable; a + reference-count message in a v1 header; malformed continuation, + reference-count and modification-time messages; unknown v2 header + flags. + - datatypes (`FormatError::InvalidDatatype`): size 0; integer bits outside + the type; float exponent/mantissa outside the type, empty or + overlapping; a compound with no members, a member outside the compound, + a duplicate name or overlapping members; an enum whose size differs from + its base type's or with an empty name; array rank over 32 or a zero + dimension; an opaque tag length that is not a multiple of 8; in a + version-1 (unchecksummed) header, a numeric type that leaves more than + half its bits unused (`Datatype::parse_in_header`, + `Datatype::check_unused_bits`). A v1/v2 float's class bit 6 was read as + VAX byte order; libhdf5 ignores it before version 3, and so does this. + - chunked layouts (`FormatError::InvalidChunkDimensions`): a zero chunk + dimension, a chunk rank that does not match the dataspace, a chunk of + 4 GiB or more (0x80000000-sized chunks hung the reader), and v1 B-tree + chunk keys whose offsets are not multiples of the chunk dimensions, + including the keys that only bound a node + (`chunked_read::collect_chunk_info_checked`). + - truncated files (`FormatError::TruncatedFile`, `Superblock::data_end`): + a file shorter than the end of file its superblock records is refused + ("truncated file"), and nothing past that end is read. `File`, + `LazyFile` and `MmapFile` do this. + + Checks newer libhdf5 releases make but HDF5 2.0 does not (bit-field + offsets, the variable-length kind, array sizes) are left out, so files + h5py opens still open. Two libhdf5 checks are skipped on purpose because + clawhdf5 up to v2.7.0 wrote files that fail them without being wrong: + the sign bit of every float at position 63, and a size-0 string type for + an empty-string attribute (new fixtures written by v2.7.0 guard this). + Conformance: 569 -> 571 ok (h5stat_err_refcount.h5, + h5clear_fsm_persist_less.h5), and 17 of the 18 CVE objects now fail as in + libhdf5 (see `docs/known-issues.md` for the one left), as do 10 files + h5py refuses as truncated. Tests: + `header_validation_interop.rs` (h5py writes, the test damages a copy, both + libraries must refuse it), `legacy_writer_files.rs`, and unit tests next + to each check. **Breaking (format crate):** `FormatError` gained + `InvalidObjectHeader`, `InvalidDatatype`, `InvalidChunkDimensions` and + `TruncatedFile`; an exhaustive `match` on it needs the new arms. - `clawhdf5-format` VDS: variable-length and reference data from a source in another file is refused. Those elements are global-heap IDs and object addresses in the source file; copied into the virtual dataset they would diff --git a/docs/known-issues.md b/docs/known-issues.md index 4ea7518..07c888f 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -134,8 +134,30 @@ fill-value item that did is fixed). - N-Bit on 64-bit scale-offset data and some N-Bit parameter layouts fail. - **Filters:** blosc, blosc2, bitshuffle, bzip2, LZF and zfp are not implemented. -- **Header checks:** on 12 CVE datasets libhdf5 rejects a corrupt header and - we read data anyway. We need stricter header checks. +- ~~**Header checks:** on 12 CVE datasets libhdf5 rejects a corrupt header and + we read data anyway. We need stricter header checks.~~ **Fixed + 2026-09-26** (counted again: 18 objects on the CVE corpus that libhdf5 + refuses; some read as wrong data, e.g. a zero chunk dimension read as all + fill values): object headers, datatypes, chunk dimensions and chunk-index + offsets are checked as libhdf5 checks them, and truncated files are + refused. 17 of the 18 now + fail as in libhdf5 (conformance on tank, `conformance/run.sh --no-fetch`, + 2026-09-26: 571 of 697 ok). Still read where libhdf5 refuses: + - `cve-2024-32624.h5` `/Dset_OBJREF`: a dataspace whose storage size + overflows 64 bits. `File::dataset` and `shape()` succeed (libhdf5 + refuses at open); reading the values fails. + - `cve-2020-10810.h5`, `cve-2020-10812.h5` (whole files libhdf5 cannot + open, not among the 18): libhdf5 decodes the superblock extension's File + Space Info and metadata-cache-image messages at open and refuses these + files; we do not decode those messages at open. + - Deliberately not refused, because clawhdf5 up to v2.7.0 wrote them: a + float sign bit position outside the type, and a size-0 string type. + - Not refused because HDF5 2.0 (h5py 3.16) reads them though newer + libhdf5 refuses them: bit-field offset/precision outside the type, an + unknown variable-length kind, an array type whose stored size is not + its element count times its base size. + - (`cve-2024-32616` `/group1/dset3` and `cve-2025-2309`'s `Comp_OBJREF` + attribute are h5py/numpy type-mapping failures, not libhdf5 refusals.) - **Writer:** - Nested groups beyond one level: path-like names are now refused, not created. From a14ccc36bfca304c27ac83b74b9f694782b4cae2 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:17:36 -0500 Subject: [PATCH 09/17] fix(format): limit chunks to 4 GiB only under a v1 B-tree index libhdf5 refuses a chunk of 4 GiB or more only when a version-1 B-tree indexes it (H5D__chunk_init: "chunk size must be < 4GB with v1 b-tree index"). HDF5 2.0 writes larger chunks with layout version 5, and h5py reads them; these were refused. chunk_geometry now takes the layout version and applies the limit to layout version 3 and earlier only. The interop test is ignored by default: h5py writes a 4 GiB chunk and both libraries hold it in memory. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 4 +- crates/clawhdf5-format/src/chunked_read.rs | 94 +++++++++++-------- crates/clawhdf5-format/src/data_read.rs | 2 +- crates/clawhdf5-format/src/error.rs | 2 +- .../tests/header_validation_interop.rs | 44 +++++++++ 5 files changed, 104 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93cf91e..7fdde48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -323,7 +323,9 @@ VAX byte order; libhdf5 ignores it before version 3, and so does this. - chunked layouts (`FormatError::InvalidChunkDimensions`): a zero chunk dimension, a chunk rank that does not match the dataspace, a chunk of - 4 GiB or more (0x80000000-sized chunks hung the reader), and v1 B-tree + 4 GiB or more indexed by a v1 B-tree (layout version 3 or earlier; + 0x80000000-sized chunks hung the reader — layout versions 4 and 5 allow + larger chunks, and HDF5 2.0 writes them), and v1 B-tree chunk keys whose offsets are not multiples of the chunk dimensions, including the keys that only bound a node (`chunked_read::collect_chunk_info_checked`). diff --git a/crates/clawhdf5-format/src/chunked_read.rs b/crates/clawhdf5-format/src/chunked_read.rs index beac35f..7abf021 100644 --- a/crates/clawhdf5-format/src/chunked_read.rs +++ b/crates/clawhdf5-format/src/chunked_read.rs @@ -152,12 +152,15 @@ pub(crate) fn checked_byte_len(elements: u64, elem_size: usize) -> Result Result<(usize, Vec), FormatError> { @@ -180,9 +183,9 @@ pub(crate) fn chunk_geometry( let bytes = spatial .iter() .fold(elem_size as u128, |acc, &c| acc * u128::from(c)); - if bytes > u128::from(u32::MAX) { + if layout_version < 4 && bytes > u128::from(u32::MAX) { return Err(FormatError::InvalidChunkDimensions(format!( - "chunk size must be < 4GB (chunk {spatial:?} of {elem_size}-byte elements)" + "chunk size must be < 4GB with v1 b-tree index (chunk {spatial:?} of {elem_size}-byte elements)" ))); } Ok((rank, spatial.iter().map(|&c| c as usize).collect())) @@ -658,7 +661,7 @@ pub fn list_chunks( .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; // Both v3 and v4 include element size as last dim (rank+1) - let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, dataspace, elem_size)?; + let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, version, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); // Collect chunks based on version and index type @@ -894,12 +897,13 @@ pub fn read_chunked_data_cached( length_size: u8, cache: &ChunkCache, ) -> Result, FormatError> { - let (chunk_dimensions, addr_opt) = match layout { + let (chunk_dimensions, version, addr_opt) = match layout { DataLayout::Chunked { chunk_dimensions, + version, btree_address, .. - } => (chunk_dimensions, *btree_address), + } => (chunk_dimensions, *version, *btree_address), _ => { return Err(FormatError::ChunkedReadError( "expected chunked layout".into(), @@ -911,7 +915,7 @@ pub fn read_chunked_data_cached( .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; let elem_size = datatype.type_size() as usize; - let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, dataspace, elem_size)?; + let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, version, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); // The per-file cache is shared across datasets (and threads); every @@ -1199,12 +1203,13 @@ pub fn read_chunked_data_sweep( cache: &ChunkCache, sweep: &mut SweepContext, ) -> Result, FormatError> { - let (chunk_dimensions, addr_opt) = match layout { + let (chunk_dimensions, version, addr_opt) = match layout { DataLayout::Chunked { chunk_dimensions, + version, btree_address, .. - } => (chunk_dimensions, *btree_address), + } => (chunk_dimensions, *version, *btree_address), _ => { return Err(FormatError::ChunkedReadError( "expected chunked layout".into(), @@ -1216,7 +1221,7 @@ pub fn read_chunked_data_sweep( .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; let elem_size = datatype.type_size() as usize; - let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, dataspace, elem_size)?; + let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, version, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); // The per-file cache is shared across datasets (and threads); every @@ -1335,12 +1340,13 @@ pub fn read_chunked_data_indexed( length_size: u8, cache: &ChunkCache, ) -> Result, FormatError> { - let (chunk_dimensions, addr_opt) = match layout { + let (chunk_dimensions, version, addr_opt) = match layout { DataLayout::Chunked { chunk_dimensions, + version, btree_address, .. - } => (chunk_dimensions, *btree_address), + } => (chunk_dimensions, *version, *btree_address), _ => { return Err(FormatError::ChunkedReadError( "expected chunked layout".into(), @@ -1352,7 +1358,7 @@ pub fn read_chunked_data_indexed( .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; let elem_size = datatype.type_size() as usize; - let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, dataspace, elem_size)?; + let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, version, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); // Chunk index and assembly plan for this dataset, built on first access @@ -1845,31 +1851,41 @@ mod tests { dimensions: dims.to_vec(), max_dimensions: None, }; + for v in [3, 4] { + assert_eq!( + chunk_geometry(&[4, 5, 8], v, &space(&[10, 10]), 8).unwrap(), + (2, vec![4, 5]) + ); + // Rank mismatch. + assert!(matches!( + chunk_geometry(&[4, 8], v, &space(&[10, 10]), 8), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("doesn't match") + )); + // Zero dimension (a layout built in memory, bypassing the parser). + assert!(matches!( + chunk_geometry(&[4, 0, 8], v, &space(&[10, 10]), 8), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("must be > 0") + )); + assert!(chunk_geometry(&[0xFFFF_FFFF, 1], v, &space(&[10]), 1).is_ok()); + } + // With a v1 B-tree index (layout version 3) the largest chunk is + // 4 GiB - 1 bytes: 0x80000000 x 4-byte elements (8 GiB) is refused. + // These dims used to hang the reader. + assert!(matches!( + chunk_geometry(&[0x8000_0000, 4], 3, &space(&[10]), 4), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("4GB with v1 b-tree") + )); + assert!(matches!( + chunk_geometry(&[0xFFFF_FFFF, 0xFFFF_FFFF, 1], 3, &space(&[10, 10]), 1), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("4GB with v1 b-tree") + )); + // The other chunk indexes (layout version 4, and 5, which is read as + // 4) allow chunks of 4 GiB and more; HDF5 2.0 writes them. assert_eq!( - chunk_geometry(&[4, 5, 8], &space(&[10, 10]), 8).unwrap(), - (2, vec![4, 5]) + chunk_geometry(&[0x2000_0001, 8], 4, &space(&[10]), 8).unwrap(), + (1, vec![0x2000_0001]) ); - // Rank mismatch. - assert!(matches!( - chunk_geometry(&[4, 8], &space(&[10, 10]), 8), - Err(FormatError::InvalidChunkDimensions(m)) if m.contains("doesn't match") - )); - // Zero dimension (a layout built in memory, bypassing the parser). - assert!(matches!( - chunk_geometry(&[4, 0, 8], &space(&[10, 10]), 8), - Err(FormatError::InvalidChunkDimensions(m)) if m.contains("must be > 0") - )); - // 0x80000000 x 4-byte elements is 8 GiB; the largest allowed chunk - // is 4 GiB - 1 bytes. These dims used to hang the reader. - assert!(matches!( - chunk_geometry(&[0x8000_0000, 4], &space(&[10]), 4), - Err(FormatError::InvalidChunkDimensions(m)) if m.contains("4GB") - )); - assert!(matches!( - chunk_geometry(&[0xFFFF_FFFF, 0xFFFF_FFFF, 1], &space(&[10, 10]), 1), - Err(FormatError::InvalidChunkDimensions(m)) if m.contains("4GB") - )); - assert!(chunk_geometry(&[0xFFFF_FFFF, 1], &space(&[10]), 1).is_ok()); + assert!(chunk_geometry(&[0xFFFF_FFFF, 0xFFFF_FFFF, 1], 4, &space(&[10, 10]), 1).is_ok()); } fn make_f64_type() -> Datatype { diff --git a/crates/clawhdf5-format/src/data_read.rs b/crates/clawhdf5-format/src/data_read.rs index 91b739a..8583473 100644 --- a/crates/clawhdf5-format/src/data_read.rs +++ b/crates/clawhdf5-format/src/data_read.rs @@ -360,7 +360,7 @@ pub fn read_raw_data_selection( chunk_index_type, .. } => { - crate::chunked_read::chunk_geometry(chunk_dimensions, dataspace, elem_size)?; + crate::chunked_read::chunk_geometry(chunk_dimensions, *version, dataspace, elem_size)?; // For chunked data, only read chunks that intersect the selection let chunk_dims: Vec = chunk_dimensions.iter().map(|&d| d as u64).collect(); let rank = dims.len(); diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index b3cfd65..755a998 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -212,7 +212,7 @@ pub enum FormatError { InvalidDatatype(String), /// A chunked layout whose chunk dimensions libhdf5 refuses: a zero /// dimension, a rank that does not match the dataspace, or a chunk of - /// 4 GiB or more. + /// 4 GiB or more indexed by a version-1 B-tree. InvalidChunkDimensions(String), /// The superblock's end-of-file address lies past the end of the file: /// the file was truncated (libhdf5 refuses to open it). diff --git a/crates/clawhdf5/tests/header_validation_interop.rs b/crates/clawhdf5/tests/header_validation_interop.rs index 14dc9d7..daa3e19 100644 --- a/crates/clawhdf5/tests/header_validation_interop.rs +++ b/crates/clawhdf5/tests/header_validation_interop.rs @@ -269,3 +269,47 @@ save("float_overlap", bad) ], ); } + +/// Runs a Python script (with `h5py`, `numpy as np` and `struct` imported, +/// `d` the output directory) and fails the test if it fails. +fn run_python(dir: &Path, body: &str) { + let script = format!( + "import h5py, numpy as np, struct, os\nd = \"{}\"\n{body}", + dir.display() + ); + let out = Command::new(python()) + .args(["-c", &script]) + .output() + .expect("failed to run python"); + assert!( + out.status.success(), + "python failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); +} + +/// libhdf5 limits a chunk to under 4 GiB only when a version-1 B-tree +/// indexes it; HDF5 2.0 writes larger chunks with layout version 5 (libver +/// v200), and h5py reads them. These were refused as "chunk size must be < +/// 4GB". Ignored by default: h5py writes a 4 GiB chunk and both libraries +/// hold it in memory (about 9 GiB in all). +#[test] +#[ignore = "writes and reads a 4 GiB chunk (about 9 GiB of memory)"] +fn chunks_of_4_gib_and_more_read_with_layout_v5() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + run_python( + dir.path(), + r#" +with h5py.File(os.path.join(d, "big.h5"), "w", libver=("v200", "v200")) as f: + ds = f.create_dataset("d", shape=(10,), maxshape=(None,), chunks=(2**29 + 1,), + dtype=">()); +} From 17f09375ad457ef01c13f636e8a0f6a028765a26 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:21:20 -0500 Subject: [PATCH 10/17] fix(format): refuse to write datatypes the reader refuses The reader now refuses a compound with a repeated field name or no fields and an enum member with an empty name, as libhdf5 does, but the writer still wrote them: CompoundTypeBuilder and EnumTypeBuilder build them without complaint, so clawhdf5 wrote files it could not read back. They were never valid HDF5; h5py refuses them. Datatype::check_encodable, which FileWriter::finish runs on every dataset and attribute type, now parses the type's own encoding back and refuses one the reader refuses, with the reader's reason. That keeps the writer in step with every reader check, not only these three. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 7 +++ crates/clawhdf5-format/src/datatype.rs | 22 ++++++-- crates/clawhdf5/tests/legacy_writer_files.rs | 58 ++++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fdde48..e555823 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -333,6 +333,13 @@ a file shorter than the end of file its superblock records is refused ("truncated file"), and nothing past that end is read. `File`, `LazyFile` and `MmapFile` do this. + - the writer: `FileWriter::finish()` / `FileBuilder::finish()` refuse a + datatype the reader would refuse (`FormatError::SerializationError`, + "datatype cannot be written: ..."), such as a compound with a repeated + field name or no fields, or an enum member with an empty name + (`CompoundTypeBuilder` and `EnumTypeBuilder` build them without + complaint). These were never valid HDF5 — h5py refuses them — and + clawhdf5 wrote them, which made files it could not read back. Checks newer libhdf5 releases make but HDF5 2.0 does not (bit-field offsets, the variable-length kind, array sizes) are left out, so files diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index 980575d..703f44a 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -1141,9 +1141,23 @@ impl Datatype { } /// Check that this datatype can be written: every part of it has an - /// on-disk encoding. [`Self::serialize`] cannot report errors, so the - /// writer calls this first. + /// on-disk encoding, and the encoding is one the reader (and libhdf5) + /// accepts. [`Self::serialize`] cannot report errors, so the writer calls + /// this first. A compound with no fields or a repeated field name, or an + /// enum member with an empty name, is refused here: libhdf5 and h5py + /// refuse such types, and so does [`Self::parse`], so writing one made a + /// file that could not be read back. pub fn check_encodable(&self) -> Result<(), FormatError> { + self.check_encodable_parts()?; + Self::parse(&self.serialize()).map_err(|e| { + FormatError::SerializationError(format!( + "datatype cannot be written: HDF5 readers refuse it ({e})" + )) + })?; + Ok(()) + } + + fn check_encodable_parts(&self) -> Result<(), FormatError> { match self { Datatype::Opaque { tag, .. } if opaque_tag_text(tag).len() > MAX_OPAQUE_TAG_LEN => { Err(FormatError::SerializationError(format!( @@ -1156,10 +1170,10 @@ impl Datatype { )), Datatype::Compound { members, .. } => members .iter() - .try_for_each(|m| m.datatype.check_encodable()), + .try_for_each(|m| m.datatype.check_encodable_parts()), Datatype::Enumeration { base_type, .. } | Datatype::VariableLength { base_type, .. } - | Datatype::Array { base_type, .. } => base_type.check_encodable(), + | Datatype::Array { base_type, .. } => base_type.check_encodable_parts(), _ => Ok(()), } } diff --git a/crates/clawhdf5/tests/legacy_writer_files.rs b/crates/clawhdf5/tests/legacy_writer_files.rs index 3e8a3b9..e826064 100644 --- a/crates/clawhdf5/tests/legacy_writer_files.rs +++ b/crates/clawhdf5/tests/legacy_writer_files.rs @@ -81,3 +81,61 @@ fn every_object_of_a_v2_7_0_file_reads() { let paged = File::open(fixture("written_by_v2_7_0_paged.h5")).unwrap(); assert_eq!(paged.dataset("d").unwrap().read_f32().unwrap(), [1.0, 2.0]); } + +/// Datatypes libhdf5 (and this reader) refuse — a compound with a repeated +/// field name or no fields, an enum member with an empty name — must not be +/// written: they made files that did not read back. Valid neighbours of each +/// still write and read back. +#[test] +fn datatypes_the_reader_refuses_are_not_written() { + use clawhdf5::{CompoundTypeBuilder, EnumTypeBuilder, FileBuilder}; + + let write_compound = |dt, raw: Vec| { + let mut b = FileBuilder::new(); + b.create_dataset("d").with_compound_data(dt, raw, 1); + b.finish() + }; + let write_enum = |dt| { + let mut b = FileBuilder::new(); + b.create_dataset("d").with_enum_u8_data(dt, &[0, 1]); + b.finish() + }; + let refused = |r: Result, clawhdf5::Error>, what: &str| { + let err = r.expect_err("written"); + let msg = err.to_string(); + assert!(msg.contains("datatype cannot be written"), "{msg}"); + assert!(msg.contains(what), "{msg}"); + }; + + let dup = CompoundTypeBuilder::new() + .f64_field("x") + .f64_field("x") + .build(); + refused( + write_compound(dup, vec![0; 16]), + "duplicated compound field name 'x'", + ); + refused( + write_compound(CompoundTypeBuilder::new().build(), vec![]), + "invalid", + ); + let empty_name = EnumTypeBuilder::u8_based() + .u8_value("A", 0) + .u8_value("", 1) + .build(); + refused(write_enum(empty_name), "0 length enum name"); + + let ok = CompoundTypeBuilder::new() + .f64_field("x") + .f64_field("y") + .build(); + let bytes = write_compound(ok, vec![0; 16]).unwrap(); + let file = File::from_bytes(bytes).unwrap(); + file.dataset("d").unwrap().dtype().unwrap(); + let ok = EnumTypeBuilder::u8_based() + .u8_value("A", 0) + .u8_value("B", 1) + .build(); + let file = File::from_bytes(write_enum(ok).unwrap()).unwrap(); + file.dataset("d").unwrap().dtype().unwrap(); +} From 92386056610508dd0666ffc3e79e6b1446404ffb Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:24:17 -0500 Subject: [PATCH 11/17] fix(format): measure compound members by their stored size The compound overlap check measured each earlier member with Datatype::type_size, which is a fixed 16 for a variable-length type. On disk a VL member takes 4 + offset size + 4 bytes, 12 in a file with 4-byte offsets, so a member right after one was refused as "member overlaps with previous member" (and with the type, every attribute of the object). libhdf5 measures members by their decoded, stored size (times a v1 member's array dimensions); so does this now. Reading VL values in such files is a separate, older gap, now recorded in known-issues. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 3 ++ crates/clawhdf5-format/src/datatype.rs | 28 ++++++---- .../tests/header_validation_interop.rs | 51 +++++++++++++++++++ docs/known-issues.md | 6 +++ 4 files changed, 78 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e555823..c0ff1e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -321,6 +321,9 @@ half its bits unused (`Datatype::parse_in_header`, `Datatype::check_unused_bits`). A v1/v2 float's class bit 6 was read as VAX byte order; libhdf5 ignores it before version 3, and so does this. + The overlap check measures each earlier member by its stored size, as + libhdf5 does, so a variable-length member (4 + offset size + 4 bytes) + in a file with 4-byte offsets does not overlap the member after it. - chunked layouts (`FormatError::InvalidChunkDimensions`): a zero chunk dimension, a chunk rank that does not match the dataspace, a chunk of 4 GiB or more indexed by a v1 B-tree (layout version 3 or earlier; diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index 703f44a..12e427f 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -215,11 +215,6 @@ fn stored_type_size(data: &[u8], pos: usize) -> Result { Ok(LittleEndian::read_u32(&data[pos + 4..pos + 8])) } -/// A compound member's size in the compound, as libhdf5 counts it. -fn member_size(dt: &Datatype) -> u64 { - u64::from(dt.type_size()) -} - /// libhdf5 refuses an array type of more than `H5S_MAX_RANK` (32) /// dimensions. fn check_array_rank(ndims: usize) -> Result<(), FormatError> { @@ -543,11 +538,17 @@ impl Datatype { return Err(invalid("invalid number of members: 0")); } let mut members: Vec = Vec::with_capacity(num_members as usize); + // Each member's size in the compound as libhdf5 decodes it: + // its stored size, times a v1 member's array dimensions. A + // variable-length member takes 4 + offset size + 4 bytes on + // disk, not the 16 of `Datatype::type_size`. + let mut member_sizes: Vec = Vec::with_capacity(num_members as usize); // libhdf5 checks each member as it is decoded: it must fit in // the compound (by its own stored size, before a v1 member's // array dimensions are applied), and must not repeat a name // or overlap an earlier member (by its final size). let check_member = |members: &[CompoundMember], + member_sizes: &[u64], name: &str, byte_offset: u64, stored_size: u32, @@ -565,9 +566,8 @@ impl Datatype { ))); } let end = byte_offset + final_size; - if members.iter().any(|m| { - let m_end = m.byte_offset + member_size(&m.datatype); - byte_offset < m_end && m.byte_offset < end + if members.iter().zip(member_sizes).any(|(m, &m_size)| { + byte_offset < m.byte_offset + m_size && m.byte_offset < end }) { return Err(invalid("member overlaps with previous member")); } @@ -588,13 +588,16 @@ impl Datatype { let (member_dt, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?; pos += consumed; + let final_size = u64::from(stored_size); check_member( &members, + &member_sizes, &name, byte_offset, stored_size, - member_size(&member_dt), + final_size, )?; + member_sizes.push(final_size); members.push(CompoundMember { name, byte_offset, @@ -652,6 +655,9 @@ impl Datatype { let (mut member_dt, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?; pos += consumed; + let final_size = array_dims.iter().fold(u64::from(stored_size), |a, &d| { + a.saturating_mul(u64::from(d)) + }); if !array_dims.is_empty() { member_dt = Datatype::Array { base_type: Box::new(member_dt), @@ -660,11 +666,13 @@ impl Datatype { } check_member( &members, + &member_sizes, &name, byte_offset, stored_size, - member_size(&member_dt), + final_size, )?; + member_sizes.push(final_size); members.push(CompoundMember { name, byte_offset, diff --git a/crates/clawhdf5/tests/header_validation_interop.rs b/crates/clawhdf5/tests/header_validation_interop.rs index daa3e19..a2cebf5 100644 --- a/crates/clawhdf5/tests/header_validation_interop.rs +++ b/crates/clawhdf5/tests/header_validation_interop.rs @@ -313,3 +313,54 @@ with h5py.File(os.path.join(d, "big.h5"), "r") as f: let values = file.dataset("d").unwrap().read_f64().unwrap(); assert_eq!(values, (0..10).map(f64::from).collect::>()); } + +/// A variable-length compound member takes 4 + offset size + 4 bytes, which +/// is 12 in a file with 4-byte offsets, and libhdf5 checks for overlapping +/// members with that stored size. A member right after one was refused as +/// "member overlaps with previous member" (the check took the 16 bytes of +/// an 8-byte-offset file), and with it every attribute of the object. +#[test] +fn variable_length_compound_members_in_files_with_4_byte_offsets() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + run_python( + dir.path(), + r#" +from h5py import h5f, h5p +dt = np.dtype([("s", h5py.string_dtype()), ("i", " Date: Sat, 26 Sep 2026 01:25:43 -0500 Subject: [PATCH 12/17] fix(format): read layout v4 chunk dimensions of any width from 1 to 8 bytes A version-4 layout stores every chunk dimension in the fewest bytes that hold the largest one (H5D__chunk_set_sizes: (log2(dim) + 8) / 8), so a chunk dimension of 65 536 to 16 777 215 takes 3 bytes. Only widths 1, 2, 4 and 8 were decoded; an h5py file with chunks=(70000,) and libver='latest' failed with UnexpectedEof. Widths 1-8 are decoded now; 0 and more than 8 are refused with libhdf5's "encoded chunk dimension size is too large", and a dimension past u32 is refused, not truncated. The review asked for libhdf5's check that the stored width matches the one computed from the dimensions. HDF5 2.0.0 (h5py 3.16) refuses any mismatch, but HDFGroup/hdf5@e124c36 ("Allow reading of files with chunk dimensions encoded using more bytes than necessary", 2026-06-05) relaxed it to refusing only a width too small for the dimensions, which cannot happen once the dimensions have been decoded from that width. Follow current libhdf5: a wider-than-needed encoding is read. clawhdf5's own writer produces such layouts (the next commit fixes that). Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 10 ++ crates/clawhdf5-format/src/data_layout.rs | 100 ++++++++++++------ .../clawhdf5/tests/h5py_chunked_read_tests.rs | 38 +++++++ 3 files changed, 114 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0ff1e2..b3afc14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -359,6 +359,16 @@ to each check. **Breaking (format crate):** `FormatError` gained `InvalidObjectHeader`, `InvalidDatatype`, `InvalidChunkDimensions` and `TruncatedFile`; an exhaustive `match` on it needs the new arms. +- **Chunked datasets whose chunk dimensions take 3, 5, 6 or 7 bytes did not + open.** A version-4 layout (`libver="latest"`) stores each chunk dimension + in the fewest bytes that hold the largest one, so a chunk dimension from + 65 536 to 16 777 215 (e.g. h5py `chunks=(70000,)`) takes 3 bytes; only 1, 2, + 4 and 8 were read, and the rest failed with `UnexpectedEof`. Widths 1-8 are + read now, and 0 or more than 8 is refused as libhdf5 refuses it. A width + larger than needed is accepted: HDF5 2.0.0 refuses one ("stored chunk + dimension encoding length does not match"), but libhdf5 since + HDFGroup/hdf5@e124c36 (2026-06-05) reads it, and clawhdf5 itself wrote such + layouts. - `clawhdf5-format` VDS: variable-length and reference data from a source in another file is refused. Those elements are global-heap IDs and object addresses in the source file; copied into the virtual dataset they would diff --git a/crates/clawhdf5-format/src/data_layout.rs b/crates/clawhdf5-format/src/data_layout.rs index 54122fc..59a9065 100644 --- a/crates/clawhdf5-format/src/data_layout.rs +++ b/crates/clawhdf5-format/src/data_layout.rs @@ -540,43 +540,30 @@ impl DataLayout { )); } - // dimension sizes + // Each dimension takes 1 to 8 bytes (libhdf5 writes the + // fewest that hold the largest one, so 3, 5, 6 and 7 occur: + // a chunk dimension of 70 000 takes 3). libhdf5 refuses 0 + // and more than 8. + if dim_size_encoded_length == 0 || dim_size_encoded_length > 8 { + return Err(FormatError::InvalidChunkDimensions( + "encoded chunk dimension size is too large".into(), + )); + } ensure_len(data, p, dimensionality * dim_size_encoded_length)?; let mut chunk_dimensions = Vec::with_capacity(dimensionality); for _ in 0..dimensionality { - let val = match dim_size_encoded_length { - 1 => data[p] as u32, - 2 => u16::from_le_bytes([data[p], data[p + 1]]) as u32, - 4 => u32::from_le_bytes([data[p], data[p + 1], data[p + 2], data[p + 3]]), - 8 => { - // V4 chunked encodes dimension sizes as 8 bytes, but - // our ChunkedStorageV4 stores them as u32. We read only - // the low 4 bytes (little-endian). This silently - // truncates dimensions > 4 GiB, which are not expected - // in practice (HDF5 chunk dimensions are always small). - // If the high bytes are non-zero, the file is malformed - // or uses dimensions we cannot represent. - let high = u32::from_le_bytes([ - data[p + 4], - data[p + 5], - data[p + 6], - data[p + 7], - ]); - if high != 0 { - return Err(FormatError::UnexpectedEof { - expected: p + 8, - available: data.len(), - }); - } - u32::from_le_bytes([data[p], data[p + 1], data[p + 2], data[p + 3]]) - } - _ => { - return Err(FormatError::UnexpectedEof { - expected: p + dim_size_encoded_length, - available: data.len(), - }); - } - }; + let val = data[p..p + dim_size_encoded_length] + .iter() + .rev() + .fold(0u64, |acc, &b| (acc << 8) | u64::from(b)); + // Chunk dimensions are held as u32; HDF5 2.0 can write + // larger ones (layout version 5), which are refused + // rather than truncated. + let val = u32::try_from(val).map_err(|_| { + FormatError::InvalidChunkDimensions(format!( + "chunk dimension {val} is larger than 2^32 - 1, which is not supported" + )) + })?; chunk_dimensions.push(val); p += dim_size_encoded_length; } @@ -838,6 +825,51 @@ mod tests { )); } + /// A v4 chunked layout (fixed array index) whose `dims` are each + /// encoded in `width` bytes. + fn v4_chunked_msg(width: u8, dims: &[u64]) -> Vec { + let mut m = vec![4u8, 2, 0, dims.len() as u8, width]; + for &d in dims { + m.extend_from_slice(&d.to_le_bytes()[..width.min(8) as usize]); + } + m.push(3); // fixed array index + m.push(0); // page bits + m.extend_from_slice(&0x1000u64.to_le_bytes()); + m + } + + #[test] + fn v4_chunk_dimensions_take_1_to_8_bytes() { + // libhdf5 encodes each dimension in the fewest bytes that hold the + // largest: a chunk dimension of 70 000 takes 3, and 3, 5, 6 and 7 + // were refused ("UnexpectedEof"). + for width in 1..=8u8 { + let dims = [if width >= 3 { 70_000 } else { 200 }, 8]; + let layout = DataLayout::parse(&v4_chunked_msg(width, &dims), 8, 8) + .unwrap_or_else(|e| panic!("width {width}: {e:?}")); + assert!( + matches!(&layout, DataLayout::Chunked { chunk_dimensions, .. } + if chunk_dimensions.iter().map(|&d| u64::from(d)).eq(dims)), + "width {width}: {layout:?}" + ); + } + // libhdf5 refuses 0 and more than 8 bytes. + for width in [0u8, 9] { + assert_eq!( + DataLayout::parse(&v4_chunked_msg(width, &[4, 8]), 8, 8).unwrap_err(), + FormatError::InvalidChunkDimensions( + "encoded chunk dimension size is too large".into() + ) + ); + } + // A dimension past u32 cannot be represented and is refused, not + // truncated. + assert!(matches!( + DataLayout::parse(&v4_chunked_msg(5, &[1 << 32, 8]), 8, 8), + Err(FormatError::InvalidChunkDimensions(m)) if m.contains("2^32") + )); + } + #[test] fn v1v2_rejects_bad_class_dimensionality_and_truncation() { assert_eq!( diff --git a/crates/clawhdf5/tests/h5py_chunked_read_tests.rs b/crates/clawhdf5/tests/h5py_chunked_read_tests.rs index 2b9e902..40bfbf5 100644 --- a/crates/clawhdf5/tests/h5py_chunked_read_tests.rs +++ b/crates/clawhdf5/tests/h5py_chunked_read_tests.rs @@ -343,3 +343,41 @@ print("OK") let want: Vec = line[990..].iter().flat_map(|v| v.to_le_bytes()).collect(); assert_eq!(tail, want); } + +// --------------------------------------------------------------------------- +// Chunk dimension widths in layout version 4 +// --------------------------------------------------------------------------- + +/// A version-4 layout stores every chunk dimension in the fewest bytes that +/// hold the largest one (the element size included). A chunk dimension of +/// 70 000 takes 3 bytes and 2^32 + 1 elements would take 5; widths other +/// than 1, 2, 4 and 8 were refused, so these h5py files did not open. +#[test] +fn h5py_layout_v4_chunk_dimensions_of_3_bytes_read() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("enc3.h5"); + let p = path.display().to_string(); + run_python(&format!( + r#" +import h5py, numpy as np +with h5py.File("{p}", "w", libver="latest") as f: + f.create_dataset("single", data=np.arange(70000, dtype=" 0 or raw.find(bytes([4, 2, 1, 2, 3])) > 0 +"# + )); + let file = File::open(&path).unwrap(); + let single = file.dataset("single").unwrap().read_u64().unwrap(); + assert!(single.iter().copied().eq(0..70000), "single"); + let ea = file.dataset("ea").unwrap().read_f64().unwrap(); + assert_eq!(ea, (0..10).map(f64::from).collect::>()); + let fa = file.dataset("fa").unwrap().read_i32().unwrap(); + assert!(fa.iter().copied().eq(0..3 * 70000), "fa"); +} From afae86f3ea6fb3a7fea7fca26daced0cd5d651a6 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:27:22 -0500 Subject: [PATCH 13/17] fix(format): write layout v4 chunk dimensions in the fewest bytes libhdf5 encodes a version-4 layout's chunk dimensions in (log2(max) + 8) / 8 bytes, and HDF5 2.0.0 (h5py 3.16) refuses any other width: "stored chunk dimension encoding length does not match value calculated from chunk dimensions". The writer rounded 3 bytes up to 4, so h5py could not open a dataset we wrote with a chunk dimension from 65 536 to 16 777 215, for every chunk index (single chunk, fixed and extensible array, v2 B-tree). The three encoders now share push_v4_chunk_dims, which writes the exact width. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 7 ++ crates/clawhdf5-format/src/chunked_write.rs | 83 +++++--------------- crates/clawhdf5-format/src/ea_writer.rs | 34 +------- crates/clawhdf5/tests/chunk_index_interop.rs | 25 ++++++ 4 files changed, 57 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3afc14..c20f408 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -197,6 +197,13 @@ takes `--f32`; it had kept printing "f32" after the default changed. ### Interop +- **h5py could not open chunked datasets we wrote with a chunk dimension + from 65 536 to 16 777 215.** A version-4 layout must store its chunk + dimensions in the fewest bytes that hold the largest (3 for 70 000); + the writer rounded 3 up to 4, and HDF5 2.0.0 (h5py 3.16) refuses that + ("stored chunk dimension encoding length does not match value calculated + from chunk dimensions"). Newer libhdf5 and clawhdf5 read those files; new + files use the exact width. Test: `we_write_chunk_dimensions_in_the_fewest_bytes`. - **Conformance sweep in the repo** (`conformance/`, report in `CONFORMANCE.md`). `conformance/run.sh` fetches eight public HDF5 corpora pinned by commit (libhdf5's test files, the HDF Group's CVE reproducers, diff --git a/crates/clawhdf5-format/src/chunked_write.rs b/crates/clawhdf5-format/src/chunked_write.rs index ff5081e..dcd0e43 100644 --- a/crates/clawhdf5-format/src/chunked_write.rs +++ b/crates/clawhdf5-format/src/chunked_write.rs @@ -383,39 +383,7 @@ fn serialize_v4_single_chunk( let ndims = chunk_dims.len() as u8 + 1; buf.push(ndims); - // dim_size_encoded_length: how many bytes per dimension - // We need to figure out the minimum encoding width - let max_dim = chunk_dims - .iter() - .map(|&d| d as u64) - .chain(core::iter::once(element_size as u64)) - .max() - .unwrap_or(1); - let dim_encoded_len: u8 = if max_dim <= 0xFF { - 1 - } else if max_dim <= 0xFFFF { - 2 - } else { - 4 - }; - buf.push(dim_encoded_len); - - // dimension sizes (chunk dims + element size) - for &d in chunk_dims { - match dim_encoded_len { - 1 => buf.push(d as u8), - 2 => buf.extend_from_slice(&(d as u16).to_le_bytes()), - 4 => buf.extend_from_slice(&d.to_le_bytes()), - _ => {} - } - } - // Element size dimension - match dim_encoded_len { - 1 => buf.push(element_size as u8), - 2 => buf.extend_from_slice(&(element_size as u16).to_le_bytes()), - 4 => buf.extend_from_slice(&element_size.to_le_bytes()), - _ => {} - } + push_v4_chunk_dims(&mut buf, chunk_dims, element_size); // chunk index type = 1 (single chunk) buf.push(1); @@ -465,6 +433,25 @@ fn serialize_v4_fixed_array( /// The part of a v4 chunked layout message before the chunk index type: /// version, class, flags and the chunk dimensions (plus the element size). +/// Append a v4 layout's dimension width and its dimensions (the chunk +/// dimensions, then the element size). Each takes the fewest bytes that hold +/// the largest, as libhdf5 computes it (`H5D__chunk_set_sizes`: +/// `(log2(dim) + 8) / 8`); HDF5 2.0.0 refuses any other width. +pub(crate) fn push_v4_chunk_dims(buf: &mut Vec, chunk_dims: &[u32], element_size: u32) { + let max_dim = chunk_dims + .iter() + .copied() + .chain(core::iter::once(element_size)) + .max() + .unwrap_or(1) + .max(1); + let width = (32 - max_dim.leading_zeros()).div_ceil(8) as usize; + buf.push(width as u8); + for &d in chunk_dims.iter().chain(core::iter::once(&element_size)) { + buf.extend_from_slice(&d.to_le_bytes()[..width]); + } +} + fn layout_v4_chunked_prefix(chunk_dims: &[u32], element_size: u32) -> Vec { let mut buf = Vec::new(); buf.push(4); // version @@ -476,35 +463,7 @@ fn layout_v4_chunked_prefix(chunk_dims: &[u32], element_size: u32) -> Vec { let ndims = chunk_dims.len() as u8 + 1; buf.push(ndims); - let max_dim = chunk_dims - .iter() - .map(|&d| d as u64) - .chain(core::iter::once(element_size as u64)) - .max() - .unwrap_or(1); - let dim_encoded_len: u8 = if max_dim <= 0xFF { - 1 - } else if max_dim <= 0xFFFF { - 2 - } else { - 4 - }; - buf.push(dim_encoded_len); - - for &d in chunk_dims { - match dim_encoded_len { - 1 => buf.push(d as u8), - 2 => buf.extend_from_slice(&(d as u16).to_le_bytes()), - 4 => buf.extend_from_slice(&d.to_le_bytes()), - _ => {} - } - } - match dim_encoded_len { - 1 => buf.push(element_size as u8), - 2 => buf.extend_from_slice(&(element_size as u16).to_le_bytes()), - 4 => buf.extend_from_slice(&element_size.to_le_bytes()), - _ => {} - } + push_v4_chunk_dims(&mut buf, chunk_dims, element_size); buf } diff --git a/crates/clawhdf5-format/src/ea_writer.rs b/crates/clawhdf5-format/src/ea_writer.rs index e0f0375..f669534 100644 --- a/crates/clawhdf5-format/src/ea_writer.rs +++ b/crates/clawhdf5-format/src/ea_writer.rs @@ -7,7 +7,9 @@ extern crate alloc; use alloc::{vec, vec::Vec}; use crate::checksum::jenkins_lookup3; -use crate::chunked_write::{WrittenChunk, filtered_chunk_size_len, push_addr, push_index_element}; +use crate::chunked_write::{ + WrittenChunk, filtered_chunk_size_len, push_addr, push_index_element, push_v4_chunk_dims, +}; /// Serialize a v4 Extensible Array layout message. pub(crate) fn serialize_v4_extensible_array( @@ -24,35 +26,7 @@ pub(crate) fn serialize_v4_extensible_array( let ndims = chunk_dims.len() as u8 + 1; buf.push(ndims); - let max_dim = chunk_dims - .iter() - .map(|&d| d as u64) - .chain(core::iter::once(element_size as u64)) - .max() - .unwrap_or(1); - let dim_encoded_len: u8 = if max_dim <= 0xFF { - 1 - } else if max_dim <= 0xFFFF { - 2 - } else { - 4 - }; - buf.push(dim_encoded_len); - - for &d in chunk_dims { - match dim_encoded_len { - 1 => buf.push(d as u8), - 2 => buf.extend_from_slice(&(d as u16).to_le_bytes()), - 4 => buf.extend_from_slice(&d.to_le_bytes()), - _ => unreachable!("unexpected dim_encoded_len: {dim_encoded_len}"), - } - } - match dim_encoded_len { - 1 => buf.push(element_size as u8), - 2 => buf.extend_from_slice(&(element_size as u16).to_le_bytes()), - 4 => buf.extend_from_slice(&element_size.to_le_bytes()), - _ => unreachable!("unexpected dim_encoded_len: {dim_encoded_len}"), - } + push_v4_chunk_dims(&mut buf, chunk_dims, element_size); // chunk index type = 4 (Extensible Array) buf.push(4); diff --git a/crates/clawhdf5/tests/chunk_index_interop.rs b/crates/clawhdf5/tests/chunk_index_interop.rs index b72b7e2..3ff0420 100644 --- a/crates/clawhdf5/tests/chunk_index_interop.rs +++ b/crates/clawhdf5/tests/chunk_index_interop.rs @@ -515,6 +515,31 @@ fn we_write_maxshape_larger_than_shape() { check_we_write(&cases); } +/// A version-4 layout must encode every chunk dimension in the fewest bytes +/// that hold the largest one, as libhdf5 does: HDF5 2.0.0 (h5py 3.16) +/// refuses a wider encoding ("stored chunk dimension encoding length does +/// not match value calculated from chunk dimensions"). We rounded 3 bytes +/// up to 4, so h5py could not open any dataset we wrote with a chunk +/// dimension from 65 536 to 16 777 215. +#[test] +fn we_write_chunk_dimensions_in_the_fewest_bytes() { + const U: u64 = u64::MAX; + let mut cases = vec![ + // Single chunk, Fixed Array, Extensible Array, v2 B-tree. + wcase("single_70000", &[70_000], &[70_000], None), + wcase("fa_70000", &[140_000], &[70_000], None), + wcase("ea_70000", &[140_000], &[70_000], Some(&[U])), + wcase("bt2_70000", &[2, 70_000], &[1, 70_000], Some(&[U, U])), + // 2 bytes and 1 byte still, with the element size (4) the largest. + wcase("fa_300", &[600], &[300], None), + wcase("fa_3", &[6], &[3], None), + ]; + let mut filtered = wcase("single_70000_deflate", &[70_000], &[70_000], None); + filtered.deflate = true; + cases.push(filtered); + check_we_write(&cases); +} + /// More than one unlimited dimension needs a version-2 B-tree chunk index, /// as the library uses; an Extensible Array for `(None, None)` made libhdf5 /// refuse the whole file ("already found unlimited dimension"). From dd40bea467e065490dc845a393e339b2b1824151 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:29:26 -0500 Subject: [PATCH 14/17] fix(format): refuse a chunk layout whose element size is not the datatype's A chunked layout records the element size as its last dimension, and libhdf5 refuses a dataset whose datatype has another size (H5D__chunk_set_sizes: "stored datatype size in chunk layout does not match datatype description"). clawhdf5 ignored the recorded size and read the chunks anyway, for v3 and v4 layouts. The check runs on every chunked read (read_chunked_data*, read_raw_data_selection) and compares against the stored size: a variable-length element is 4 + offset size + 4 bytes, not Datatype::type_size's 16. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 4 +- crates/clawhdf5-format/src/chunked_read.rs | 51 ++++++++++ crates/clawhdf5-format/src/data_read.rs | 1 + crates/clawhdf5-format/src/error.rs | 5 +- .../tests/header_validation_interop.rs | 96 +++++++++++++++++++ 5 files changed, 154 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c20f408..97bfb97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -335,7 +335,9 @@ dimension, a chunk rank that does not match the dataspace, a chunk of 4 GiB or more indexed by a v1 B-tree (layout version 3 or earlier; 0x80000000-sized chunks hung the reader — layout versions 4 and 5 allow - larger chunks, and HDF5 2.0 writes them), and v1 B-tree + larger chunks, and HDF5 2.0 writes them), an element size in the + layout that differs from the datatype's stored size (the chunks were + laid out with the wrong element size), and v1 B-tree chunk keys whose offsets are not multiples of the chunk dimensions, including the keys that only bound a node (`chunked_read::collect_chunk_info_checked`). diff --git a/crates/clawhdf5-format/src/chunked_read.rs b/crates/clawhdf5-format/src/chunked_read.rs index 7abf021..afff80d 100644 --- a/crates/clawhdf5-format/src/chunked_read.rs +++ b/crates/clawhdf5-format/src/chunked_read.rs @@ -191,6 +191,53 @@ pub(crate) fn chunk_geometry( Ok((rank, spatial.iter().map(|&c| c as usize).collect())) } +/// The size of one element of `dt` as stored in the file: a +/// variable-length element is its length (4), a global heap address +/// (`offset_size`) and an index (4), not the 16 of [`Datatype::type_size`]. +fn stored_element_size(dt: &Datatype, offset_size: u8) -> u64 { + match dt { + Datatype::VariableLength { .. } => 8 + u64::from(offset_size), + Datatype::Array { + base_type, + dimensions, + } => dimensions + .iter() + .fold(stored_element_size(base_type, offset_size), |acc, &d| { + acc.saturating_mul(u64::from(d)) + }), + _ => u64::from(dt.type_size()), + } +} + +/// A chunked layout records the element size as its last dimension, and +/// libhdf5 refuses a dataset whose datatype has another size +/// (`H5D__chunk_set_sizes`: "stored datatype size in chunk layout does not +/// match datatype description"). Reading it anyway laid the chunks out with +/// the wrong element size. +pub(crate) fn check_chunk_element_size( + layout: &DataLayout, + datatype: &Datatype, + offset_size: u8, +) -> Result<(), FormatError> { + let DataLayout::Chunked { + chunk_dimensions, .. + } = layout + else { + return Ok(()); + }; + let Some(&stored) = chunk_dimensions.last() else { + return Ok(()); + }; + let expected = stored_element_size(datatype, offset_size); + if u64::from(stored) != expected { + return Err(FormatError::InvalidChunkDimensions(format!( + "stored datatype size in chunk layout does not match datatype description \ + (layout {stored} bytes, datatype {expected})" + ))); + } + Ok(()) +} + /// Product of chunk dimensions times the element size, overflow-checked. pub(crate) fn checked_chunk_byte_len( chunk_dims: &[usize], @@ -774,6 +821,7 @@ pub fn read_chunked_data( offset_size: u8, length_size: u8, ) -> Result, FormatError> { + check_chunk_element_size(layout, datatype, offset_size)?; let elem_size = datatype.type_size() as usize; let (chunks, chunk_dims) = list_chunks( file_data, @@ -914,6 +962,7 @@ pub fn read_chunked_data_cached( let addr = addr_opt .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; + check_chunk_element_size(layout, datatype, offset_size)?; let elem_size = datatype.type_size() as usize; let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, version, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); @@ -1220,6 +1269,7 @@ pub fn read_chunked_data_sweep( let addr = addr_opt .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; + check_chunk_element_size(layout, datatype, offset_size)?; let elem_size = datatype.type_size() as usize; let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, version, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); @@ -1357,6 +1407,7 @@ pub fn read_chunked_data_indexed( let addr = addr_opt .ok_or_else(|| FormatError::ChunkedReadError("no address for chunked layout".into()))?; + check_chunk_element_size(layout, datatype, offset_size)?; let elem_size = datatype.type_size() as usize; let (rank, chunk_dims) = chunk_geometry(chunk_dimensions, version, dataspace, elem_size)?; let ds_dims: Vec = dataspace.dimensions.iter().map(|&d| d as usize).collect(); diff --git a/crates/clawhdf5-format/src/data_read.rs b/crates/clawhdf5-format/src/data_read.rs index 8583473..b8f2ffe 100644 --- a/crates/clawhdf5-format/src/data_read.rs +++ b/crates/clawhdf5-format/src/data_read.rs @@ -303,6 +303,7 @@ pub fn read_raw_data_selection( use crate::selection::Selection; crate::partial_read::validate(selection, &dataspace.dimensions)?; + crate::chunked_read::check_chunk_element_size(layout, datatype, offset_size)?; // Read only what the selection's bounding box touches when that is // possible; everything below is the decode-everything-then-pick path, diff --git a/crates/clawhdf5-format/src/error.rs b/crates/clawhdf5-format/src/error.rs index 755a998..a9a0c2b 100644 --- a/crates/clawhdf5-format/src/error.rs +++ b/crates/clawhdf5-format/src/error.rs @@ -211,8 +211,9 @@ pub enum FormatError { /// an empty enum name, a compound member outside its compound, … InvalidDatatype(String), /// A chunked layout whose chunk dimensions libhdf5 refuses: a zero - /// dimension, a rank that does not match the dataspace, or a chunk of - /// 4 GiB or more indexed by a version-1 B-tree. + /// dimension, a rank that does not match the dataspace, an element size + /// that is not the datatype's, or a chunk of 4 GiB or more indexed by a + /// version-1 B-tree. InvalidChunkDimensions(String), /// The superblock's end-of-file address lies past the end of the file: /// the file was truncated (libhdf5 refuses to open it). diff --git a/crates/clawhdf5/tests/header_validation_interop.rs b/crates/clawhdf5/tests/header_validation_interop.rs index a2cebf5..7cdd4b4 100644 --- a/crates/clawhdf5/tests/header_validation_interop.rs +++ b/crates/clawhdf5/tests/header_validation_interop.rs @@ -156,6 +156,102 @@ for libver in ("earliest", "latest"): ); } +/// Python: `fix_ohdr(buf, off)` recomputes the Jenkins lookup3 checksum of +/// the version-2 object header chunk 0 at `off`, so a field can be changed +/// in a `libver="latest"` file without the checksum failing first. +const FIX_OHDR_PY: &str = r#" +def _rot(x, k): + return ((x << k) | (x >> (32 - k))) & 0xFFFFFFFF +def lookup3(data): + M = 0xFFFFFFFF + n = len(data); a = b = c = (0xDEADBEEF + n) & M; i = 0 + w = lambda j: int.from_bytes(data[j:j + 4], "little") + while n > 12: + a = (a + w(i)) & M; b = (b + w(i + 4)) & M; c = (c + w(i + 8)) & M + a = (a - c) & M; a ^= _rot(c, 4); c = (c + b) & M + b = (b - a) & M; b ^= _rot(a, 6); a = (a + c) & M + c = (c - b) & M; c ^= _rot(b, 8); b = (b + a) & M + a = (a - c) & M; a ^= _rot(c, 16); c = (c + b) & M + b = (b - a) & M; b ^= _rot(a, 19); a = (a + c) & M + c = (c - b) & M; c ^= _rot(b, 4); b = (b + a) & M + n -= 12; i += 12 + if n == 0: + return c + t = bytes(data[i:]) + bytes(12) + w = lambda j: int.from_bytes(t[j:j + 4], "little") + a = (a + w(0)) & M; b = (b + w(4)) & M; c = (c + w(8)) & M + c ^= b; c = (c - _rot(b, 14)) & M + a ^= c; a = (a - _rot(c, 11)) & M + b ^= a; b = (b - _rot(a, 25)) & M + c ^= b; c = (c - _rot(b, 16)) & M + a ^= c; a = (a - _rot(c, 4)) & M + b ^= a; b = (b - _rot(a, 14)) & M + c ^= b; c = (c - _rot(b, 24)) & M + return c +def fix_ohdr(buf, off): + assert buf[off:off + 4] == b"OHDR" + flags = buf[off + 5]; p = off + 6 + if flags & 0x20: p += 16 + if flags & 0x10: p += 4 + width = 1 << (flags & 3) + end = p + width + int.from_bytes(buf[p:p + width], "little") + buf[end:end + 4] = lookup3(bytes(buf[off:end])).to_bytes(4, "little") +"#; + +/// A chunked layout records the element size as its last dimension; libhdf5 +/// refuses a dataset whose datatype has another size ("stored datatype size +/// in chunk layout does not match datatype description"). This read the +/// chunks laid out with the wrong element size. +#[test] +fn chunk_layout_element_size_must_match_the_datatype() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let body = format!( + "{FIX_OHDR_PY}{}", + r#" +for libver in ("earliest", "latest"): + good = os.path.join(d, f"{libver}_good.h5") + with h5py.File(good, "w", libver=libver) as f: + f.create_dataset("d", data=np.arange(100, dtype=" 6 + for size in (2, 8): + bad = bytearray(data) + bad[at:at + width] = size.to_bytes(width, "little") + if libver == "latest": + fix_ohdr(bad, bad.rfind(b"OHDR", 0, at)) + open(os.path.join(d, f"{libver}_size{size}.h5"), "wb").write(bad) +"# + ); + let verdicts = h5py_verdicts(dir.path(), &body); + assert_agrees_with_h5py( + dir.path(), + &verdicts, + &[ + "earliest_good ok", + "earliest_size2 ERROR", + "earliest_size8 ERROR", + "latest_good ok", + "latest_size2 ERROR", + "latest_size8 ERROR", + ], + ); + for name in ["earliest_size2", "latest_size8"] { + let err = clawhdf5_reads(&dir.path().join(format!("{name}.h5"))).unwrap_err(); + assert!( + err.contains("stored datatype size in chunk layout"), + "{name}: {err}" + ); + } +} + #[test] fn truncated_files_are_refused_and_nothing_past_the_end_of_file_is_read() { skip_if_no_python!(); From 3938f7f8a2feb385340e021d25756dadd0bafa4b Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:30:56 -0500 Subject: [PATCH 15/17] fix(io): refuse truncated files in the VOL, async and MPI readers The truncated-file check and the end-of-file clamp reached File, LazyFile and MmapFile but not clawhdf5-io's readers, which still opened truncated files and read past the recorded end of file. NativeVol (open, and read_dataset for from_bytes), AsyncHDF5File::from_bytes and MpiVol's collective read now view the file through the new vol::hdf5_view: from the superblock to Superblock::data_end, refusing a file shorter than that. MpiVol's read is compiled only with the mpi-io feature, which needs an MPI installation; it was not built here. The edit there only swaps its two-line superblock setup for hdf5_view. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 7 +++- crates/clawhdf5-io/src/async_read.rs | 27 +++++++++++++ crates/clawhdf5-io/src/mpi_vol.rs | 9 ++--- crates/clawhdf5-io/src/vol.rs | 57 +++++++++++++++++++++++++--- 4 files changed, 88 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97bfb97..75b1ba3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -343,8 +343,11 @@ (`chunked_read::collect_chunk_info_checked`). - truncated files (`FormatError::TruncatedFile`, `Superblock::data_end`): a file shorter than the end of file its superblock records is refused - ("truncated file"), and nothing past that end is read. `File`, - `LazyFile` and `MmapFile` do this. + ("truncated file"), and nothing past that end is read. Every reader + does this: `File`, `LazyFile` and `MmapFile`, and in `clawhdf5-io` + `NativeVol` (at `open`, and on read for `from_bytes`), + `AsyncHDF5File` and `MpiVol` (the MPI path is not built in CI: it + needs an MPI installation). - the writer: `FileWriter::finish()` / `FileBuilder::finish()` refuse a datatype the reader would refuse (`FormatError::SerializationError`, "datatype cannot be written: ..."), such as a compound with a repeated diff --git a/crates/clawhdf5-io/src/async_read.rs b/crates/clawhdf5-io/src/async_read.rs index ddfce7b..30969fa 100644 --- a/crates/clawhdf5-io/src/async_read.rs +++ b/crates/clawhdf5-io/src/async_read.rs @@ -281,8 +281,13 @@ impl AsyncHDF5File { // HDF5 addresses are relative to the superblock: drop any user block // so they index `data` directly. let user_block = find_signature(&data)?; + let whole_len = data.len() as u64; data.drain(..user_block); let superblock = Superblock::parse(&data, 0)?; + // Refuse a truncated file, and keep nothing past the end of file the + // superblock records, as libhdf5 does. + let end = superblock.data_end(user_block as u64, whole_len)?; + data.truncate(end as usize); Ok(Self { data, superblock }) } @@ -605,6 +610,28 @@ mod tests { tokio::fs::remove_file(&path).await.ok(); } + /// As libhdf5 does: a truncated file is refused, and bytes past the end + /// of file the superblock records are dropped. + #[tokio::test] + async fn async_refuses_truncated_files_and_drops_trailing_bytes() { + let bytes = make_test_hdf5_f64("v", &[1.0, 2.0]); + let truncated = bytes[..bytes.len() - 8].to_vec(); + let err = AsyncHDF5File::from_bytes(truncated).err().unwrap(); + assert!( + matches!( + err, + AsyncHDF5Error::Format(FormatError::TruncatedFile { .. }) + ), + "{err}" + ); + + let mut appended = bytes.clone(); + appended.extend_from_slice(&[0xAB; 64]); + let file = AsyncHDF5File::from_bytes(appended).unwrap(); + assert_eq!(file.as_bytes().len(), bytes.len()); + assert_eq!(file.read_f64("v").await.unwrap(), vec![1.0, 2.0]); + } + #[tokio::test] async fn async_error_display() { let io_err = AsyncHDF5Error::Io(io::Error::new(io::ErrorKind::NotFound, "gone")); diff --git a/crates/clawhdf5-io/src/mpi_vol.rs b/crates/clawhdf5-io/src/mpi_vol.rs index fe39c4e..87a82ea 100644 --- a/crates/clawhdf5-io/src/mpi_vol.rs +++ b/crates/clawhdf5-io/src/mpi_vol.rs @@ -180,8 +180,7 @@ fn mpi_collective_read(vol: &MpiVol, location: &str, path: &str) -> Result Result, } +/// The HDF5 bytes of a whole file and its superblock: from the superblock +/// (addresses are relative to it, so any user block is skipped) to the end +/// of file the superblock records. A file shorter than that is truncated +/// and refused, and nothing past it is read, as in libhdf5. +pub(crate) fn hdf5_view( + whole: &[u8], +) -> Result<(&[u8], clawhdf5_format::superblock::Superblock), VolError> { + use clawhdf5_format::{signature::split_user_block, superblock::Superblock}; + let err = |e: clawhdf5_format::error::FormatError| VolError::DataError(e.to_string()); + let (user_block, data) = split_user_block(whole).map_err(err)?; + let sb = Superblock::parse(data, 0).map_err(err)?; + let end = sb + .data_end(user_block.len() as u64, whole.len() as u64) + .map_err(err)?; + // data_end is at most the file length less the user block. + Ok((&data[..end as usize], sb)) +} + impl NativeVol { /// Create a new native VOL connector. pub fn new() -> Self { @@ -264,6 +282,8 @@ impl VirtualObjectLayer for NativeVol { fn open(&mut self, location: &str) -> Result<(), VolError> { let data = std::fs::read(location)?; + // Refuse a truncated file at open, as libhdf5 does. + hdf5_view(&data)?; self.data = Some(data); self.location = Some(location.to_string()); Ok(()) @@ -283,13 +303,10 @@ impl VirtualObjectLayer for NativeVol { use clawhdf5_format::{ data_layout::DataLayout, data_read::read_raw_data_full, dataspace::Dataspace, datatype::Datatype, filter_pipeline::FilterPipeline, group_v2::resolve_path_any, - message_type::MessageType, object_header::ObjectHeader, signature::split_user_block, - superblock::Superblock, + message_type::MessageType, object_header::ObjectHeader, }; - // Addresses are relative to the superblock: skip any user block. - let (_, data) = split_user_block(data).map_err(|e| VolError::DataError(e.to_string()))?; - let sb = Superblock::parse(data, 0).map_err(|e| VolError::DataError(e.to_string()))?; + let (data, sb) = hdf5_view(data)?; let addr = resolve_path_any(data, &sb, path) .map_err(|e| VolError::NotFound(format!("{path}: {e}")))?; @@ -387,6 +404,36 @@ mod tests { assert!(vol.as_bytes().is_none()); } + /// As libhdf5 does: a file shorter than the end of file its superblock + /// records is truncated and refused (at open, and when read from + /// memory), and bytes appended past that end are not part of the file. + #[test] + fn native_vol_refuses_truncated_files_and_ignores_trailing_bytes() { + use clawhdf5_format::file_writer::FileWriter as FmtWriter; + + let mut fw = FmtWriter::new(); + fw.create_dataset("x").with_f64_data(&[1.0, 2.0, 3.0]); + let bytes = fw.finish().unwrap(); + + let truncated = bytes[..bytes.len() - 8].to_vec(); + let err = NativeVol::from_bytes(truncated.clone()) + .read_dataset("x") + .unwrap_err(); + assert!(err.to_string().contains("truncated"), "{err}"); + let dir = std::env::temp_dir().join(format!("clawhdf5_vol_trunc_{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("truncated.h5"); + std::fs::write(&path, &truncated).unwrap(); + let err = NativeVol::open_path(path.to_str().unwrap()).err().unwrap(); + assert!(err.to_string().contains("truncated"), "{err}"); + std::fs::remove_dir_all(&dir).ok(); + + let mut appended = bytes.clone(); + appended.extend_from_slice(&[0xAB; 64]); + let raw = NativeVol::from_bytes(appended).read_dataset("x").unwrap(); + assert_eq!(raw.len(), 24); + } + #[test] fn vol_error_display() { let err = VolError::Unsupported("read_dataset".into()); From 993214723e6842aea43362ada12ee4cd85fce32a Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:32:00 -0500 Subject: [PATCH 16/17] test: a v2 header message running into the checksum is refused, as in libhdf5 The review read libhdf5's H5O__chunk_deserialize as accepting a v2 message that runs up to 4 bytes into the chunk's checksum, since it bounds message bodies by the whole chunk buffer. It does not accept it: the message loop stops at the checksum, and the checksum read that follows starts past it and overruns the chunk ("ran off end of input buffer while decoding"). h5py refuses such files whether the message runs 1, 4 or 5 bytes in, and so does clawhdf5, with its own error text. No code change; the test pins the agreement and a comment records why. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/object_header.rs | 7 +++ .../tests/header_validation_interop.rs | 48 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/crates/clawhdf5-format/src/object_header.rs b/crates/clawhdf5-format/src/object_header.rs index b28fdad..2c3c819 100644 --- a/crates/clawhdf5-format/src/object_header.rs +++ b/crates/clawhdf5-format/src/object_header.rs @@ -421,6 +421,13 @@ impl ObjectHeader { }; pos += msg_header_size; + // `end` is where the messages stop and the checksum starts. + // libhdf5 bounds a message by the chunk including its checksum, + // but a message that runs into the checksum still fails there: + // its loop stops at the checksum, and reading the checksum from + // past its start overruns the chunk ("ran off end of input + // buffer while decoding"). Both refuse it; only the text + // differs. if msg_data_size > end - pos { return Err(FormatError::InvalidObjectHeader( "message size exceeds buffer end", diff --git a/crates/clawhdf5/tests/header_validation_interop.rs b/crates/clawhdf5/tests/header_validation_interop.rs index 7cdd4b4..2f52d27 100644 --- a/crates/clawhdf5/tests/header_validation_interop.rs +++ b/crates/clawhdf5/tests/header_validation_interop.rs @@ -252,6 +252,54 @@ for libver in ("earliest", "latest"): } } +/// A v2 object header message whose size runs past the messages into the +/// chunk's checksum is refused by libhdf5 whether it runs 1 byte or more +/// into the checksum (its loop stops at the checksum, and then the checksum +/// read and the size check fail), so it is refused here too. +#[test] +fn v2_header_message_running_into_the_checksum_is_refused() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let body = format!( + "{FIX_OHDR_PY}{}", + r#" +good = os.path.join(d, "good.h5") +with h5py.File(good, "w", libver="latest") as f: + f.create_dataset("d", data=np.arange(4, dtype=" Date: Sat, 26 Sep 2026 01:32:09 -0500 Subject: [PATCH 17/17] docs: record the chunk dimension width libhdf5 2.0.0 refuses and we read Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/known-issues.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/known-issues.md b/docs/known-issues.md index 7444f15..617784a 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -158,6 +158,11 @@ fill-value item that did is fixed). files; we do not decode those messages at open. - Deliberately not refused, because clawhdf5 up to v2.7.0 wrote them: a float sign bit position outside the type, and a size-0 string type. + - Not refused because current libhdf5 reads it though HDF5 2.0.0 + (h5py 3.16) refuses it: a v4 chunked layout whose dimensions are + encoded in more bytes than they need (HDFGroup/hdf5@e124c36, + 2026-06-05, relaxed that check; clawhdf5 wrote such layouts until + 2026-09-26). - Not refused because HDF5 2.0 (h5py 3.16) reads them though newer libhdf5 refuses them: bit-field offset/precision outside the type, an unknown variable-length kind, an array type whose stored size is not