From 10da8f0d09833189cc11ff34384af21d1f3b9e7d Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 08:18:57 -0500 Subject: [PATCH 1/6] fix(format): read VL values in files with 4-byte offsets In a file with sizeof_addr = 4, a VL string attribute came back as AttrValue::Raw, a compound's VL member failed with GlobalHeapObjectNotFound and VL datasets failed with a size mismatch. Two bugs: Datatype::type_size() said 16 for every VL type, while the element is 4 + offset size + 4 bytes (12 here); and the global heap was parsed without the padding libhdf5 puts after its collection and object headers (both round up to 8), so with 4-byte lengths every object was looked up 4 bytes early. Datatype::VariableLength now carries the size its datatype message stores, and writes it back. Checked against h5py in tests/vl_offset4_interop.rs (fails with either fix reverted). Conformance unchanged at 575 of 697; in cve-2024-32608 a VL attribute whose datatype claims 524304-byte elements is now an error (h5py cannot iterate those attributes at all). Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 15 +++ crates/clawhdf5-format/src/datatype.rs | 28 ++++- crates/clawhdf5-format/src/global_heap.rs | 19 ++- crates/clawhdf5/tests/vl_offset4_interop.rs | 126 ++++++++++++++++++++ docs/known-issues.md | 5 +- 5 files changed, 184 insertions(+), 9 deletions(-) create mode 100644 crates/clawhdf5/tests/vl_offset4_interop.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ef4bd2f..3af6328 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,21 @@ ## Unreleased +### Variable-length data (2026-09-26) +- **VL values in files with 4-byte offsets** (`sizeof_addr = 4`). A VL + string attribute came back as `AttrValue::Raw`, a VL member of a compound + failed with `GlobalHeapObjectNotFound`, and VL datasets failed with a + size mismatch. Two causes: `Datatype::type_size()` reported 16 for every + VL type (the element is 4 + offset size + 4 bytes: 12 here), and the + global heap was parsed without the padding libhdf5 puts after its + collection and object headers (`H5HG_SIZEOF_HDR`/`H5HG_SIZEOF_OBJHDR` + round up to 8), so with 4-byte lengths every object was looked up 4 + bytes early. `Datatype::VariableLength` now carries the element `size` + stored in the datatype message (**breaking** for code that builds or + 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`). + ### Plugin filters (2026-09-26) - **LZF, bitshuffle, bzip2 and Blosc read and write, in pure Rust.** Files written by h5py with `compression="lzf"`, or with hdf5plugin's diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index 12e427f..974c249 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -125,6 +125,11 @@ pub enum Datatype { }, /// Class 9: Variable-length type. VariableLength { + /// Size of one element as stored in the file: a sequence length (4 + /// bytes), a global heap collection address (the file's + /// `offset_size`) and an object index (4 bytes) — 16 in a file with + /// 8-byte offsets, 12 with 4-byte offsets. + size: u32, is_string: bool, padding: Option, charset: Option, @@ -771,6 +776,7 @@ impl Datatype { pos += consumed; Ok(( Datatype::VariableLength { + size, is_string, padding, charset, @@ -1017,6 +1023,7 @@ impl Datatype { Self::build_header(3, 1, [bf0, 0, 0], *size) } Datatype::VariableLength { + size, is_string, padding, charset, @@ -1039,7 +1046,7 @@ impl Datatype { } else { 0 }; - let mut buf = Self::build_header(9, 1, [bf0, bf1, 0], 16); + let mut buf = Self::build_header(9, 1, [bf0, bf1, 0], *size); buf.extend_from_slice(&base_type.serialize()); buf } @@ -1208,7 +1215,7 @@ impl Datatype { Datatype::Compound { size, .. } => *size, Datatype::Reference { size, .. } => *size, Datatype::Enumeration { size, .. } => *size, - Datatype::VariableLength { .. } => 16, // typically pointer + length + Datatype::VariableLength { size, .. } => *size, Datatype::Array { base_type, dimensions, @@ -1889,11 +1896,13 @@ mod tests { let (dt, _) = Datatype::parse(&buf).unwrap(); match dt { Datatype::VariableLength { + size, is_string, padding, charset, base_type, } => { + assert_eq!(size, 16); assert!(is_string); assert_eq!(padding, Some(StringPadding::NullTerminate)); assert_eq!(charset, Some(CharacterSet::Utf8)); @@ -1914,11 +1923,13 @@ mod tests { let (dt, _) = Datatype::parse(&buf).unwrap(); match dt { Datatype::VariableLength { + size, is_string, padding, charset, base_type, } => { + assert_eq!(size, 16); assert!(!is_string); assert_eq!(padding, None); assert_eq!(charset, None); @@ -1928,6 +1939,19 @@ mod tests { } } + #[test] + fn variable_length_size_is_the_stored_size() { + // A file with 4-byte offsets stores 12-byte VL elements (length 4 + + // address 4 + index 4); the type used to report 16 regardless, so + // every read laid the elements out 16 bytes apart. + let mut buf = build_dt_header(9, 1, [0x01, 0x00, 0], 12); + buf.extend_from_slice(&build_fixed_point(1, false, false, 0, 8)); + let (dt, _) = Datatype::parse(&buf).unwrap(); + assert_eq!(dt.type_size(), 12); + // And it is written back as stored. + assert_eq!(dt.serialize()[4..8], 12u32.to_le_bytes()); + } + #[test] fn test_array_2d() { // Array [3][4] of i32 LE, version 3 diff --git a/crates/clawhdf5-format/src/global_heap.rs b/crates/clawhdf5-format/src/global_heap.rs index dfba3f0..5eab713 100644 --- a/crates/clawhdf5-format/src/global_heap.rs +++ b/crates/clawhdf5-format/src/global_heap.rs @@ -64,8 +64,11 @@ impl GlobalHeapCollection { offset: usize, length_size: u8, ) -> Result { - // signature(4) + version(1) + reserved(3) + collection_size(length_size) - let header_size = 8 + length_size as usize; + // signature(4) + version(1) + reserved(3) + collection_size(length_size), + // padded to a multiple of 8 as libhdf5 lays it out (`H5HG_SIZEOF_HDR`). + // With 8-byte lengths the padding is 0; with 4-byte lengths it is 4, + // and reading without it put every object 4 bytes early. + let header_size = pad8(8 + length_size as usize); ensure_len(file_data, offset, header_size)?; if file_data[offset..offset + 4] != GCOL_SIGNATURE { @@ -104,8 +107,9 @@ impl GlobalHeapCollection { break; } - // object_index(2) + reference_count(2) + reserved(4) + object_size(length_size) - let obj_header_size = 8 + length_size as usize; + // object_index(2) + reference_count(2) + reserved(4) + + // object_size(length_size), padded to 8 (`H5HG_SIZEOF_OBJHDR`). + let obj_header_size = pad8(8 + length_size as usize); ensure_len(file_data, pos, obj_header_size)?; let reference_count = u16::from_le_bytes([file_data[pos + 2], file_data[pos + 3]]); @@ -149,10 +153,11 @@ mod tests { let ls = length_size as usize; // Calculate total size - let header_size = 8 + ls; + // libhdf5 pads both headers to a multiple of 8. + let header_size = pad8(8 + ls); let mut obj_size_total = 0usize; for (_, _, data) in objects { - let obj_header = 8 + ls; + let obj_header = pad8(8 + ls); obj_size_total += obj_header + pad8(data.len()); } // Free space marker (2 bytes for index 0) @@ -170,6 +175,7 @@ mod tests { 8 => buf.extend_from_slice(&(collection_size as u64).to_le_bytes()), _ => panic!("unsupported length_size"), } + buf.resize(header_size, 0); // Objects for (index, ref_count, data) in objects { @@ -181,6 +187,7 @@ mod tests { 8 => buf.extend_from_slice(&(data.len() as u64).to_le_bytes()), _ => panic!("unsupported"), } + buf.resize(buf.len() + (pad8(8 + ls) - (8 + ls)), 0); buf.extend_from_slice(data); // Pad to 8 bytes let padded = pad8(data.len()); diff --git a/crates/clawhdf5/tests/vl_offset4_interop.rs b/crates/clawhdf5/tests/vl_offset4_interop.rs new file mode 100644 index 0000000..cc7ce08 --- /dev/null +++ b/crates/clawhdf5/tests/vl_offset4_interop.rs @@ -0,0 +1,126 @@ +//! Variable-length values in files with 4-byte offsets and lengths +//! (`sizeof_addr = 4`), checked against h5py/libhdf5 through the +//! `clawhdf5_format` decoders. +//! +//! These failed with `GlobalHeapObjectNotFound` or came back as +//! `AttrValue::Raw`: the VL datatype claimed 16-byte elements whatever the +//! file's offset size, and the global heap was read without the padding +//! libhdf5 puts after its collection and object headers. Skipped when +//! python3 with h5py is unavailable, unless `CLAWHDF5_REQUIRE_INTEROP=1`. + +use std::process::Command; + +use clawhdf5::{AttrValue, File, Selection}; +use clawhdf5_format::data_read::{read_as_i64, read_compound_fields}; +use clawhdf5_format::vl_data::{read_vl_bytes, read_vl_strings}; + +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) +} + +#[test] +fn vl_values_in_a_file_with_4_byte_offsets_read_like_h5py() { + if !python_available() { + assert!( + !interop_required(), + "CLAWHDF5_REQUIRE_INTEROP=1 but python3 with h5py is not available" + ); + eprintln!("SKIP: python3 with h5py not available"); + return; + } + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("offset4.h5"); + let script = format!( + r#" +import h5py, numpy as np +S = h5py.string_dtype() +fcpl = h5py.h5p.create(h5py.h5p.FILE_CREATE); fcpl.set_sizes(4, 4) +with h5py.File(h5py.h5f.create({path:?}.encode(), h5py.h5f.ACC_TRUNC, fcpl=fcpl)) as f: + f.attrs['vlattr'] = 'attr-value' + f.attrs.create('vlattr_arr', np.array(['p', 'qq', ''], dtype=object), dtype=S) + ct = np.dtype([('id', ' = stdout.lines().collect(); + let (vlattr, vlattr_arr, names, seqs) = (lines[0], lines[1], lines[2], lines[3]); + + let file = File::open(&path).unwrap(); + let sb = file.superblock(); + assert_eq!((sb.offset_size, sb.length_size), (4, 4)); + + let attrs = file.root().attrs().unwrap(); + match &attrs["vlattr"] { + AttrValue::String(s) => assert_eq!(s, vlattr), + other => panic!("vlattr: {other:?}"), + } + match &attrs["vlattr_arr"] { + AttrValue::StringArray(s) => assert_eq!(s.join(","), vlattr_arr), + other => panic!("vlattr_arr: {other:?}"), + } + + // The compound's VL string member. + let ds = file.dataset("compound").unwrap(); + let dt = ds.raw_datatype().unwrap(); + assert_eq!(dt.type_size(), 24, "4 + 12-byte VL element + 8"); + let raw = ds.read_selection(&Selection::All).unwrap(); + let fields = read_compound_fields(&raw, &dt).unwrap(); + let name = fields.iter().find(|f| f.name == "name").unwrap(); + assert_eq!(name.datatype.type_size(), 12); + let got = read_vl_strings(file.as_bytes(), &name.raw_data, 3, 4, 4).unwrap(); + assert_eq!(got.join(","), names); + let id = fields.iter().find(|f| f.name == "id").unwrap(); + assert_eq!( + read_as_i64(&id.raw_data, &id.datatype).unwrap(), + vec![1, 2, 3] + ); + + // A VL sequence attribute. + let AttrValue::Raw { datatype, data, .. } = &attrs["vlen_attr"] else { + panic!("vlen_attr is Raw"); + }; + let clawhdf5_format::datatype::Datatype::VariableLength { base_type, .. } = datatype else { + panic!("vlen_attr is VL"); + }; + let got: Vec = read_vl_bytes(file.as_bytes(), data, 2, 4, 4) + .unwrap() + .iter() + .map(|b| { + let v = read_as_i64(b, base_type).unwrap(); + v.iter().map(i64::to_string).collect::>().join(" ") + }) + .collect(); + assert_eq!(got.join(";"), seqs); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index bf8f77a..b9a19b6 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -146,7 +146,10 @@ fill-value item that did is fixed). `GlobalHeapObjectNotFound` or come back as `Raw`: these paths assume the 16-byte element of an 8-byte-offset file. The datatype itself reads (it was refused as "member overlaps with previous member" until - 2026-09-26). + 2026-09-26). **Fixed 2026-09-26:** a VL type's element size is the one + its datatype message stores (12 with 4-byte offsets), and the global + heap is read with libhdf5's header padding + (`crates/clawhdf5/tests/vl_offset4_interop.rs`). - Metadata cache images are not supported. - x87 long double and binary128 are refused. - N-Bit on 64-bit scale-offset data and some N-Bit parameter layouts fail. From f99587c27db89d3c92bccbc4987c386dd90b6cf6 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 08:20:19 -0500 Subject: [PATCH 2/6] 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 From 8ce6eca34da93fa28407fad7f00595d60ba03d31 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 08:25:05 -0500 Subject: [PATCH 3/6] feat(facade): read VL strings and VL sequences through File VL-string datasets (h5py's default str dtype) failed read_string with "type mismatch: expected String, got VariableLength". read_string now reads fixed- and variable-length strings, with h5py's values (a string ends at a NUL, a null element is ""). New: - Dataset::read_string_bytes: each VL string's exact bytes; - Dataset::read_string_selection: hyperslabs/points of either kind; - Dataset::read_vlen::() and read_vlen_selection::(): VL sequences of numbers as Vec>, T in f64/f32/i64/i32/u64, converted like the other typed readers; - File::decode_strings / decode_string_bytes / decode_vlen: VL values in compound fields and AttrValue::Raw attributes; - MmapDataset and LazyDataset: read_string for VL strings, read_string_bytes and read_vlen. tests/vl_data_interop.rs checks every path against h5py with 8- and 4-byte offsets: scalar, 1-D and 2-D, ASCII and UTF-8, empty strings, contiguous, compact, chunked with gzip and shuffle, unwritten and partly written chunks, hyperslabs, compound members, attributes, a big-endian base type, and a patched file with an embedded NUL and mis-sized heap objects. NetCDF-4 string variables read too (netCDF4-python test). Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 18 + .../clawhdf5-netcdf4/tests/interop_tests.rs | 27 ++ crates/clawhdf5/src/lazy.rs | 40 +- crates/clawhdf5/src/lib.rs | 2 + crates/clawhdf5/src/mmap_file.rs | 40 +- crates/clawhdf5/src/reader.rs | 101 +++- crates/clawhdf5/src/vlen.rs | 143 ++++++ crates/clawhdf5/tests/vl_data_interop.rs | 444 ++++++++++++++++++ docs/known-issues.md | 11 +- 9 files changed, 818 insertions(+), 8 deletions(-) create mode 100644 crates/clawhdf5/src/vlen.rs create mode 100644 crates/clawhdf5/tests/vl_data_interop.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 40781cf..0a16e0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,24 @@ 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. +- **VL data through the facade.** VL-string datasets (h5py's default `str` + dtype) failed `read_string` with "type mismatch: expected String, got + VariableLength". `Dataset::read_string` now reads fixed- and + variable-length strings; new `read_string_bytes` (a VL string's exact + bytes, as h5py's `Dataset[()]` returns them), `read_string_selection`, + `read_vlen::()` / `read_vlen_selection::()` for VL sequences of + numbers (`T` = `f64`, `f32`, `i64`, `i32`, `u64`; converted like the + other typed readers), and `File::decode_strings` / `decode_string_bytes` + / `decode_vlen` for VL values in compound fields and `AttrValue::Raw` + attributes. `MmapDataset` and `LazyDataset` gain `read_string` for VL + strings, `read_string_bytes` and `read_vlen`. Checked against h5py with + 8- and 4-byte offsets: scalar and 1-/2-D, ASCII and UTF-8, empty strings, + contiguous, compact, chunked with gzip/shuffle, never-written and + partly written chunks, hyperslab selections, VL members of compound + datasets and attributes (`crates/clawhdf5/tests/vl_data_interop.rs`). + NetCDF-4 `string` variables now read through + `clawhdf5_netcdf4::Variable::read_string` (checked against netCDF4-python + in `crates/clawhdf5-netcdf4/tests/interop_tests.rs`). ### Plugin filters (2026-09-26) - **LZF, bitshuffle, bzip2 and Blosc read and write, in pure Rust.** Files diff --git a/crates/clawhdf5-netcdf4/tests/interop_tests.rs b/crates/clawhdf5-netcdf4/tests/interop_tests.rs index af75136..eba85ac 100644 --- a/crates/clawhdf5-netcdf4/tests/interop_tests.rs +++ b/crates/clawhdf5-netcdf4/tests/interop_tests.rs @@ -350,3 +350,30 @@ ds.close() let press_vals = press_var.read_raw_f32().unwrap(); assert_eq!(press_vals, vec![1000.0f32, 850.0, 500.0, 200.0]); } + +#[test] +fn netcdf4_python_string_variable_clawhdf5_reads() { + // NC_STRING variables are HDF5 variable-length strings, which + // `read_string` refused ("expected String, got VariableLength") until + // 2026-09-26. + skip_if_no_netcdf4!(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("strings.nc"); + let path_str = path.display().to_string(); + let script = format!( + r#" +import netCDF4 as nc +import numpy as np +ds = nc.Dataset("{path_str}", "w", format="NETCDF4") +ds.createDimension("station", 4) +v = ds.createVariable("name", str, ("station",)) +v[:] = np.array(["Oslo", "", "São Paulo", "x"], dtype=object) +ds.close() +"# + ); + run_python(&script); + + let file = NetCDF4File::open(&path).unwrap(); + let names = file.variable("name").unwrap().read_string().unwrap(); + assert_eq!(names, vec!["Oslo", "", "São Paulo", "x"]); +} diff --git a/crates/clawhdf5/src/lazy.rs b/crates/clawhdf5/src/lazy.rs index d893485..a33b9bb 100644 --- a/crates/clawhdf5/src/lazy.rs +++ b/crates/clawhdf5/src/lazy.rs @@ -422,11 +422,47 @@ impl<'f, R: HDF5Read> LazyDataset<'f, R> { Ok(data_read::read_as_u64(&raw, &dt)?) } - /// Read all data as `String` values. + /// Read all data as `String` values: fixed- or variable-length strings + /// (see [`Dataset::read_string`](crate::Dataset::read_string)). pub fn read_string(&self) -> Result, Error> { let raw = self.read_raw()?; let dt = self.datatype()?; - Ok(data_read::read_as_strings(&raw, &dt)?) + crate::vlen::decode_strings( + self.file.hdf5_bytes(), + &dt, + &raw, + self.file.offset_size(), + self.file.length_size(), + ) + } + + /// Read a variable-length string dataset as the exact bytes of each + /// string (see + /// [`Dataset::read_string_bytes`](crate::Dataset::read_string_bytes)). + pub fn read_string_bytes(&self) -> Result>, Error> { + let raw = self.read_raw()?; + let dt = self.datatype()?; + crate::vlen::decode_string_bytes( + self.file.hdf5_bytes(), + &dt, + &raw, + self.file.offset_size(), + self.file.length_size(), + ) + } + + /// Read a variable-length sequence dataset as one `Vec` per element + /// (see [`Dataset::read_vlen`](crate::Dataset::read_vlen)). + pub fn read_vlen(&self) -> Result>, Error> { + let raw = self.read_raw()?; + let dt = self.datatype()?; + crate::vlen::decode_vlen( + self.file.hdf5_bytes(), + &dt, + &raw, + self.file.offset_size(), + self.file.length_size(), + ) } /// Read all attributes of this dataset. diff --git a/crates/clawhdf5/src/lib.rs b/crates/clawhdf5/src/lib.rs index 12e2f69..f8098fa 100644 --- a/crates/clawhdf5/src/lib.rs +++ b/crates/clawhdf5/src/lib.rs @@ -30,6 +30,7 @@ pub mod lazy; pub mod mmap_file; pub mod reader; pub mod types; +pub mod vlen; pub mod writer; pub use error::Error; @@ -38,6 +39,7 @@ pub use lazy::{LazyDataset, LazyFile, LazyGroup}; pub use mmap_file::{MmapDataset, MmapFile, MmapGroup}; pub use reader::{Dataset, File, Group}; pub use types::{AttrValue, DType}; +pub use vlen::VlenValue; pub use writer::FileBuilder; #[cfg(feature = "parallel")] pub use writer::{DatasetSpec, create_datasets_parallel}; diff --git a/crates/clawhdf5/src/mmap_file.rs b/crates/clawhdf5/src/mmap_file.rs index 7f544ca..76d9ea1 100644 --- a/crates/clawhdf5/src/mmap_file.rs +++ b/crates/clawhdf5/src/mmap_file.rs @@ -336,11 +336,47 @@ impl<'f> MmapDataset<'f> { Ok(data_read::read_as_u64(&raw, &dt)?) } - /// Read all data as `String` values. + /// Read all data as `String` values: fixed- or variable-length strings + /// (see [`Dataset::read_string`](crate::Dataset::read_string)). pub fn read_string(&self) -> Result, Error> { let raw = self.read_raw()?; let dt = self.datatype()?; - Ok(data_read::read_as_strings(&raw, &dt)?) + crate::vlen::decode_strings( + self.file.hdf5_bytes(), + &dt, + &raw, + self.file.offset_size(), + self.file.length_size(), + ) + } + + /// Read a variable-length string dataset as the exact bytes of each + /// string (see + /// [`Dataset::read_string_bytes`](crate::Dataset::read_string_bytes)). + pub fn read_string_bytes(&self) -> Result>, Error> { + let raw = self.read_raw()?; + let dt = self.datatype()?; + crate::vlen::decode_string_bytes( + self.file.hdf5_bytes(), + &dt, + &raw, + self.file.offset_size(), + self.file.length_size(), + ) + } + + /// Read a variable-length sequence dataset as one `Vec` per element + /// (see [`Dataset::read_vlen`](crate::Dataset::read_vlen)). + pub fn read_vlen(&self) -> Result>, Error> { + let raw = self.read_raw()?; + let dt = self.datatype()?; + crate::vlen::decode_vlen( + self.file.hdf5_bytes(), + &dt, + &raw, + self.file.offset_size(), + self.file.length_size(), + ) } /// For contiguous datasets, return a zero-copy slice into the mmap. diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 83cbfa8..f0afa6d 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -262,6 +262,56 @@ impl File { } } + /// Decode the strings in `raw`, a buffer of elements of `datatype` read + /// from this file — for instance a variable-length string field of a + /// compound ([`clawhdf5_format::data_read::read_compound_fields`]) or an + /// [`AttrValue::Raw`] attribute. Variable-length strings are resolved in + /// this file's global heap; see [`Dataset::read_string`] for the values. + pub fn decode_strings(&self, datatype: &Datatype, raw: &[u8]) -> Result, Error> { + crate::vlen::decode_strings( + self.as_bytes(), + datatype, + raw, + self.offset_size(), + self.length_size(), + ) + } + + /// Like [`decode_strings`](Self::decode_strings) for variable-length + /// strings, returning each string's exact bytes (see + /// [`Dataset::read_string_bytes`]). + pub fn decode_string_bytes( + &self, + datatype: &Datatype, + raw: &[u8], + ) -> Result>, Error> { + crate::vlen::decode_string_bytes( + self.as_bytes(), + datatype, + raw, + self.offset_size(), + self.length_size(), + ) + } + + /// Decode the variable-length sequences in `raw`, a buffer of elements + /// of the sequence type `datatype` read from this file (a compound + /// field, an [`AttrValue::Raw`] attribute, ...). See + /// [`Dataset::read_vlen`]. + pub fn decode_vlen( + &self, + datatype: &Datatype, + raw: &[u8], + ) -> Result>, Error> { + crate::vlen::decode_vlen( + self.as_bytes(), + datatype, + raw, + self.offset_size(), + self.length_size(), + ) + } + fn parse_header(&self, address: u64) -> Result { ObjectHeader::parse( self.data.as_bytes(), @@ -498,11 +548,58 @@ impl<'f> Dataset<'f> { Ok(data_read::read_as_u64(&raw, &dt)?) } - /// Read all data as `String` values. + /// Read all data as `String` values, in row-major order. + /// + /// Works for fixed-length and variable-length string datasets (h5py's + /// default `str` dtype). A variable-length string ends at its first NUL + /// and a null element (e.g. never written) is `""`, as h5py returns + /// them; bytes that are not valid UTF-8 are replaced with U+FFFD — use + /// [`read_string_bytes`](Self::read_string_bytes) for the exact bytes. pub fn read_string(&self) -> Result, Error> { let raw = self.read_raw()?; let dt = self.datatype()?; - Ok(data_read::read_as_strings(&raw, &dt)?) + self.file.decode_strings(&dt, &raw) + } + + /// Read a variable-length string dataset as the exact bytes of each + /// string (what h5py's `Dataset[()]` returns), in row-major order. + pub fn read_string_bytes(&self) -> Result>, Error> { + let raw = self.read_raw()?; + let dt = self.datatype()?; + self.file.decode_string_bytes(&dt, &raw) + } + + /// Read the selected elements of a fixed- or variable-length string + /// dataset (see [`read_string`](Self::read_string)). + pub fn read_string_selection( + &self, + selection: &clawhdf5_format::selection::Selection, + ) -> Result, Error> { + let raw = self.read_selection(selection)?; + let dt = self.datatype()?; + self.file.decode_strings(&dt, &raw) + } + + /// Read a variable-length sequence dataset (h5py + /// `vlen_dtype(np.int32)`, ...) as one `Vec` per element, in + /// row-major order. The base type must be an integer or float type; it + /// is converted to `T` as [`read_f64`](Self::read_f64) and the other + /// typed readers convert. A null element is an empty sequence. + pub fn read_vlen(&self) -> Result>, Error> { + let raw = self.read_raw()?; + let dt = self.datatype()?; + self.file.decode_vlen(&dt, &raw) + } + + /// Read the selected elements of a variable-length sequence dataset + /// (see [`read_vlen`](Self::read_vlen)). + pub fn read_vlen_selection( + &self, + selection: &clawhdf5_format::selection::Selection, + ) -> Result>, Error> { + let raw = self.read_selection(selection)?; + let dt = self.datatype()?; + self.file.decode_vlen(&dt, &raw) } // ----- Selection-based read methods ----- diff --git a/crates/clawhdf5/src/vlen.rs b/crates/clawhdf5/src/vlen.rs new file mode 100644 index 0000000..01ba02d --- /dev/null +++ b/crates/clawhdf5/src/vlen.rs @@ -0,0 +1,143 @@ +//! Variable-length data: VL strings and VL sequences of numbers. +//! +//! A variable-length element stores a reference into the file's global heap; +//! these helpers resolve the references in a buffer of raw elements (from a +//! dataset read, a selection, a compound field or an [`AttrValue::Raw`] +//! attribute) against the file they came from. +//! +//! Values match libhdf5 (and h5py): a string ends at its first NUL, a null +//! element is an empty string or sequence, and a heap object whose size +//! disagrees with its element is an error rather than a truncated value. +//! +//! [`AttrValue::Raw`]: crate::AttrValue::Raw + +use clawhdf5_format::data_read; +use clawhdf5_format::datatype::Datatype; +use clawhdf5_format::error::FormatError; +use clawhdf5_format::vl_data::{VlResolver, check_element_size}; + +use crate::error::Error; + +mod sealed { + pub trait Sealed {} +} + +/// A number type that [`Dataset::read_vlen`](crate::Dataset::read_vlen) can +/// return: the sequence's base type is converted to it as libhdf5 converts +/// numbers (the same rules as `read_f64`, `read_i64`, ...). +pub trait VlenValue: sealed::Sealed + Sized { + #[doc(hidden)] + fn decode(raw: &[u8], base: &Datatype) -> Result, FormatError>; +} + +macro_rules! vlen_value { + ($t:ty, $f:path) => { + impl sealed::Sealed for $t {} + impl VlenValue for $t { + fn decode(raw: &[u8], base: &Datatype) -> Result, FormatError> { + $f(raw, base) + } + } + }; +} + +vlen_value!(f64, data_read::read_as_f64); +vlen_value!(f32, data_read::read_as_f32); +vlen_value!(i64, data_read::read_as_i64); +vlen_value!(i32, data_read::read_as_i32); +vlen_value!(u64, data_read::read_as_u64); + +fn class_name(dt: &Datatype) -> &'static str { + match dt { + Datatype::FixedPoint { .. } => "integer", + Datatype::FloatingPoint { .. } => "float", + Datatype::Time { .. } => "time", + Datatype::String { .. } => "fixed-length string", + Datatype::BitField { .. } => "bitfield", + Datatype::Opaque { .. } => "opaque", + Datatype::Compound { .. } => "compound", + Datatype::Reference { .. } => "reference", + Datatype::Enumeration { .. } => "enum", + Datatype::VariableLength { + is_string: true, .. + } => "variable-length string", + Datatype::VariableLength { .. } => "variable-length sequence", + Datatype::Array { .. } => "array", + } +} + +/// The strings in `raw`, elements of `dt`: fixed-length strings decoded as +/// `read_string` always has, variable-length strings resolved in the heap. +pub(crate) fn decode_strings( + file_data: &[u8], + dt: &Datatype, + raw: &[u8], + offset_size: u8, + length_size: u8, +) -> Result, Error> { + match dt { + Datatype::VariableLength { + size, + is_string: true, + .. + } => { + check_element_size(*size, offset_size)?; + Ok(VlResolver::new(file_data, offset_size, length_size).strings(raw)?) + } + _ => Ok(data_read::read_as_strings(raw, dt)?), + } +} + +/// The exact bytes of the variable-length strings in `raw`. +pub(crate) fn decode_string_bytes( + file_data: &[u8], + dt: &Datatype, + raw: &[u8], + offset_size: u8, + length_size: u8, +) -> Result>, Error> { + match dt { + Datatype::VariableLength { + size, + is_string: true, + .. + } => { + check_element_size(*size, offset_size)?; + Ok(VlResolver::new(file_data, offset_size, length_size).string_bytes(raw)?) + } + other => Err(Error::Format(FormatError::TypeMismatch { + expected: "variable-length string", + actual: class_name(other), + })), + } +} + +/// The sequences in `raw`, elements of the variable-length sequence type +/// `dt`, converted to `T`. +pub(crate) fn decode_vlen( + file_data: &[u8], + dt: &Datatype, + raw: &[u8], + offset_size: u8, + length_size: u8, +) -> Result>, Error> { + let Datatype::VariableLength { + size, + is_string: false, + base_type, + .. + } = dt + else { + return Err(Error::Format(FormatError::TypeMismatch { + expected: "variable-length sequence", + actual: class_name(dt), + })); + }; + check_element_size(*size, offset_size)?; + let base_size = base_type.type_size() as usize; + VlResolver::new(file_data, offset_size, length_size) + .sequences(raw, base_size)? + .iter() + .map(|bytes| Ok(T::decode(bytes, base_type)?)) + .collect() +} diff --git a/crates/clawhdf5/tests/vl_data_interop.rs b/crates/clawhdf5/tests/vl_data_interop.rs new file mode 100644 index 0000000..63f97f9 --- /dev/null +++ b/crates/clawhdf5/tests/vl_data_interop.rs @@ -0,0 +1,444 @@ +//! Variable-length data (VL strings and VL sequences) read through the +//! facade, checked against h5py/libhdf5. +//! +//! h5py writes each file — once with the default 8-byte offsets and once +//! with 4-byte offsets and lengths (`sizeof_addr = 4`) — and prints what +//! libhdf5 reads back; `File`, `MmapFile` and `LazyFile` must return the same +//! values. Skipped when python3 with h5py is unavailable, unless +//! `CLAWHDF5_REQUIRE_INTEROP=1`. + +// `Selection::slice(&[0..1])` is one range per dimension, not a Vec of a range. +#![allow(clippy::single_range_in_vec_init)] + +use std::collections::HashMap; +use std::path::Path; +use std::process::Command; + +use clawhdf5::{AttrValue, File, LazyFile, MmapFile, Selection}; + +fn python() -> String { + std::env::var("CLAWHDF5_PYTHON").unwrap_or_else(|_| "python3".to_string()) +} + +fn interop_required() -> bool { + std::env::var("CLAWHDF5_REQUIRE_INTEROP").is_ok_and(|v| v == "1") +} + +fn python_available() -> bool { + Command::new(python()) + .args(["-c", "import h5py, numpy"]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +macro_rules! skip_if_no_python { + () => { + if !python_available() { + assert!( + !interop_required(), + "CLAWHDF5_REQUIRE_INTEROP=1 but python3 with h5py is not available" + ); + eprintln!("SKIP: python3 with h5py not available"); + return; + } + }; +} + +/// Run `script` and return its stdout as `key -> value`, one +/// `keyvalue` line per key. +fn run_python(script: &str) -> HashMap { + let output = Command::new(python()) + .args(["-c", script]) + .output() + .expect("failed to run python"); + assert!( + output.status.success(), + "python failed:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout) + .lines() + .filter_map(|line| { + let (k, v) = line.split_once('\t')?; + Some((k.to_string(), v.to_string())) + }) + .collect() +} + +/// `hex,hex,...` -> the strings' bytes. +fn parse_strings(v: &str) -> Vec> { + v.split(',') + .map(|h| { + (0..h.len()) + .step_by(2) + .map(|i| u8::from_str_radix(&h[i..i + 2], 16).unwrap()) + .collect() + }) + .collect() +} + +/// `1 2 3|| -5` -> sequences. +fn parse_seqs(v: &str) -> Vec> { + v.split('|') + .map(|s| s.split_whitespace().map(|x| x.parse().unwrap()).collect()) + .collect() +} + +fn utf8(bytes: &[Vec]) -> Vec { + bytes + .iter() + .map(|b| String::from_utf8(b.clone()).unwrap()) + .collect() +} + +/// Writes `vl8.h5` (8-byte offsets) and `vl4.h5` (4-byte offsets and +/// lengths) into `dir` and prints h5py's reading of both. +const SCRIPT: &str = r#" +import sys, h5py, numpy as np +d = sys.argv[1] +S = h5py.string_dtype('utf-8'); A = h5py.string_dtype('ascii') +def make(path, sizes): + if sizes: + fcpl = h5py.h5p.create(h5py.h5p.FILE_CREATE); fcpl.set_sizes(*sizes) + f = h5py.File(h5py.h5f.create(path.encode(), h5py.h5f.ACC_TRUNC, fcpl=fcpl)) + else: + f = h5py.File(path, 'w') + f.create_dataset('scalar_utf8', data='héllo', dtype=S) + f.create_dataset('scalar_ascii', data=b'hello', dtype=A) + f.create_dataset('d1', data=np.array(['a', '', 'ccc', 'δδ'], dtype=object), dtype=S) + f.create_dataset('d2', data=np.array([['x', 'yy', 'zzz'], ['', 'w', 'vv']], dtype=object), dtype=S) + f.create_dataset('chunked', data=np.array(['s%d' % i * (i % 5) for i in range(100)], dtype=object), + dtype=S, chunks=(7,), compression='gzip') + f.create_dataset('chunked2d', data=np.array([['r%dc%d' % (r, c) for c in range(9)] for r in range(11)], dtype=object), + dtype=S, chunks=(4, 4), compression='gzip', shuffle=True) + f.create_dataset('unwritten', shape=(5,), dtype=S, chunks=(2,)) + p = f.create_dataset('partial', shape=(6,), dtype=S, chunks=(2,)); p[0] = 'first'; p[5] = 'last' + f.create_dataset('contig_empty', shape=(3,), dtype=S) + dcpl = h5py.h5p.create(h5py.h5p.DATASET_CREATE); dcpl.set_layout(h5py.h5d.COMPACT) + f.create_dataset('compact', data=np.array(['c1', '', 'c3'], dtype=object), dtype=S, dcpl=dcpl) + assert f['compact'].id.get_create_plist().get_layout() == h5py.h5d.COMPACT + f.attrs['vlattr'] = 'attr-value' + f.attrs.create('vlattr_arr', np.array(['p', 'qq', ''], dtype=object), dtype=S) + ct = np.dtype([('id', 'u2'))) + v[0] = [1, 65535]; v[1] = [300] + f.attrs.create('vlen_attr', np.array([np.array([1, 2], dtype=' HashMap { + let script = format!( + "import sys; sys.argv = ['x', {:?}]\n{SCRIPT}", + dir.display().to_string() + ); + run_python(&script) +} + +const STRING_DATASETS: [&str; 10] = [ + "compact", + "scalar_utf8", + "scalar_ascii", + "d1", + "d2", + "chunked", + "chunked2d", + "unwritten", + "partial", + "contig_empty", +]; + +#[test] +fn vl_string_datasets_read_like_h5py() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let expected = make_files(dir.path()); + for tag in ["8", "4"] { + let path = dir.path().join(format!("vl{tag}.h5")); + let file = File::open(&path).unwrap(); + let mmap = MmapFile::open(&path).unwrap(); + let lazy = LazyFile::open_mmap(&path).unwrap(); + for name in STRING_DATASETS { + let want = parse_strings(&expected[&format!("{tag}:{name}")]); + let ctx = format!("vl{tag}.h5 {name}"); + let ds = file.dataset(name).unwrap(); + assert_eq!(ds.read_string_bytes().unwrap(), want, "{ctx}"); + assert_eq!(ds.read_string().unwrap(), utf8(&want), "{ctx}"); + let m = mmap.dataset(name).unwrap(); + assert_eq!(m.read_string_bytes().unwrap(), want, "{ctx} (mmap)"); + assert_eq!(m.read_string().unwrap(), utf8(&want), "{ctx} (mmap)"); + let l = lazy.dataset(name).unwrap(); + assert_eq!(l.read_string_bytes().unwrap(), want, "{ctx} (lazy)"); + assert_eq!(l.read_string().unwrap(), utf8(&want), "{ctx} (lazy)"); + } + } +} + +#[test] +fn vl_string_selections_read_like_h5py() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let expected = make_files(dir.path()); + let hyperslab = |start: &[u64], stride: &[u64], count: &[u64]| Selection::Hyperslab { + start: start.to_vec(), + stride: stride.to_vec(), + count: count.to_vec(), + block: vec![1; start.len()], + }; + let cases = [ + ("d2", "d2[1,1:3]", Selection::slice(&[1..2, 1..3])), + ("chunked", "chunked[5:60:3]", hyperslab(&[5], &[3], &[19])), + ( + "chunked2d", + "chunked2d[2:9:2,3:8]", + hyperslab(&[2, 3], &[2, 1], &[4, 5]), + ), + ]; + for tag in ["8", "4"] { + let file = File::open(dir.path().join(format!("vl{tag}.h5"))).unwrap(); + for (name, key, sel) in &cases { + let want = utf8(&parse_strings(&expected[&format!("{tag}:{key}")])); + let got = file + .dataset(name) + .unwrap() + .read_string_selection(sel) + .unwrap(); + assert_eq!(got, want, "vl{tag}.h5 {key}"); + } + // A selection of VL integers is not strings. + assert!( + file.dataset("vlen_i4") + .unwrap() + .read_string_selection(&Selection::slice(&[0..1])) + .is_err() + ); + } +} + +#[test] +fn vl_values_in_compounds_and_attributes_read_like_h5py() { + // With 4-byte offsets these failed with GlobalHeapObjectNotFound or came + // back as `AttrValue::Raw`: the VL type claimed 16-byte elements and the + // global heap was read without the padding libhdf5 puts after its + // headers. + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let expected = make_files(dir.path()); + for tag in ["8", "4"] { + let file = File::open(dir.path().join(format!("vl{tag}.h5"))).unwrap(); + let want = |key: &str| utf8(&parse_strings(&expected[&format!("{tag}:{key}")])); + + let attrs = file.root().attrs().unwrap(); + match &attrs["vlattr"] { + AttrValue::String(s) => assert_eq!(*s, want("vlattr")[0], "vl{tag}.h5"), + other => panic!("vl{tag}.h5 vlattr: {other:?}"), + } + match &attrs["vlattr_arr"] { + AttrValue::StringArray(s) => assert_eq!(*s, want("vlattr_arr"), "vl{tag}.h5"), + other => panic!("vl{tag}.h5 vlattr_arr: {other:?}"), + } + + // Compound with a VL string member: dataset and attribute. + let ds = file.dataset("compound").unwrap(); + let dt = ds.raw_datatype().unwrap(); + let raw = ds.read_selection(&Selection::All).unwrap(); + let fields = clawhdf5_format::data_read::read_compound_fields(&raw, &dt).unwrap(); + let name = fields.iter().find(|f| f.name == "name").unwrap(); + assert_eq!( + file.decode_strings(&name.datatype, &name.raw_data).unwrap(), + want("compound.name"), + "vl{tag}.h5 compound" + ); + let id = fields.iter().find(|f| f.name == "id").unwrap(); + assert_eq!( + clawhdf5_format::data_read::read_as_i64(&id.raw_data, &id.datatype).unwrap(), + vec![1, 2, 3] + ); + let v = fields.iter().find(|f| f.name == "v").unwrap(); + assert_eq!( + clawhdf5_format::data_read::read_as_f64(&v.raw_data, &v.datatype).unwrap(), + vec![0.5, 1.5, 2.5] + ); + + let AttrValue::Raw { datatype, data, .. } = &attrs["compound_attr"] else { + panic!("compound attribute is Raw"); + }; + let fields = clawhdf5_format::data_read::read_compound_fields(data, datatype).unwrap(); + let name = fields.iter().find(|f| f.name == "name").unwrap(); + assert_eq!( + file.decode_strings(&name.datatype, &name.raw_data).unwrap(), + want("compound_attr.name"), + "vl{tag}.h5 compound attribute" + ); + + // A VL sequence attribute. + let AttrValue::Raw { datatype, data, .. } = &attrs["vlen_attr"] else { + panic!("vlen attribute is Raw"); + }; + let got: Vec> = file.decode_vlen(datatype, data).unwrap(); + let want_seqs = parse_seqs(&expected[&format!("{tag}:vlen_attr")]); + assert_eq!( + got, + want_seqs + .iter() + .map(|s| s.iter().map(|&x| x as i64).collect::>()) + .collect::>(), + "vl{tag}.h5 vlen_attr" + ); + } +} + +#[test] +fn vl_sequence_datasets_read_like_h5py() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let expected = make_files(dir.path()); + for tag in ["8", "4"] { + let path = dir.path().join(format!("vl{tag}.h5")); + let file = File::open(&path).unwrap(); + let seqs = |key: &str| parse_seqs(&expected[&format!("{tag}:{key}")]); + + let i4 = file.dataset("vlen_i4").unwrap(); + let want: Vec> = seqs("vlen_i4") + .iter() + .map(|s| s.iter().map(|&x| x as i32).collect()) + .collect(); + assert_eq!(i4.read_vlen::().unwrap(), want, "vl{tag}.h5 vlen_i4"); + let as_f64: Vec> = i4.read_vlen().unwrap(); + assert_eq!(as_f64, seqs("vlen_i4")); + + let f8 = file.dataset("vlen_f8").unwrap(); + assert_eq!( + f8.read_vlen::().unwrap(), + seqs("vlen_f8"), + "vl{tag}.h5" + ); + assert_eq!( + f8.read_vlen_selection::(&Selection::slice(&[1..2, 0..2])) + .unwrap(), + seqs("vlen_f8[1,:]"), + "vl{tag}.h5 vlen_f8[1,:]" + ); + + // h5py returns big-endian VL elements byte-swapped (an h5py bug, see + // CONFORMANCE.md); the values written are [1, 65535] and [300]. + assert_eq!( + file.dataset("vlen_u2_be") + .unwrap() + .read_vlen::() + .unwrap(), + vec![vec![1, 65535], vec![300]] + ); + + let mmap = MmapFile::open(&path).unwrap(); + assert_eq!( + mmap.dataset("vlen_f8").unwrap().read_vlen::().unwrap(), + seqs("vlen_f8") + ); + let lazy = LazyFile::open_mmap(&path).unwrap(); + assert_eq!( + lazy.dataset("vlen_f8").unwrap().read_vlen::().unwrap(), + seqs("vlen_f8") + ); + + // Wrong kind of data is an error, not a value. + assert!(i4.read_string().is_err()); + assert!(i4.read_string_bytes().is_err()); + assert!(file.dataset("d1").unwrap().read_vlen::().is_err()); + } +} + +#[test] +fn vl_strings_end_at_nul_and_mis_sized_elements_fail_like_h5py() { + // h5py cannot write a VL string with a NUL in it, so the file is patched: + // one string gets an embedded NUL, and two elements get a length that + // disagrees with their heap object. libhdf5 returns the string up to the + // NUL and refuses the others ("Expected global heap object size does + // not match"); we used to return the NUL and a truncated string. + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("patched.h5"); + let script = format!( + r#" +import struct, h5py, numpy as np +path = {path:?} +with h5py.File(path, 'w') as f: + f.create_dataset('d', data=np.array(['aXb', 'cdefgh', 'ij', 'ok'], dtype=object), + dtype=h5py.string_dtype()) + s = f.create_dataset('seq', shape=(2,), dtype=h5py.vlen_dtype(np.dtype(' 3 +struct.pack_into(' 9 +struct.pack_into(' 2 +open(path, 'wb').write(bytes(b)) +with h5py.File(path, 'r') as f: + for i in range(4): + try: + print('d%d\t%s' % (i, f['d'][i].hex())) + except OSError as e: + print('d%d\terror' % i) + for i in range(2): + try: + print('seq%d\t%s' % (i, ' '.join(str(x) for x in f['seq'][i]))) + except OSError as e: + print('seq%d\terror' % i) +"#, + path = path.display().to_string() + ); + let expected = run_python(&script); + assert_eq!(expected["d0"], "61", "h5py cuts 'a\\0b' at the NUL"); + assert_eq!(expected["d1"], "error"); + assert_eq!(expected["d2"], "error"); + assert_eq!(expected["d3"], "6f6b"); + assert_eq!(expected["seq0"], "error"); + assert_eq!(expected["seq1"], "4"); + + let file = File::open(&path).unwrap(); + let d = file.dataset("d").unwrap(); + let one = |i: u64| d.read_string_selection(&Selection::slice(&[i..i + 1])); + assert_eq!(one(0).unwrap(), vec!["a"]); + assert!(one(1).is_err()); + assert!(one(2).is_err()); + assert_eq!(one(3).unwrap(), vec!["ok"]); + assert!(d.read_string().is_err()); + let seq = file.dataset("seq").unwrap(); + let one = |i: u64| seq.read_vlen_selection::(&Selection::slice(&[i..i + 1])); + assert!(one(0).is_err()); + assert_eq!(one(1).unwrap(), vec![vec![4]]); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index b9a19b6..4ea8307 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -140,7 +140,13 @@ fill-value item that did is fixed). is left out of `attrs()` (reported by `attrs_with_errors()`) instead of failing the others. - **Other readers:** - - VL-string datasets are not readable through `File`. + - VL-string datasets are not readable through `File`. **Fixed + 2026-09-26:** `read_string` reads them (also `read_string_bytes`, + `read_string_selection`, and on `MmapFile`/`LazyFile`), with h5py's + values: strings end at a NUL, null elements are `""`; VL sequences of + numbers read with `read_vlen::()`, and VL values inside compounds or + `AttrValue::Raw` attributes decode with `File::decode_strings` / + `File::decode_vlen` (`crates/clawhdf5/tests/vl_data_interop.rs`). - Variable-length values inside a compound (and VL-string attributes) in a file with 4-byte offsets (`sizeof_addr = 4`) fail with `GlobalHeapObjectNotFound` or come back as `Raw`: these paths assume @@ -505,7 +511,8 @@ which is what libhdf5 itself writes. followed (no file system). - Variable-length string datasets are read by decoding `read_selection`'s bytes with `clawhdf5_format::vl_data` in the wasm crate; `File` itself still - cannot (see the audit gaps above). + cannot (see the audit gaps above). (`File` can since 2026-09-26; the wasm + crate still decodes them itself.) ## The Node.js package (`packages/clawhdf5-node`) does not work From 41b7837d0acac060b8fe1f484c9474eae2551901 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 08:56:10 -0500 Subject: [PATCH 4/6] fix(format): bound what a VL read retains on a crafted global heap VlResolver kept an owned copy of every object of every heap collection it parsed, for the whole read. Collections nested inside each other's object data, 32 bytes apart with each element pointing at a different one, made retained memory O(elements x file size): 1.58 GB for a 744 KB file (read_vl_strings did the same before VlResolver). Chaining every collection's objects into one shared run of tiny objects made parse time O(elements x objects) as well. libhdf5 refuses these files. - The cache records where each object lies (GlobalHeapCollection:: parse_index, new) instead of copying it, and is dropped past a 32 MiB budget. - A collection overlapping one already read is an error: libhdf5 gives every collection its own block, so only a crafted file has them. - parse and parse_index refuse a collection that runs past the end of the file and an object that runs past the end of its collection. tests/vl_heap_bounds.rs measures peak heap use with a counting allocator: 129 MB and 350 MB live before on its two crafted files (64 KB and 176 KB), 97 KB and 0.9 MB now. Conformance unchanged at 575 of 697. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 12 + crates/clawhdf5-format/src/global_heap.rs | 108 +++++++-- crates/clawhdf5-format/src/vl_data.rs | 207 ++++++++++++++---- .../clawhdf5-format/tests/vl_heap_bounds.rs | 161 ++++++++++++++ docs/known-issues.md | 26 +++ 5 files changed, 453 insertions(+), 61 deletions(-) create mode 100644 crates/clawhdf5-format/tests/vl_heap_bounds.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a16e0a..847f3c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,18 @@ `clawhdf5_netcdf4::Variable::read_string` (checked against netCDF4-python in `crates/clawhdf5-netcdf4/tests/interop_tests.rs`). +- **Crafted global heaps could exhaust memory.** `VlResolver` kept an owned + copy of every object of every heap collection it read, so collections + nested inside each other's object data made a 744 KB file take 1.58 GB + (and `read_vl_strings` before it did the same). The cache now records + where objects lie instead of copying them, is dropped past a 32 MiB + budget, and a collection overlapping one already read is an error + (libhdf5 never writes one). New `GlobalHeapCollection::parse_index` + locates a collection's objects without copying them; `parse` and + `parse_index` refuse a collection running past the end of the file or an + object running past its collection. Conformance unchanged at 575 of 697 + (`crates/clawhdf5-format/tests/vl_heap_bounds.rs`). + ### Plugin filters (2026-09-26) - **LZF, bitshuffle, bzip2 and Blosc read and write, in pure Rust.** Files written by h5py with `compression="lzf"`, or with hdf5plugin's diff --git a/crates/clawhdf5-format/src/global_heap.rs b/crates/clawhdf5-format/src/global_heap.rs index 5eab713..474c7fd 100644 --- a/crates/clawhdf5-format/src/global_heap.rs +++ b/crates/clawhdf5-format/src/global_heap.rs @@ -1,7 +1,7 @@ //! HDF5 Global Heap collection parsing. #[cfg(not(feature = "std"))] -use alloc::vec::Vec; +use alloc::{format, string::String, vec::Vec}; use crate::error::FormatError; @@ -52,11 +52,42 @@ fn read_length(data: &[u8], offset: usize, length_size: u8) -> Result String { + format!( + "global heap object {index} ({size} bytes) runs past the end of its \ + {collection_size}-byte collection" + ) +} + /// Round up to next multiple of 8. fn pad8(x: usize) -> usize { (x + 7) & !7 } +/// Where one object of a global heap collection lies in the file, without +/// its data: see [`GlobalHeapCollection::parse_index`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct GlobalHeapObjectRef { + /// Object index (1-based; 0 is the free space marker). + pub index: u16, + /// Reference count. + pub reference_count: u16, + /// Offset of the object's data in the file data the collection was + /// parsed from. + pub offset: usize, + /// Size of the object's data in bytes. + pub size: usize, +} + +/// A global heap collection's objects, located but not copied. +#[derive(Debug, Clone)] +pub struct GlobalHeapIndex { + /// Total size of this collection including header. + pub collection_size: u64, + /// The objects, in file order. + pub objects: Vec, +} + impl GlobalHeapCollection { /// Parse a global heap collection at the given offset in the file data. pub fn parse( @@ -64,6 +95,33 @@ impl GlobalHeapCollection { offset: usize, length_size: u8, ) -> Result { + let index = Self::parse_index(file_data, offset, length_size)?; + Ok(GlobalHeapCollection { + collection_size: index.collection_size, + objects: index + .objects + .iter() + .map(|o| GlobalHeapObject { + index: o.index, + reference_count: o.reference_count, + data: file_data[o.offset..o.offset + o.size].to_vec(), + }) + .collect(), + }) + } + + /// Locate the objects of the global heap collection at `offset` without + /// copying their data, so a caller can keep many collections indexed + /// for the cost of their object headers. + /// + /// The collection must lie inside `file_data`, and every object inside + /// the collection, as libhdf5 lays them out; an object that runs past + /// its collection is an error. + pub fn parse_index( + file_data: &[u8], + offset: usize, + length_size: u8, + ) -> Result { // signature(4) + version(1) + reserved(3) + collection_size(length_size), // padded to a multiple of 8 as libhdf5 lays it out (`H5HG_SIZEOF_HDR`). // With 8-byte lengths the padding is 0; with 4-byte lengths it is 4, @@ -81,25 +139,25 @@ impl GlobalHeapCollection { } let collection_size = read_length(file_data, offset + 8, length_size)?; - let collection_size_usize = - usize::try_from(collection_size).map_err(|_| FormatError::UnexpectedEof { - expected: u64::MAX as usize, + let collection_end = usize::try_from(collection_size) + .ok() + .and_then(|size| offset.checked_add(size)) + .ok_or(FormatError::UnexpectedEof { + expected: usize::MAX, available: file_data.len(), })?; - let collection_end = - offset - .checked_add(collection_size_usize) - .ok_or(FormatError::UnexpectedEof { - expected: usize::MAX, - available: file_data.len(), - })?; + if collection_end > file_data.len() { + return Err(FormatError::UnexpectedEof { + expected: collection_end, + available: file_data.len(), + }); + } let mut pos = offset + header_size; let mut objects = Vec::new(); // Parse objects until we hit index 0 (free space) or run out of space while pos + 2 <= collection_end { - ensure_len(file_data, pos, 2)?; let object_index = u16::from_le_bytes([file_data[pos], file_data[pos + 1]]); if object_index == 0 { @@ -110,26 +168,36 @@ impl GlobalHeapCollection { // object_index(2) + reference_count(2) + reserved(4) + // object_size(length_size), padded to 8 (`H5HG_SIZEOF_OBJHDR`). let obj_header_size = pad8(8 + length_size as usize); - ensure_len(file_data, pos, obj_header_size)?; + ensure_len(&file_data[..collection_end], pos, obj_header_size)?; let reference_count = u16::from_le_bytes([file_data[pos + 2], file_data[pos + 3]]); - let object_size = read_length(file_data, pos + 8, length_size)? as usize; + let object_size = usize::try_from(read_length(file_data, pos + 8, length_size)?) + .map_err(|_| FormatError::Overflow("global heap object size".into()))?; pos += obj_header_size; - ensure_len(file_data, pos, object_size)?; - let data = file_data[pos..pos + object_size].to_vec(); + if pos + .checked_add(object_size) + .is_none_or(|end| end > collection_end) + { + return Err(FormatError::VlDataError(object_overrun_msg( + object_index, + object_size, + collection_size, + ))); + } - objects.push(GlobalHeapObject { + objects.push(GlobalHeapObjectRef { index: object_index, reference_count, - data, + offset: pos, + size: object_size, }); // Advance past data + padding to 8-byte boundary - pos += pad8(object_size); + pos = pos.saturating_add(pad8(object_size)); } - Ok(GlobalHeapCollection { + Ok(GlobalHeapIndex { collection_size, objects, }) diff --git a/crates/clawhdf5-format/src/vl_data.rs b/crates/clawhdf5-format/src/vl_data.rs index 58015a4..f183ef0 100644 --- a/crates/clawhdf5-format/src/vl_data.rs +++ b/crates/clawhdf5-format/src/vl_data.rs @@ -10,7 +10,7 @@ use alloc::{collections::BTreeMap, format, string::String, vec, vec::Vec}; use std::collections::BTreeMap; use crate::error::FormatError; -use crate::global_heap::GlobalHeapCollection; +use crate::global_heap::{GlobalHeapCollection, GlobalHeapIndex}; /// A parsed variable-length element reference (global heap ID). #[derive(Debug, Clone)] @@ -134,38 +134,43 @@ pub fn check_element_size(stored_size: u32, offset_size: u8) -> Result<(), Forma Ok(()) } -/// A parsed collection, with its objects indexed for lookup. +/// A collection's objects, located in the file data but not copied: +/// `(index, offset, size)` of the first object with each index, sorted by +/// index. struct CachedCollection { - collection: GlobalHeapCollection, - /// `slots[index]` is the position in `collection.objects` of the first - /// object with that index. - slots: Vec>, + objects: Vec<(u16, usize, usize)>, } impl CachedCollection { - fn new(collection: GlobalHeapCollection) -> Self { - let max = collection + fn new(index: GlobalHeapIndex) -> Self { + let mut objects: Vec<(u16, usize, usize)> = index .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 } + .map(|o| (o.index, o.offset, o.size)) + .collect(); + // Stable, so the first object with a repeated index is kept. + objects.sort_by_key(|o| o.0); + objects.dedup_by_key(|o| o.0); + Self { objects } } - fn get(&self, index: u32) -> Option<&[u8]> { - let pos = (*self.slots.get(usize::try_from(index).ok()?)?)?; - Some(&self.collection.objects[pos].data) + /// What this entry costs to keep, in bytes (roughly). + fn cost(&self) -> usize { + 64 + self.objects.len() * core::mem::size_of::<(u16, usize, usize)>() + } + + fn get(&self, index: u32) -> Option<(usize, usize)> { + let index = u16::try_from(index).ok()?; + let i = self.objects.binary_search_by_key(&index, |o| o.0).ok()?; + Some((self.objects[i].1, self.objects[i].2)) } } +/// How many bytes of collection indexes a [`VlResolver`] keeps before it +/// drops them and starts again. Values are never copied into the cache, so +/// this bounds what a read retains however many collections it visits. +const CACHE_BUDGET: usize = 32 << 20; + /// Resolves variable-length elements against a file's global heap, parsing /// each heap collection once however many elements point into it. /// @@ -173,11 +178,22 @@ impl CachedCollection { /// 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. +/// +/// Memory stays bounded on hostile files: the cache holds where each +/// object lies, not a copy of it, up to a fixed budget; and collections +/// that overlap one another are refused (libhdf5 never writes them), so a +/// file cannot make the resolver parse the same bytes as the objects of +/// many collections. pub struct VlResolver<'a> { file_data: &'a [u8], offset_size: u8, length_size: u8, cache: BTreeMap, + cached_bytes: usize, + budget: usize, + /// Start → end of every collection parsed so far (kept when the cache + /// is dropped, to check overlaps). + extents: BTreeMap, } impl<'a> VlResolver<'a> { @@ -189,6 +205,9 @@ impl<'a> VlResolver<'a> { offset_size, length_size, cache: BTreeMap::new(), + cached_bytes: 0, + budget: CACHE_BUDGET, + extents: BTreeMap::new(), } } @@ -210,11 +229,18 @@ impl<'a> VlResolver<'a> { } /// 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> { + /// or `None` for a null element. + fn resolve( + &mut self, + vl: &VlElement, + base_size: usize, + ) -> Result, FormatError> { let addr = vl.collection_address; - if addr == 0 || (vl.length == 0 && is_undefined_address(addr, self.offset_size)) { - return Ok(&[]); + if addr == 0 { + return Ok(None); + } + if vl.length == 0 && is_undefined_address(addr, self.offset_size) { + return Ok(Some(&[])); } let data = self.object(vl)?; let expected = (vl.length as usize) @@ -229,7 +255,27 @@ impl<'a> VlResolver<'a> { vl.length ))); } - Ok(data) + Ok(Some(data)) + } + + /// One element (the first [`element_size`](Self::element_size) bytes of + /// `elem`) of a variable-length sequence whose base type is `base_size` + /// bytes: its `length × base_size` bytes, or `None` for a null element + /// (heap address 0). + pub fn element( + &mut self, + elem: &[u8], + base_size: usize, + ) -> Result, FormatError> { + let vl = parse_vl_references(elem, 1, self.offset_size)?; + self.resolve(&vl[0], base_size) + } + + /// One variable-length string element: its bytes up to the first NUL, + /// or `None` for a null element (h5dump prints it as `NULL`, h5py + /// returns it as empty). + pub fn string_element(&mut self, elem: &[u8]) -> Result, FormatError> { + Ok(self.element(elem, 1)?.map(cut_at_nul)) } /// The strings of the variable-length string elements in `raw`, as @@ -238,11 +284,7 @@ impl<'a> VlResolver<'a> { 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()) - }) + .map(|vl| Ok(self.resolve(vl, 1)?.map(cut_at_nul).unwrap_or(&[]).to_vec())) .collect() } @@ -270,11 +312,16 @@ impl<'a> VlResolver<'a> { } self.elements(raw)? .iter() - .map(|vl| self.resolve(vl, base_size).map(<[u8]>::to_vec)) + .map(|vl| Ok(self.resolve(vl, base_size)?.unwrap_or(&[]).to_vec())) .collect() } } +/// A string's bytes up to its first NUL. +fn cut_at_nul(s: &[u8]) -> &[u8] { + &s[..s.iter().position(|&b| b == 0).unwrap_or(s.len())] +} + /// 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 @@ -342,25 +389,66 @@ pub fn read_vl_bytes( Ok(result) } -impl VlResolver<'_> { +impl<'a> VlResolver<'a> { /// 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> { + fn object(&mut self, vl: &VlElement) -> Result<&'a [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)); + let index = + GlobalHeapCollection::parse_index(self.file_data, offset, self.length_size)?; + // parse_index checked that the collection lies in the file. + let end = offset + index.collection_size as usize; + self.check_overlap(offset, end)?; + let coll = CachedCollection::new(index); + if self.cached_bytes.saturating_add(coll.cost()) > self.budget { + self.cache.clear(); + self.cached_bytes = 0; + } + self.cached_bytes += coll.cost(); + self.cache.insert(addr, coll); } - self.cache[&addr] - .get(vl.object_index) - .ok_or(FormatError::GlobalHeapObjectNotFound { + let (start, size) = self.cache[&addr].get(vl.object_index).ok_or( + FormatError::GlobalHeapObjectNotFound { collection_address: addr, index: vl.object_index as u16, - }) + }, + )?; + Ok(&self.file_data[start..start + size]) + } + + /// Record the collection at `start..end`, refusing one that overlaps a + /// collection already read. libhdf5 allocates each collection its own + /// block; overlapping ones only come from a crafted file, where they let + /// every byte be parsed again as the objects of each collection. + fn check_overlap(&mut self, start: usize, end: usize) -> Result<(), FormatError> { + if let Some(&known) = self.extents.get(&start) { + return if known == end { + Ok(()) + } else { + Err(FormatError::VlDataError(format!( + "global heap collection at {start} changed size" + ))) + }; + } + let before = self.extents.range(..start).next_back(); + let after = self.extents.range(start..).next(); + let clash = match (before, after) { + (Some((&s, &e)), _) if e > start => Some(s), + (_, Some((&s, _))) if s < end => Some(s), + _ => None, + }; + if let Some(other) = clash { + return Err(FormatError::VlDataError(format!( + "global heap collection at {start} overlaps the one at {other}" + ))); + } + self.extents.insert(start, end); + Ok(()) } } @@ -581,6 +669,43 @@ mod tests { assert!(r.strings(&raw[..30]).is_err()); } + #[test] + fn the_cache_stays_within_its_budget_and_rereads_what_it_dropped() { + // Twenty collections of three objects each; a budget that holds + // about two of them. Reading every element twice must still return + // the right strings after the cache is dropped. + let mut file_data = vec![0u8; 64]; + let mut raw = Vec::new(); + for c in 0..20u64 { + let at = file_data.len(); + let names: Vec = (0..3).map(|i| format!("c{c}o{i}")).collect(); + let objs: Vec<(u16, &[u8])> = names + .iter() + .enumerate() + .map(|(i, n)| (i as u16 + 1, n.as_bytes())) + .collect(); + build_gcol_at(&mut file_data, at, &objs); + for (i, n) in names.iter().enumerate() { + raw.extend(element(n.len() as u32, at as u64, i as u32 + 1, 8)); + } + } + raw.extend(raw.clone()); + let mut r = VlResolver::new(&file_data, 8, 8); + let one = CachedCollection { + objects: vec![(0, 0, 0); 3], + } + .cost(); + r.budget = 2 * one + 1; + let want: Vec = (0..2) + .flat_map(|_| (0..20).flat_map(|c| (0..3).map(move |i| format!("c{c}o{i}")))) + .collect(); + for (k, chunk) in raw.chunks(16).enumerate() { + assert_eq!(r.strings(chunk).unwrap(), [want[k].clone()]); + assert!(r.cached_bytes <= r.budget); + assert!(r.cache.len() <= 2); + } + } + #[test] fn element_size_is_checked_against_the_offset_size() { assert!(check_element_size(16, 8).is_ok()); diff --git a/crates/clawhdf5-format/tests/vl_heap_bounds.rs b/crates/clawhdf5-format/tests/vl_heap_bounds.rs new file mode 100644 index 0000000..2993087 --- /dev/null +++ b/crates/clawhdf5-format/tests/vl_heap_bounds.rs @@ -0,0 +1,161 @@ +//! Crafted files cannot make variable-length reads retain memory, or take +//! time, out of proportion to the file. +//! +//! `VlResolver` used to keep an owned copy of every object of every +//! collection it parsed, for the whole read. A file whose global heap +//! collections nest inside each other's object data — each element +//! pointing at a different one — then made retained memory O(K × file +//! size): a 744 KB file took 1.58 GB. The same nesting, with every +//! collection's object chain jumping to one shared run of tiny objects, +//! made the parse time O(K × M) as well. libhdf5 never writes overlapping +//! collections; they are now refused, and the cache holds only where +//! objects lie. +//! +//! Peak heap use is measured with a counting global allocator, so the +//! cases run one after another in a single test. + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::{Duration, Instant}; + +use clawhdf5_format::vl_data::VlResolver; + +struct Counting; + +static CURRENT: AtomicUsize = AtomicUsize::new(0); +static PEAK: AtomicUsize = AtomicUsize::new(0); + +unsafe impl GlobalAlloc for Counting { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + let p = unsafe { System.alloc(layout) }; + if !p.is_null() { + let now = CURRENT.fetch_add(layout.size(), Ordering::Relaxed) + layout.size(); + PEAK.fetch_max(now, Ordering::Relaxed); + } + p + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + unsafe { System.dealloc(ptr, layout) }; + CURRENT.fetch_sub(layout.size(), Ordering::Relaxed); + } +} + +#[global_allocator] +static ALLOC: Counting = Counting; + +/// Bytes allocated at the peak of `f`, above what was live when it started. +fn peak_during(f: impl FnOnce() -> T) -> (T, usize) { + let base = CURRENT.load(Ordering::Relaxed); + PEAK.store(base, Ordering::Relaxed); + let out = f(); + (out, PEAK.load(Ordering::Relaxed) - base) +} + +fn put_header(file: &mut [u8], at: usize, size: u64) { + file[at..at + 4].copy_from_slice(b"GCOL"); + file[at + 4] = 1; + file[at + 8..at + 16].copy_from_slice(&size.to_le_bytes()); +} + +fn put_object(file: &mut [u8], at: usize, index: u16, size: u64) { + file[at..at + 2].copy_from_slice(&index.to_le_bytes()); + file[at + 2..at + 4].copy_from_slice(&1u16.to_le_bytes()); + file[at + 8..at + 16].copy_from_slice(&size.to_le_bytes()); +} + +fn element(length: u32, addr: u64, index: u32) -> Vec { + let mut e = length.to_le_bytes().to_vec(); + e.extend_from_slice(&addr.to_le_bytes()); + e.extend_from_slice(&index.to_le_bytes()); + e +} + +/// K collections 32 bytes apart, each running to the end of the file with +/// one object covering the rest of it (and so every later collection). +/// Element i is that object of collection i. +fn nested(k: usize) -> (Vec, Vec) { + let base = 64; + let end = base + 32 * k + 64; + let mut file = vec![0u8; end]; + let mut raw = Vec::new(); + for i in 0..k { + let at = base + 32 * i; + put_header(&mut file, at, (end - at) as u64); + let obj = (end - at - 32) as u64; + put_object(&mut file, at + 16, 1, obj); + raw.extend(element(obj as u32, at as u64, 1)); + } + (file, raw) +} + +/// K collections 32 bytes apart, each with a first object that jumps over +/// the later collections to one shared run of M empty objects, so parsing +/// every collection walks all M. +fn shared_tail(k: usize, m: usize) -> (Vec, Vec) { + let base = 64; + let tail = base + 32 * k + 32; + let end = tail + 16 * m + 16; + let mut file = vec![0u8; end]; + let mut raw = Vec::new(); + for i in 0..k { + let at = base + 32 * i; + put_header(&mut file, at, (end - at) as u64); + let jump = (tail - at - 32) as u64; + put_object(&mut file, at + 16, 1, jump); + raw.extend(element(jump as u32, at as u64, 1)); + } + for j in 0..m { + put_object(&mut file, tail + 16 * j, (j % 65_000 + 2) as u16, 0); + } + (file, raw) +} + +#[test] +fn overlapping_collections_are_refused_in_bounded_memory_and_time() { + for (name, (file, raw)) in [ + ("nested", nested(2000)), + ("shared tail", shared_tail(500, 10_000)), + ] { + let start = Instant::now(); + let (result, peak) = peak_during(|| { + let mut r = VlResolver::new(&file, 8, 8); + (r.string_bytes(&raw), r.sequences(&raw, 1).map(|s| s.len())) + }); + let took = start.elapsed(); + // libhdf5 never writes overlapping collections, and refuses these + // files; so do we, rather than returning what they claim. + let (strings, sequences) = result; + let e = strings.expect_err(name).to_string(); + assert!(e.contains("overlaps"), "{name}: {e}"); + assert!(sequences.is_err(), "{name}"); + // Measured before the fix: 129 MB ("nested", 64 KB file) and 350 MB + // ("shared tail", 176 KB file) live at the peak; after, 97 KB and + // 0.9 MB. + assert!( + peak < 4 * file.len() + (1 << 20), + "{name}: peak {peak} bytes for a {}-byte file", + file.len() + ); + assert!(took < Duration::from_secs(5), "{name}: took {took:?}"); + } +} + +/// Collections that do not overlap still read, however many elements point +/// into them, and the first object of a collection is returned for its +/// index (as before). +#[test] +fn separate_collections_still_read() { + let mut file = vec![0u8; 64 + 3 * 64]; + let mut raw = Vec::new(); + for i in 0..3usize { + let at = 64 + 64 * i; + put_header(&mut file, at, 64); + put_object(&mut file, at + 16, 1, 3); + file[at + 32..at + 35].copy_from_slice(format!("s{i}!").as_bytes()); + raw.extend(element(3, at as u64, 1)); + } + raw.extend(element(3, 64, 1)); + let mut r = VlResolver::new(&file, 8, 8); + assert_eq!(r.strings(&raw).unwrap(), ["s0!", "s1!", "s2!", "s0!"]); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index 4ea8307..ceb6ab6 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -409,6 +409,32 @@ has produced more records than the file could physically hold. --- +## Crafted global heaps exhaust the variable-length reader's memory + +**Status:** fixed on `feat/p2-vl-strings` (2026-09-26). Not a regression of +that branch: every earlier release is affected through `read_vl_strings`. + +Reading variable-length values kept an owned copy of every object of every +global heap collection visited, for the whole read. A file whose collections +nest inside one another's object data (32 bytes apart, each element pointing +at a different one) made retained memory O(elements × file size): a 744 KB +file reached 1.58 GB. Letting every collection's object chain jump to one +shared run of tiny objects made the parse time O(elements × objects) too. +libhdf5 refuses such files. + +Now `VlResolver` caches where each object lies instead of a copy, drops its +cache past a 32 MiB budget, and refuses a collection that overlaps one it +has already read (libhdf5 gives each collection its own block, so only a +crafted file has them). `GlobalHeapCollection::parse` (and the new +`parse_index`) also refuse a collection that runs past the end of the file, +or an object that runs past the end of its collection. Guarded by +`crates/clawhdf5-format/tests/vl_heap_bounds.rs`, which measures peak heap +use with a counting allocator. Still open: a file may point many elements +at one large heap object, and a VL-*sequence* read then returns that +object once per element, as h5py would. + +--- + ## Extensible Array chunk indexes read back wrong data past the inline elements **Status:** fixed on `main` (2026-09-20), after v2.6.0. **Every release up to From d345ffbf807443871257ed211d0e2ae21b10810d Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 09:04:21 -0500 Subject: [PATCH 5/6] fix(tools,wasm): resolve VL data through the library's VlResolver h5rs (dump, ls, diff, check --data) kept its own lenient VL decoder: a heap object longer than its element was cut to the element's length (libhdf5 and h5py refuse it), a null string printed "" where h5dump prints NULL, the stored element size was trusted, and every heap collection was kept as an owned copy for the whole run. It now resolves each element with VlResolver::element / string_element (new: one element in place, borrowing from the file), and refuses a VL type whose stored element size is not 4 + offset size + 4, as File does. H5::heap_object and its cache are gone. h5diff compares a null VL string equal to an empty one; so does h5rs diff. clawhdf5-wasm already resolved VL strings with read_vl_strings; it now uses VlResolver and checks the stored element size before reading, as File::read_string does. Tests (h5py writes the files, patched for "a\0b", a null element and mis-sized heap objects, with 8- and 4-byte offsets): - h5rs_interop dump_prints_vl_data_like_h5dump: byte-identical to h5dump; - dump_json_vl_values_match_h5py: h5py's values, errors where h5py fails; - check_data_flags_mis_sized_vl_heap_objects; - clawhdf5-wasm tests/vl_strings.rs: wasm, File and h5py agree. All four fail before. check --data over the 150 cve_hdf5 CVE and fuzzer files now passes 15 (h5dump rejects 8 of them), was 16 and 9: the stored-size check flags cve-2024-32608. h5rs-check-ok-files.sh --data: 0 of 422 flagged; h5rs-fuzz.sh: clean on 180 files. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 20 ++- crates/clawhdf5-tools/README.md | 22 ++- crates/clawhdf5-tools/src/check.rs | 82 +++++------ crates/clawhdf5-tools/src/diff.rs | 3 + crates/clawhdf5-tools/src/h5.rs | 27 ---- crates/clawhdf5-tools/src/value.rs | 98 ++++++------- crates/clawhdf5-tools/tests/gen_vl_files.py | 121 ++++++++++++++++ crates/clawhdf5-tools/tests/h5rs_interop.rs | 122 ++++++++++++++++ crates/clawhdf5-wasm/src/core.rs | 28 ++-- crates/clawhdf5-wasm/tests/vl_strings.rs | 150 ++++++++++++++++++++ docs/known-issues.md | 12 +- 11 files changed, 533 insertions(+), 152 deletions(-) create mode 100644 crates/clawhdf5-tools/tests/gen_vl_files.py create mode 100644 crates/clawhdf5-wasm/tests/vl_strings.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 847f3c6..fc22849 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,22 @@ object running past its collection. Conformance unchanged at 575 of 697 (`crates/clawhdf5-format/tests/vl_heap_bounds.rs`). +- **Every reader resolves VL data the same way.** `h5rs` (`dump`, `ls`, + `diff`, `check --data`) had its own lenient VL decoder: a heap object + longer than the element's length was cut to it (h5py refuses it), a null + string printed `""` where h5dump prints `NULL`, the stored element size + was trusted, and each heap collection was kept as a copy for the whole + run. It now resolves through `VlResolver`, so `dump` matches h5dump byte + for byte on VL strings (`"a\0b"` as `"a"`, null as `NULL`), VL sequences + and 4-byte-offset files, `dump --json` gives h5py's values, and + `check --data` reports any heap object whose size is not exactly the + element's length × base size. `clawhdf5-wasm` already resolved VL strings + with `read_vl_strings`; it now uses `VlResolver` and refuses a VL type + whose stored element size disagrees with the file, as `File` does + (`crates/clawhdf5-tools/tests/h5rs_interop.rs`, + `crates/clawhdf5-wasm/tests/vl_strings.rs`). New + `VlResolver::element` / `string_element` resolve one element in place. + ### Plugin filters (2026-09-26) - **LZF, bitshuffle, bzip2 and Blosc read and write, in pure Rust.** Files written by h5py with `compression="lzf"`, or with hdf5plugin's @@ -264,10 +280,10 @@ printed with its address; exit 1 when there are any. libhdf5's h5check reads only the 1.8 format. On the conformance corpus it passes all 418 files that both clawhdf5 and h5py read in full, and `check --data` flags - 134 of the 150 CVE and fuzzer files of the `cve_hdf5` corpus (tank, + 135 of the 150 CVE and fuzzer files of the `cve_hdf5` corpus (tank, 2026-09-26). `--data` also follows variable-length data into its global heap collections and reports a damaged one at its address. It inherits - the library's tolerance, though: 9 of the 16 it passes are files h5dump + the library's tolerance, though: 8 of the 15 it passes are files h5dump 1.14.6 rejects (see `docs/known-issues.md`, header checks). - Values over `--max-bytes` (default 1 GiB) are reported instead of read; a panic is caught and reported as an internal error (exit 3). diff --git a/crates/clawhdf5-tools/README.md b/crates/clawhdf5-tools/README.md index 79f775d..8f089c3 100644 --- a/crates/clawhdf5-tools/README.md +++ b/crates/clawhdf5-tools/README.md @@ -76,8 +76,13 @@ print the same bytes as h5dump 1.14.6 and as Debian's h5dump 1.14.5 (the was run in that image on 2026-09-26) — `dump_matches_h5dump` in `tests/h5rs_interop.rs` checks this, and `dump_shows_nul_padding_in_nested_strings` that null-padded strings show their NULs (`"a\000b"`) at any depth, as -h5dump's do. Not covered by those tests: references, opaque, bitfield, -variable-length sequences and virtual datasets. Known differences from +h5dump's do. `dump_prints_vl_data_like_h5dump` covers variable-length +strings (one with an embedded NUL, which prints up to the NUL; empty; null, +which prints `NULL`), variable-length sequences, a VL compound member and +a VL attribute, with 8- and 4-byte offsets. Not covered by those tests: +references, opaque, bitfield, non-ASCII UTF-8 (h5dump prints each byte +above 0x7f as a sign-extended octal escape, h5rs the character) and +virtual datasets. Known differences from h5dump: - Floats print at their own precision (a `float32` 0.1 prints as `0.1`), @@ -233,9 +238,11 @@ extension) and checks: (which catches corrupt compressed data and Fletcher-32 mismatches), and follows every variable-length element (strings and sequences, also inside compounds and arrays) of every dataset and attribute into its global heap -collection: a collection that does not parse, a missing heap object, or a -sequence longer than its heap object is a problem at the collection's -address. Data the +collection: a collection that does not parse or overlaps another, a missing +heap object, or a heap object whose size is not exactly the element's +length times its base size (libhdf5 refuses such an element) is a problem +at the collection's address. Variable-length elements are resolved by the +library's `VlResolver`, as `clawhdf5::File` resolves them. Data the tool cannot decode (a filter it does not implement, such as szip, or a dataset over `--max-bytes`) is a `note:`, not a problem. Every problem is printed with the address of the structure involved; the exit status is 0 @@ -252,9 +259,10 @@ none at all without `--data`), and objects reachable only by external links. It clawhdf5's parsers, so it accepts what they accept: some header damage that libhdf5 refuses goes unreported. Of the 150 CVE and fuzzer files of the HDF Group's `cve_hdf5` corpus (`cvefiles/` and `fuzzerfiles/`), -`check --data` passes 16, and h5dump 1.14.6 rejects 9 of those (tank, +`check --data` passes 15, and h5dump 1.14.6 rejects 8 of those (tank, 2026-09-26, `h5rs check --data F` and `h5dump F` per file; before the -library's header checks it passed 28, of which h5dump rejects 21). +library's header checks it passed 28, of which h5dump rejects 21, and 16 +and 9 before a VL type's stored element size was checked). ## Robustness diff --git a/crates/clawhdf5-tools/src/check.rs b/crates/clawhdf5-tools/src/check.rs index 8c56771..da90247 100644 --- a/crates/clawhdf5-tools/src/check.rs +++ b/crates/clawhdf5-tools/src/check.rs @@ -15,11 +15,13 @@ use clawhdf5_format::btree_v2::{BTreeV2Header, collect_btree_v2_records}; use clawhdf5_format::data_layout::DataLayout; use clawhdf5_format::dataspace::{Dataspace, DataspaceType}; use clawhdf5_format::datatype::Datatype; +use clawhdf5_format::error::FormatError; use clawhdf5_format::group_info::GroupInfoMessage; use clawhdf5_format::link_info::LinkInfoMessage; use clawhdf5_format::message_type::MessageType; use clawhdf5_format::object_header::ObjectHeader; use clawhdf5_format::symbol_table::SymbolTableMessage; +use clawhdf5_format::vl_data::{VlResolver, check_element_size, parse_vl_references}; use crate::cli::{Args, Out}; use crate::h5::{Error, ErrorKind, H5, Kind}; @@ -49,6 +51,15 @@ found, 3 internal error."; const MAX_CHUNKS_CHECKED: usize = 10_000_000; +/// A variable-length element's problem, worded as `check` reports heap +/// problems ("global heap ..."). +fn heap_problem(e: FormatError) -> String { + match e { + FormatError::VlDataError(m) if m.starts_with("global heap") => m, + e => format!("global heap: {e}"), + } +} + /// Whether values of `dt` hold variable-length data (in the global heap). fn has_vl(dt: &Datatype, depth: u32) -> bool { if depth > 32 { @@ -104,6 +115,8 @@ struct Checker<'a> { btrees_seen: HashSet, /// Global heap collections already read (with --data). gcols_seen: HashSet, + /// Resolves variable-length elements (with --data), for the whole file. + vl: VlResolver<'a>, panicked: bool, } @@ -157,6 +170,7 @@ pub fn run(args: &mut Args, out: &mut Out) -> std::io::Result { heaps_seen: HashSet::new(), btrees_seen: HashSet::new(), gcols_seen: HashSet::new(), + vl: VlResolver::new(h5.data(), h5.os(), h5.ls()), panicked: false, }; c.superblock(); @@ -707,62 +721,48 @@ impl Checker<'_> { } match dt { Datatype::VariableLength { + size, is_string, base_type, .. } => { - let os = usize::from(self.h5.os()); - let (Some(lenb), Some(addrb), Some(idxb)) = - (b.get(..4), b.get(4..4 + os), b.get(4 + os..8 + os)) - else { + // Resolved by the library's VlResolver, as every other + // reader resolves them (and as libhdf5 does): a heap object + // whose size is not the element's length × base size, a + // collection that overlaps another, or a missing object is + // a problem at the collection's address. + let Ok(vl) = parse_vl_references(b, 1, self.h5.os()) else { return; }; - let le = |x: &[u8]| { - x.iter() - .enumerate() - .fold(0u64, |a, (i, &v)| a | (u64::from(v) << (8 * i))) - }; - let (len, gcol, idx) = (le(lenb), le(addrb), le(idxb)); - let undef = if os >= 8 { - u64::MAX - } else { - (1u64 << (8 * os)) - 1 - }; - if len == 0 || gcol == 0 || gcol == undef || bad.contains_key(&gcol) { + let gcol = vl[0].collection_address; + if gcol == 0 || bad.contains_key(&gcol) { return; } - let obj = match self.h5.heap_object(gcol, idx as u32) { - Ok(o) => o, + if let Err(e) = check_element_size(*size, self.h5.os()) { + bad.insert(gcol, e.to_string()); + return; + } + let bs = if *is_string { + 1 + } else { + base_type.type_size() as usize + }; + if bs == 0 { + return; + } + let obj = match self.vl.element(b, bs) { + Ok(o) => o.unwrap_or(&[]), Err(e) => { - bad.insert(e.addr.unwrap_or(gcol), e.msg); + bad.insert(gcol, heap_problem(e)); return; } }; if self.gcols_seen.insert(gcol) { self.counts.global_heaps += 1; } - let bs = if *is_string { - 1 - } else { - u64::from(base_type.type_size()) - }; - if len - .checked_mul(bs) - .is_none_or(|need| need > obj.len() as u64) - { - bad.insert( - gcol, - format!( - "global heap object {idx} holds {} bytes; the element needs {len} x {bs}", - obj.len() - ), - ); - return; - } - if !*is_string && bs > 0 && has_vl(base_type, depth + 1) { - let bs = bs as usize; - for k in 0..len as usize { - self.vl_element(base_type, &obj[k * bs..(k + 1) * bs], depth + 1, bad); + if !*is_string && has_vl(base_type, depth + 1) { + for eb in obj.chunks_exact(bs) { + self.vl_element(base_type, eb, depth + 1, bad); } } } diff --git a/crates/clawhdf5-tools/src/diff.rs b/crates/clawhdf5-tools/src/diff.rs index c7ad723..98d4ba5 100644 --- a/crates/clawhdf5-tools/src/diff.rs +++ b/crates/clawhdf5-tools/src/diff.rs @@ -699,6 +699,9 @@ impl Diff { } match (x, y) { (Value::Str(p), Value::Str(q)) => p == q, + // h5diff compares a null VL string equal to an empty one. + (Value::NullStr, Value::NullStr) => true, + (Value::NullStr, Value::Str(s)) | (Value::Str(s), Value::NullStr) => s.is_empty(), (Value::Bytes(p), Value::Bytes(q)) | (Value::OtherRef(p), Value::OtherRef(q)) => p == q, (Value::Compound(p), Value::Compound(q)) => { p.len() == q.len() diff --git a/crates/clawhdf5-tools/src/h5.rs b/crates/clawhdf5-tools/src/h5.rs index e2e7931..1f4354a 100644 --- a/crates/clawhdf5-tools/src/h5.rs +++ b/crates/clawhdf5-tools/src/h5.rs @@ -8,7 +8,6 @@ use std::cell::RefCell; use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::rc::Rc; use clawhdf5::File; use clawhdf5_format::attribute::{AttributeMessage, extract_attributes_tolerant}; @@ -20,7 +19,6 @@ use clawhdf5_format::datatype::Datatype; use clawhdf5_format::error::FormatError; use clawhdf5_format::filter_pipeline::FilterPipeline; use clawhdf5_format::fractal_heap::FractalHeapHeader; -use clawhdf5_format::global_heap::GlobalHeapCollection; use clawhdf5_format::group_v1; use clawhdf5_format::link_info::LinkInfoMessage; use clawhdf5_format::link_message::{LinkMessage, LinkTarget}; @@ -191,7 +189,6 @@ pub struct H5 { pub path: PathBuf, pub file: File, pub max_bytes: u64, - heaps: RefCell, String>>>, /// Fractal heaps whose blocks were verified: `None` = sound. verified_heaps: RefCell>>, } @@ -211,7 +208,6 @@ impl H5 { path: path.to_path_buf(), file, max_bytes: DEFAULT_MAX_BYTES, - heaps: RefCell::new(HashMap::new()), verified_heaps: RefCell::new(HashMap::new()), }) } @@ -433,29 +429,6 @@ impl H5 { r.map_or(Ok(()), Err) } - /// The global heap object `idx` of the collection at `addr` (cached per - /// collection). - pub fn heap_object(&self, addr: u64, idx: u32) -> Result> { - let coll = { - let mut cache = self.heaps.borrow_mut(); - cache - .entry(addr) - .or_insert_with(|| match usize::try_from(addr) { - Ok(a) => GlobalHeapCollection::parse(self.data(), a, self.ls()) - .map(Rc::new) - .map_err(|e| e.to_string()), - Err(_) => Err("address out of range".into()), - }) - .clone() - .map_err(|e| Error::at(addr, format!("global heap: {e}")))? - }; - let idx16 = u16::try_from(idx) - .map_err(|_| Error::at(addr, format!("global heap object index {idx} out of range")))?; - coll.get_object(idx16) - .map(|o| o.data.clone()) - .ok_or_else(|| Error::at(addr, format!("global heap has no object {idx}"))) - } - /// The dataspace of the dataset at `path` with a virtual dataset's /// extent resolved from its sources (as libhdf5 reports it) instead of /// the stored one. diff --git a/crates/clawhdf5-tools/src/value.rs b/crates/clawhdf5-tools/src/value.rs index 8160574..d9255f1 100644 --- a/crates/clawhdf5-tools/src/value.rs +++ b/crates/clawhdf5-tools/src/value.rs @@ -3,7 +3,10 @@ //! Decoding never panics: a short buffer, an unknown byte order or a //! dangling heap reference becomes [`Value::Error`]. +use std::cell::RefCell; + use clawhdf5_format::datatype::{Datatype, DatatypeByteOrder, ReferenceType, StringPadding}; +use clawhdf5_format::vl_data::{VlResolver, check_element_size}; use serde_json::Value as J; use crate::dtype; @@ -16,6 +19,9 @@ pub enum Value { /// its own precision. Float(f64, u8), Str(String), + /// A null variable-length string (heap address 0): h5dump prints it as + /// `NULL`, h5py reads it as empty. + NullStr, /// Opaque, bitfield, time and oversized integers. Bytes(Vec), /// An enum member (name, when the value matches one) and its value. @@ -129,10 +135,10 @@ fn decode_float(dt: &Datatype, b: &[u8]) -> Value { } } -fn trim_string(b: &[u8], pad: Option<&StringPadding>) -> String { +fn trim_string(b: &[u8], pad: &StringPadding) -> String { let cut = b.iter().position(|&c| c == 0).unwrap_or(b.len()); let mut s = &b[..cut]; - if matches!(pad, Some(StringPadding::SpacePad)) { + if matches!(pad, StringPadding::SpacePad) { while let [rest @ .., b' '] = s { s = rest; } @@ -140,22 +146,20 @@ fn trim_string(b: &[u8], pad: Option<&StringPadding>) -> String { String::from_utf8_lossy(s).into_owned() } -/// Little-endian unsigned integer of `b` (up to 8 bytes). -fn le(b: &[u8]) -> u64 { - b.iter() - .take(8) - .enumerate() - .fold(0u64, |a, (i, &x)| a | (u64::from(x) << (8 * i))) -} - /// Decodes elements of one file. pub struct Decoder<'a> { pub h5: &'a H5, + /// Variable-length elements are resolved as the library resolves them + /// (so as libhdf5 does), not by a decoder of our own. + vl: RefCell>, } impl<'a> Decoder<'a> { pub fn new(h5: &'a H5) -> Self { - Self { h5 } + Self { + h5, + vl: RefCell::new(VlResolver::new(h5.data(), h5.os(), h5.ls())), + } } /// Decode element `i` of `raw`, an array of `dt` elements. @@ -187,7 +191,7 @@ impl<'a> Decoder<'a> { Datatype::Time { .. } | Datatype::BitField { .. } | Datatype::Opaque { .. } => { Value::Bytes(b.to_vec()) } - Datatype::String { padding, .. } => Value::Str(trim_string(b, Some(padding))), + Datatype::String { padding, .. } => Value::Str(trim_string(b, padding)), Datatype::Compound { members, .. } => { let mut out = Vec::with_capacity(members.len()); for m in members { @@ -246,61 +250,43 @@ impl<'a> Decoder<'a> { Value::Array(out) } Datatype::VariableLength { + size, is_string, - padding, base_type, .. - } => self.decode_vlen(*is_string, padding.as_ref(), base_type, b, depth), + } => match check_element_size(*size, self.h5.os()) { + Ok(()) => self.decode_vlen(*is_string, base_type, b, depth), + Err(e) => Value::Error(e.to_string()), + }, } } - fn decode_vlen( - &self, - is_string: bool, - padding: Option<&StringPadding>, - base: &Datatype, - b: &[u8], - depth: u32, - ) -> Value { - let os = usize::from(self.h5.os()); - let (Some(lenb), Some(addrb), Some(idxb)) = - (b.get(..4), b.get(4..4 + os), b.get(4 + os..8 + os)) - else { - return Value::Error("short VL element".into()); - }; - let len = le(lenb) as usize; - let addr = le(addrb); - let idx = le(idxb) as u32; - let undef = if os >= 8 { - u64::MAX - } else { - (1u64 << (8 * os)) - 1 - }; - let obj = if len == 0 || addr == 0 || addr == undef { - Vec::new() - } else { - match self.h5.heap_object(addr, idx) { - Ok(o) => o, - Err(e) => return Value::Error(e.to_string()), - } - }; + /// A variable-length element, resolved by the library's + /// [`VlResolver`]: a string ends at its first NUL, a heap object whose + /// size is not the element's length × base size is an error, and a + /// heap address of 0 is null — all as libhdf5 (and so h5dump and h5py) + /// has it. + fn decode_vlen(&self, is_string: bool, base: &Datatype, b: &[u8], depth: u32) -> Value { if is_string { - let l = len.min(obj.len()); - return Value::Str(trim_string(&obj[..l], padding)); + return match self.vl.borrow_mut().string_element(b) { + Ok(Some(s)) => Value::Str(String::from_utf8_lossy(s).into_owned()), + Ok(None) => Value::NullStr, + Err(e) => Value::Error(e.to_string()), + }; } let bs = base.type_size() as usize; if bs == 0 { return Value::Error("VL base type of size 0".into()); } - match len.checked_mul(bs) { - Some(need) if need <= obj.len() => {} - _ => return Value::Error("VL sequence longer than its heap object".into()), - } - let mut out = Vec::with_capacity(len); - for k in 0..len { - out.push(self.decode(base, &obj[k * bs..], depth + 1)); - } - Value::Seq(out) + let obj = match self.vl.borrow_mut().element(b, bs) { + Ok(o) => o.unwrap_or(&[]), + Err(e) => return Value::Error(e.to_string()), + }; + Value::Seq( + obj.chunks_exact(bs) + .map(|e| self.decode(base, e, depth + 1)) + .collect(), + ) } } @@ -351,6 +337,7 @@ pub fn text(v: &Value, h5paths: &dyn Fn(u64) -> Option) -> String { Value::Int(i) => i.to_string(), Value::Float(f, w) => fmt_float(*f, *w), Value::Str(s) => format!("\"{}\"", escape(s)), + Value::NullStr => "NULL".into(), Value::Bytes(b) => hex(b), Value::Enum(Some(n), _) => n.clone(), Value::Enum(None, i) => i.to_string(), @@ -411,6 +398,7 @@ pub fn to_json(v: &Value, h5paths: &dyn Fn(u64) -> Option) -> J { } } Value::Str(s) => J::from(s.as_str()), + Value::NullStr => J::from(""), Value::Bytes(b) | Value::OtherRef(b) => J::from(hex(b)), Value::Enum(_, i) => to_json(&Value::Int(*i), h5paths), Value::Compound(ms) => J::Array(ms.iter().map(|(_, v)| to_json(v, h5paths)).collect()), diff --git a/crates/clawhdf5-tools/tests/gen_vl_files.py b/crates/clawhdf5-tools/tests/gen_vl_files.py new file mode 100644 index 0000000..3937c19 --- /dev/null +++ b/crates/clawhdf5-tools/tests/gen_vl_files.py @@ -0,0 +1,121 @@ +"""Write the variable-length data files the h5rs VL tests run on. + +usage: gen_vl_files.py OUTDIR + +For 8-byte (`vl8`) and 4-byte (`vl4`) offsets, writes OUTDIR/vl8.h5 and +OUTDIR/vl4.h5, which libhdf5 reads in full, and OUTDIR/bad8.h5 and +OUTDIR/bad4.h5, whose `bad` and `badseq` elements 0 have a length that +disagrees with their global heap object (libhdf5: "Expected global heap +object size does not match"). h5py cannot write a VL string with a NUL in +it or a null element in a contiguous dataset, so those are patched in. + +Prints one JSON object: for each file, each dataset's values as h5py reads +them one element at a time (strings as text, sequences as lists, a compound +as a list of its fields), with null for an element h5py cannot read; the +root attribute `va`; and the addresses of the `bad` elements' collections. +""" + +import json +import os +import struct +import sys + +import h5py +import numpy as np + +out = sys.argv[1] +S = h5py.string_dtype("utf-8") +I4 = h5py.vlen_dtype(np.dtype(" null + open(path, "wb").write(bytes(b)) + + +def bad(path, sizes): + os_ = 8 if sizes is None else sizes[0] + with create(path, sizes) as f: + f.create_dataset("bad", data=np.array(["cdefgh", "ok"], dtype=object), dtype=S) + s = f.create_dataset("badseq", shape=(2,), dtype=I4) + s[0] = [1, 2, 3] + s[1] = [4] + off, soff = f["bad"].id.get_offset(), f["badseq"].id.get_offset() + b = bytearray(open(path, "rb").read()) + gcol = int.from_bytes(b[off + 4 : off + 4 + os_], "little") + struct.pack_into(" 3 + struct.pack_into(" 2 + open(path, "wb").write(bytes(b)) + return gcol + + +def value(v): + if isinstance(v, bytes): + return v.decode() + if isinstance(v, str): + return v + if isinstance(v, np.void): + return [value(x) for x in v] + if isinstance(v, np.ndarray): + return [value(x) for x in v] + return v.item() if hasattr(v, "item") else v + + +def read(ds): + got = [] + for i in range(ds.shape[0]): + try: + got.append(value(ds[i])) + except OSError: + got.append(None) + return got + + +result = {} +for tag, sizes in (("8", None), ("4", (4, 4))): + g, x = os.path.join(out, f"vl{tag}.h5"), os.path.join(out, f"bad{tag}.h5") + good(g, sizes) + gcol = bad(x, sizes) + with h5py.File(g, "r") as f: + result[f"vl{tag}"] = {n: read(f[n]) for n in ("d", "u", "seq", "sequ", "cmp")} + result[f"vl{tag}"]["va"] = [value(s) for s in f.attrs["va"]] + with h5py.File(x, "r") as f: + result[f"bad{tag}"] = {n: read(f[n]) for n in ("bad", "badseq")} + result[f"bad{tag}"]["gcol"] = gcol +json.dump(result, sys.stdout) diff --git a/crates/clawhdf5-tools/tests/h5rs_interop.rs b/crates/clawhdf5-tools/tests/h5rs_interop.rs index 7d97cc5..a1b4492 100644 --- a/crates/clawhdf5-tools/tests/h5rs_interop.rs +++ b/crates/clawhdf5-tools/tests/h5rs_interop.rs @@ -773,3 +773,125 @@ fn every_subcommand_rejects_a_non_hdf5_file_cleanly() { assert_eq!(code(&h5rs(&["ls"])), 2); assert_eq!(code(&h5rs(&["--help"])), 0); } + +// --------------------------------------------------------------------------- +// variable-length data +// --------------------------------------------------------------------------- + +/// Runs `tests/gen_vl_files.py`: VL strings (with an embedded NUL, empty +/// and null elements), VL sequences, a VL compound member and a VL +/// attribute, with 8- and 4-byte offsets, plus files whose heap objects +/// disagree with their elements' lengths. +fn generate_vl() -> Option { + if missing(python_available(), "python3 with h5py") { + return None; + } + let dir = tempfile::tempdir().unwrap(); + let script = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/gen_vl_files.py"); + let out = Command::new(python()) + .arg(&script) + .arg(dir.path()) + .output() + .expect("run gen_vl_files.py"); + assert!( + out.status.success(), + "gen_vl_files.py failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let values = serde_json::from_slice(&out.stdout).expect("gen_vl_files.py output"); + Some(Files { dir, values }) +} + +/// `dump` resolves VL elements through the library's `VlResolver`, as +/// libhdf5 does: "a\0b" prints as "a", a null string as NULL (it printed +/// ""), and with 4-byte offsets too; the output is h5dump's byte for byte. +#[test] +fn dump_prints_vl_data_like_h5dump() { + let Some(f) = generate_vl() else { return }; + for name in ["vl8.h5", "vl4.h5"] { + let p = f.p(name); + let ours = stdout(&h5rs(&["dump", &p])); + assert!( + ours.contains(r#"(0): "a", "", NULL, "zz", "hello""#), + "{name}:\n{ours}" + ); + assert!(ours.contains(r#"(0): NULL, "w", NULL, NULL"#), "{name}"); + assert!(ours.contains("(0): (1, 2, 3), (), (-5)"), "{name}"); + if missing(tool_available("h5dump"), "h5dump") { + continue; + } + let reference = run("h5dump", &[&p]); + assert!(reference.status.success(), "{name}: {reference:?}"); + assert_eq!(ours, stdout(&reference).replacen(&p, name, 1), "{name}"); + } +} + +/// `dump --json` gives the values h5py reads, element by element; and an +/// element whose heap object is not its length × base size is an error, as +/// in h5py, not a truncated value (it printed "cde" and (1, 2)). +#[test] +fn dump_json_vl_values_match_h5py() { + let Some(f) = generate_vl() else { return }; + for tag in ["8", "4"] { + let (good, bad) = (format!("vl{tag}"), format!("bad{tag}")); + let o = h5rs(&["dump", "--json", &f.p(&format!("{good}.h5"))]); + assert!(o.status.success(), "{good}: {o:?}"); + let doc: serde_json::Value = serde_json::from_slice(&o.stdout).unwrap(); + let want = &f.values[&good]; + for d in doc["datasets"].as_object().unwrap().values() { + let path = d["alias"][0].as_str().unwrap(); + assert_eq!(d["value"], want[&path[1..]], "{good}: {path}"); + } + let attrs = &doc["groups"][doc["root"].as_str().unwrap()]["attributes"]; + assert_eq!(attrs[0]["name"], "va"); + assert_eq!(attrs[0]["value"], want["va"], "{good}: va"); + + let o = h5rs(&["dump", "--json", &f.p(&format!("{bad}.h5"))]); + let doc: serde_json::Value = serde_json::from_slice(&o.stdout).unwrap(); + let want = &f.values[&bad]; + for d in doc["datasets"].as_object().unwrap().values() { + let path = d["alias"][0].as_str().unwrap(); + let got = d["value"].as_array().unwrap(); + let want = want[&path[1..]].as_array().unwrap(); + assert_eq!(got.len(), want.len(), "{bad}: {path}"); + for (g, w) in got.iter().zip(want) { + if w.is_null() { + // h5py cannot read it: neither can we. + let e = g["error"] + .as_str() + .unwrap_or_else(|| panic!("{bad}: {path}: {g}")); + assert!(e.contains("holds"), "{bad}: {path}: {e}"); + } else { + assert_eq!(g, w, "{bad}: {path}"); + } + } + } + } +} + +/// `check --data` holds VL elements to libhdf5's rule: a heap object whose +/// size is not exactly the element's length × base size is a problem (it +/// only caught objects shorter than the element). +#[test] +fn check_data_flags_mis_sized_vl_heap_objects() { + let Some(f) = generate_vl() else { return }; + for tag in ["8", "4"] { + let o = h5rs(&["check", "--data", &f.p(&format!("vl{tag}.h5"))]); + let s = stdout(&o); + assert_eq!(code(&o), 0, "vl{tag}: {s}"); + assert!( + s.contains("global heap collections read: 1"), + "vl{tag}: {s}" + ); + + let o = h5rs(&["check", "--data", &f.p(&format!("bad{tag}.h5"))]); + let s = stdout(&o); + assert_eq!(code(&o), 1, "bad{tag}: {s}"); + let at = f.values[format!("bad{tag}")]["gcol"].as_u64().unwrap(); + for (path, what) in [("/bad", "6 bytes"), ("/badseq", "12 bytes")] { + let want = format!("problem: {at:#x} {path}: variable-length data: global heap object"); + assert!(s.contains(&want), "bad{tag}: no {want:?} in\n{s}"); + assert!(s.contains(what), "bad{tag}: {s}"); + } + } +} diff --git a/crates/clawhdf5-wasm/src/core.rs b/crates/clawhdf5-wasm/src/core.rs index baadcda..4c6beab 100644 --- a/crates/clawhdf5-wasm/src/core.rs +++ b/crates/clawhdf5-wasm/src/core.rs @@ -9,6 +9,7 @@ use clawhdf5::{AttrValue, File, Selection}; use clawhdf5_format::data_read; use clawhdf5_format::datatype::{Datatype, DatatypeByteOrder}; +use clawhdf5_format::vl_data::{VlResolver, check_element_size}; /// Errors are reported to JavaScript as messages. pub type Result = std::result::Result; @@ -221,6 +222,12 @@ impl Reader { None => (Selection::All, shape.clone()), Some(h) => hyperslab_selection(h, &shape)?, }; + // A VL type whose stored element size is not the one the file's + // offset size implies is refused before its data is read, as + // `File::read_string` refuses it. + if let Datatype::VariableLength { size, .. } = array_base(&dt) { + check_element_size(*size, self.file.superblock().offset_size).map_err(err)?; + } let raw = ds.read_selection(&selection).map_err(err)?; let data = self.decode(&raw, &dt)?; out_shape.extend(element_shape(&dt)); @@ -270,23 +277,14 @@ impl Reader { Datatype::VariableLength { is_string: true, .. } if !is_array => { - let size = dt.type_size() as usize; - if size == 0 || !raw.len().is_multiple_of(size) { - return Err(format!( - "{} bytes is not a whole number of {size}-byte string references", - raw.len() - )); - } + // The library's resolver, as File::read_string uses: a + // string ends at its first NUL and a heap object of the + // wrong size is an error, as in libhdf5 and h5py. let sb = self.file.superblock(); Data::Strings( - clawhdf5_format::vl_data::read_vl_strings( - self.file.as_bytes(), - raw, - (raw.len() / size) as u64, - sb.offset_size, - sb.length_size, - ) - .map_err(err)?, + VlResolver::new(self.file.as_bytes(), sb.offset_size, sb.length_size) + .strings(raw) + .map_err(err)?, ) } Datatype::Enumeration { .. } if !is_array => { diff --git a/crates/clawhdf5-wasm/tests/vl_strings.rs b/crates/clawhdf5-wasm/tests/vl_strings.rs new file mode 100644 index 0000000..1baddd0 --- /dev/null +++ b/crates/clawhdf5-wasm/tests/vl_strings.rs @@ -0,0 +1,150 @@ +//! The wasm reader resolves VL strings with the library's `VlResolver`, so +//! it returns what `File::read_string` and h5py return: a string ends at +//! its first NUL, a null element is empty, a heap object of the wrong size +//! is an error, and a VL datatype whose stored element size disagrees with +//! the file's offset size is refused. Checked with 8- and 4-byte offsets. +//! +//! Skipped when python3 with h5py is missing, unless +//! `CLAWHDF5_REQUIRE_INTEROP=1`. `CLAWHDF5_PYTHON` names the interpreter. + +use std::process::Command; + +use clawhdf5::File; +use clawhdf5_wasm::core::{Data, Reader}; + +fn python() -> String { + std::env::var("CLAWHDF5_PYTHON").unwrap_or_else(|_| "python3".to_string()) +} + +fn h5py_available() -> bool { + let ok = Command::new(python()) + .args(["-c", "import h5py, numpy"]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false); + if !ok { + assert!( + std::env::var("CLAWHDF5_REQUIRE_INTEROP").as_deref() != Ok("1"), + "CLAWHDF5_REQUIRE_INTEROP=1 but python with h5py is not available" + ); + eprintln!("SKIP: python with h5py not available"); + } + ok +} + +/// For each offset size: `vl{8,4}.h5` with dataset `d` = "a\0b", "", null, +/// "zz" (patched: h5py writes neither a NUL nor a null element); +/// `bad{8,4}.h5` whose element 0 claims 3 bytes of a 6-byte heap object; +/// and `size{8,4}.h5` whose VL datatype message stores a 24-byte element. +/// Prints h5py's reading of each element as hex, or "error". +const SCRIPT: &str = r#" +import struct, sys, h5py, numpy as np +out = sys.argv[1] +S = h5py.string_dtype('utf-8') +def create(path, os_): + if os_ == 8: + return h5py.File(path, 'w', libver='earliest') + # The earliest format, so the patched object header has no checksum. + fcpl = h5py.h5p.create(h5py.h5p.FILE_CREATE); fcpl.set_sizes(4, 4) + fapl = h5py.h5p.create(h5py.h5p.FILE_ACCESS) + fapl.set_libver_bounds(h5py.h5f.LIBVER_EARLIEST, h5py.h5f.LIBVER_V18) + return h5py.File(h5py.h5f.create(path.encode(), h5py.h5f.ACC_TRUNC, fcpl=fcpl, fapl=fapl)) +def elem(length, addr, index, os_): + return struct.pack(' = String::from_utf8(out.stdout) + .unwrap() + .lines() + .map(|l| { + let (k, v) = l.split_once('\t').unwrap(); + (k.to_string(), v.to_string()) + }) + .collect(); + let read = |name: &str| { + let bytes = std::fs::read(dir.path().join(format!("{name}.h5"))).unwrap(); + let wasm = Reader::open(bytes).unwrap().read("/d", None); + let file = File::open(dir.path().join(format!("{name}.h5"))) + .unwrap() + .dataset("d") + .unwrap() + .read_string(); + (wasm, file) + }; + for os in [8, 4] { + // h5py: "a\0b" is "a"; the null element (address 0) is empty. + assert_eq!(h5py[&format!("vl{os}")], "61,,,7a7a"); + let (wasm, file) = read(&format!("vl{os}")); + let Data::Strings(wasm) = wasm.unwrap().data else { + panic!("vl{os}: not strings") + }; + assert_eq!(wasm, ["a", "", "", "zz"], "vl{os}"); + assert_eq!(wasm, file.unwrap(), "vl{os}"); + + // h5py refuses the mis-sized element; so do both readers. + assert_eq!(h5py[&format!("bad{os}")], "error,6f6b"); + let (wasm, file) = read(&format!("bad{os}")); + assert!(wasm.unwrap_err().contains("holds 6 bytes"), "bad{os}"); + assert!(file.is_err(), "bad{os}"); + + // libhdf5 ignores the stored element size and reads the values; + // File refuses the datatype rather than guess its layout, and the + // wasm reader now does the same (it read with the stored size). + assert_eq!(h5py[&format!("size{os}")], "78,7979"); + let (wasm, file) = read(&format!("size{os}")); + let e = wasm.unwrap_err(); + assert!(e.contains("stores 24-byte elements"), "size{os}: {e}"); + assert!(file.is_err(), "size{os}"); + } +} diff --git a/docs/known-issues.md b/docs/known-issues.md index ceb6ab6..5f43bdb 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -207,9 +207,10 @@ fill-value item that did is fixed). - (`cve-2024-32616` `/group1/dset3` and `cve-2025-2309`'s `Comp_OBJREF` attribute are h5py/numpy type-mapping failures, not libhdf5 refusals.) - `h5rs check` validates with the library's parsers, so it inherits what - they accept: of the 150 CVE and fuzzer files, `check --data` passes 16, - and h5dump 1.14.6 rejects 9 of those (tank, 2026-09-26; 28 and 21 - before these checks). + they accept: of the 150 CVE and fuzzer files, `check --data` passes 15, + and h5dump 1.14.6 rejects 8 of those (tank, 2026-09-26; 28 and 21 + before these checks, 16 and 9 before a VL type's stored element size + was checked, which flags `cve-2024-32608`). - **Writer:** - Nested groups beyond one level: path-like names are now refused, not created. @@ -537,8 +538,9 @@ which is what libhdf5 itself writes. followed (no file system). - Variable-length string datasets are read by decoding `read_selection`'s bytes with `clawhdf5_format::vl_data` in the wasm crate; `File` itself still - cannot (see the audit gaps above). (`File` can since 2026-09-26; the wasm - crate still decodes them itself.) + cannot (see the audit gaps above). (`File` can since 2026-09-26. Since + 2026-09-26 the wasm crate resolves them with the same `VlResolver` as + `File` and `h5rs`, so all three return h5py's values.) ## The Node.js package (`packages/clawhdf5-node`) does not work From 5a202f3791c3365af73f1ee6b8d56ae9dd86866d Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 09:06:46 -0500 Subject: [PATCH 6/6] fix(format): a VL element at the undefined heap address is an error libhdf5 fails to read a VL element whose global heap address is undefined (all 0xff), even at length 0 ("addr undefined"); we returned "" (or an empty sequence) in every reader. Checked with h5py first: libhdf5 writes a null element with address 0, which still reads as empty, and h5py writes "" as a zero-size heap object at a real address, so no file they write relies on the old behaviour. read_vl_bytes now treats address 0 as null whatever the length, as VlResolver does. Tests, each failing before: vl_data unit test (8- and 4-byte offsets, lengths 0 and 1); clawhdf5 vl_data_interop a_vl_element_at_the_undefined_heap_address_fails_like_h5py (also checks where h5py writes ""); h5rs dump --json and check --data on the patched `undef` dataset; clawhdf5-wasm vl_strings. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 10 +++++ crates/clawhdf5-format/src/vl_data.rs | 49 +++++++++++++------- crates/clawhdf5-tools/tests/gen_vl_files.py | 11 ++++- crates/clawhdf5-tools/tests/h5rs_interop.rs | 19 ++++++-- crates/clawhdf5-wasm/tests/vl_strings.rs | 27 +++++++++-- crates/clawhdf5/tests/vl_data_interop.rs | 50 +++++++++++++++++++++ docs/known-issues.md | 4 +- 7 files changed, 143 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc22849..e656dff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,16 @@ `crates/clawhdf5-wasm/tests/vl_strings.rs`). New `VlResolver::element` / `string_element` resolve one element in place. +- **A VL element at the undefined heap address is an error**, as in + libhdf5 ("addr undefined"). One of length 0 read as `""` in every reader + (`File`, `h5rs`, `clawhdf5-wasm`, `read_vl_strings`, `read_vl_bytes`). + libhdf5 writes a null element with heap address 0, which still reads as + empty, and h5py writes `""` as a zero-size heap object at a real address, + so no file libhdf5 or h5py writes is affected + (`a_vl_element_at_the_undefined_heap_address_fails_like_h5py` in + `crates/clawhdf5/tests/vl_data_interop.rs`). `read_vl_bytes` now also + treats address 0 as null whatever the length, as `VlResolver` does. + ### Plugin filters (2026-09-26) - **LZF, bitshuffle, bzip2 and Blosc read and write, in pure Rust.** Files written by h5py with `compression="lzf"`, or with hdf5plugin's diff --git a/crates/clawhdf5-format/src/vl_data.rs b/crates/clawhdf5-format/src/vl_data.rs index f183ef0..3a54c1a 100644 --- a/crates/clawhdf5-format/src/vl_data.rs +++ b/crates/clawhdf5-format/src/vl_data.rs @@ -239,9 +239,6 @@ impl<'a> VlResolver<'a> { if addr == 0 { return Ok(None); } - if vl.length == 0 && is_undefined_address(addr, self.offset_size) { - return Ok(Some(&[])); - } let data = self.object(vl)?; let expected = (vl.length as usize) .checked_mul(base_size) @@ -372,10 +369,8 @@ pub fn read_vl_bytes( let mut result = Vec::with_capacity(refs.len()); for vl in &refs { - if vl.length == 0 - && (is_undefined_address(vl.collection_address, offset_size) - || vl.collection_address == 0) - { + // A heap address of 0 is a null element, as in VlResolver. + if vl.collection_address == 0 { result.push(Vec::new()); continue; } @@ -394,6 +389,15 @@ impl<'a> VlResolver<'a> { /// parsed on first use. fn object(&mut self, vl: &VlElement) -> Result<&'a [u8], FormatError> { let addr = vl.collection_address; + // libhdf5 writes a null element with address 0, never the undefined + // address, and fails to read one ("addr undefined") even when its + // length is 0; we returned an empty value. + if is_undefined_address(addr, self.offset_size) { + return Err(FormatError::VlDataError(format!( + "variable-length element (length {}) has the undefined global heap address", + vl.length + ))); + } if !self.cache.contains_key(&addr) { let offset = usize::try_from(addr).map_err(|_| FormatError::UnexpectedEof { expected: usize::MAX, @@ -546,16 +550,27 @@ mod tests { } #[test] - fn null_vl_element_empty_string() { - // length=0, address=undefined - let mut raw = Vec::new(); - raw.extend_from_slice(&0u32.to_le_bytes()); // length=0 - raw.extend_from_slice(&u64::MAX.to_le_bytes()); // undefined address - raw.extend_from_slice(&0u32.to_le_bytes()); // index - - let file_data = vec![0u8; 16]; - let strings = read_vl_strings(&file_data, &raw, 1, 8, 8).unwrap(); - assert_eq!(strings, vec![""]); + fn an_undefined_heap_address_is_an_error_even_at_length_0() { + // libhdf5 fails the read ("addr undefined"); h5py and libhdf5 write + // a null element with address 0. We returned "". + let mut file_data = vec![0u8; 256]; + build_gcol_at(&mut file_data, 64, &[(1, b"x")]); + for (os, undef) in [(8u8, u64::MAX), (4, 0xFFFF_FFFF)] { + for length in [0, 1] { + let mut raw = element(1, 64, 1, os); + raw.extend(element(length, undef, 1, os)); + let mut r = VlResolver::new(&file_data, os, 8); + let e = r.string_bytes(&raw).unwrap_err().to_string(); + assert!(e.contains("undefined"), "{e}"); + assert!(r.sequences(&raw, 1).is_err()); + assert!(r.string_element(&raw[raw.len() / 2..]).is_err()); + let n = 2; + assert!(read_vl_strings(&file_data, &raw, n, os, 8).is_err()); + assert!(read_vl_bytes(&file_data, &raw, n, os, 8).is_err()); + // The defined element alone still reads. + assert_eq!(r.strings(&raw[..raw.len() / 2]).unwrap(), ["x"]); + } + } } #[test] diff --git a/crates/clawhdf5-tools/tests/gen_vl_files.py b/crates/clawhdf5-tools/tests/gen_vl_files.py index 3937c19..cb50f2d 100644 --- a/crates/clawhdf5-tools/tests/gen_vl_files.py +++ b/crates/clawhdf5-tools/tests/gen_vl_files.py @@ -6,7 +6,8 @@ For 8-byte (`vl8`) and 4-byte (`vl4`) offsets, writes OUTDIR/vl8.h5 and OUTDIR/vl4.h5, which libhdf5 reads in full, and OUTDIR/bad8.h5 and OUTDIR/bad4.h5, whose `bad` and `badseq` elements 0 have a length that disagrees with their global heap object (libhdf5: "Expected global heap -object size does not match"). h5py cannot write a VL string with a NUL in +object size does not match"), and whose `undef` element 1 has length 0 and +the undefined heap address (libhdf5: "addr undefined"). h5py cannot write a VL string with a NUL in it or a null element in a contiguous dataset, so those are patched in. Prints one JSON object: for each file, each dataset's values as h5py reads @@ -76,11 +77,17 @@ def bad(path, sizes): s = f.create_dataset("badseq", shape=(2,), dtype=I4) s[0] = [1, 2, 3] s[1] = [4] + f.create_dataset("undef", data=np.array(["x", "", "yz"], dtype=object), dtype=S) off, soff = f["bad"].id.get_offset(), f["badseq"].id.get_offset() + uoff = f["undef"].id.get_offset() b = bytearray(open(path, "rb").read()) gcol = int.from_bytes(b[off + 4 : off + 4 + os_], "little") struct.pack_into(" 3 struct.pack_into(" 2 + # "": length 0 at the undefined address (all 0xff), which libhdf5 fails + # to read ("addr undefined"); it writes a null element as address 0. + es = 8 + os_ + b[uoff + es : uoff + 2 * es] = element(0, (1 << (8 * os_)) - 1, 1, os_) open(path, "wb").write(bytes(b)) return gcol @@ -116,6 +123,6 @@ for tag, sizes in (("8", None), ("4", (4, 4))): result[f"vl{tag}"] = {n: read(f[n]) for n in ("d", "u", "seq", "sequ", "cmp")} result[f"vl{tag}"]["va"] = [value(s) for s in f.attrs["va"]] with h5py.File(x, "r") as f: - result[f"bad{tag}"] = {n: read(f[n]) for n in ("bad", "badseq")} + result[f"bad{tag}"] = {n: read(f[n]) for n in ("bad", "badseq", "undef")} result[f"bad{tag}"]["gcol"] = gcol json.dump(result, sys.stdout) diff --git a/crates/clawhdf5-tools/tests/h5rs_interop.rs b/crates/clawhdf5-tools/tests/h5rs_interop.rs index a1b4492..202606f 100644 --- a/crates/clawhdf5-tools/tests/h5rs_interop.rs +++ b/crates/clawhdf5-tools/tests/h5rs_interop.rs @@ -828,7 +828,8 @@ fn dump_prints_vl_data_like_h5dump() { /// `dump --json` gives the values h5py reads, element by element; and an /// element whose heap object is not its length × base size is an error, as -/// in h5py, not a truncated value (it printed "cde" and (1, 2)). +/// in h5py, not a truncated value (it printed "cde" and (1, 2)); so is a +/// length-0 element at the undefined heap address (it printed ""). #[test] fn dump_json_vl_values_match_h5py() { let Some(f) = generate_vl() else { return }; @@ -860,7 +861,12 @@ fn dump_json_vl_values_match_h5py() { let e = g["error"] .as_str() .unwrap_or_else(|| panic!("{bad}: {path}: {g}")); - assert!(e.contains("holds"), "{bad}: {path}: {e}"); + let why = if path == "/undef" { + "undefined" + } else { + "holds" + }; + assert!(e.contains(why), "{bad}: {path}: {e}"); } else { assert_eq!(g, w, "{bad}: {path}"); } @@ -871,7 +877,8 @@ fn dump_json_vl_values_match_h5py() { /// `check --data` holds VL elements to libhdf5's rule: a heap object whose /// size is not exactly the element's length × base size is a problem (it -/// only caught objects shorter than the element). +/// only caught objects shorter than the element), and so is an element at +/// the undefined heap address. #[test] fn check_data_flags_mis_sized_vl_heap_objects() { let Some(f) = generate_vl() else { return }; @@ -893,5 +900,11 @@ fn check_data_flags_mis_sized_vl_heap_objects() { assert!(s.contains(&want), "bad{tag}: no {want:?} in\n{s}"); assert!(s.contains(what), "bad{tag}: {s}"); } + // A length-0 element at the undefined heap address: libhdf5 fails + // to read it; check skipped it. + let undef: u64 = if tag == "8" { u64::MAX } else { 0xffff_ffff }; + let want = format!("problem: {undef:#x} /undef: variable-length data: global heap:"); + assert!(s.contains(&want), "bad{tag}: no {want:?} in\n{s}"); + assert!(s.contains("undefined global heap address"), "bad{tag}: {s}"); } } diff --git a/crates/clawhdf5-wasm/tests/vl_strings.rs b/crates/clawhdf5-wasm/tests/vl_strings.rs index 1baddd0..096cc16 100644 --- a/crates/clawhdf5-wasm/tests/vl_strings.rs +++ b/crates/clawhdf5-wasm/tests/vl_strings.rs @@ -1,8 +1,9 @@ //! The wasm reader resolves VL strings with the library's `VlResolver`, so //! it returns what `File::read_string` and h5py return: a string ends at //! its first NUL, a null element is empty, a heap object of the wrong size -//! is an error, and a VL datatype whose stored element size disagrees with -//! the file's offset size is refused. Checked with 8- and 4-byte offsets. +//! is an error, an element at the undefined heap address is an error, and a +//! VL datatype whose stored element size disagrees with the file's offset +//! size is refused. Checked with 8- and 4-byte offsets. //! //! Skipped when python3 with h5py is missing, unless //! `CLAWHDF5_REQUIRE_INTEROP=1`. `CLAWHDF5_PYTHON` names the interpreter. @@ -35,7 +36,9 @@ fn h5py_available() -> bool { /// For each offset size: `vl{8,4}.h5` with dataset `d` = "a\0b", "", null, /// "zz" (patched: h5py writes neither a NUL nor a null element); /// `bad{8,4}.h5` whose element 0 claims 3 bytes of a 6-byte heap object; -/// and `size{8,4}.h5` whose VL datatype message stores a 24-byte element. +/// `size{8,4}.h5` whose VL datatype message stores a 24-byte element; and +/// `undef{8,4}.h5` whose element 1 has length 0 and the undefined heap +/// address. /// Prints h5py's reading of each element as hex, or "error". const SCRIPT: &str = r#" import struct, sys, h5py, numpy as np @@ -77,7 +80,12 @@ for os_ in (8, 4): i = b.index(pat) struct.pack_into('()`, and VL values inside compounds or `AttrValue::Raw` attributes decode with `File::decode_strings` / `File::decode_vlen` (`crates/clawhdf5/tests/vl_data_interop.rs`).