From f99587c27db89d3c92bccbc4987c386dd90b6cf6 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 08:20:19 -0500 Subject: [PATCH] fix(format): resolve VL elements as libhdf5 does Checked with h5py on a patched file: - a VL string with an embedded NUL reads up to the NUL (libhdf5 converts VL strings to C strings); read_vl_strings returned "a\0b"; - an element whose global heap object is not length x base size bytes is an error ("Expected global heap object size does not match"); we returned the object cut to the length; - a heap address of 0 is a null element whatever its length. vl_data::VlResolver does this, caching each parsed heap collection: read_vl_strings parsed the whole collection again for every element. read_vl_strings and read_vl_bytes use it; check_element_size refuses a VL type whose stored element size is not 4 + offset size + 4. The conformance probe resolves VL values through VlResolver instead of its own lenient copy (575 of 697, unchanged). The new unit tests fail against the old read_vl_strings. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 14 ++ conformance/probe/src/main.rs | 75 ++---- crates/clawhdf5-format/src/vl_data.rs | 338 ++++++++++++++++++++++---- 3 files changed, 329 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3af6328..40781cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,20 @@ exhaustively destructures that variant; patterns with `..` are unaffected), and it is written back as stored. Tested against h5py (`crates/clawhdf5/tests/vl_offset4_interop.rs`). +- **Wrong data: VL strings with an embedded NUL, and VL elements whose heap + object has the wrong size.** libhdf5 hands VL strings over as C strings, + so h5py reads `"a\0b"` as `"a"`; `read_vl_strings` returned the NUL and + what followed. An element whose heap object is not exactly + `length × base size` bytes is refused by libhdf5 ("Expected global heap + object size does not match"); we returned the object cut or padded to + the length. Both now behave as libhdf5, and a heap address of 0 is a null + element (empty) whatever its length. The new + `clawhdf5_format::vl_data::VlResolver` does this and parses each global + heap collection once per read: `read_vl_strings` parsed the whole + collection again for every element. `vl_data::check_element_size` refuses + a VL datatype whose stored size is not 4 + offset size + 4 (libhdf5 + ignores the stored size). The conformance probe resolves VL elements + with `VlResolver` too; conformance unchanged at 575 of 697. ### Plugin filters (2026-09-26) - **LZF, bitshuffle, bzip2 and Blosc read and write, in pure Rust.** Files diff --git a/conformance/probe/src/main.rs b/conformance/probe/src/main.rs index 87c660d..72f02a1 100644 --- a/conformance/probe/src/main.rs +++ b/conformance/probe/src/main.rs @@ -19,9 +19,8 @@ //! with its message, location and the clawhdf5 frames of its backtrace. use std::cell::RefCell; -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::panic::{self, AssertUnwindSafe}; -use std::rc::Rc; use clawhdf5_format::attribute::extract_attributes_full; use clawhdf5_format::data_layout::DataLayout; @@ -29,7 +28,6 @@ use clawhdf5_format::data_read; use clawhdf5_format::dataspace::{Dataspace, DataspaceType}; use clawhdf5_format::datatype::{Datatype, DatatypeByteOrder}; use clawhdf5_format::filter_pipeline::FilterPipeline; -use clawhdf5_format::global_heap::GlobalHeapCollection; use clawhdf5_format::group_v1::{self, GroupEntry}; use clawhdf5_format::group_v2; use clawhdf5_format::message_type::MessageType; @@ -37,6 +35,7 @@ use clawhdf5_format::object_header::ObjectHeader; use clawhdf5_format::signature; use clawhdf5_format::superblock::Superblock; use clawhdf5_format::symbol_table::SymbolTableMessage; +use clawhdf5_format::vl_data::{VlResolver, check_element_size}; use serde_json::{Map, Value, json}; use sha2::{Digest, Sha256}; @@ -111,7 +110,10 @@ struct Ctx<'a> { os: u8, ls: u8, base_dir: std::path::PathBuf, - heaps: RefCell, String>>>, + /// Resolves variable-length elements as the library does (null + /// elements, strings cut at a NUL, heap objects of the wrong size + /// refused), caching each heap collection. + vl: RefCell>, } impl<'a> Ctx<'a> { @@ -130,33 +132,6 @@ impl<'a> Ctx<'a> { } } - fn heap_obj(&self, addr: u64, idx: u32) -> Result, String> { - let coll = { - let mut cache = self.heaps.borrow_mut(); - cache - .entry(addr) - .or_insert_with(|| { - GlobalHeapCollection::parse(self.data, addr as usize, self.ls) - .map(Rc::new) - .map_err(e) - }) - .clone()? - }; - coll.get_object(idx as u16) - .map(|o| o.data.clone()) - .ok_or_else(|| { - format!("GlobalHeapObjectNotFound {{ collection_address: {addr}, index: {idx} }}") - }) - } - - fn read_offset(&self, b: &[u8]) -> u64 { - let mut v = 0u64; - for (i, x) in b.iter().take(self.os as usize).enumerate() { - v |= (*x as u64) << (8 * i); - } - v - } - fn canon(&self, dt: &Datatype, b: &[u8], out: &mut Vec) -> Result<(), String> { let size = dt.type_size() as usize; if b.len() < size { @@ -204,41 +179,27 @@ impl<'a> Ctx<'a> { } } Datatype::VariableLength { + size: vl_size, is_string, base_type, .. } => { - let len = u32::from_le_bytes([b[0], b[1], b[2], b[3]]) as usize; - let addr = self.read_offset(&b[4..]); - let idx_off = 4 + self.os as usize; - let idx = u32::from_le_bytes([ - b[idx_off], - b[idx_off + 1], - b[idx_off + 2], - b[idx_off + 3], - ]); - let obj = if len == 0 || addr == 0 || addr == u64::MAX >> (64 - 8 * self.os as u32) - { - Vec::new() - } else { - self.heap_obj(addr, idx)? - }; + check_element_size(*vl_size, self.os).map_err(e)?; + let el = &b[..size]; if *is_string { - let l = len.min(obj.len()); - canon_str(&obj[..l], out); + let s = self.vl.borrow_mut().string_bytes(el).map_err(e)?; + canon_str(&s[0], out); } else { let bs = base_type.type_size() as usize; - if bs == 0 { - return Err("canon: VL base size 0".into()); - } - let need = len.checked_mul(bs).ok_or("canon: VL overflow")?; - if len > 0 && obj.len() < need { - return Err(format!("canon: VL object {} < {need}", obj.len())); - } + // The borrow ends here: the base type may itself be + // variable-length. + let seq = self.vl.borrow_mut().sequences(el, bs).map_err(e)?; + let seq = &seq[0]; + let len = seq.len() / bs; out.push(b'V'); out.extend_from_slice(&(len as u32).to_le_bytes()); for i in 0..len { - self.canon(base_type, &obj[i * bs..], out)?; + self.canon(base_type, &seq[i * bs..], out)?; } } } @@ -744,7 +705,7 @@ fn main() { .parent() .map(|p| p.to_path_buf()) .unwrap_or_default(), - heaps: RefCell::new(HashMap::new()), + vl: RefCell::new(VlResolver::new(hdf5, sb.offset_size, sb.length_size)), }; let mut objects: Vec = Vec::new(); let mut visited = HashSet::new(); diff --git a/crates/clawhdf5-format/src/vl_data.rs b/crates/clawhdf5-format/src/vl_data.rs index 9a50d51..58015a4 100644 --- a/crates/clawhdf5-format/src/vl_data.rs +++ b/crates/clawhdf5-format/src/vl_data.rs @@ -5,7 +5,9 @@ //! `sequence_length(4 LE) + collection_address(offset_size LE) + object_index(4 LE)`. #[cfg(not(feature = "std"))] -use alloc::{string::String, vec::Vec}; +use alloc::{collections::BTreeMap, format, string::String, vec, vec::Vec}; +#[cfg(feature = "std")] +use std::collections::BTreeMap; use crate::error::FormatError; use crate::global_heap::GlobalHeapCollection; @@ -109,7 +111,174 @@ fn is_undefined_address(addr: u64, offset_size: u8) -> bool { } } +/// The size of one variable-length element in a file with `offset_size`-byte +/// addresses: a sequence length (4), a global heap collection address and an +/// object index (4). libhdf5 computes it this way rather than trusting the +/// datatype message (`H5T_set_loc`). +pub fn element_size(offset_size: u8) -> usize { + 4 + offset_size as usize + 4 +} + +/// Refuse a variable-length datatype whose stored element size is not the +/// one this file's offset size implies. Its elements would be laid out with +/// a stride libhdf5 does not use, so every value after the first would be +/// read from the wrong place. +pub fn check_element_size(stored_size: u32, offset_size: u8) -> Result<(), FormatError> { + let expected = element_size(offset_size); + if stored_size as usize != expected { + return Err(FormatError::VlDataError(format!( + "variable-length datatype stores {stored_size}-byte elements; a file with \ + {offset_size}-byte offsets uses {expected}" + ))); + } + Ok(()) +} + +/// A parsed collection, with its objects indexed for lookup. +struct CachedCollection { + collection: GlobalHeapCollection, + /// `slots[index]` is the position in `collection.objects` of the first + /// object with that index. + slots: Vec>, +} + +impl CachedCollection { + fn new(collection: GlobalHeapCollection) -> Self { + let max = collection + .objects + .iter() + .map(|o| o.index as usize) + .max() + .unwrap_or(0); + let mut slots = vec![None; max + 1]; + for (pos, obj) in collection.objects.iter().enumerate() { + let slot = &mut slots[obj.index as usize]; + if slot.is_none() { + *slot = Some(pos); + } + } + Self { collection, slots } + } + + fn get(&self, index: u32) -> Option<&[u8]> { + let pos = (*self.slots.get(usize::try_from(index).ok()?)?)?; + Some(&self.collection.objects[pos].data) + } +} + +/// Resolves variable-length elements against a file's global heap, parsing +/// each heap collection once however many elements point into it. +/// +/// Values follow libhdf5: an element whose heap address is 0 is null (an +/// empty string or sequence), and an element whose heap object is not +/// exactly `length × base size` bytes is an error ("Expected global heap +/// object size does not match"), not a truncated or padded value. +pub struct VlResolver<'a> { + file_data: &'a [u8], + offset_size: u8, + length_size: u8, + cache: BTreeMap, +} + +impl<'a> VlResolver<'a> { + /// A resolver over `file_data` (the file from its superblock on), with + /// the superblock's offset and length sizes. + pub fn new(file_data: &'a [u8], offset_size: u8, length_size: u8) -> Self { + Self { + file_data, + offset_size, + length_size, + cache: BTreeMap::new(), + } + } + + /// The size of one element in this file (see [`element_size`]). + pub fn element_size(&self) -> usize { + element_size(self.offset_size) + } + + /// Split `raw` into elements; its length must be a whole number of them. + fn elements(&self, raw: &[u8]) -> Result, FormatError> { + let size = self.element_size(); + if !raw.len().is_multiple_of(size) { + return Err(FormatError::VlDataError(format!( + "{} bytes is not a whole number of {size}-byte variable-length elements", + raw.len() + ))); + } + parse_vl_references(raw, (raw.len() / size) as u64, self.offset_size) + } + + /// The bytes of one element: `length × base_size` bytes from the heap, + /// or empty for a null or zero-length element. + fn resolve(&mut self, vl: &VlElement, base_size: usize) -> Result<&[u8], FormatError> { + let addr = vl.collection_address; + if addr == 0 || (vl.length == 0 && is_undefined_address(addr, self.offset_size)) { + return Ok(&[]); + } + let data = self.object(vl)?; + let expected = (vl.length as usize) + .checked_mul(base_size) + .ok_or_else(|| FormatError::Overflow("variable-length element size".into()))?; + if data.len() != expected { + return Err(FormatError::VlDataError(format!( + "global heap object {} in the collection at {addr} holds {} bytes; the element \ + says {} × {base_size}", + vl.object_index, + data.len(), + vl.length + ))); + } + Ok(data) + } + + /// The strings of the variable-length string elements in `raw`, as + /// bytes. A string ends at its first NUL, as libhdf5 returns it (it + /// converts each to a C string); a null element is empty. + pub fn string_bytes(&mut self, raw: &[u8]) -> Result>, FormatError> { + self.elements(raw)? + .iter() + .map(|vl| { + let s = self.resolve(vl, 1)?; + let end = s.iter().position(|&b| b == 0).unwrap_or(s.len()); + Ok(s[..end].to_vec()) + }) + .collect() + } + + /// The strings of the variable-length string elements in `raw`, decoded + /// as UTF-8 with invalid sequences replaced by U+FFFD (see + /// [`string_bytes`](Self::string_bytes) for the exact bytes). + pub fn strings(&mut self, raw: &[u8]) -> Result, FormatError> { + Ok(self + .string_bytes(raw)? + .into_iter() + .map(|b| match String::from_utf8(b) { + Ok(s) => s, + Err(e) => String::from_utf8_lossy(e.as_bytes()).into_owned(), + }) + .collect()) + } + + /// The sequences of the variable-length sequence elements in `raw`, each + /// as its `length × base_size` bytes in the base type's encoding. + pub fn sequences(&mut self, raw: &[u8], base_size: usize) -> Result>, FormatError> { + if base_size == 0 { + return Err(FormatError::VlDataError( + "variable-length sequence of a zero-size base type".into(), + )); + } + self.elements(raw)? + .iter() + .map(|vl| self.resolve(vl, base_size).map(<[u8]>::to_vec)) + .collect() + } +} + /// Resolve VL strings from raw data by looking up each element in the global heap. +/// +/// Reads the first `num_elements` elements of `raw`. Strings end at their +/// first NUL and invalid UTF-8 is replaced, as in [`VlResolver::strings`]. pub fn read_vl_strings( file_data: &[u8], raw_data: &[u8], @@ -117,35 +286,23 @@ pub fn read_vl_strings( offset_size: u8, length_size: u8, ) -> Result, FormatError> { - let refs = parse_vl_references(raw_data, num_elements, offset_size)?; - let mut result = Vec::with_capacity(refs.len()); + let raw = first_elements(raw_data, num_elements, offset_size)?; + VlResolver::new(file_data, offset_size, length_size).strings(raw) +} - for vl in &refs { - if vl.length == 0 && is_undefined_address(vl.collection_address, offset_size) { - result.push(String::new()); - continue; - } - if vl.length == 0 && vl.collection_address == 0 { - result.push(String::new()); - continue; - } - - let coll = - GlobalHeapCollection::parse(file_data, vl.collection_address as usize, length_size)?; - let obj = coll.get_object(vl.object_index as u16).ok_or( - FormatError::GlobalHeapObjectNotFound { - collection_address: vl.collection_address, - index: vl.object_index as u16, - }, - )?; - - // The object data is the raw string bytes - let len = (vl.length as usize).min(obj.data.len()); - let s = String::from_utf8_lossy(&obj.data[..len]).into_owned(); - result.push(s); - } - - Ok(result) +/// The first `num_elements` elements of `raw`, or an error if it is shorter. +fn first_elements(raw: &[u8], num_elements: u64, offset_size: u8) -> Result<&[u8], FormatError> { + let total = usize::try_from(num_elements) + .ok() + .and_then(|n| n.checked_mul(element_size(offset_size))) + .ok_or(FormatError::UnexpectedEof { + expected: usize::MAX, + available: raw.len(), + })?; + raw.get(..total).ok_or(FormatError::UnexpectedEof { + expected: total, + available: raw.len(), + }) } /// Resolve VL sequences from raw data, returning each element's bytes. @@ -153,7 +310,9 @@ pub fn read_vl_strings( /// Each element is the sequence's full encoding — element count × base type /// size bytes, in the base type's byte order — so a sequence of `i32` yields /// four bytes per value. Decode it with the base type (e.g. -/// [`crate::data_read::read_as_i64`]). +/// [`crate::data_read::read_as_i64`]). This does not know the base type, so +/// it returns each heap object whole; [`VlResolver::sequences`] also checks +/// the object's size against the element's length. pub fn read_vl_bytes( file_data: &[u8], raw_data: &[u8], @@ -162,6 +321,7 @@ pub fn read_vl_bytes( length_size: u8, ) -> Result>, FormatError> { let refs = parse_vl_references(raw_data, num_elements, offset_size)?; + let mut resolver = VlResolver::new(file_data, offset_size, length_size); let mut result = Vec::with_capacity(refs.len()); for vl in &refs { @@ -172,25 +332,38 @@ pub fn read_vl_bytes( result.push(Vec::new()); continue; } - - let coll = - GlobalHeapCollection::parse(file_data, vl.collection_address as usize, length_size)?; - let obj = coll.get_object(vl.object_index as u16).ok_or( - FormatError::GlobalHeapObjectNotFound { - collection_address: vl.collection_address, - index: vl.object_index as u16, - }, - )?; - // The heap object holds the whole sequence. `vl.length` counts // elements, not bytes, so it is only the byte length when the base // type is one byte wide. - result.push(obj.data.clone()); + let obj = resolver.object(vl)?; + result.push(obj.to_vec()); } Ok(result) } +impl VlResolver<'_> { + /// The heap object `vl` points to, whatever its size; its collection is + /// parsed on first use. + fn object(&mut self, vl: &VlElement) -> Result<&[u8], FormatError> { + let addr = vl.collection_address; + if !self.cache.contains_key(&addr) { + let offset = usize::try_from(addr).map_err(|_| FormatError::UnexpectedEof { + expected: usize::MAX, + available: self.file_data.len(), + })?; + let coll = GlobalHeapCollection::parse(self.file_data, offset, self.length_size)?; + self.cache.insert(addr, CachedCollection::new(coll)); + } + self.cache[&addr] + .get(vl.object_index) + .ok_or(FormatError::GlobalHeapObjectNotFound { + collection_address: addr, + index: vl.object_index as u16, + }) + } +} + #[cfg(test)] mod tests { use super::*; @@ -333,6 +506,89 @@ mod tests { assert_eq!(bytes, vec![vec![0xDE, 0xAD], vec![0xBE, 0xEF, 0xCA]]); } + fn element(length: u32, addr: u64, index: u32, offset_size: u8) -> Vec { + let mut raw = length.to_le_bytes().to_vec(); + raw.extend_from_slice(&addr.to_le_bytes()[..offset_size as usize]); + raw.extend_from_slice(&index.to_le_bytes()); + raw + } + + #[test] + fn strings_end_at_the_first_nul() { + // libhdf5 hands each VL string over as a C string, so h5py sees + // "a\0b" as "a"; we used to return the NUL and what followed. + let mut file_data = vec![0u8; 512]; + build_gcol_at(&mut file_data, 64, &[(1, b"a\0b"), (2, b"cd")]); + let mut raw = element(3, 64, 1, 8); + raw.extend(element(2, 64, 2, 8)); + let mut r = VlResolver::new(&file_data, 8, 8); + assert_eq!( + r.string_bytes(&raw).unwrap(), + vec![b"a".to_vec(), b"cd".to_vec()] + ); + assert_eq!( + read_vl_strings(&file_data, &raw, 2, 8, 8).unwrap(), + ["a", "cd"] + ); + } + + #[test] + fn a_heap_object_of_the_wrong_size_is_an_error() { + // libhdf5: "Expected global heap object size does not match". We + // used to return the object cut to the element's length. + let mut file_data = vec![0u8; 512]; + build_gcol_at(&mut file_data, 64, &[(1, b"cdefgh"), (2, &[1, 0, 0, 0])]); + let mut r = VlResolver::new(&file_data, 8, 8); + assert!(r.string_bytes(&element(3, 64, 1, 8)).is_err()); + assert!(r.string_bytes(&element(9, 64, 1, 8)).is_err()); + assert!(read_vl_strings(&file_data, &element(3, 64, 1, 8), 1, 8, 8).is_err()); + // A sequence of one i32 is 4 bytes; of two, 8. + assert_eq!( + r.sequences(&element(1, 64, 2, 8), 4).unwrap(), + vec![vec![1, 0, 0, 0]] + ); + assert!(r.sequences(&element(2, 64, 2, 8), 4).is_err()); + assert!(r.sequences(&element(1, 64, 2, 8), 0).is_err()); + } + + #[test] + fn address_zero_is_null_whatever_the_length() { + // libhdf5 treats a heap address of 0 as a null element. + let file_data = vec![0u8; 64]; + let mut r = VlResolver::new(&file_data, 8, 8); + assert_eq!( + r.string_bytes(&element(5, 0, 1, 8)).unwrap(), + vec![Vec::::new()] + ); + assert_eq!( + r.sequences(&element(5, 0, 1, 8), 4).unwrap(), + vec![Vec::::new()] + ); + } + + #[test] + fn four_byte_offsets_use_twelve_byte_elements() { + let mut file_data = vec![0u8; 512]; + build_gcol_at(&mut file_data, 64, &[(1, b"one"), (2, b""), (3, b"three")]); + let mut raw = element(3, 64, 1, 4); + raw.extend(element(0, 64, 2, 4)); + raw.extend(element(5, 64, 3, 4)); + assert_eq!(raw.len(), 36); + let mut r = VlResolver::new(&file_data, 4, 8); + assert_eq!(r.element_size(), 12); + assert_eq!(r.strings(&raw).unwrap(), ["one", "", "three"]); + // Not a whole number of elements. + assert!(r.strings(&raw[..30]).is_err()); + } + + #[test] + fn element_size_is_checked_against_the_offset_size() { + assert!(check_element_size(16, 8).is_ok()); + assert!(check_element_size(12, 4).is_ok()); + assert!(check_element_size(16, 4).is_err()); + assert!(check_element_size(524_304, 8).is_err()); + } + #[test] fn parse_vl_references_truncated_error() { let raw = vec![0u8; 10]; // too short for 1 element with offset_size=8