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

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<HashMap<u64, Result<Rc<GlobalHeapCollection>, 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<VlResolver<'a>>,
 }
 
 impl<'a> Ctx<'a> {
@@ -130,33 +132,6 @@ impl<'a> Ctx<'a> {
         }
     }
 
-    fn heap_obj(&self, addr: u64, idx: u32) -> Result<Vec<u8>, 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<u8>) -> 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<Value> = Vec::new();
     let mut visited = HashSet::new();

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<Option<usize>>,
+}
+
+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<u64, CachedCollection>,
+}
+
+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<Vec<VlElement>, 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<Vec<Vec<u8>>, 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<Vec<String>, 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<Vec<Vec<u8>>, 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<Vec<String>, 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<Vec<Vec<u8>>, 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<u8> {
+        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::<u8>::new()]
+        );
+        assert_eq!(
+            r.sequences(&element(5, 0, 1, 8), 4).unwrap(),
+            vec![Vec::<u8>::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