From 17f09375ad457ef01c13f636e8a0f6a028765a26 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:21:20 -0500 Subject: [PATCH] fix(format): refuse to write datatypes the reader refuses The reader now refuses a compound with a repeated field name or no fields and an enum member with an empty name, as libhdf5 does, but the writer still wrote them: CompoundTypeBuilder and EnumTypeBuilder build them without complaint, so clawhdf5 wrote files it could not read back. They were never valid HDF5; h5py refuses them. Datatype::check_encodable, which FileWriter::finish runs on every dataset and attribute type, now parses the type's own encoding back and refuses one the reader refuses, with the reader's reason. That keeps the writer in step with every reader check, not only these three. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 7 +++ crates/clawhdf5-format/src/datatype.rs | 22 ++++++-- crates/clawhdf5/tests/legacy_writer_files.rs | 58 ++++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fdde48..e555823 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -333,6 +333,13 @@ a file shorter than the end of file its superblock records is refused ("truncated file"), and nothing past that end is read. `File`, `LazyFile` and `MmapFile` do this. + - the writer: `FileWriter::finish()` / `FileBuilder::finish()` refuse a + datatype the reader would refuse (`FormatError::SerializationError`, + "datatype cannot be written: ..."), such as a compound with a repeated + field name or no fields, or an enum member with an empty name + (`CompoundTypeBuilder` and `EnumTypeBuilder` build them without + complaint). These were never valid HDF5 — h5py refuses them — and + clawhdf5 wrote them, which made files it could not read back. Checks newer libhdf5 releases make but HDF5 2.0 does not (bit-field offsets, the variable-length kind, array sizes) are left out, so files diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index 980575d..703f44a 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -1141,9 +1141,23 @@ impl Datatype { } /// Check that this datatype can be written: every part of it has an - /// on-disk encoding. [`Self::serialize`] cannot report errors, so the - /// writer calls this first. + /// on-disk encoding, and the encoding is one the reader (and libhdf5) + /// accepts. [`Self::serialize`] cannot report errors, so the writer calls + /// this first. A compound with no fields or a repeated field name, or an + /// enum member with an empty name, is refused here: libhdf5 and h5py + /// refuse such types, and so does [`Self::parse`], so writing one made a + /// file that could not be read back. pub fn check_encodable(&self) -> Result<(), FormatError> { + self.check_encodable_parts()?; + Self::parse(&self.serialize()).map_err(|e| { + FormatError::SerializationError(format!( + "datatype cannot be written: HDF5 readers refuse it ({e})" + )) + })?; + Ok(()) + } + + fn check_encodable_parts(&self) -> Result<(), FormatError> { match self { Datatype::Opaque { tag, .. } if opaque_tag_text(tag).len() > MAX_OPAQUE_TAG_LEN => { Err(FormatError::SerializationError(format!( @@ -1156,10 +1170,10 @@ impl Datatype { )), Datatype::Compound { members, .. } => members .iter() - .try_for_each(|m| m.datatype.check_encodable()), + .try_for_each(|m| m.datatype.check_encodable_parts()), Datatype::Enumeration { base_type, .. } | Datatype::VariableLength { base_type, .. } - | Datatype::Array { base_type, .. } => base_type.check_encodable(), + | Datatype::Array { base_type, .. } => base_type.check_encodable_parts(), _ => Ok(()), } } diff --git a/crates/clawhdf5/tests/legacy_writer_files.rs b/crates/clawhdf5/tests/legacy_writer_files.rs index 3e8a3b9..e826064 100644 --- a/crates/clawhdf5/tests/legacy_writer_files.rs +++ b/crates/clawhdf5/tests/legacy_writer_files.rs @@ -81,3 +81,61 @@ fn every_object_of_a_v2_7_0_file_reads() { let paged = File::open(fixture("written_by_v2_7_0_paged.h5")).unwrap(); assert_eq!(paged.dataset("d").unwrap().read_f32().unwrap(), [1.0, 2.0]); } + +/// Datatypes libhdf5 (and this reader) refuse — a compound with a repeated +/// field name or no fields, an enum member with an empty name — must not be +/// written: they made files that did not read back. Valid neighbours of each +/// still write and read back. +#[test] +fn datatypes_the_reader_refuses_are_not_written() { + use clawhdf5::{CompoundTypeBuilder, EnumTypeBuilder, FileBuilder}; + + let write_compound = |dt, raw: Vec| { + let mut b = FileBuilder::new(); + b.create_dataset("d").with_compound_data(dt, raw, 1); + b.finish() + }; + let write_enum = |dt| { + let mut b = FileBuilder::new(); + b.create_dataset("d").with_enum_u8_data(dt, &[0, 1]); + b.finish() + }; + let refused = |r: Result, clawhdf5::Error>, what: &str| { + let err = r.expect_err("written"); + let msg = err.to_string(); + assert!(msg.contains("datatype cannot be written"), "{msg}"); + assert!(msg.contains(what), "{msg}"); + }; + + let dup = CompoundTypeBuilder::new() + .f64_field("x") + .f64_field("x") + .build(); + refused( + write_compound(dup, vec![0; 16]), + "duplicated compound field name 'x'", + ); + refused( + write_compound(CompoundTypeBuilder::new().build(), vec![]), + "invalid", + ); + let empty_name = EnumTypeBuilder::u8_based() + .u8_value("A", 0) + .u8_value("", 1) + .build(); + refused(write_enum(empty_name), "0 length enum name"); + + let ok = CompoundTypeBuilder::new() + .f64_field("x") + .f64_field("y") + .build(); + let bytes = write_compound(ok, vec![0; 16]).unwrap(); + let file = File::from_bytes(bytes).unwrap(); + file.dataset("d").unwrap().dtype().unwrap(); + let ok = EnumTypeBuilder::u8_based() + .u8_value("A", 0) + .u8_value("B", 1) + .build(); + let file = File::from_bytes(write_enum(ok).unwrap()).unwrap(); + file.dataset("d").unwrap().dtype().unwrap(); +}