From 57e938c4383db3dfd7ae2ce9a7cab0d7dcb39d3c Mon Sep 17 00:00:00 2001 From: osobh Date: Fri, 25 Sep 2026 21:14:30 -0500 Subject: [PATCH] fix(format): honour unknown-message flags the way libhdf5 does The object header parser failed on an unknown message with flag bit 3 set and ignored bit 7. Per the spec, bit 3 means "fail if unknown and the file is opened for writing" and bit 7 "fail if unknown, always". The parser only reads, so it now ignores bit 3 (as libhdf5 does for a read-only open) and refuses bit 7, in v1 headers, v2 headers and their continuation chunks. On libhdf5's conformance file tbogus.h5 (added as a fixture) we used to refuse Dataset2 and open Dataset3; we now match libhdf5: Dataset1, 2, 4 and 5 open, Dataset3 is refused. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/object_header.rs | 67 +++++++++++++----- .../clawhdf5-format/tests/fixtures/tbogus.h5 | Bin 0 -> 5056 bytes .../tests/writer_meta_tests.rs | 34 +++++++++ 3 files changed, 82 insertions(+), 19 deletions(-) create mode 100644 crates/clawhdf5-format/tests/fixtures/tbogus.h5 diff --git a/crates/clawhdf5-format/src/object_header.rs b/crates/clawhdf5-format/src/object_header.rs index 8306185..4b8067f 100644 --- a/crates/clawhdf5-format/src/object_header.rs +++ b/crates/clawhdf5-format/src/object_header.rs @@ -146,12 +146,7 @@ impl ObjectHeader { ensure_len(data, pos, msg_data_size)?; let msg_type = MessageType::from_u16(msg_type_raw); - // Check if unknown + must-understand (bit 3 of msg_flags) - if let MessageType::Unknown(id) = msg_type - && msg_flags & 0x08 != 0 - { - return Err(FormatError::UnsupportedMessage(id)); - } + check_unknown_message(msg_type, msg_flags)?; if msg_type != MessageType::Nil { messages.push(HeaderMessage { @@ -229,11 +224,7 @@ impl ObjectHeader { let msg_type = MessageType::from_u16(msg_type_raw); - if let MessageType::Unknown(id) = msg_type - && msg_flags & 0x08 != 0 - { - return Err(FormatError::UnsupportedMessage(id)); - } + check_unknown_message(msg_type, msg_flags)?; if msg_type != MessageType::Nil { messages.push(HeaderMessage { @@ -424,11 +415,7 @@ impl ObjectHeader { let msg_type = MessageType::from_u16(msg_type_raw); - if let MessageType::Unknown(id) = msg_type - && msg_flags & 0x08 != 0 - { - return Err(FormatError::UnsupportedMessage(id)); - } + check_unknown_message(msg_type, msg_flags)?; let msg_data = data[pos..pos + msg_data_size].to_vec(); @@ -509,6 +496,24 @@ impl ObjectHeader { } } +/// Header message flag bit 7: fail if the message is unknown, always. +const MSG_FLAG_FAIL_IF_UNKNOWN_ALWAYS: u8 = 0x80; + +/// Refuse an unknown message the file says no reader may skip. +/// +/// The parser only ever reads, so bit 3 (fail only when opened for writing) +/// is ignored, as libhdf5 ignores it for a read-only open; bit 7 fails +/// regardless of access mode. This had the two the wrong way round, failing +/// objects libhdf5 reads and reading ones it refuses (`tbogus.h5`). +fn check_unknown_message(msg_type: MessageType, msg_flags: u8) -> Result<(), FormatError> { + match msg_type { + MessageType::Unknown(id) if msg_flags & MSG_FLAG_FAIL_IF_UNKNOWN_ALWAYS != 0 => { + Err(FormatError::UnsupportedMessage(id)) + } + _ => Ok(()), + } +} + #[cfg(test)] mod tests { use super::*; @@ -632,14 +637,38 @@ mod tests { } #[test] - fn parse_v1_unknown_must_understand_errors() { - // Bit 3 of msg_flags = must understand - let messages = [(0x00FFu16, &[0xAA][..], 0x08u8)]; + fn parse_v1_unknown_fail_always_errors() { + // Bit 7 of msg_flags = fail if unknown, whatever the access mode. + let messages = [(0x00FFu16, &[0xAA][..], 0x80u8)]; let data = build_v1_header(&messages, 8, 8); let err = ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(); assert_eq!(err, FormatError::UnsupportedMessage(0x00FF)); } + #[test] + fn parse_v1_unknown_fail_on_write_is_ignored_when_reading() { + // Bit 3 = fail if unknown *and the file is opened for writing*. This + // parser only reads, so libhdf5 (read-only) opens such an object and + // so must we. Bits 4/5 (mark if unknown / was unknown) never fail. + for flags in [0x08u8, 0x10, 0x20, 0x38] { + let messages = [(0x00FFu16, &[0xAA][..], flags)]; + let data = build_v1_header(&messages, 8, 8); + let hdr = ObjectHeader::parse(&data, 0, 8, 8).unwrap(); + assert_eq!(hdr.messages[0].msg_type, MessageType::Unknown(0x00FF)); + } + } + + #[test] + fn parse_v2_unknown_message_flags() { + let data = build_v2_header(0x00, &[(0xF0, &[1, 2], 0x08)], None); + assert!(ObjectHeader::parse(&data, 0, 8, 8).is_ok()); + let data = build_v2_header(0x00, &[(0xF0, &[1, 2], 0x80)], None); + assert_eq!( + ObjectHeader::parse(&data, 0, 8, 8).unwrap_err(), + FormatError::UnsupportedMessage(0xF0) + ); + } + #[test] fn parse_v2_no_timestamps_one_message() { let data = build_v2_header(0x00, &[(0x01, &[10, 20], 0)], None); diff --git a/crates/clawhdf5-format/tests/fixtures/tbogus.h5 b/crates/clawhdf5-format/tests/fixtures/tbogus.h5 new file mode 100644 index 0000000000000000000000000000000000000000..f64229e9356628aa8995a04099f85a952e056805 GIT binary patch literal 5056 zcmeD5aB<`1lHy_j0S*oZ76t(@6Gr@pf&;=35f~pPp8#brLg@}Dy@CnCU}OM61_lYJ zxFFPgbaf#?uC5F~l`!*RG*lad0Skl`0TURdM^p%SxH<-aJiGzw>jWrW!2@N`h+<@5 z2d7^M0ZO49V4Gm+of(*(L2Ln_FeHg8faO_%>OkU5OiW;<9MBxV%m_=_&;$)u&A`A3 zHTV6#wf8_mLP+);Y(A60z|a6yIj~f)pT7$u0~^$J3=9g)_}v4`_Z6)8)oDPbJJ|56 zvw%v^V8^e{11h}&7%%t$tUTGl2~h=$*9TBO1C7!bJ<}B^2nKt)qGxzCjD`m!u>(m^ zxdW>4N7Dx+NI>D?FeJhQd%Fs~+#=MjvfzXG8&+OIc%$S<2?1EU3RVxo>OTdvde0@X zB(XTP#1IxPP`(iw-x!T=g2p$6@nJNz%}p=LFD(EX4)X`N(Fn7Q0-9d+lQgttHQ38z zNIMYJ%7p+8Ui^UzYX>&)<5#Bvm7V~ql<)vpJ8*#@9z{SYSh==A2*0|4lBH+50>#x} fPgnE|kA~6kfG2xUxii`hga-!$C_Eg7K>7dxb&LV5 literal 0 HcmV?d00001 diff --git a/crates/clawhdf5-format/tests/writer_meta_tests.rs b/crates/clawhdf5-format/tests/writer_meta_tests.rs index 587f65f..88a08a1 100644 --- a/crates/clawhdf5-format/tests/writer_meta_tests.rs +++ b/crates/clawhdf5-format/tests/writer_meta_tests.rs @@ -563,3 +563,37 @@ fn slash_in_a_group_or_dataset_name_is_an_error() { let bytes = fw.finish().unwrap(); header_at(&bytes, "g/c"); } + +// ---- 7. unknown-message flags on read ---- + +#[test] +fn unknown_message_flags_follow_libhdf5_on_tbogus() { + // libhdf5's own test file (test/testfiles/tbogus.h5): datasets carrying + // an unknown message with various flags. libhdf5 (read-only) opens + // Dataset1, 2, 4 and 5 and refuses Dataset3 ("unknown message with 'fail + // if unknown' flag found"). We used to refuse Dataset2 (bit 3, which only + // applies when writing) and open Dataset3 (bit 7, fail always). + let bytes = include_bytes!("fixtures/tbogus.h5"); + let sig = signature::find_signature(bytes).unwrap(); + let sb = Superblock::parse(bytes, sig).unwrap(); + for (name, readable) in [ + ("Dataset1", true), + ("Dataset2", true), + ("Dataset3", false), + ("Dataset4", true), + ("Dataset5", true), + ] { + let addr = resolve_path_any(bytes, &sb, name).unwrap(); + let parsed = ObjectHeader::parse(bytes, addr as usize, sb.offset_size, sb.length_size); + match parsed { + Ok(_) => assert!(readable, "{name} must be refused"), + Err(e) => { + assert!(!readable, "{name} must be readable, got {e:?}"); + assert!(matches!( + e, + clawhdf5_format::error::FormatError::UnsupportedMessage(_) + )); + } + } + } +}