diff --git a/crates/clawhdf5-format/src/datatype.rs b/crates/clawhdf5-format/src/datatype.rs index d97a61d..980575d 100644 --- a/crates/clawhdf5-format/src/datatype.rs +++ b/crates/clawhdf5-format/src/datatype.rs @@ -289,9 +289,15 @@ fn ranges_overlap(a0: u64, a1: u64, b0: u64, b1: u64) -> bool { a0 <= b1 && b0 <= a1 } -/// libhdf5's checks on a floating-point type's fields: sign, exponent and -/// mantissa must lie inside the type, be non-empty, and not overlap. -/// (libhdf5 does not check a float's bit offset and precision.) +/// libhdf5's checks on a floating-point type's fields: exponent and mantissa +/// must lie inside the type, be non-empty, and not overlap each other or the +/// sign bit. (libhdf5 does not check a float's bit offset and precision.) +/// +/// One libhdf5 check is left out on purpose: a sign bit position outside the +/// type ("sign bit position out of bounds"). clawhdf5 up to v2.7.0 wrote 63 +/// there for every float, so every `f32` it wrote (every agent store's +/// embeddings) would stop opening. The position is not used to decode an +/// IEEE float, so reading such a type returns the right values. fn check_float_fields( size: u32, sign: u8, @@ -308,9 +314,6 @@ fn check_float_fields( u64::from(mpos), u64::from(msize), ); - if sign >= bits { - return Err(invalid("sign bit position out of bounds")); - } if esize == 0 { return Err(invalid("exponent size can't be zero")); } @@ -380,7 +383,12 @@ impl Datatype { let size = LittleEndian::read_u32(&data[4..8]); let mut pos = 8; - if size == 0 { + // libhdf5 refuses size 0 for every class. A fixed-length string is + // exempt: clawhdf5 up to v2.7.0 wrote an empty-string attribute + // with a size-0 string type, and refusing it would fail every + // attribute of such objects, while reading it (an empty string) is + // harmless. + if size == 0 && class_id != 3 { return Err(invalid("invalid datatype size")); } @@ -2335,10 +2343,9 @@ mod tests { let mut data = build_dt_header(9, 1, [1, 0, 0], 0); data.extend_from_slice(&build_fixed_point(1, false, false, 0, 8)); assert_eq!(invalid_reason(&data), "invalid datatype size"); - assert_eq!( - invalid_reason(&build_dt_header(3, 1, [0, 0, 0], 0)), - "invalid datatype size" - ); + // Except a fixed-length string, which clawhdf5 <= v2.7.0 wrote for an + // empty-string attribute. + assert!(Datatype::parse(&build_dt_header(3, 1, [0, 0, 0], 0)).is_ok()); assert_eq!( invalid_reason(&build_fixed_point(0, false, false, 0, 0)), "invalid datatype size" @@ -2376,7 +2383,6 @@ mod tests { }; assert!(Datatype::parse(&f32_with(31, 23, 8, 0, 23)).is_ok()); for (fields, why) in [ - ((32, 23, 8, 0, 23), "sign bit position out of bounds"), ((31, 23, 0, 0, 23), "exponent size can't be zero"), ( (31, 32, 8, 0, 23), @@ -2447,6 +2453,18 @@ mod tests { assert!(Datatype::parse_in_header(&data, 1).is_err()); } + #[test] + fn f32_written_by_clawhdf5_up_to_2_7_0_still_parses() { + // Those versions put the sign bit at 63 whatever the float's size; + // libhdf5 refuses it ("sign bit position out of bounds"). + let mut data = build_dt_header(1, 1, [0x20, 63, 0], 4); + data.extend_from_slice(&0u16.to_le_bytes()); + data.extend_from_slice(&32u16.to_le_bytes()); + data.extend_from_slice(&[23, 8, 0, 23]); + data.extend_from_slice(&127u32.to_le_bytes()); + assert!(Datatype::parse(&data).is_ok()); + } + #[test] fn float_bit_6_is_vax_order_only_from_version_3() { // h5py opens a v1 float with bit 6 set as an ordinary little-endian diff --git a/crates/clawhdf5/tests/fixtures/written_by_v2_7_0.h5 b/crates/clawhdf5/tests/fixtures/written_by_v2_7_0.h5 new file mode 100644 index 0000000..ebb7cf1 Binary files /dev/null and b/crates/clawhdf5/tests/fixtures/written_by_v2_7_0.h5 differ diff --git a/crates/clawhdf5/tests/fixtures/written_by_v2_7_0_paged.h5 b/crates/clawhdf5/tests/fixtures/written_by_v2_7_0_paged.h5 new file mode 100644 index 0000000..8f83714 Binary files /dev/null and b/crates/clawhdf5/tests/fixtures/written_by_v2_7_0_paged.h5 differ diff --git a/crates/clawhdf5/tests/legacy_writer_files.rs b/crates/clawhdf5/tests/legacy_writer_files.rs new file mode 100644 index 0000000..3e8a3b9 --- /dev/null +++ b/crates/clawhdf5/tests/legacy_writer_files.rs @@ -0,0 +1,83 @@ +//! Files written by older clawhdf5 releases must keep opening, even where +//! libhdf5 refuses them: stricter validation of corrupt files must not lock +//! users out of their own data. +//! +//! `fixtures/written_by_v2_7_0.h5` and `written_by_v2_7_0_paged.h5` were +//! written by clawhdf5 v2.7.0 (`FileBuilder` / `FileWriter` with every +//! datatype, layout and attribute kind it could write). v2.7.0 wrote the sign +//! bit of every float at position 63 and a size-0 string type for an empty +//! string attribute; libhdf5 refuses both, this reader must not. + +use std::path::PathBuf; + +use clawhdf5::{AttrValue, File}; + +fn fixture(name: &str) -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures") + .join(name) +} + +#[test] +fn every_object_of_a_v2_7_0_file_reads() { + let file = File::open(fixture("written_by_v2_7_0.h5")).unwrap(); + + let (attrs, errors) = file.root().attrs_with_errors().unwrap(); + assert!(errors.is_empty(), "{errors:?}"); + // (It reads as no strings, as it did before.) + assert!( + matches!(&attrs["empty"], AttrValue::StringArray(v) if v.iter().all(String::is_empty)), + "{:?}", + attrs["empty"] + ); + assert!(matches!(&attrs["title"], AttrValue::String(s) if s == "old")); + + let f32s = |name: &str| file.dataset(name).unwrap().read_f32().unwrap(); + assert_eq!(f32s("f32"), [1.0, 2.0, 3.0]); + assert_eq!( + f32s("f32_2d"), + (0..60).map(|x| x as f32).collect::>() + ); + assert_eq!( + f32s("chunked"), + (0..1000).map(|x| x as f32).collect::>() + ); + assert!(f32s("empty").is_empty()); + assert_eq!( + file.dataset("f64").unwrap().read_f64().unwrap(), + [1.0, 2.0, 3.0] + ); + assert_eq!(file.dataset("i32").unwrap().read_i32().unwrap(), [1, -2, 3]); + assert_eq!(file.dataset("i64").unwrap().read_i64().unwrap(), [1, -2, 3]); + assert_eq!(file.dataset("u64").unwrap().read_u64().unwrap(), [1, 2, 3]); + assert_eq!( + file.dataset("chunked_2d").unwrap().read_i32().unwrap(), + (0..600).collect::>() + ); + for name in ["unlimited", "maxshape"] { + assert_eq!( + file.dataset(name).unwrap().read_f64().unwrap(), + (0..100).map(|x| x as f64).collect::>(), + "{name}" + ); + } + assert_eq!( + file.dataset("compact").unwrap().read_i32().unwrap(), + [7, 8, 9] + ); + for name in ["u8", "compound", "enum", "enum8"] { + file.dataset(name) + .unwrap() + .dtype() + .unwrap_or_else(|e| panic!("{name}: {e}")); + } + + let grp = file.group("grp").unwrap(); + let (attrs, errors) = grp.attrs_with_errors().unwrap(); + assert!(errors.is_empty(), "{errors:?}"); + assert_eq!(attrs.len(), 21); + assert_eq!(grp.dataset("d").unwrap().read_f32().unwrap(), [4.0, 5.0]); + + 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]); +}