Fix silent wrong data and libhdf5 interop found by the HDF5 audit #11
@@ -98,15 +98,50 @@ pub fn parse_fill_value(msg: &HeaderMessage) -> Result<Option<Vec<u8>>, FormatEr
|
|||||||
|
|
||||||
/// The fill value that applies to a dataset given its header messages. The new
|
/// The fill value that applies to a dataset given its header messages. The new
|
||||||
/// message wins over the old one when both are present.
|
/// message wins over the old one when both are present.
|
||||||
|
///
|
||||||
|
/// A *shared* fill value message holds only a reference to the real message,
|
||||||
|
/// which cannot be followed without the file: this returns
|
||||||
|
/// [`FormatError::UnresolvedSharedMessage`] for one (it used to answer "zeros").
|
||||||
|
/// Use [`dataset_fill_value_in`] when the file bytes are at hand.
|
||||||
pub fn dataset_fill_value(messages: &[HeaderMessage]) -> Result<Option<Vec<u8>>, FormatError> {
|
pub fn dataset_fill_value(messages: &[HeaderMessage]) -> Result<Option<Vec<u8>>, FormatError> {
|
||||||
|
fill_value_from(messages, |_| Err(FormatError::UnresolvedSharedMessage))
|
||||||
|
}
|
||||||
|
|
||||||
|
/// [`dataset_fill_value`] for a dataset in `file_data`, following a shared
|
||||||
|
/// fill value message to where it lives: another object header, or the
|
||||||
|
/// file's shared-message (SOHM) heap, as libhdf5 writes it when the file has
|
||||||
|
/// a SOHM index for fill values.
|
||||||
|
pub fn dataset_fill_value_in(
|
||||||
|
file_data: &[u8],
|
||||||
|
messages: &[HeaderMessage],
|
||||||
|
offset_size: u8,
|
||||||
|
length_size: u8,
|
||||||
|
) -> Result<Option<Vec<u8>>, FormatError> {
|
||||||
|
fill_value_from(messages, |msg| {
|
||||||
|
crate::shared_message::message_data_with_sohm(file_data, msg, offset_size, length_size)
|
||||||
|
.map(|data| data.into_owned())
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
fn fill_value_from(
|
||||||
|
messages: &[HeaderMessage],
|
||||||
|
resolve_shared: impl Fn(&HeaderMessage) -> Result<Vec<u8>, FormatError>,
|
||||||
|
) -> Result<Option<Vec<u8>>, FormatError> {
|
||||||
for wanted in [MessageType::FillValue, MessageType::FillValueOld] {
|
for wanted in [MessageType::FillValue, MessageType::FillValueOld] {
|
||||||
if let Some(msg) = messages.iter().find(|m| m.msg_type == wanted) {
|
if let Some(msg) = messages.iter().find(|m| m.msg_type == wanted) {
|
||||||
if crate::shared_message::is_shared(msg.flags) {
|
let value = if crate::shared_message::is_shared(msg.flags) {
|
||||||
// A shared fill value is legal but vanishingly rare; treat it
|
let data = resolve_shared(msg)?;
|
||||||
// as the default rather than misparsing the reference.
|
parse_fill_value(&HeaderMessage {
|
||||||
return Ok(None);
|
msg_type: msg.msg_type,
|
||||||
}
|
size: data.len(),
|
||||||
if let Some(value) = parse_fill_value(msg)? {
|
flags: msg.flags & !0x02,
|
||||||
|
creation_order: msg.creation_order,
|
||||||
|
data,
|
||||||
|
})?
|
||||||
|
} else {
|
||||||
|
parse_fill_value(msg)?
|
||||||
|
};
|
||||||
|
if let Some(value) = value {
|
||||||
return Ok(Some(value));
|
return Ok(Some(value));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -174,7 +209,7 @@ pub fn read_full_with_fill<E: From<FormatError>>(
|
|||||||
{
|
{
|
||||||
return Err(FormatError::ExternalDataFilesUnsupported.into());
|
return Err(FormatError::ExternalDataFilesUnsupported.into());
|
||||||
}
|
}
|
||||||
let fill = dataset_fill_value(messages)?;
|
let fill = dataset_fill_value_in(file_data, messages, offset_size, length_size)?;
|
||||||
if !has_storage(layout) {
|
if !has_storage(layout) {
|
||||||
return Ok(filled_dataset(dataspace, elem_size, fill.as_deref())?);
|
return Ok(filled_dataset(dataspace, elem_size, fill.as_deref())?);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -225,9 +225,12 @@ pub fn parse_sohm_table_message(
|
|||||||
|
|
||||||
/// Parse the SOHM table structure (signature "SMTB") from the file.
|
/// Parse the SOHM table structure (signature "SMTB") from the file.
|
||||||
///
|
///
|
||||||
/// Each index entry: index_type(1) + mesg_types(2) + min_mesg_size(4) +
|
/// Each index entry: version(1) + index_type(1) + mesg_types(2) +
|
||||||
/// list_max(2) + btree_min(2) + num_messages(2) + index_addr(offset_size) +
|
/// min_mesg_size(4) + list_max(2) + btree_min(2) + num_messages(2) +
|
||||||
/// heap_addr(offset_size)
|
/// index_addr(offset_size) + heap_addr(offset_size)
|
||||||
|
///
|
||||||
|
/// The leading per-index version byte (0) was missing here, so every field
|
||||||
|
/// after it was read one byte off — verified against an HDF5 2.0 file.
|
||||||
pub fn parse_sohm_table(
|
pub fn parse_sohm_table(
|
||||||
file_data: &[u8],
|
file_data: &[u8],
|
||||||
table_addr: usize,
|
table_addr: usize,
|
||||||
@@ -240,11 +243,16 @@ pub fn parse_sohm_table(
|
|||||||
}
|
}
|
||||||
let mut pos = table_addr + 4;
|
let mut pos = table_addr + 4;
|
||||||
let os = offset_size as usize;
|
let os = offset_size as usize;
|
||||||
let entry_size = 1 + 2 + 4 + 2 + 2 + 2 + os + os; // 13 + 2*offset_size
|
let entry_size = 1 + 1 + 2 + 4 + 2 + 2 + 2 + os + os; // 14 + 2*offset_size
|
||||||
|
|
||||||
let mut indexes = Vec::with_capacity(nindexes as usize);
|
let mut indexes = Vec::with_capacity(nindexes as usize);
|
||||||
for _ in 0..nindexes {
|
for _ in 0..nindexes {
|
||||||
ensure_len(file_data, pos, entry_size)?;
|
ensure_len(file_data, pos, entry_size)?;
|
||||||
|
let version = file_data[pos];
|
||||||
|
if version != 0 {
|
||||||
|
return Err(FormatError::InvalidSohmTableVersion(version));
|
||||||
|
}
|
||||||
|
pos += 1;
|
||||||
let index_type = file_data[pos];
|
let index_type = file_data[pos];
|
||||||
pos += 1;
|
pos += 1;
|
||||||
let mesg_types = u16::from_le_bytes([file_data[pos], file_data[pos + 1]]);
|
let mesg_types = u16::from_le_bytes([file_data[pos], file_data[pos + 1]]);
|
||||||
@@ -381,6 +389,68 @@ pub fn parse_sohm_btree_entries(
|
|||||||
// ---- SOHM resolution ----
|
// ---- SOHM resolution ----
|
||||||
|
|
||||||
/// Find the SOHM index that handles the given message type.
|
/// Find the SOHM index that handles the given message type.
|
||||||
|
/// Load a file's SOHM table: superblock → superblock extension → Shared
|
||||||
|
/// Message Table message → SMTB. `Ok(None)` when the file has no superblock
|
||||||
|
/// extension or no shared-message table.
|
||||||
|
pub fn load_sohm_table(
|
||||||
|
file_data: &[u8],
|
||||||
|
offset_size: u8,
|
||||||
|
length_size: u8,
|
||||||
|
) -> Result<Option<SohmTable>, FormatError> {
|
||||||
|
let sig = crate::signature::find_signature(file_data)?;
|
||||||
|
let sb = crate::superblock::Superblock::parse(file_data, sig)?;
|
||||||
|
let Some(ext_addr) = sb
|
||||||
|
.superblock_extension_address
|
||||||
|
.filter(|&a| !is_undefined(a, offset_size))
|
||||||
|
else {
|
||||||
|
return Ok(None);
|
||||||
|
};
|
||||||
|
let ext = ObjectHeader::parse(file_data, ext_addr as usize, offset_size, length_size)?;
|
||||||
|
let Some(msg) = ext
|
||||||
|
.messages
|
||||||
|
.iter()
|
||||||
|
.find(|m| m.msg_type == MessageType::SharedMessageTable)
|
||||||
|
else {
|
||||||
|
return Ok(None);
|
||||||
|
};
|
||||||
|
let table_msg = parse_sohm_table_message(&msg.data, offset_size)?;
|
||||||
|
parse_sohm_table(
|
||||||
|
file_data,
|
||||||
|
table_msg.table_address as usize,
|
||||||
|
table_msg.nindexes,
|
||||||
|
offset_size,
|
||||||
|
)
|
||||||
|
.map(Some)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Like [`message_data`], but also follows references into the file's SOHM
|
||||||
|
/// heap (shared object header messages), loading the SOHM table on demand.
|
||||||
|
pub fn message_data_with_sohm<'a>(
|
||||||
|
file_data: &[u8],
|
||||||
|
msg: &'a crate::object_header::HeaderMessage,
|
||||||
|
offset_size: u8,
|
||||||
|
length_size: u8,
|
||||||
|
) -> Result<Cow<'a, [u8]>, FormatError> {
|
||||||
|
if !is_shared(msg.flags) {
|
||||||
|
return Ok(Cow::Borrowed(&msg.data));
|
||||||
|
}
|
||||||
|
let shared_ref = parse_shared_ref(&msg.data, offset_size)?;
|
||||||
|
let table = if shared_ref.heap_id.is_some() {
|
||||||
|
load_sohm_table(file_data, offset_size, length_size)?
|
||||||
|
} else {
|
||||||
|
None
|
||||||
|
};
|
||||||
|
resolve_shared_message_with_sohm(
|
||||||
|
file_data,
|
||||||
|
&shared_ref,
|
||||||
|
msg.msg_type,
|
||||||
|
offset_size,
|
||||||
|
length_size,
|
||||||
|
table.as_ref(),
|
||||||
|
)
|
||||||
|
.map(Cow::Owned)
|
||||||
|
}
|
||||||
|
|
||||||
fn find_index_for_msg_type(table: &SohmTable, msg_type: MessageType) -> Option<&SohmIndex> {
|
fn find_index_for_msg_type(table: &SohmTable, msg_type: MessageType) -> Option<&SohmIndex> {
|
||||||
let type_bit = 1u16 << msg_type.to_u16();
|
let type_bit = 1u16 << msg_type.to_u16();
|
||||||
table
|
table
|
||||||
@@ -707,6 +777,7 @@ mod tests {
|
|||||||
let mut buf = Vec::new();
|
let mut buf = Vec::new();
|
||||||
buf.extend_from_slice(b"SMTB");
|
buf.extend_from_slice(b"SMTB");
|
||||||
for idx in indexes {
|
for idx in indexes {
|
||||||
|
buf.push(0); // version
|
||||||
buf.push(idx.index_type);
|
buf.push(idx.index_type);
|
||||||
buf.extend_from_slice(&idx.mesg_types.to_le_bytes());
|
buf.extend_from_slice(&idx.mesg_types.to_le_bytes());
|
||||||
buf.extend_from_slice(&idx.min_mesg_size.to_le_bytes());
|
buf.extend_from_slice(&idx.min_mesg_size.to_le_bytes());
|
||||||
|
|||||||
@@ -0,0 +1,49 @@
|
|||||||
|
"""Generate shared_fill_value.h5: datasets whose Fill Value message is
|
||||||
|
*shared*, in the two ways libhdf5 can share one.
|
||||||
|
|
||||||
|
- /sohm_a, /sohm_b: the file has a shared-object-header-message (SOHM) index
|
||||||
|
for fill values, so libhdf5 stores the fill value (-7, int32) in the SOHM
|
||||||
|
heap and /sohm_b's header holds only a reference to it. Chunked, with only
|
||||||
|
the first chunk written, so the rest reads as the fill value.
|
||||||
|
- /unwritten_a, /unwritten_b: the same, never written: no storage at all,
|
||||||
|
read entirely as the fill value.
|
||||||
|
|
||||||
|
h5py has no API for SOHM indexes, so the file creation property list is
|
||||||
|
configured by calling the libhdf5 bundled in the h5py wheel through ctypes.
|
||||||
|
Written with h5py 3.16.0 / HDF5 2.0.0. Re-run only to regenerate:
|
||||||
|
|
||||||
|
python gen_shared_fill.py shared_fill_value.h5
|
||||||
|
"""
|
||||||
|
import ctypes
|
||||||
|
import glob
|
||||||
|
import os
|
||||||
|
import sys
|
||||||
|
|
||||||
|
import h5py
|
||||||
|
import numpy as np
|
||||||
|
|
||||||
|
libdir = os.path.join(os.path.dirname(os.path.dirname(h5py.__file__)), "h5py.libs")
|
||||||
|
libs = [p for p in glob.glob(os.path.join(libdir, "libhdf5*.so*")) if "_hl" not in os.path.basename(p)]
|
||||||
|
lib = ctypes.CDLL(libs[0])
|
||||||
|
lib.H5open()
|
||||||
|
|
||||||
|
H5O_SHMESG_FILL_FLAG = 1 << 0x0005
|
||||||
|
|
||||||
|
fcpl = h5py.h5p.create(h5py.h5p.FILE_CREATE)
|
||||||
|
lib.H5Pset_shared_mesg_nindexes.argtypes = [ctypes.c_int64, ctypes.c_uint]
|
||||||
|
lib.H5Pset_shared_mesg_index.argtypes = [ctypes.c_int64, ctypes.c_uint, ctypes.c_uint, ctypes.c_uint]
|
||||||
|
assert lib.H5Pset_shared_mesg_nindexes(fcpl.id, 1) >= 0
|
||||||
|
assert lib.H5Pset_shared_mesg_index(fcpl.id, 0, H5O_SHMESG_FILL_FLAG, 0) >= 0
|
||||||
|
|
||||||
|
fapl = h5py.h5p.create(h5py.h5p.FILE_ACCESS)
|
||||||
|
fapl.set_libver_bounds(h5py.h5f.LIBVER_LATEST, h5py.h5f.LIBVER_LATEST)
|
||||||
|
fid = h5py.h5f.create(sys.argv[1].encode(), h5py.h5f.ACC_TRUNC, fcpl=fcpl, fapl=fapl)
|
||||||
|
with h5py.File(fid) as f:
|
||||||
|
# Chunked, with only the first chunk written: the rest reads as fill.
|
||||||
|
# libhdf5 keeps the first copy of a message in its own header; the second
|
||||||
|
# identical one (the `_b` datasets) is the SOHM reference.
|
||||||
|
for name in ("sohm_a", "sohm_b"):
|
||||||
|
d = f.create_dataset(name, shape=(8,), chunks=(4,), dtype="<i4", fillvalue=-7)
|
||||||
|
d[:4] = np.arange(4)
|
||||||
|
for name in ("unwritten_a", "unwritten_b"):
|
||||||
|
f.create_dataset(name, shape=(3,), dtype="<i4", fillvalue=-7)
|
||||||
Binary file not shown.
@@ -597,3 +597,50 @@ fn unknown_message_flags_follow_libhdf5_on_tbogus() {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ---- 8. shared fill value messages ----
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn shared_fill_value_is_resolved_not_zero() {
|
||||||
|
// gen_shared_fill.py: HDF5 2.0 with a SOHM index for fill values, so each
|
||||||
|
// dataset's fill value message is a reference into the SOHM heap. It
|
||||||
|
// used to be read as "no fill value" (zeros) instead of -7.
|
||||||
|
let bytes = include_bytes!("fixtures/shared_fill_value.h5");
|
||||||
|
for (name, shared) in [
|
||||||
|
("sohm_a", false),
|
||||||
|
("sohm_b", true),
|
||||||
|
("unwritten_a", false),
|
||||||
|
("unwritten_b", true),
|
||||||
|
] {
|
||||||
|
let (sb, oh) = header_at(bytes, name);
|
||||||
|
let msg = oh
|
||||||
|
.messages
|
||||||
|
.iter()
|
||||||
|
.find(|m| m.msg_type == MessageType::FillValue)
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(
|
||||||
|
clawhdf5_format::shared_message::is_shared(msg.flags),
|
||||||
|
shared,
|
||||||
|
"{name}: fixture layout"
|
||||||
|
);
|
||||||
|
if shared {
|
||||||
|
// Without the file the reference cannot be followed: an error,
|
||||||
|
// never a silent default.
|
||||||
|
assert_eq!(
|
||||||
|
clawhdf5_format::fill_value::dataset_fill_value(&oh.messages),
|
||||||
|
Err(clawhdf5_format::error::FormatError::UnresolvedSharedMessage)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
assert_eq!(
|
||||||
|
clawhdf5_format::fill_value::dataset_fill_value_in(
|
||||||
|
bytes,
|
||||||
|
&oh.messages,
|
||||||
|
sb.offset_size,
|
||||||
|
sb.length_size
|
||||||
|
)
|
||||||
|
.unwrap(),
|
||||||
|
Some((-7i32).to_le_bytes().to_vec()),
|
||||||
|
"{name}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -478,7 +478,12 @@ impl<'f> Dataset<'f> {
|
|||||||
// sparse) dataset — select from a fill-aware full read instead. (The
|
// sparse) dataset — select from a fill-aware full read instead. (The
|
||||||
// selection reader currently decodes the full dataset too, so this
|
// selection reader currently decodes the full dataset too, so this
|
||||||
// costs nothing extra.)
|
// costs nothing extra.)
|
||||||
let fill = clawhdf5_format::fill_value::dataset_fill_value(&self.header.messages)?;
|
let fill = clawhdf5_format::fill_value::dataset_fill_value_in(
|
||||||
|
self.file.data.as_bytes(),
|
||||||
|
&self.header.messages,
|
||||||
|
self.file.offset_size(),
|
||||||
|
self.file.length_size(),
|
||||||
|
)?;
|
||||||
let fill_matters = !clawhdf5_format::fill_value::has_storage(&dl)
|
let fill_matters = !clawhdf5_format::fill_value::has_storage(&dl)
|
||||||
|| (matches!(dl, DataLayout::Chunked { .. })
|
|| (matches!(dl, DataLayout::Chunked { .. })
|
||||||
&& !clawhdf5_format::fill_value::is_default(fill.as_deref()));
|
&& !clawhdf5_format::fill_value::is_default(fill.as_deref()));
|
||||||
|
|||||||
@@ -0,0 +1,49 @@
|
|||||||
|
//! Datasets whose Fill Value message is shared through the file's SOHM heap
|
||||||
|
//! (fixture written by HDF5 2.0, see `gen_shared_fill.py`). Their unwritten
|
||||||
|
//! storage must read as the fill value (-7), not as zeros.
|
||||||
|
|
||||||
|
use clawhdf5::File;
|
||||||
|
use clawhdf5_format::selection::Selection;
|
||||||
|
|
||||||
|
const FIXTURE: &[u8] = include_bytes!("../../clawhdf5-format/tests/fixtures/shared_fill_value.h5");
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn shared_fill_value_applies_to_unwritten_storage() {
|
||||||
|
let file = File::from_bytes(FIXTURE.to_vec()).unwrap();
|
||||||
|
// `_a` keeps its fill value in its own header, `_b` references the SOHM
|
||||||
|
// heap; both must read the same.
|
||||||
|
for name in ["sohm_a", "sohm_b"] {
|
||||||
|
assert_eq!(
|
||||||
|
file.dataset(name).unwrap().read_i32().unwrap(),
|
||||||
|
[0, 1, 2, 3, -7, -7, -7, -7],
|
||||||
|
"{name}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
for name in ["unwritten_a", "unwritten_b"] {
|
||||||
|
assert_eq!(
|
||||||
|
file.dataset(name).unwrap().read_i32().unwrap(),
|
||||||
|
[-7, -7, -7],
|
||||||
|
"{name}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// The selection path decides on its own whether the fill value matters.
|
||||||
|
let slab = Selection::Hyperslab {
|
||||||
|
start: vec![2],
|
||||||
|
stride: vec![1],
|
||||||
|
count: vec![4],
|
||||||
|
block: vec![1],
|
||||||
|
};
|
||||||
|
let raw = file
|
||||||
|
.dataset("sohm_b")
|
||||||
|
.unwrap()
|
||||||
|
.read_selection(&slab)
|
||||||
|
.unwrap();
|
||||||
|
let values: Vec<i32> = raw
|
||||||
|
.as_chunks::<4>()
|
||||||
|
.0
|
||||||
|
.iter()
|
||||||
|
.map(|b| i32::from_le_bytes(*b))
|
||||||
|
.collect();
|
||||||
|
assert_eq!(values, [2, 3, -7, -7]);
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user