diff --git a/CHANGELOG.md b/CHANGELOG.md index d256a15..8912fed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,18 @@ ZFP filters; the 2 mismatches are the known h5py big-endian VL bug. files as agreeing with h5py that the library did not open; probe and library now take the decision from the same `superblock_ext::cache_image_state`. +- **Every other opener applies the superblock extension and the cache + image too** (`superblock_ext::apply_cache_image_in_place`, writing into + the buffer each already owns): `clawhdf5_io`'s `NativeVol` (at `open`, + and on read for `from_bytes`), `AsyncHDF5File`, `MpiVol` (a minimal edit + through the same `vol::load_hdf5`; the `mpi-io` feature cannot be built + without an MPI installation, so it was not compiled), and the external + source files of a virtual dataset. They read a file with an image from + its own bytes — stale metadata, or none (`h5clear_mdc_image.h5` failed + with `InvalidObjectHeaderVersion(0)`) — and skipped the extension checks + `File::open` makes. These readers cannot open a file and fail each + object, so an image libhdf5 cannot load is refused with the image's + error. - **The superblock extension is decoded at open, as libhdf5 does:** a File Space Info or Metadata Cache Image message libhdf5 cannot decode makes the open fail (`cve-2020-10810`, `cve-2020-10812` were opened). diff --git a/crates/clawhdf5-format/src/vds.rs b/crates/clawhdf5-format/src/vds.rs index f5d5237..d67b832 100644 --- a/crates/clawhdf5-format/src/vds.rs +++ b/crates/clawhdf5-format/src/vds.rs @@ -803,7 +803,11 @@ impl<'a, 'r> Sources<'a, 'r> { let resolver = self.resolver.ok_or_else(|| { vds_err("external-file virtual dataset sources require a file resolver") })?; - self.cached_file = Some((String::from(name), resolver(name)?)); + let mut bytes = resolver(name)?; + if let Some(b) = bytes.as_mut() { + load_source_file(b)?; + } + self.cached_file = Some((String::from(name), bytes)); } // An external file is handed over whole; its addresses are relative // to its superblock, so skip any user block. @@ -851,6 +855,23 @@ impl<'a, 'r> Sources<'a, 'r> { } } +/// Check an external source file's superblock extension as libhdf5 does +/// when it opens the file, and write any metadata cache image over its +/// metadata in place: libhdf5 reads the image's entries instead of the +/// file's own, possibly stale, bytes (`crate::superblock_ext`). A source +/// file whose image cannot be loaded is an error, as other corrupt source +/// files are here. +fn load_source_file(whole: &mut [u8]) -> Result<(), FormatError> { + let base = crate::signature::find_signature(whole)?; + let sb = crate::superblock::Superblock::parse(&whole[base..], 0)?; + // The end of file the superblock records; a truncated source file is + // read as before, up to its length. + let end = sb + .data_end(base as u64, whole.len() as u64) + .map_or(whole.len(), |e| base + e as usize); + crate::superblock_ext::apply_cache_image_in_place(&mut whole[base..end], &sb) +} + /// Whether elements of `dt` contain addresses into their own file: /// variable-length data (global-heap IDs) or references. fn holds_file_addresses(dt: &Datatype) -> bool { diff --git a/crates/clawhdf5-io/src/async_read.rs b/crates/clawhdf5-io/src/async_read.rs index 30969fa..f5277c0 100644 --- a/crates/clawhdf5-io/src/async_read.rs +++ b/crates/clawhdf5-io/src/async_read.rs @@ -288,6 +288,12 @@ impl AsyncHDF5File { // superblock records, as libhdf5 does. let end = superblock.data_end(user_block as u64, whole_len)?; data.truncate(end as usize); + // Check the superblock extension as libhdf5 does at open, and write + // any metadata cache image over the file's metadata (libhdf5 reads + // the image's entries instead of the file's own, possibly stale, + // bytes). An image libhdf5 cannot load is refused: this reader has + // no way to open the file and fail each object instead. + clawhdf5_format::superblock_ext::apply_cache_image_in_place(&mut data, &superblock)?; Ok(Self { data, superblock }) } @@ -430,6 +436,41 @@ mod tests { fw.finish().unwrap() } + /// libhdf5's `h5clear_mdc_image.h5` (see the clawhdf5 crate's + /// `tests/metadata_cache_image.rs`): the root group's header exists + /// only in the file's metadata cache image. + fn cache_image_fixture() -> Vec { + std::fs::read(concat!( + env!("CARGO_MANIFEST_DIR"), + "/../clawhdf5/tests/fixtures/h5clear_mdc_image.h5" + )) + .unwrap() + } + + #[tokio::test] + async fn reads_through_a_metadata_cache_image() { + let bytes = cache_image_fixture(); + let file = AsyncHDF5File::from_bytes(bytes.clone()).unwrap(); + let info = file.read_dataset_raw("DSET").await.unwrap(); + assert_eq!(info.shape, [50, 100]); + let values: Vec = info + .raw + .chunks_exact(4) + .map(|b| i32::from_le_bytes(b.try_into().unwrap())) + .collect(); + let expected: Vec = (0..50).flat_map(|i| (0..100).map(move |j| i * j)).collect(); + assert_eq!(values, expected); + + // An image libhdf5 cannot load is refused at open. + let mut bad = bytes; + let at = bad.windows(4).position(|w| w == b"MDCI").unwrap(); + bad[at] = b'X'; + assert!(matches!( + AsyncHDF5File::from_bytes(bad), + Err(AsyncHDF5Error::Format(FormatError::InvalidCacheImage(_))) + )); + } + // --- AsyncMemoryReader tests --- #[tokio::test] diff --git a/crates/clawhdf5-io/src/mpi_vol.rs b/crates/clawhdf5-io/src/mpi_vol.rs index 87a82ea..fdf4d56 100644 --- a/crates/clawhdf5-io/src/mpi_vol.rs +++ b/crates/clawhdf5-io/src/mpi_vol.rs @@ -191,7 +191,10 @@ fn mpi_collective_read(vol: &MpiVol, location: &str, path: &str) -> Result>, location: Option, + /// Why the bytes given to [`NativeVol::from_bytes`] cannot be read + /// (what `open` would have refused them for). + load_error: Option, } /// The HDF5 bytes of a whole file and its superblock: from the superblock @@ -228,12 +231,35 @@ pub(crate) fn hdf5_view( Ok((&data[..end as usize], sb)) } +/// Check a whole file as libhdf5 does when it opens it ([`hdf5_view`], and +/// the superblock extension), and write any metadata cache image over the +/// file's metadata in place: libhdf5 reads the image's entries instead of +/// the file's own bytes at their addresses, which may be stale +/// (`clawhdf5_format::superblock_ext`). A file whose image libhdf5 cannot +/// load is refused: a connector that reads whole datasets has no way to +/// open the file and fail each object instead. +pub(crate) fn load_hdf5(whole: &mut [u8]) -> Result<(), VolError> { + use clawhdf5_format::superblock_ext::apply_cache_image_in_place; + let err = |e: clawhdf5_format::error::FormatError| VolError::DataError(e.to_string()); + let (len, sb) = { + let (data, sb) = hdf5_view(whole)?; + (data.len(), sb) + }; + // hdf5_view's bytes start at the superblock, after any user block. + let base = clawhdf5_format::signature::split_user_block(whole) + .map_err(err)? + .0 + .len(); + apply_cache_image_in_place(&mut whole[base..base + len], &sb).map_err(err) +} + impl NativeVol { /// Create a new native VOL connector. pub fn new() -> Self { Self { data: None, location: None, + load_error: None, } } @@ -245,10 +271,12 @@ impl NativeVol { } /// Create a native VOL connector from bytes already in memory. - pub fn from_bytes(data: Vec) -> Self { + pub fn from_bytes(mut data: Vec) -> Self { + let load_error = load_hdf5(&mut data).err().map(|e| e.to_string()); Self { data: Some(data), location: Some("".into()), + load_error, } } @@ -281,10 +309,12 @@ impl VirtualObjectLayer for NativeVol { } fn open(&mut self, location: &str) -> Result<(), VolError> { - let data = std::fs::read(location)?; - // Refuse a truncated file at open, as libhdf5 does. - hdf5_view(&data)?; + let mut data = std::fs::read(location)?; + // Refuse a truncated file at open, as libhdf5 does, and load any + // metadata cache image. + load_hdf5(&mut data)?; self.data = Some(data); + self.load_error = None; self.location = Some(location.to_string()); Ok(()) } @@ -299,6 +329,9 @@ impl VirtualObjectLayer for NativeVol { let data = self.data.as_ref().ok_or_else(|| { VolError::Io(io::Error::new(io::ErrorKind::NotConnected, "file not open")) })?; + if let Some(e) = &self.load_error { + return Err(VolError::DataError(e.clone())); + } use clawhdf5_format::{ data_layout::DataLayout, data_read::read_raw_data_full, dataspace::Dataspace, @@ -434,6 +467,43 @@ mod tests { assert_eq!(raw.len(), 24); } + /// libhdf5's `h5clear_mdc_image.h5` (see the clawhdf5 crate's + /// `tests/metadata_cache_image.rs`): the root group's header exists + /// only in the file's metadata cache image, so reading the file's own + /// bytes finds zeros there. + #[test] + fn native_vol_reads_through_a_metadata_cache_image() { + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../clawhdf5/tests/fixtures/h5clear_mdc_image.h5" + ); + let expected: Vec = (0..50) + .flat_map(|i| (0..100).map(move |j| i * j)) + .flat_map(i32::to_le_bytes) + .collect(); + let vol = NativeVol::open_path(path).unwrap(); + assert_eq!(vol.read_dataset("DSET").unwrap(), expected); + let bytes = std::fs::read(path).unwrap(); + let vol = NativeVol::from_bytes(bytes.clone()); + assert_eq!(vol.read_dataset("DSET").unwrap(), expected); + + // An image libhdf5 cannot load is refused, not read around. + let mut bad = bytes; + let at = bad.windows(4).position(|w| w == b"MDCI").unwrap(); + bad[at] = b'X'; + let err = NativeVol::from_bytes(bad.clone()) + .read_dataset("DSET") + .unwrap_err(); + assert!(err.to_string().contains("cache image"), "{err}"); + let dir = tempfile::tempdir().unwrap(); + let bad_path = dir.path().join("bad_image.h5"); + std::fs::write(&bad_path, &bad).unwrap(); + let err = NativeVol::open_path(bad_path.to_str().unwrap()) + .err() + .unwrap(); + assert!(err.to_string().contains("cache image"), "{err}"); + } + #[test] fn vol_error_display() { let err = VolError::Unsupported("read_dataset".into()); diff --git a/crates/clawhdf5/tests/vds_interop.rs b/crates/clawhdf5/tests/vds_interop.rs index 17069b0..71e512e 100644 --- a/crates/clawhdf5/tests/vds_interop.rs +++ b/crates/clawhdf5/tests/vds_interop.rs @@ -486,3 +486,26 @@ fn vds_libhdf5_test_files() { vec![5, 10, 10] ); } + +/// A source file with a metadata cache image (libhdf5's +/// `h5clear_mdc_image.h5`, whose root group's header exists only in the +/// image) is read through the image, as libhdf5 opens it. Source files were +/// read from their own bytes, and this one failed on the root group. +#[test] +fn vds_source_file_with_a_metadata_cache_image() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let fixture = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/h5clear_mdc_image.h5"); + std::fs::copy(&fixture, dir.path().join("src.h5")).unwrap(); + generate( + dir.path(), + r#" +with h5py.File("vds.h5", "w", libver="latest") as f: + lay = h5py.VirtualLayout(shape=(3, 100), dtype="i4") + lay[0:3, :] = h5py.VirtualSource("src.h5", "DSET", shape=(50, 100))[5:8, :] + f.create_virtual_dataset("v", lay) +expect("vds.h5", "v", "image_source") +"#, + ); + assert_matches_libhdf5(dir.path(), "vds.h5", "v", "image_source"); +}