diff --git a/crates/clawhdf5-format/src/file_writer.rs b/crates/clawhdf5-format/src/file_writer.rs index c262cd4..d5260c2 100644 --- a/crates/clawhdf5-format/src/file_writer.rs +++ b/crates/clawhdf5-format/src/file_writer.rs @@ -33,6 +33,11 @@ pub(crate) const OFFSET_SIZE: u8 = 8; pub(crate) const LENGTH_SIZE: u8 = 8; const SUPERBLOCK_SIZE: usize = 48; +/// Largest raw data a compact dataset can hold: the layout message (version, +/// class, 2-byte size, data) must fit an object header message, whose size +/// field is 2 bytes. Bigger "compact" requests fall back to contiguous storage. +const MAX_COMPACT_DATA_SIZE: usize = crate::object_header_writer::MAX_MESSAGE_SIZE - 4; + /// Threshold for switching from compact (inline) to dense attribute storage. const DENSE_ATTR_THRESHOLD: usize = 8; @@ -51,7 +56,7 @@ pub(crate) fn build_chunked_dataset_oh( attrs: &[AttributeMessage], dense_blob: Option<&DenseAttrBlob>, fill_time: FillTime, -) -> Vec { +) -> Result, FormatError> { let mut w = ObjectHeaderWriter::new(); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); @@ -78,7 +83,7 @@ pub(crate) fn build_dataset_oh( attrs: &[AttributeMessage], dense_blob: Option<&DenseAttrBlob>, fill_time: FillTime, -) -> Vec { +) -> Result, FormatError> { let mut w = ObjectHeaderWriter::new(); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); @@ -113,7 +118,7 @@ pub(crate) fn build_compact_dataset_oh( attrs: &[AttributeMessage], dense_blob: Option<&DenseAttrBlob>, fill_time: FillTime, -) -> Vec { +) -> Result, FormatError> { let mut w = ObjectHeaderWriter::new(); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); @@ -140,7 +145,7 @@ pub(crate) fn build_group_oh( dense_link_info: Option<&[u8]>, attrs: &[AttributeMessage], dense_blob: Option<&DenseAttrBlob>, -) -> Vec { +) -> Result, FormatError> { let mut w = ObjectHeaderWriter::new(); if let Some(li) = dense_link_info { // Dense link storage: a LinkInfo pointing at the fractal heap + name @@ -903,7 +908,7 @@ pub(crate) fn build_vds_dataset_oh( attrs: &[AttributeMessage], dense_blob: Option<&DenseAttrBlob>, fill_time: FillTime, -) -> Vec { +) -> Result, FormatError> { let mut w = ObjectHeaderWriter::new(); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); @@ -1130,7 +1135,9 @@ impl FileWriter { let is_compact: Vec = all_ds .iter() .enumerate() - .map(|(i, d)| !is_vds[i] && !is_chunked[i] && d.compact && d.raw.len() <= 65535) + .map(|(i, d)| { + !is_vds[i] && !is_chunked[i] && d.compact && d.raw.len() <= MAX_COMPACT_DATA_SIZE + }) .collect(); let root_dense = root_attrs.len() > DENSE_ATTR_THRESHOLD; let group_dense: Vec = groups @@ -1169,9 +1176,9 @@ impl FileWriter { } let attr_blob = group_dense[gi].then(|| build_dense_attrs(&g.attrs, 0)); let dl = group_links_dense[gi].then_some(dummy_link_info.as_slice()); - build_group_oh(&dummy_links, dl, &g.attrs, attr_blob.as_ref()).len() + build_group_oh(&dummy_links, dl, &g.attrs, attr_blob.as_ref()).map(|oh| oh.len()) }) - .collect(); + .collect::>()?; let root_dummy_links: Vec = { let mut links = Vec::new(); @@ -1186,7 +1193,7 @@ impl FileWriter { let root_oh_size = { let attr_blob = root_dense.then(|| build_dense_attrs(&root_attrs, 0)); let dl = root_links_dense.then_some(dummy_link_info.as_slice()); - build_group_oh(&root_dummy_links, dl, &root_attrs, attr_blob.as_ref()).len() + build_group_oh(&root_dummy_links, dl, &root_attrs, attr_blob.as_ref())?.len() }; struct DataBlob { @@ -1215,7 +1222,7 @@ impl FileWriter { &d.attrs, dense_blob.as_ref(), d.fill_time, - ); + )?; // Global heap blob size is address-independent; compute it now // so pass 2 can place it correctly. let vds_mappings = d.virtual_sources.as_deref().unwrap_or(&[]); @@ -1259,7 +1266,7 @@ impl FileWriter { &d.attrs, dense_blob.as_ref(), d.fill_time, - ); + )?; dummy_blobs.push(DataBlob { data: result.data_bytes, oh_bytes: oh, @@ -1278,7 +1285,7 @@ impl FileWriter { &d.attrs, dense_blob.as_ref(), d.fill_time, - ); + )?; dummy_blobs.push(DataBlob { data: vec![], oh_bytes: oh, @@ -1298,7 +1305,7 @@ impl FileWriter { &d.attrs, dense_blob.as_ref(), d.fill_time, - ); + )?; dummy_blobs.push(DataBlob { data: d.raw.clone(), oh_bytes: oh, @@ -1407,7 +1414,7 @@ impl FileWriter { &d.attrs, ds_dense_blobs[i].as_ref(), d.fill_time, - ); + )?; ds_blobs2.push(DataBlob { data: gcol_bytes.clone(), oh_bytes: oh, @@ -1434,7 +1441,7 @@ impl FileWriter { &d.attrs, ds_dense_blobs[i].as_ref(), d.fill_time, - ); + )?; ds_blobs2.push(DataBlob { data: result.data_bytes, oh_bytes: oh, @@ -1449,7 +1456,7 @@ impl FileWriter { &d.attrs, ds_dense_blobs[i].as_ref(), d.fill_time, - ); + )?; ds_blobs2.push(DataBlob { data: vec![], oh_bytes: oh, @@ -1474,7 +1481,7 @@ impl FileWriter { &d.attrs, ds_dense_blobs[i].as_ref(), d.fill_time, - ); + )?; let mut data = vec![0u8; padding]; data.extend_from_slice(&d.raw); cursor2 += d.raw.len(); @@ -1530,7 +1537,7 @@ impl FileWriter { root_dl, &root_attrs, root_dense_blob.as_ref(), - )); + )?); if let Some(ref b) = root_link_blob { buf.extend_from_slice(&b.blob); } @@ -1555,7 +1562,7 @@ impl FileWriter { dl, &g.attrs, group_dense_blobs[gi].as_ref(), - )); + )?); if let Some(ref b) = link_blob { buf.extend_from_slice(&b.blob); } diff --git a/crates/clawhdf5-format/src/object_header_writer.rs b/crates/clawhdf5-format/src/object_header_writer.rs index 9142452..8d52ad4 100644 --- a/crates/clawhdf5-format/src/object_header_writer.rs +++ b/crates/clawhdf5-format/src/object_header_writer.rs @@ -1,11 +1,17 @@ //! Object header writer for v2 format. #[cfg(not(feature = "std"))] -use alloc::vec::Vec; +use alloc::{format, vec::Vec}; use crate::checksum::jenkins_lookup3; +use crate::error::FormatError; use crate::message_type::MessageType; +/// Largest message payload a v2 object header can describe: the per-message +/// size field is 2 bytes. A bigger message cannot be encoded at all — writing +/// its size truncated to 16 bits produced files libhdf5 refuses. +pub const MAX_MESSAGE_SIZE: usize = u16::MAX as usize; + /// Writer for v2 object headers with proper checksums. pub struct ObjectHeaderWriter { messages: Vec<(MessageType, Vec, u8)>, // (type, data, msg_flags) @@ -30,7 +36,22 @@ impl ObjectHeaderWriter { } /// Serialize the complete v2 object header (OHDR + messages + checksum). - pub fn serialize(&self) -> Vec { + /// + /// Fails with [`FormatError::SerializationError`] when a message is larger + /// than [`MAX_MESSAGE_SIZE`] (e.g. an attribute over ~64 KiB, which would + /// need dense attribute storage), rather than writing a corrupt header. + pub fn serialize(&self) -> Result, FormatError> { + if let Some((msg_type, data, _)) = self + .messages + .iter() + .find(|(_, data, _)| data.len() > MAX_MESSAGE_SIZE) + { + return Err(FormatError::SerializationError(format!( + "{msg_type:?} message is {} bytes; an object header message holds at most \ + {MAX_MESSAGE_SIZE} bytes", + data.len() + ))); + } // Calculate total message bytes: each message has type(1) + size(2) + flags(1) + data let msg_bytes_total: usize = self .messages @@ -80,7 +101,7 @@ impl ObjectHeaderWriter { let checksum = jenkins_lookup3(&buf); buf.extend_from_slice(&checksum.to_le_bytes()); - buf + Ok(buf) } } @@ -125,15 +146,22 @@ impl BatchObjectHeaderWriter { /// Compute the serialized size of each header without actually serializing. /// Returns sizes in the same order as headers were added. - pub fn compute_sizes(&self) -> Vec { - self.headers.iter().map(|h| h.serialize().len()).collect() + pub fn compute_sizes(&self) -> Result, FormatError> { + self.headers + .iter() + .map(|h| h.serialize().map(|b| b.len())) + .collect() } /// Serialize all headers into a single contiguous buffer. /// Returns `(combined_bytes, offsets)` where `offsets[i]` is the byte /// offset of header `i` within the combined buffer. - pub fn serialize_all(&self) -> (Vec, Vec) { - let serialized: Vec> = self.headers.iter().map(|h| h.serialize()).collect(); + pub fn serialize_all(&self) -> Result<(Vec, Vec), FormatError> { + let serialized: Vec> = self + .headers + .iter() + .map(|h| h.serialize()) + .collect::>()?; let total: usize = serialized.iter().map(|s| s.len()).sum(); let mut buf = Vec::with_capacity(total); let mut offsets = Vec::with_capacity(serialized.len()); @@ -141,7 +169,7 @@ impl BatchObjectHeaderWriter { offsets.push(buf.len()); buf.extend_from_slice(s); } - (buf, offsets) + Ok((buf, offsets)) } } @@ -159,7 +187,7 @@ mod tests { #[test] fn empty_header_roundtrip() { let writer = ObjectHeaderWriter::new(); - let bytes = writer.serialize(); + let bytes = writer.serialize().unwrap(); let hdr = ObjectHeader::parse(&bytes, 0, 8, 8).unwrap(); assert_eq!(hdr.version, 2); assert_eq!(hdr.messages.len(), 0); @@ -170,7 +198,7 @@ mod tests { let mut writer = ObjectHeaderWriter::new(); writer.add_message(MessageType::Dataspace, vec![1, 2, 3, 4]); writer.add_message(MessageType::Datatype, vec![5, 6]); - let bytes = writer.serialize(); + let bytes = writer.serialize().unwrap(); let hdr = ObjectHeader::parse(&bytes, 0, 8, 8).unwrap(); assert_eq!(hdr.messages.len(), 2); assert_eq!(hdr.messages[0].msg_type, MessageType::Dataspace); @@ -184,12 +212,30 @@ mod tests { let mut writer = ObjectHeaderWriter::new(); // Add a message with >255 bytes of payload writer.add_message(MessageType::Datatype, vec![0xAA; 300]); - let bytes = writer.serialize(); + let bytes = writer.serialize().unwrap(); let hdr = ObjectHeader::parse(&bytes, 0, 8, 8).unwrap(); assert_eq!(hdr.messages.len(), 1); assert_eq!(hdr.messages[0].data.len(), 300); } + #[test] + fn oversized_message_is_an_error_not_a_truncated_size() { + // 65535 bytes is the largest encodable payload. + let mut writer = ObjectHeaderWriter::new(); + writer.add_message(MessageType::Attribute, vec![0; MAX_MESSAGE_SIZE]); + let bytes = writer.serialize().unwrap(); + let hdr = ObjectHeader::parse(&bytes, 0, 8, 8).unwrap(); + assert_eq!(hdr.messages[0].data.len(), MAX_MESSAGE_SIZE); + + // One byte more used to be written with its size wrapped to 0. + let mut writer = ObjectHeaderWriter::new(); + writer.add_message(MessageType::Attribute, vec![0; MAX_MESSAGE_SIZE + 1]); + assert!(matches!( + writer.serialize(), + Err(FormatError::SerializationError(_)) + )); + } + #[test] fn batch_writer_serialize_all() { let mut batch = BatchObjectHeaderWriter::new(); @@ -204,7 +250,7 @@ mod tests { batch.add(w2); assert_eq!(batch.len(), 2); - let (buf, offsets) = batch.serialize_all(); + let (buf, offsets) = batch.serialize_all().unwrap(); assert_eq!(offsets.len(), 2); assert_eq!(offsets[0], 0); @@ -222,7 +268,7 @@ mod tests { fn batch_writer_empty() { let batch = BatchObjectHeaderWriter::new(); assert!(batch.is_empty()); - let (buf, offsets) = batch.serialize_all(); + let (buf, offsets) = batch.serialize_all().unwrap(); assert!(buf.is_empty()); assert!(offsets.is_empty()); } diff --git a/crates/clawhdf5-format/src/type_builders.rs b/crates/clawhdf5-format/src/type_builders.rs index e698899..9ee2520 100644 --- a/crates/clawhdf5-format/src/type_builders.rs +++ b/crates/clawhdf5-format/src/type_builders.rs @@ -718,7 +718,7 @@ impl DatasetBuilder { /// Use compact (inline) storage for this dataset. /// /// The raw data is stored directly in the dataset's object header rather - /// than as a separate data blob. Only effective when raw data <= 65536 bytes + /// than as a separate data blob. Only effective when raw data <= 65531 bytes /// and the dataset is not chunked. pub fn compact(&mut self) -> &mut Self { self.compact = true; diff --git a/crates/clawhdf5-format/tests/writer_meta_tests.rs b/crates/clawhdf5-format/tests/writer_meta_tests.rs new file mode 100644 index 0000000..1175ab4 --- /dev/null +++ b/crates/clawhdf5-format/tests/writer_meta_tests.rs @@ -0,0 +1,153 @@ +//! Regression tests for writer metadata bugs that produced files libhdf5 +//! refuses (or reads differently from us), plus the reader-side counterparts. +//! +//! The plain tests check the bytes we write with our own parser. The +//! `#[ignore]`d ones are the interop half: they open what we write in h5py +//! (`CLAWHDF5_PYTHON`, as in `writer_h5py_tests.rs`) and run `h5dump` over it. + +use clawhdf5_format::data_layout::DataLayout; +use clawhdf5_format::file_writer::{AttrValue, FileWriter}; +use clawhdf5_format::group_v2::resolve_path_any; +use clawhdf5_format::message_type::MessageType; +use clawhdf5_format::object_header::ObjectHeader; +use clawhdf5_format::signature; +use clawhdf5_format::superblock::Superblock; +use clawhdf5_format::type_builders::make_u8_type; + +// ---- helpers ---- + +fn header_at(bytes: &[u8], path: &str) -> (Superblock, ObjectHeader) { + let sig = signature::find_signature(bytes).unwrap(); + let sb = Superblock::parse(bytes, sig).unwrap(); + let addr = if path == "/" { + sb.root_group_address + } else { + resolve_path_any(bytes, &sb, path).unwrap() + }; + let oh = ObjectHeader::parse(bytes, addr as usize, sb.offset_size, sb.length_size).unwrap(); + (sb, oh) +} + +fn layout_of(bytes: &[u8], path: &str) -> DataLayout { + let (sb, oh) = header_at(bytes, path); + let msg = oh + .messages + .iter() + .find(|m| m.msg_type == MessageType::DataLayout) + .unwrap(); + DataLayout::parse(&msg.data, sb.offset_size, sb.length_size).unwrap() +} + +fn python() -> String { + std::env::var("CLAWHDF5_PYTHON").unwrap_or_else(|_| "python3".to_string()) +} + +fn write_tmp(name: &str, bytes: &[u8]) -> std::path::PathBuf { + let path = std::env::temp_dir().join(format!("clawhdf5_writer_meta_{name}.h5")); + std::fs::write(&path, bytes).unwrap(); + path +} + +/// Run `script` (with `path` bound to the file) under h5py; return stdout. +fn h5py(path: &std::path::Path, script: &str) -> String { + let full = format!( + "import h5py, numpy as np, json\npath = {:?}\n{script}", + path.display().to_string() + ); + let o = std::process::Command::new(python()) + .args(["-c", &full]) + .output() + .expect("python interpreter"); + assert!( + o.status.success(), + "h5py failed: {}", + String::from_utf8_lossy(&o.stderr) + ); + String::from_utf8(o.stdout).unwrap().trim().to_string() +} + +/// `h5dump` must read the whole file without error. +fn h5dump_ok(path: &std::path::Path) { + let o = std::process::Command::new("h5dump") + .arg(path) + .output() + .expect("h5dump"); + assert!( + o.status.success(), + "h5dump failed: {}{}", + String::from_utf8_lossy(&o.stdout), + String::from_utf8_lossy(&o.stderr) + ); +} + +fn u8_ramp(n: usize) -> Vec { + (0..n).map(|i| (i % 251) as u8).collect() +} + +// ---- 1. object header message size limit ---- + +#[test] +fn attribute_too_big_for_a_header_message_is_an_error() { + // Measured: a 70000-byte attribute was written with its message size + // wrapped to 16 bits, and libhdf5 refused the whole root group. + let mut fw = FileWriter::new(); + fw.set_root_attr( + "a", + AttrValue::Raw { + datatype: make_u8_type(), + shape: vec![70_000], + data: u8_ramp(70_000), + }, + ); + assert!(fw.finish().is_err()); + + // 65500 bytes still fits and still works. + let mut fw = FileWriter::new(); + fw.set_root_attr( + "a", + AttrValue::Raw { + datatype: make_u8_type(), + shape: vec![65_500], + data: u8_ramp(65_500), + }, + ); + let bytes = fw.finish().unwrap(); + let (sb, oh) = header_at(&bytes, "/"); + let attrs = clawhdf5_format::attribute::extract_attributes(&oh, sb.length_size).unwrap(); + assert_eq!(attrs[0].raw_data, u8_ramp(65_500)); +} + +#[test] +fn compact_layout_falls_back_to_contiguous_past_the_message_limit() { + // Layout message = 4 bytes + data; data may be at most 65531 bytes. + for (n, compact) in [(65_531, true), (65_532, false), (65_534, false)] { + let mut fw = FileWriter::new(); + fw.create_dataset("d").with_u8_data(&u8_ramp(n)).compact(); + let bytes = fw.finish().unwrap(); + match layout_of(&bytes, "d") { + DataLayout::Compact { data } => { + assert!(compact, "{n} bytes must not be compact"); + assert_eq!(data, u8_ramp(n)); + } + DataLayout::Contiguous { .. } => assert!(!compact, "{n} bytes should be compact"), + other => panic!("unexpected layout {other:?}"), + } + } +} + +#[test] +#[ignore = "requires Python h5py module and h5dump"] +fn h5py_reads_compact_datasets_at_the_limit() { + for n in [65_531usize, 65_534] { + let mut fw = FileWriter::new(); + fw.create_dataset("d").with_u8_data(&u8_ramp(n)).compact(); + let path = write_tmp(&format!("compact_{n}"), &fw.finish().unwrap()); + let out = h5py( + &path, + "f = h5py.File(path, 'r'); v = f['d'][()]\n\ + print(bool((v == (np.arange(v.size) % 251).astype(np.uint8)).all()), v.size)", + ); + assert_eq!(out, format!("True {n}")); + h5dump_ok(&path); + } +}