Fix silent wrong data and libhdf5 interop found by the HDF5 audit #11

Merged
osobh merged 41 commits from fix/phase0-correctness into main 2026-09-26 02:42:54 +00:00
4 changed files with 193 additions and 35 deletions
Showing only changes of commit 8c3ef996ea - Show all commits
+21 -19
View File
@@ -19,7 +19,7 @@ use crate::metadata_index::{DatasetMetadata, MetadataBlock, MetadataIndex};
use crate::object_header_writer::ObjectHeaderWriter; use crate::object_header_writer::ObjectHeaderWriter;
use crate::superblock::Superblock; use crate::superblock::Superblock;
use crate::type_builders::{ use crate::type_builders::{
DatasetBuilder, FillTime, FinishedGroup, GroupBuilder, build_attr_message, DatasetBuilder, FinishedGroup, GroupBuilder, build_attr_message, fill_value_message,
}; };
// Re-export public types that moved to type_builders for API compatibility. // Re-export public types that moved to type_builders for API compatibility.
@@ -80,12 +80,12 @@ pub(crate) fn build_chunked_dataset_oh(
pipeline_message: Option<&[u8]>, pipeline_message: Option<&[u8]>,
attrs: &[AttributeMessage], attrs: &[AttributeMessage],
dense_blob: Option<&DenseAttrBlob>, dense_blob: Option<&DenseAttrBlob>,
fill_time: FillTime, fill_message: &[u8],
) -> Result<Vec<u8>, FormatError> { ) -> Result<Vec<u8>, FormatError> {
let mut w = ObjectHeaderWriter::new(); let mut w = ObjectHeaderWriter::new();
w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01);
w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE));
w.add_message_with_flags(MessageType::FillValue, vec![3, fill_time.to_byte()], 0x01); w.add_message_with_flags(MessageType::FillValue, fill_message.to_vec(), 0x01);
w.add_message(MessageType::DataLayout, layout_message.to_vec()); w.add_message(MessageType::DataLayout, layout_message.to_vec());
if let Some(pm) = pipeline_message { if let Some(pm) = pipeline_message {
w.add_message(MessageType::FilterPipeline, pm.to_vec()); w.add_message(MessageType::FilterPipeline, pm.to_vec());
@@ -107,12 +107,12 @@ pub(crate) fn build_dataset_oh(
data_size: u64, data_size: u64,
attrs: &[AttributeMessage], attrs: &[AttributeMessage],
dense_blob: Option<&DenseAttrBlob>, dense_blob: Option<&DenseAttrBlob>,
fill_time: FillTime, fill_message: &[u8],
) -> Result<Vec<u8>, FormatError> { ) -> Result<Vec<u8>, FormatError> {
let mut w = ObjectHeaderWriter::new(); let mut w = ObjectHeaderWriter::new();
w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01);
w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE));
w.add_message_with_flags(MessageType::FillValue, vec![3, fill_time.to_byte()], 0x01); w.add_message_with_flags(MessageType::FillValue, fill_message.to_vec(), 0x01);
let mut dl = Vec::new(); let mut dl = Vec::new();
dl.push(4); // version dl.push(4); // version
dl.push(1); // class = contiguous dl.push(1); // class = contiguous
@@ -142,12 +142,12 @@ pub(crate) fn build_compact_dataset_oh(
data: &[u8], data: &[u8],
attrs: &[AttributeMessage], attrs: &[AttributeMessage],
dense_blob: Option<&DenseAttrBlob>, dense_blob: Option<&DenseAttrBlob>,
fill_time: FillTime, fill_message: &[u8],
) -> Result<Vec<u8>, FormatError> { ) -> Result<Vec<u8>, FormatError> {
let mut w = ObjectHeaderWriter::new(); let mut w = ObjectHeaderWriter::new();
w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01);
w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE));
w.add_message_with_flags(MessageType::FillValue, vec![3, fill_time.to_byte()], 0x01); w.add_message_with_flags(MessageType::FillValue, fill_message.to_vec(), 0x01);
// Compact layout message: version=4, class=0, u16 size, inline data // Compact layout message: version=4, class=0, u16 size, inline data
let mut dl = Vec::new(); let mut dl = Vec::new();
dl.push(4); // version dl.push(4); // version
@@ -932,12 +932,12 @@ pub(crate) fn build_vds_dataset_oh(
global_heap_addr: u64, global_heap_addr: u64,
attrs: &[AttributeMessage], attrs: &[AttributeMessage],
dense_blob: Option<&DenseAttrBlob>, dense_blob: Option<&DenseAttrBlob>,
fill_time: FillTime, fill_message: &[u8],
) -> Result<Vec<u8>, FormatError> { ) -> Result<Vec<u8>, FormatError> {
let mut w = ObjectHeaderWriter::new(); let mut w = ObjectHeaderWriter::new();
w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01);
w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE));
w.add_message_with_flags(MessageType::FillValue, vec![3, fill_time.to_byte()], 0x01); w.add_message_with_flags(MessageType::FillValue, fill_message.to_vec(), 0x01);
// VDS layout message: version=4, class=3, global_heap_address(8), global_heap_index=1(4) // VDS layout message: version=4, class=3, global_heap_address(8), global_heap_index=1(4)
let mut dl = Vec::new(); let mut dl = Vec::new();
dl.push(4u8); // version dl.push(4u8); // version
@@ -1070,7 +1070,8 @@ impl FileWriter {
attrs: Vec<AttributeMessage>, attrs: Vec<AttributeMessage>,
chunk_options: ChunkOptions, chunk_options: ChunkOptions,
maxshape: Option<Vec<u64>>, maxshape: Option<Vec<u64>>,
fill_time: FillTime, /// Serialized Fill Value message.
fill_message: Vec<u8>,
compact: bool, compact: bool,
alignment: usize, alignment: usize,
/// VDS source mappings (set for Virtual datasets). /// VDS source mappings (set for Virtual datasets).
@@ -1120,6 +1121,7 @@ impl FileWriter {
}; };
attrs.extend(p.build_attrs(&raw)); attrs.extend(p.build_attrs(&raw));
} }
let fill_message = fill_value_message(db.fill_time, db.fill_value.as_deref(), &dt)?;
Ok(DsFlat { Ok(DsFlat {
name: db.name, name: db.name,
dt, dt,
@@ -1128,7 +1130,7 @@ impl FileWriter {
attrs, attrs,
chunk_options: db.chunk_options, chunk_options: db.chunk_options,
maxshape: db.maxshape, maxshape: db.maxshape,
fill_time: db.fill_time, fill_message,
compact: db.compact, compact: db.compact,
alignment: db.alignment, alignment: db.alignment,
virtual_sources: db.virtual_sources, virtual_sources: db.virtual_sources,
@@ -1274,7 +1276,7 @@ impl FileWriter {
0, // dummy address 0, // dummy address
&d.attrs, &d.attrs,
dense_blob.as_ref(), dense_blob.as_ref(),
d.fill_time, &d.fill_message,
)?; )?;
// Global heap blob size is address-independent; compute it now // Global heap blob size is address-independent; compute it now
// so pass 2 can place it correctly. // so pass 2 can place it correctly.
@@ -1318,7 +1320,7 @@ impl FileWriter {
result.pipeline_message.as_deref(), result.pipeline_message.as_deref(),
&d.attrs, &d.attrs,
dense_blob.as_ref(), dense_blob.as_ref(),
d.fill_time, &d.fill_message,
)?; )?;
dummy_blobs.push(DataBlob { dummy_blobs.push(DataBlob {
data: result.data_bytes, data: result.data_bytes,
@@ -1337,7 +1339,7 @@ impl FileWriter {
&d.raw, &d.raw,
&d.attrs, &d.attrs,
dense_blob.as_ref(), dense_blob.as_ref(),
d.fill_time, &d.fill_message,
)?; )?;
dummy_blobs.push(DataBlob { dummy_blobs.push(DataBlob {
data: vec![], data: vec![],
@@ -1357,7 +1359,7 @@ impl FileWriter {
d.raw.len() as u64, d.raw.len() as u64,
&d.attrs, &d.attrs,
dense_blob.as_ref(), dense_blob.as_ref(),
d.fill_time, &d.fill_message,
)?; )?;
dummy_blobs.push(DataBlob { dummy_blobs.push(DataBlob {
data: d.raw.clone(), data: d.raw.clone(),
@@ -1466,7 +1468,7 @@ impl FileWriter {
heap_addr, heap_addr,
&d.attrs, &d.attrs,
ds_dense_blobs[i].as_ref(), ds_dense_blobs[i].as_ref(),
d.fill_time, &d.fill_message,
)?; )?;
ds_blobs2.push(DataBlob { ds_blobs2.push(DataBlob {
data: gcol_bytes.clone(), data: gcol_bytes.clone(),
@@ -1493,7 +1495,7 @@ impl FileWriter {
result.pipeline_message.as_deref(), result.pipeline_message.as_deref(),
&d.attrs, &d.attrs,
ds_dense_blobs[i].as_ref(), ds_dense_blobs[i].as_ref(),
d.fill_time, &d.fill_message,
)?; )?;
ds_blobs2.push(DataBlob { ds_blobs2.push(DataBlob {
data: result.data_bytes, data: result.data_bytes,
@@ -1508,7 +1510,7 @@ impl FileWriter {
&d.raw, &d.raw,
&d.attrs, &d.attrs,
ds_dense_blobs[i].as_ref(), ds_dense_blobs[i].as_ref(),
d.fill_time, &d.fill_message,
)?; )?;
ds_blobs2.push(DataBlob { ds_blobs2.push(DataBlob {
data: vec![], data: vec![],
@@ -1533,7 +1535,7 @@ impl FileWriter {
d.raw.len() as u64, d.raw.len() as u64,
&d.attrs, &d.attrs,
ds_dense_blobs[i].as_ref(), ds_dense_blobs[i].as_ref(),
d.fill_time, &d.fill_message,
)?; )?;
let mut data = vec![0u8; padding]; let mut data = vec![0u8; padding];
data.extend_from_slice(&d.raw); data.extend_from_slice(&d.raw);
+2 -2
View File
@@ -43,7 +43,7 @@ impl Default for DatasetCreateProps {
fletcher32: false, fletcher32: false,
lz4: false, lz4: false,
zstd_level: None, zstd_level: None,
fill_time: FillTime::Alloc, fill_time: FillTime::IfSet,
compact: false, compact: false,
alignment: 0, alignment: 0,
} }
@@ -335,7 +335,7 @@ mod tests {
fn dcpl_defaults() { fn dcpl_defaults() {
let dcpl = DatasetCreateProps::new(); let dcpl = DatasetCreateProps::new();
assert!(dcpl.chunk_dims.is_none()); assert!(dcpl.chunk_dims.is_none());
assert_eq!(dcpl.fill_time, FillTime::Alloc); assert_eq!(dcpl.fill_time, FillTime::IfSet);
assert!(!dcpl.compact); assert!(!dcpl.compact);
} }
+78 -13
View File
@@ -15,29 +15,81 @@ use crate::datatype::{
/// Controls when fill values are written to dataset storage. /// Controls when fill values are written to dataset storage.
/// ///
/// Corresponds to the HDF5 fill value message's "fill time" field. /// Corresponds to the HDF5 fill value message's "fill time" field
/// (`H5D_fill_time_t`).
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
pub enum FillTime { pub enum FillTime {
/// Never write fill values (0x02). Avoids initialization overhead /// Never write fill values (`H5D_FILL_TIME_NEVER`). Avoids
/// for datasets that will be fully written before any read. /// initialization overhead for datasets that will be fully written
/// before any read.
Never, Never,
/// Write fill values at allocation time (0x0a). This is the default /// Write fill values when storage is allocated (`H5D_FILL_TIME_ALLOC`).
/// and matches the HDF5 C library's behavior.
#[default]
Alloc, Alloc,
/// Write fill values only when the fill value has been explicitly set (0x06). /// Write fill values at allocation only if one was set explicitly
/// (`H5D_FILL_TIME_IFSET`). The default, as in the HDF5 C library.
#[default]
IfSet, IfSet,
} }
/// Space allocation time written with every fill value message: late
/// (`H5D_ALLOC_TIME_LATE`), bits 0-1 of the flags byte.
const ALLOC_TIME_LATE: u8 = 2;
impl FillTime { impl FillTime {
/// Serialize to the byte used in the fill value message (version 3). /// Serialize to the flags byte of a version 3 fill value message: the
/// space allocation time (late) in bits 0-1 and the fill time in bits
/// 2-3 (`H5D_FILL_TIME_ALLOC` = 0, `NEVER` = 1, `IFSET` = 2).
///
/// This used to put `Never` in the ALLOC slot, `Alloc` in IFSET and
/// `IfSet` in NEVER, so libhdf5 saw every choice as a different one.
pub fn to_byte(self) -> u8 { pub fn to_byte(self) -> u8 {
match self { ALLOC_TIME_LATE | (self.code() << 2)
FillTime::Never => 0x02, }
FillTime::Alloc => 0x0a,
FillTime::IfSet => 0x06, /// Decode the fill time from a version 3 fill value message's flags.
pub fn from_byte(flags: u8) -> Option<FillTime> {
match (flags >> 2) & 0x03 {
0 => Some(FillTime::Alloc),
1 => Some(FillTime::Never),
2 => Some(FillTime::IfSet),
_ => None,
} }
} }
fn code(self) -> u8 {
match self {
FillTime::Alloc => 0,
FillTime::Never => 1,
FillTime::IfSet => 2,
}
}
}
/// Serialize a version 3 Fill Value message for a dataset of `dt`: the fill
/// time, and the user-defined fill value if there is one (bit 5).
pub(crate) fn fill_value_message(
fill_time: FillTime,
value: Option<&[u8]>,
dt: &Datatype,
) -> Result<Vec<u8>, crate::error::FormatError> {
let mut msg = vec![3, fill_time.to_byte()];
if let Some(value) = value {
if matches!(dt, Datatype::VariableLength { .. }) {
return Err(crate::error::FormatError::SerializationError(
"a fill value for a variable-length datatype is not supported".into(),
));
}
if value.len() != dt.type_size() as usize {
return Err(crate::error::FormatError::DataSizeMismatch {
expected: dt.type_size() as usize,
actual: value.len(),
});
}
msg[1] |= 0x20; // fill value defined
msg.extend_from_slice(&(value.len() as u32).to_le_bytes());
msg.extend_from_slice(value);
}
Ok(msg)
} }
// ---- Datatype constructors ---- // ---- Datatype constructors ----
@@ -431,8 +483,10 @@ pub struct DatasetBuilder {
pub(crate) data: Option<Vec<u8>>, pub(crate) data: Option<Vec<u8>>,
pub(crate) attrs: Vec<(String, AttrValue)>, pub(crate) attrs: Vec<(String, AttrValue)>,
pub(crate) chunk_options: ChunkOptions, pub(crate) chunk_options: ChunkOptions,
/// Controls when fill values are written. Default is `FillTime::Alloc`. /// Controls when fill values are written. Default is `FillTime::IfSet`.
pub(crate) fill_time: FillTime, pub(crate) fill_time: FillTime,
/// User-defined fill value: one element's bytes, as stored.
pub(crate) fill_value: Option<Vec<u8>>,
/// Use compact (inline) storage: data is stored in the object header. /// Use compact (inline) storage: data is stored in the object header.
/// Only valid when raw data is <= 65536 bytes and dataset is not chunked. /// Only valid when raw data is <= 65536 bytes and dataset is not chunked.
pub(crate) compact: bool, pub(crate) compact: bool,
@@ -459,6 +513,7 @@ impl DatasetBuilder {
attrs: Vec::new(), attrs: Vec::new(),
chunk_options: ChunkOptions::default(), chunk_options: ChunkOptions::default(),
fill_time: FillTime::default(), fill_time: FillTime::default(),
fill_value: None,
compact: false, compact: false,
alignment: 0, alignment: 0,
virtual_sources: None, virtual_sources: None,
@@ -715,6 +770,16 @@ impl DatasetBuilder {
self self
} }
/// Set the dataset's fill value: what readers return for storage that
/// was never written (e.g. after the dataset is extended). `value` is one
/// element's bytes as stored — the dataset datatype's size and byte order
/// (`(-1i32).to_le_bytes()` for an `i32` dataset). A size mismatch, or a
/// variable-length datatype, makes `finish` fail.
pub fn with_fill_value(&mut self, value: &[u8]) -> &mut Self {
self.fill_value = Some(value.to_vec());
self
}
/// Use compact (inline) storage for this dataset. /// Use compact (inline) storage for this dataset.
/// ///
/// The raw data is stored directly in the dataset's object header rather /// The raw data is stored directly in the dataset's object header rather
@@ -13,7 +13,7 @@ use clawhdf5_format::message_type::MessageType;
use clawhdf5_format::object_header::ObjectHeader; use clawhdf5_format::object_header::ObjectHeader;
use clawhdf5_format::signature; use clawhdf5_format::signature;
use clawhdf5_format::superblock::Superblock; use clawhdf5_format::superblock::Superblock;
use clawhdf5_format::type_builders::make_u8_type; use clawhdf5_format::type_builders::{FillTime, make_u8_type};
// ---- helpers ---- // ---- helpers ----
@@ -373,3 +373,94 @@ fn h5py_opens_paged_files() {
h5dump_ok(&path); h5dump_ok(&path);
} }
} }
// ---- 4. fill time and fill value ----
fn fill_message(bytes: &[u8], path: &str) -> clawhdf5_format::object_header::HeaderMessage {
let (_, oh) = header_at(bytes, path);
oh.messages
.into_iter()
.find(|m| m.msg_type == MessageType::FillValue)
.unwrap()
}
fn fill_file() -> Vec<u8> {
let mut fw = FileWriter::new();
fw.create_dataset("never")
.with_f64_data(&[1.0, 2.0])
.fill_time(FillTime::Never);
fw.create_dataset("alloc")
.with_f64_data(&[1.0, 2.0])
.fill_time(FillTime::Alloc);
fw.create_dataset("ifset")
.with_f64_data(&[1.0, 2.0])
.fill_time(FillTime::IfSet);
fw.create_dataset("default").with_f64_data(&[1.0, 2.0]);
fw.create_dataset("filled")
.with_i32_data(&[1, 2, 3, 4])
.with_chunks(&[2])
.with_maxshape(&[u64::MAX])
.with_fill_value(&(-1i32).to_le_bytes());
fw.finish().unwrap()
}
#[test]
fn fill_time_uses_libhdf5_codes() {
// H5D_FILL_TIME_ALLOC = 0, NEVER = 1, IFSET = 2, in bits 2-3. Measured:
// h5py saw our Never as ALLOC, Alloc as IFSET and IfSet as NEVER.
let bytes = fill_file();
for (path, code) in [("never", 1), ("alloc", 0), ("ifset", 2), ("default", 2)] {
let msg = fill_message(&bytes, path);
assert_eq!((msg.data[1] >> 2) & 3, code, "{path}");
assert_eq!(msg.data[1] & 3, 2, "{path}: allocation time stays late");
}
for ft in [FillTime::Never, FillTime::Alloc, FillTime::IfSet] {
assert_eq!(FillTime::from_byte(ft.to_byte()), Some(ft));
}
assert_eq!(FillTime::default(), FillTime::IfSet);
}
#[test]
fn fill_value_is_written_and_read_back() {
let bytes = fill_file();
let msg = fill_message(&bytes, "filled");
assert_eq!(
clawhdf5_format::fill_value::parse_fill_value(&msg).unwrap(),
Some((-1i32).to_le_bytes().to_vec())
);
assert_eq!(
clawhdf5_format::fill_value::parse_fill_value(&fill_message(&bytes, "ifset")).unwrap(),
None
);
// One element's bytes, no more, no less.
let mut fw = FileWriter::new();
fw.create_dataset("d")
.with_f64_data(&[1.0])
.with_fill_value(&[0; 4]);
assert!(fw.finish().is_err());
}
#[test]
#[ignore = "requires Python h5py module and h5dump"]
fn h5py_sees_our_fill_time_and_fill_value() {
let path = write_tmp("fill", &fill_file());
let out = h5py(
&path,
"from h5py import h5d\n\
f = h5py.File(path, 'r')\n\
names = {h5d.FILL_TIME_NEVER: 'never', h5d.FILL_TIME_ALLOC: 'alloc', h5d.FILL_TIME_IFSET: 'ifset'}\n\
t = [names[f[n].id.get_create_plist().get_fill_time()] for n in ('never', 'alloc', 'ifset', 'default')]\n\
print(json.dumps([t, int(f['filled'].fillvalue), f['filled'][()].tolist()]))\n\
f.close()\n\
f = h5py.File(path, 'r+')\n\
f['filled'].resize((7,))\n\
f.close()\n\
print(json.dumps(h5py.File(path, 'r')['filled'][()].tolist()))",
);
assert_eq!(
out,
"[[\"never\", \"alloc\", \"ifset\", \"ifset\"], -1, [1, 2, 3, 4]]\n[1, 2, 3, 4, -1, -1, -1]"
);
h5dump_ok(&path);
}