diff --git a/CHANGELOG.md b/CHANGELOG.md index b3ba430..ce688d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -337,6 +337,14 @@ old-style (symbol table) groups a soft link made the listing fail. New `group_v2::resolve_group_children` / `resolve_path_from` and `group_v1::v1_soft_links` in `clawhdf5-format`. +- `clawhdf5` — one unreadable attribute no longer fails `attrs()` for every + attribute on its object: it is left out of the map, and the new + `attrs_with_errors()` (on every group and dataset handle) returns the map + plus one error per attribute left out. Returned values are always complete. + An error in the attribute index itself (attribute info message, dense heap + header or B-tree) still fails the call. `clawhdf5-format` gains + `attribute::extract_attributes_tolerant`; `extract_attributes_full` stays + strict. - `clawhdf5-format` writer — **files libhdf5 rejects or reads wrong:** - Extensible Array (one unlimited dimension): chunks from index 244 on were written but never indexed and read as 0, by libhdf5 and by us. diff --git a/crates/clawhdf5-format/src/attribute.rs b/crates/clawhdf5-format/src/attribute.rs index bb96bd5..62ee2bf 100644 --- a/crates/clawhdf5-format/src/attribute.rs +++ b/crates/clawhdf5-format/src/attribute.rs @@ -394,42 +394,80 @@ pub fn find_attribute<'a>( /// /// Use this instead of `extract_attributes` when reading files that may use dense storage /// (e.g., objects with many attributes, typically >8). +/// +/// Fails if any attribute cannot be read; see [`extract_attributes_tolerant`] +/// to read the others. pub fn extract_attributes_full( file_data: &[u8], header: &ObjectHeader, offset_size: u8, length_size: u8, +) -> Result, FormatError> { + extract_attributes_with(file_data, header, offset_size, length_size, &mut Err) +} + +/// Like [`extract_attributes_full`], but an attribute that cannot be read +/// (a corrupt or unsupported attribute message, or a heap object that cannot +/// be located) is left out and its error returned alongside the attributes +/// that could be read, instead of failing them all. +/// +/// Errors in the structures that index the attributes (the Attribute Info +/// message, the dense-storage heap header or B-tree) still fail the call: +/// then it is unknown which attributes exist at all. +pub fn extract_attributes_tolerant( + file_data: &[u8], + header: &ObjectHeader, + offset_size: u8, + length_size: u8, +) -> Result<(Vec, Vec), FormatError> { + let mut errors = Vec::new(); + let attrs = extract_attributes_with(file_data, header, offset_size, length_size, &mut |e| { + errors.push(e); + Ok(()) + })?; + Ok((attrs, errors)) +} + +/// Read every attribute; each one that fails goes to `on_error`, which +/// either stops the read (returns the error) or skips that attribute. +fn extract_attributes_with( + file_data: &[u8], + header: &ObjectHeader, + offset_size: u8, + length_size: u8, + on_error: &mut dyn FnMut(FormatError) -> Result<(), FormatError>, ) -> Result, FormatError> { let mut attrs = Vec::new(); // Collect compact attributes (inline in OH) for msg in &header.messages { if msg.msg_type == MessageType::Attribute { - if shared_message::is_shared(msg.flags) { + let attr = if shared_message::is_shared(msg.flags) { // Shared attribute: resolve the reference to get actual attribute data - let shared_ref = shared_message::parse_shared_ref(&msg.data, offset_size)?; - let resolved_data = shared_message::resolve_shared_message( - file_data, - &shared_ref, - MessageType::Attribute, - offset_size, - length_size, - )?; - let attr = AttributeMessage::parse_in_file( - &resolved_data, - file_data, - offset_size, - length_size, - )?; - attrs.push(attr); + shared_message::parse_shared_ref(&msg.data, offset_size) + .and_then(|shared_ref| { + shared_message::resolve_shared_message( + file_data, + &shared_ref, + MessageType::Attribute, + offset_size, + length_size, + ) + }) + .and_then(|resolved| { + AttributeMessage::parse_in_file( + &resolved, + file_data, + offset_size, + length_size, + ) + }) } else { - let attr = AttributeMessage::parse_in_file( - &msg.data, - file_data, - offset_size, - length_size, - )?; - attrs.push(attr); + AttributeMessage::parse_in_file(&msg.data, file_data, offset_size, length_size) + }; + match attr { + Ok(attr) => attrs.push(attr), + Err(e) => on_error(e)?, } } } @@ -439,9 +477,15 @@ pub fn extract_attributes_full( if let Some(info) = attr_info && let Some(fh_addr) = info.fractal_heap_address { - let dense_attrs = - extract_dense_attributes(file_data, &info, fh_addr, offset_size, length_size)?; - attrs.extend(dense_attrs); + extract_dense_attributes( + file_data, + &info, + fh_addr, + offset_size, + length_size, + &mut attrs, + on_error, + )?; } Ok(attrs) @@ -468,7 +512,9 @@ fn extract_dense_attributes( fh_addr: u64, offset_size: u8, length_size: u8, -) -> Result, FormatError> { + attrs: &mut Vec, + on_error: &mut dyn FnMut(FormatError) -> Result<(), FormatError>, +) -> Result<(), FormatError> { // Parse fractal heap let fh = FractalHeapHeader::parse(file_data, fh_addr as usize, offset_size, length_size)?; @@ -482,28 +528,32 @@ fn extract_dense_attributes( let btree_hdr = BTreeV2Header::parse(file_data, btree_addr as usize, offset_size, length_size)?; let records = collect_btree_v2_records(file_data, &btree_hdr, offset_size, length_size)?; - let mut attrs = Vec::new(); for record in &records { // Per HDF5 spec, both type 8 and type 9 records start with heap_id: // Type 8: heap_id(8) + msg_flags(1) + creation_order(4) + hash(4) // Type 9: heap_id(8) + msg_flags(1) + creation_order(4) - let id_offset = 0; - - if record.data.len() < id_offset + fh.heap_id_length as usize { + let id_len = fh.heap_id_length as usize; + let Some(id_bytes) = record.data.get(..id_len) else { + on_error(FormatError::UnexpectedEof { + expected: id_len, + available: record.data.len(), + })?; continue; - } - let id_bytes = &record.data[id_offset..id_offset + fh.heap_id_length as usize]; - - // Read attribute message from fractal heap - let attr_data = fh.read_managed_object(file_data, id_bytes, offset_size)?; + }; // The data in the heap is a complete attribute message - let attr = - AttributeMessage::parse_in_file(&attr_data, file_data, offset_size, length_size)?; - attrs.push(attr); + let attr = fh + .read_managed_object(file_data, id_bytes, offset_size) + .and_then(|attr_data| { + AttributeMessage::parse_in_file(&attr_data, file_data, offset_size, length_size) + }); + match attr { + Ok(attr) => attrs.push(attr), + Err(e) => on_error(e)?, + } } - Ok(attrs) + Ok(()) } #[cfg(test)] diff --git a/crates/clawhdf5/src/lazy.rs b/crates/clawhdf5/src/lazy.rs index 314234c..6ceea55 100644 --- a/crates/clawhdf5/src/lazy.rs +++ b/crates/clawhdf5/src/lazy.rs @@ -12,7 +12,6 @@ use std::cell::RefCell; use std::collections::HashMap; -use clawhdf5_format::attribute::extract_attributes_full; use clawhdf5_format::data_layout::DataLayout; use clawhdf5_format::data_read; use clawhdf5_format::dataspace::Dataspace; @@ -29,7 +28,7 @@ use clawhdf5_format::superblock::Superblock; use clawhdf5_io::HDF5Read; use crate::error::Error; -use crate::types::{AttrValue, DType, attrs_to_map, classify_datatype}; +use crate::types::{AttrValue, DType, classify_datatype, read_attrs}; /// A lazy HDF5 file handle that parses metadata on demand. /// @@ -231,17 +230,26 @@ impl<'f, R: HDF5Read> LazyGroup<'f, R> { } /// Read all attributes of this group. + /// + /// An attribute that cannot be read — a corrupt or unsupported attribute + /// message, or a dense-storage heap object that cannot be located — is + /// left out of the map instead of failing every attribute on the object; + /// [`attrs_with_errors`](Self::attrs_with_errors) reports which failed. + /// Values that are returned are complete (never partially decoded). An + /// error in the index of the attributes itself (the attribute info + /// message, the dense heap header or B-tree) still fails the call. pub fn attrs(&self) -> Result, Error> { + self.attrs_with_errors().map(|(attrs, _)| attrs) + } + + /// Like [`attrs`](Self::attrs), also returning one error for each + /// attribute that could not be read and was left out. + pub fn attrs_with_errors( + &self, + ) -> Result<(HashMap, Vec), Error> { let hdr = self.file.get_or_parse_header(self.address)?; let data = self.file.reader.as_bytes(); - let attr_msgs = - extract_attributes_full(data, &hdr, self.file.offset_size(), self.file.length_size())?; - Ok(attrs_to_map( - &attr_msgs, - data, - self.file.offset_size(), - self.file.length_size(), - )) + read_attrs(data, &hdr, self.file.offset_size(), self.file.length_size()) } /// Get a dataset within this group by name. @@ -401,20 +409,30 @@ impl<'f, R: HDF5Read> LazyDataset<'f, R> { } /// Read all attributes of this dataset. + /// + /// An attribute that cannot be read — a corrupt or unsupported attribute + /// message, or a dense-storage heap object that cannot be located — is + /// left out of the map instead of failing every attribute on the object; + /// [`attrs_with_errors`](Self::attrs_with_errors) reports which failed. + /// Values that are returned are complete (never partially decoded). An + /// error in the index of the attributes itself (the attribute info + /// message, the dense heap header or B-tree) still fails the call. pub fn attrs(&self) -> Result, Error> { + self.attrs_with_errors().map(|(attrs, _)| attrs) + } + + /// Like [`attrs`](Self::attrs), also returning one error for each + /// attribute that could not be read and was left out. + pub fn attrs_with_errors( + &self, + ) -> Result<(HashMap, Vec), Error> { let data = self.file.reader.as_bytes(); - let attr_msgs = extract_attributes_full( + read_attrs( data, &self.header, self.file.offset_size(), self.file.length_size(), - )?; - Ok(attrs_to_map( - &attr_msgs, - data, - self.file.offset_size(), - self.file.length_size(), - )) + ) } /// A header message's payload, resolved through the shared-message diff --git a/crates/clawhdf5/src/mmap_file.rs b/crates/clawhdf5/src/mmap_file.rs index bf098cf..b57d8b0 100644 --- a/crates/clawhdf5/src/mmap_file.rs +++ b/crates/clawhdf5/src/mmap_file.rs @@ -7,7 +7,6 @@ use std::collections::HashMap; -use clawhdf5_format::attribute::extract_attributes_full; use clawhdf5_format::data_layout::DataLayout; use clawhdf5_format::data_read; use clawhdf5_format::dataspace::Dataspace; @@ -24,7 +23,7 @@ use clawhdf5_format::superblock::Superblock; use clawhdf5_io::MmapReader; use crate::error::Error; -use crate::types::{AttrValue, DType, attrs_to_map, classify_datatype}; +use crate::types::{AttrValue, DType, classify_datatype, read_attrs}; /// An HDF5 file opened via memory mapping. /// @@ -153,17 +152,26 @@ impl<'f> MmapGroup<'f> { } /// Read all attributes of this group. + /// + /// An attribute that cannot be read — a corrupt or unsupported attribute + /// message, or a dense-storage heap object that cannot be located — is + /// left out of the map instead of failing every attribute on the object; + /// [`attrs_with_errors`](Self::attrs_with_errors) reports which failed. + /// Values that are returned are complete (never partially decoded). An + /// error in the index of the attributes itself (the attribute info + /// message, the dense heap header or B-tree) still fails the call. pub fn attrs(&self) -> Result, Error> { - let data = self.file.reader.as_bytes(); + self.attrs_with_errors().map(|(attrs, _)| attrs) + } + + /// Like [`attrs`](Self::attrs), also returning one error for each + /// attribute that could not be read and was left out. + pub fn attrs_with_errors( + &self, + ) -> Result<(HashMap, Vec), Error> { let hdr = self.file.parse_header(self.address)?; - let attr_msgs = - extract_attributes_full(data, &hdr, self.file.offset_size(), self.file.length_size())?; - Ok(attrs_to_map( - &attr_msgs, - data, - self.file.offset_size(), - self.file.length_size(), - )) + let data = self.file.reader.as_bytes(); + read_attrs(data, &hdr, self.file.offset_size(), self.file.length_size()) } /// Get a dataset within this group by name. @@ -342,20 +350,30 @@ impl<'f> MmapDataset<'f> { } /// Read all attributes of this dataset. + /// + /// An attribute that cannot be read — a corrupt or unsupported attribute + /// message, or a dense-storage heap object that cannot be located — is + /// left out of the map instead of failing every attribute on the object; + /// [`attrs_with_errors`](Self::attrs_with_errors) reports which failed. + /// Values that are returned are complete (never partially decoded). An + /// error in the index of the attributes itself (the attribute info + /// message, the dense heap header or B-tree) still fails the call. pub fn attrs(&self) -> Result, Error> { + self.attrs_with_errors().map(|(attrs, _)| attrs) + } + + /// Like [`attrs`](Self::attrs), also returning one error for each + /// attribute that could not be read and was left out. + pub fn attrs_with_errors( + &self, + ) -> Result<(HashMap, Vec), Error> { let data = self.file.reader.as_bytes(); - let attr_msgs = extract_attributes_full( + read_attrs( data, &self.header, self.file.offset_size(), self.file.length_size(), - )?; - Ok(attrs_to_map( - &attr_msgs, - data, - self.file.offset_size(), - self.file.length_size(), - )) + ) } /// A header message's payload, resolved through the shared-message diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 9c9a805..b8e7829 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -7,7 +7,6 @@ use std::collections::HashMap; -use clawhdf5_format::attribute::extract_attributes_full; use clawhdf5_format::chunk_cache::ChunkCache; use clawhdf5_format::data_layout::DataLayout; use clawhdf5_format::data_read; @@ -23,7 +22,7 @@ use clawhdf5_format::signature; use clawhdf5_format::superblock::Superblock; use crate::error::Error; -use crate::types::{AttrValue, DType, attrs_to_map, classify_datatype}; +use crate::types::{AttrValue, DType, classify_datatype, read_attrs}; // --------------------------------------------------------------------------- // FileData — internal storage for either owned bytes or an mmap @@ -293,17 +292,26 @@ impl<'f> Group<'f> { } /// Read all attributes of this group. + /// + /// An attribute that cannot be read — a corrupt or unsupported attribute + /// message, or a dense-storage heap object that cannot be located — is + /// left out of the map instead of failing every attribute on the object; + /// [`attrs_with_errors`](Self::attrs_with_errors) reports which failed. + /// Values that are returned are complete (never partially decoded). An + /// error in the index of the attributes itself (the attribute info + /// message, the dense heap header or B-tree) still fails the call. pub fn attrs(&self) -> Result, Error> { - let data = self.file.data.as_bytes(); + self.attrs_with_errors().map(|(attrs, _)| attrs) + } + + /// Like [`attrs`](Self::attrs), also returning one error for each + /// attribute that could not be read and was left out. + pub fn attrs_with_errors( + &self, + ) -> Result<(HashMap, Vec), Error> { let hdr = self.file.parse_header(self.address)?; - let attr_msgs = - extract_attributes_full(data, &hdr, self.file.offset_size(), self.file.length_size())?; - Ok(attrs_to_map( - &attr_msgs, - data, - self.file.offset_size(), - self.file.length_size(), - )) + let data = self.file.data.as_bytes(); + read_attrs(data, &hdr, self.file.offset_size(), self.file.length_size()) } /// Get a dataset within this group by name. @@ -732,20 +740,30 @@ impl<'f> Dataset<'f> { } /// Read all attributes of this dataset. + /// + /// An attribute that cannot be read — a corrupt or unsupported attribute + /// message, or a dense-storage heap object that cannot be located — is + /// left out of the map instead of failing every attribute on the object; + /// [`attrs_with_errors`](Self::attrs_with_errors) reports which failed. + /// Values that are returned are complete (never partially decoded). An + /// error in the index of the attributes itself (the attribute info + /// message, the dense heap header or B-tree) still fails the call. pub fn attrs(&self) -> Result, Error> { + self.attrs_with_errors().map(|(attrs, _)| attrs) + } + + /// Like [`attrs`](Self::attrs), also returning one error for each + /// attribute that could not be read and was left out. + pub fn attrs_with_errors( + &self, + ) -> Result<(HashMap, Vec), Error> { let data = self.file.data.as_bytes(); - let attr_msgs = extract_attributes_full( + read_attrs( data, &self.header, self.file.offset_size(), self.file.length_size(), - )?; - Ok(attrs_to_map( - &attr_msgs, - data, - self.file.offset_size(), - self.file.length_size(), - )) + ) } /// Verify this dataset's content against its stored provenance hash diff --git a/crates/clawhdf5/src/types.rs b/crates/clawhdf5/src/types.rs index 91cb884..7023b14 100644 --- a/crates/clawhdf5/src/types.rs +++ b/crates/clawhdf5/src/types.rs @@ -155,6 +155,33 @@ pub(crate) fn classify_datatype(dt: &clawhdf5_format::datatype::Datatype) -> DTy /// Read attribute messages into a `HashMap`. /// /// Best-effort: attributes that can't be decoded are silently skipped. +/// The attributes of the object with header `header` that could be read, +/// and one error for each that could not (see +/// [`extract_attributes_tolerant`](clawhdf5_format::attribute::extract_attributes_tolerant)). +pub(crate) fn read_attrs( + file_data: &[u8], + header: &clawhdf5_format::object_header::ObjectHeader, + offset_size: u8, + length_size: u8, +) -> Result< + ( + HashMap, + Vec, + ), + crate::Error, +> { + let (msgs, errors) = clawhdf5_format::attribute::extract_attributes_tolerant( + file_data, + header, + offset_size, + length_size, + )?; + Ok(( + attrs_to_map(&msgs, file_data, offset_size, length_size), + errors, + )) +} + pub(crate) fn attrs_to_map( attrs: &[clawhdf5_format::attribute::AttributeMessage], file_data: &[u8], diff --git a/crates/clawhdf5/tests/dense_storage_interop.rs b/crates/clawhdf5/tests/dense_storage_interop.rs index cbb3b63..b5aa8d1 100644 --- a/crates/clawhdf5/tests/dense_storage_interop.rs +++ b/crates/clawhdf5/tests/dense_storage_interop.rs @@ -364,3 +364,55 @@ fn soft_links_are_listed_as_their_targets() { check_soft_links("latest"); check_soft_links("earliest"); } + +/// One unreadable attribute used to fail `attrs()` for every attribute on +/// the object. Now it is left out (and reported by `attrs_with_errors`), +/// and the others are returned with their full values. +#[test] +fn one_unreadable_attribute_does_not_hide_the_others() { + skip_if_no_python!(); + let (dir, path) = h5py_file( + "d = f.create_dataset('d', data=[1.0])\n\ + for i in range(10):\n\ + \x20 d.attrs['a%d' % i] = float(i)\n\ + d.attrs['zz_broken_attribute'] = 42.0\n", + ); + // Dense attributes live in a fractal heap whose blocks carry no checksum + // by default: give the one named `zz_broken_attribute` a nonexistent + // attribute message version (the byte 9 before its name in a v3 message). + let mut bytes = std::fs::read(&path).unwrap(); + let needle = b"zz_broken_attribute"; + let at = bytes + .windows(needle.len()) + .position(|w| w == needle) + .expect("attribute name in the file"); + assert_eq!(bytes[at - 9], 3, "expected a version-3 attribute message"); + bytes[at - 9] = 0x7f; + let broken = dir.path().join("broken.h5"); + std::fs::write(&broken, &bytes).unwrap(); + + for file in [ + File::open(&broken).unwrap(), + File::from_bytes(bytes.clone()).unwrap(), + ] { + let ds = file.dataset("d").unwrap(); + let (attrs, errors) = ds.attrs_with_errors().unwrap(); + assert_eq!(errors.len(), 1, "{errors:?}"); + assert_eq!(attrs.len(), 10); + for i in 0..10 { + assert!( + matches!(attrs[&format!("a{i}")], AttrValue::F64(v) if v == f64::from(i)), + "a{i}" + ); + } + assert!(!attrs.contains_key("zz_broken_attribute")); + assert_eq!(ds.attrs().unwrap().len(), 10); + } + let m = clawhdf5::MmapFile::open(&broken).unwrap(); + assert_eq!( + m.dataset("d").unwrap().attrs_with_errors().unwrap().1.len(), + 1 + ); + let l = clawhdf5::LazyFile::from_bytes(bytes).unwrap(); + assert_eq!(l.dataset("d").unwrap().attrs().unwrap().len(), 10); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index 3dba53d..a05688f 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -87,7 +87,9 @@ the VDS item, which is marked. - **Dense attributes:** a large attribute stored as a fractal-heap "huge" object makes every attribute on the object fail. This affects real NetCDF files (`issue671.nc`). **Fixed 2026-09-25:** huge and tiny heap objects, - and filtered heaps, are read. + and filtered heaps, are read; and an attribute that still cannot be read + 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`. - Metadata cache images are not supported.