From bb39be7f24bd4604130bd4e7fcc2003fb9930cb3 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 00:34:44 -0500 Subject: [PATCH] fix(format): keep reading the floats and empty strings clawhdf5 v2.7.0 wrote Two of the datatype checks added on this branch refused files clawhdf5 itself wrote up to v2.7.0: it put the sign bit of every float at position 63 (so every f32 it wrote failed "sign bit position out of bounds", including every agent store's embeddings), and wrote an empty-string attribute with a size-0 string type ("invalid datatype size", failing every attribute of the object). libhdf5 refuses both, but neither decodes to wrong values (an IEEE float's sign position is not used; a size-0 string is empty), so this reader keeps accepting them. New fixtures written by clawhdf5 v2.7.0 (FileBuilder with every datatype, layout and attribute kind it could write, and a FileWriter paged file) and legacy_writer_files.rs, which reads every object of them. The agent's v2.5.0 store fixture (float16_store) passes again. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/datatype.rs | 42 ++++++--- .../tests/fixtures/written_by_v2_7_0.h5 | Bin 0 -> 12264 bytes .../tests/fixtures/written_by_v2_7_0_paged.h5 | Bin 0 -> 192 bytes crates/clawhdf5/tests/legacy_writer_files.rs | 83 ++++++++++++++++++ 4 files changed, 113 insertions(+), 12 deletions(-) create mode 100644 crates/clawhdf5/tests/fixtures/written_by_v2_7_0.h5 create mode 100644 crates/clawhdf5/tests/fixtures/written_by_v2_7_0_paged.h5 create mode 100644 crates/clawhdf5/tests/legacy_writer_files.rs 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 0000000000000000000000000000000000000000..ebb7cf1a07589c25375a9a8337696101066acb61 GIT binary patch literal 12264 zcmeI&2~-rvx(DzY24sXmMnxrx01gU&X)+Qni~sI-kdQbR)@)$ zF-wY18XLd3=>jJUkCM2cm3h<420l71eGW;iJlaeOJCytu*x&jH4 z@QQef(*2E&j+VFQ8yGPw>@SYhx9f>bc~UW@Ia9tud0wrMp7)e@<9R7c%UN7h?ZvT| z*PHMSANQo~Rq~d?&g9rxJX2s#;?DRMFX^Po-NaIImdq4bQ(a20Zt)b;`D1mPXWaIS z2nc7U*ud!iE7~W5Sn?Vz67$tZ_S@z%obNxCaK);(6k1`&B&>P$vUrJ?qhElJrurK5 zT6cwmlAm~@zaW_{FQx}IeMLfwO-LQfqjpq4O!We-HEIfe%50JUeBkn zW++gPb??3(+2$z9I)-O+$8-)pjht-{YnY}F&% zBkUM{He`3zkIyhyUD=hE_0O@ny0RayJbaaf=$1~M6rPSB|D?Zn26R&b^A=_Vad;AA z9XY>OBr$8~zGWxZDnmEG6#l>Kj zpxQAB{kEj5K}n%eeDeHlNjHO%;*edlWM_kt;`~{&q|%_Icy4NzbT=qzLWfkdq=!LC zajdLavWr1U@ha3T>1j|>m|G3cl9xfry0y2{)$kgb{Z2{4Yec=b<~4FPyhgYSbg9M> zxf)(0nmY~ZG`vPMSsIiyyhb!<8k97=Ml?ejlr+3XG%p&IG`vQ1=NptXyhaV_Ed7@R z-Sj8p(;q+B7zYhq??$?rhc|M~kPb$==}*Qk@!JrOqrB+ydr?Nrncu-!z&o>hN~8&E z!*4$<;73`1%y=1lMJ0IWxc~FvM(lH1zXW6qxsmUMZ`tMTxog>U zs)OG%jrXN~!nWZDFTsa}fLW7rSsK;BdzRzs&v!CMTEM$+qtk~E=pF_5CVY8hVcH=U zMu%^O1tZ_zme2aqCcGaz4vxFRq_kT;P`q;_=mGnR7Vz_4r>n~olWA#7@$Kf_2yO=ceYV*42(b`d4lwRVS_sy_cjF zstXo7f!I+UVb{mipH{Ix?yb!;mrFj3ruQsoBN1 zgzC&~&;lvH>m<64z|WiTOW4-keBd|YIjPxSsPX*f<*9`b^v?<2tR<%A*xjBs!pdje z1}lW>!ou4kCPgn16(?Z>Vk)UB7t4uyu-dAdWMEo6LppLM%wyRM-Kpf=@|ed(}*X znNYkzYio5eRVuEF93amYTK$JNsJMy#Nw`hb2|bRn^*x-&=nhWz=k-S&VVO+Dn3lg; zBC0@W_^;@W;&>q(2xEL1Pc@f|ZKz&uB>v*H>DR4QF}4o!;WjWeP8HaJ3#cF*M#2Qp z!W{Sjmcu&u6t=@Y$cIyK5sKk9+=IvP9Qc_x1{ugf0ajoQHed&>!2z7W1(e_k-k<_} z24Vpa4B^lZ2EtGn2{AAl#%fZAM~}4rX5I9HPofH@o63#M>eQ3eHL+;kFwSYnS>BJG@v%=CG)7EYf#bHR-Hv%^#YKEawGzfg6cZ%D7cy@&WmhLvUn z_T6}Xx%;S+bl={~atG$c-I^6LWY3Sqd810F_wrx3#xuF>=FHxaTW{@4E-#tkAGY>j z%G0u2v%^Lldinfmx$TUDQftp|S*WrO^G}br)@IjRf-~}lB(z_dTXpWxtzDN+hu3ZY zbl-;ETfWTRx%t4xJzGmswG#H(F10#ha96uGG`5yiDZ`H!%#@^Kjm!HLu0c&VG&5fM zFfrwP=VEp{?)E{oB7LN_I{e8m$6RdM$(C$zSQ%%%?C!p6^UF=tR&4^8HS+E69&*j% zsM7iDp5`Z$OUD;hyeurQdigTlPQKn@;fJk5b~zQ!&vyEJ^VU@x$9=K&(d;OjP4R0# ziXFChT<<@4i;t!R6Tv(YfXwSqwwm5p9lX1%}i;nOL_RaH~nuuf*Lku^X0&d5wx`;71)$LP#8<*5@d>{n<49CIRq z;&by{PYk`5JSZV|(uLGVX)^npm1!rfqk|mpuAbzWb9Hb+LhjR&D(mQTUgd)memq!N z9yM+MkoW@s@n_c8a!&rrBDDWJ`XAcgXVEOjjg|F0wk)~iHaN@i#fGXwcT(PZ{$44j zoeLG-I5YMcE+>{NG+XZgkCRA#>+E5b2nchT3@(OdG0yvo0w;l@kzaw`1+P)>W}X-we7k7%lc&g(jE#wS~erI|SIJ)mbli)Ag7{nQx8CF0XyI6cc{3mYrw%NdQ27b%-5to=au7ReZ8 z!!XzR4bGJ)&{FelbfX|csd4#{?@@HfHrz!r7%pWyv z!4B-9CA5Opz@IeRLOXB(Z|Dv^fTnY%#%~Su0w3@NKNt-Bdo{n{!(cd!fRPXdZ$mW1 z03P31DolZ?@E)WAKmKW;h3SwE%itqe4l7_Ktb*0B2G+vIunu-Z9_)d=un+db0r&#G zgoAJh^b>y}=1*VzZJWP7TLORF;*XmAosz$u@XO0DB7bIX4;{b>oS`G|2PAY;-P=Eg z1lAe+Apim)2zo;>gaCgw41;j!1F;YXqhK_|!x%__u`mu2VLZ%$444VCU^dKwxiAmr zLnbVMPaqrC!v@HKjj#zmh0kC!d=7^p9}3_I9ED?W98SPVI0dIcKk?qo`z-IMymvN* zW?%!&K|d*xaR(3R0-oRnU7;K3rwC;FLO=a4ps01Uc#^N3i#kD0VCjF z&5XeW-h!sU|LM#InnMe)1v{{Zme2~ifG2oCSLg=b&>ea}Pf&px`ojR=9~TG0AQ%io zU?>cO;V=RwK@udxWOx@+AQh&-RCo{4U@>mct5I39CRq-9zSk_yO+2 z1Nadh!Xwa6b+D}|$e=EmfjP**0`yaB2!4Lc)00Kcj zjYK{Q-iBz1fmn!xQJ|mRM}8V;VLGJ449I|)pr2MFzXsOA$FL4Qfoxb0zn3;(T@Gx7 zP4Fpv2Akn?*aEq*6^_7BI0nbz1e}CZa2n3QSvUu`;T!lC?!b3&7s{XStz*DG;_M3q@$iV{Yfh8!QJ~V)a&<-4cf2D5^9l!~kp(Atx7jOk%@B@Dc zfItX>-Vh8S5DH=Nchdp1{|opM4#FWg4Eaz1`l%TC>+m()fSYg&N}v?<(=+5N;W_*Q z|AH!b0WU#6S)u)npfNN7Yj_KqLNm}$Zpe2ACAfnJbOHV?%?tEXIP!fU0{B01^n?B| z03zXkBI%y;nar#%Rd%_J@WXFNQMB$UpNSt~C-X-eq&@uc2I(1p#6j|CA;ihHLfm5~ z#Ao(G^k^l-$*qMbPwmeiK=e{=-_ZDKg-R%7MbVm~4MjVOEh)C9*p{LL#r71PD0Zai zLeZ6?8^z8Pl@#46dQj{_(UYPV#jX^)QS_$RonjA)Jt?Y$sHMo%l&8ovl&8pgQJy06 zp*%(AOL>aSkMb0mKjkU10LoKjft07nf+$as^`<;U7EF1HEQInDSt#WxvM|b1WZ^nUm}=2K)5w4S1tVm?LIm)29%Qp~5w`q6rdT8jA;S$|qjQA;tOA{#*K zDQYR^Q)H2}o}!jwK1DW=)>G6{%%{lybIF&#+cTM+vBKD_=3d0XZ5gdFrD#R*iKFnN zc*Od#zUn{?=6G7-)K*xtg`!hC%2RC7LC6c=z5oCG9lfZeu`mB(QhlcQd2n)|vqRB| z&f@>pc^Qyhwo7P%`1hZ2SU3~^jKjL~@Xt7`67P)vR1}}S#4$F!ldz|>$;%H2Pha9c vDetDSQ|Tm>*iBLCOnHjRj+Cc3$~-$)Sn^l?@;N}92gJ7FJyM1DLe;+kWu-@h literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..8f837143813386424b68eec7b23ba444ba8ce8fa GIT binary patch literal 192 zcmeD5aB<`1lHy|F;9!7(|4?uMDqsSW5MW^Ndoc5@zlTc@6N4I)5J()R7)G=4FfcGO zFs7tJHC@zNCIHdk!YsnTD5zl1zyg+1U=ZhE5U&Ta8G%}ufO?pqT3DGF7@4^^fX1^h V?tqHHTvz|U=paO>!5&CE008~VD9r!> literal 0 HcmV?d00001 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]); +}