From 6185874f9caaeb7edf61688d003e8a93cc517cc7 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 18:43:00 -0500 Subject: [PATCH] format, clawhdf5: cut every Storage read to the range asked for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ExtentBytes and read_exact_at/read_upto rejected short results but passed longer-than-asked ones through, and FileData forwarded them too, so a Storage that broke read_at's contract by returning extra bytes had them decoded or returned as data (a contiguous dataset read gained 37 junk bytes). gather_storage alone trimmed. - storage::exact_len (new, pub): a read of len bytes as exactly len — cut when longer, an error when short. read_exact_at, read_upto and ExtentBytes (so chunk fetches and selection gathers) go through it. - FileData cuts a backend's answer to what it asked for before laying the cache image over it. - Tests: over a storage that appends 37 junk bytes to every read, every format-crate fixture reads exactly as from the slice (overlong_reads_are_cut_to_the_range_asked_for), and every facade fixture opens and reads through File::open_storage as through File::open (overlong_storage_reads_identically). Both failed before. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 4 + crates/clawhdf5-format/src/storage.rs | 43 ++-- .../tests/storage_equivalence.rs | 195 +++++++++++------- crates/clawhdf5/src/reader.rs | 26 ++- crates/clawhdf5/tests/storage_equivalence.rs | 47 +++++ 5 files changed, 225 insertions(+), 90 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c6a7b6..bbd2eb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,10 @@ - Each extent's bounds error is the one the slice readers gave, reported when the read reaches that extent, so a damaged file fails with the same error, in the same order, through either path. + - A backend that answers a read with more bytes than asked (breaking + `read_at`'s contract) never has the extra bytes used: every read is + cut to the range asked for (`storage::exact_len`), and a short answer + inside the file is an error. - **No behaviour change for in-memory and mapped files:** with `as_contiguous()` every path slices the file as before (checked below). - Tests (2026-09-26, tank): diff --git a/crates/clawhdf5-format/src/storage.rs b/crates/clawhdf5-format/src/storage.rs index 7425296..482cfad 100644 --- a/crates/clawhdf5-format/src/storage.rs +++ b/crates/clawhdf5-format/src/storage.rs @@ -43,6 +43,9 @@ pub trait Storage { /// end of the storage (and empty when `offset` is at or past the end); /// a backend that cannot serve a range returns an error instead of a /// short read. + /// It is never longer than `len`; the parsers cut a longer result to + /// `len` (see [`exact_len`]) rather than read bytes from outside the + /// range. fn read_at(&self, offset: u64, len: usize) -> Result, FormatError>; /// Current length of the storage in bytes. @@ -218,13 +221,30 @@ pub fn read_exact_at( Some(end) if end <= file.len() => {} _ => return Err(eof()), } - let bytes = file.read_at(offset, len)?; - if bytes.len() < len { - // The storage shrank or the backend served a short read inside the - // file: never parse a partial structure. - return Err(short_read()); + // A short read (the storage shrank, or the backend served less inside + // the file) is an error: never parse a partial structure. + exact_len(file.read_at(offset, len)?, len) +} + +/// `bytes`, the result of asking a [`Storage`] for `len` bytes, as exactly +/// `len` bytes: a longer result (a backend that broke +/// [`Storage::read_at`]'s contract) is cut to `len`, so bytes from outside +/// the range asked for are never parsed or returned; a shorter one is an +/// error (the storage shrank, or the backend failed), never a partial +/// structure. +#[inline] +pub fn exact_len(bytes: Cow<'_, [u8]>, len: usize) -> Result, FormatError> { + match bytes.len().cmp(&len) { + core::cmp::Ordering::Equal => Ok(bytes), + core::cmp::Ordering::Less => Err(short_read()), + core::cmp::Ordering::Greater => Ok(match bytes { + Cow::Borrowed(b) => Cow::Borrowed(&b[..len]), + Cow::Owned(mut v) => { + v.truncate(len); + Cow::Owned(v) + } + }), } - Ok(bytes) } #[cold] @@ -329,11 +349,7 @@ pub fn read_upto( } let avail = file.len().saturating_sub(offset); let len = usize::try_from(avail).map_or(max, |a| a.min(max)); - let bytes = file.read_at(offset, len)?; - if bytes.len() < len { - return Err(short_read()); - } - Ok(bytes) + exact_len(file.read_at(offset, len)?, len) } /// Most stored bytes fetched by one [`Storage::read_ranges`] call when a @@ -461,10 +477,7 @@ impl<'a> ExtentBytes<'a> { )); } for ((slot, bytes), r) in slots.into_iter().zip(got).zip(&ranges) { - if (bytes.len() as u64) < r.end - r.start { - return Err(short_read()); - } - out[slot] = Extent::Bytes(bytes); + out[slot] = Extent::Bytes(exact_len(bytes, (r.end - r.start) as usize)?); } } Ok(ExtentBytes::Fetched { base, extents: out }) diff --git a/crates/clawhdf5-format/tests/storage_equivalence.rs b/crates/clawhdf5-format/tests/storage_equivalence.rs index 71b0dc0..a25a79a 100644 --- a/crates/clawhdf5-format/tests/storage_equivalence.rs +++ b/crates/clawhdf5-format/tests/storage_equivalence.rs @@ -753,6 +753,73 @@ fn corpus_parses_identically_through_storage() { assert!(tally.files > 0); } +/// The whole read of every object reachable from the root group of `sb`'s +/// file — each dataset whole (fill-aware) and half of it through a +/// selection, and each group's listing — as one result to compare. +fn read_everything(file: &dyn Storage, sb: &Superblock) -> Result { + let (os, ls) = (sb.offset_size, sb.length_size); + let mut out = String::new(); + let mut queue = VecDeque::from([sb.root_group_address]); + let mut seen = HashSet::new(); + while let Some(addr) = queue.pop_front() { + if seen.len() > 200 || !seen.insert(addr) { + continue; + } + let header = ObjectHeader::parse_in(file, addr, os, ls)?; + let find = |t: MessageType| { + header + .messages + .iter() + .find(|m| m.msg_type == t) + .map(|m| message_data_with_sohm_in(file, m, os, ls)) + .transpose() + }; + if let (Some(dt), Some(ds), Some(dl)) = ( + find(MessageType::Datatype)?, + find(MessageType::Dataspace)?, + find(MessageType::DataLayout)?, + ) { + let dt = Datatype::parse(&dt)?.0; + let ds = Dataspace::parse(&ds, ls)?; + let dl = DataLayout::parse(&dl, os, ls)?; + let pl = find(MessageType::FilterPipeline)? + .map(|p| FilterPipeline::parse(&p)) + .transpose()?; + let data = read_full_with_fill_in( + &header.messages, + file, + &dl, + &ds, + dt.type_size() as usize, + os, + ls, + || read_raw_data_full_in(file, &dl, &ds, &dt, pl.as_ref(), os, ls), + ); + out.push_str(&format!("{addr}: {data:?}\n")); + if let Some(&d0) = ds.dimensions.first() { + let rank = ds.dimensions.len(); + let sel = Selection::Hyperslab { + start: vec![0; rank], + stride: vec![1; rank], + count: std::iter::once(d0.div_ceil(2)) + .chain(ds.dimensions[1..].iter().copied()) + .collect(), + block: vec![1; rank], + }; + let part = + read_raw_data_selection_in(file, &dl, &ds, &dt, pl.as_ref(), os, ls, &sel); + out.push_str(&format!("{addr} half: {part:?}\n")); + } + } + let children = group_v2::resolve_group_children_in(file, sb, addr); + out.push_str(&format!("{addr} children: {children:?}\n")); + if let Ok(c) = children { + queue.extend(c.iter().map(|c| c.object_header_address)); + } + } + Ok(out) +} + /// A storage that misbehaves: fails its `fail_at`-th read (1-based, `0` /// never), and, with `short`, serves one byte less than asked for inside /// the file (a truncated response). @@ -808,78 +875,7 @@ fn misbehaving_storage_never_returns_wrong_data() { let Ok(sb) = Superblock::parse(hdf5, 0) else { continue; }; - // The whole read of every object, as one result to compare. - let everything = |file: &dyn Storage| -> Result { - let (os, ls) = (sb.offset_size, sb.length_size); - let mut out = String::new(); - let mut queue = VecDeque::from([sb.root_group_address]); - let mut seen = HashSet::new(); - while let Some(addr) = queue.pop_front() { - if seen.len() > 200 || !seen.insert(addr) { - continue; - } - let header = ObjectHeader::parse_in(file, addr, os, ls)?; - let find = |t: MessageType| { - header - .messages - .iter() - .find(|m| m.msg_type == t) - .map(|m| message_data_with_sohm_in(file, m, os, ls)) - .transpose() - }; - if let (Some(dt), Some(ds), Some(dl)) = ( - find(MessageType::Datatype)?, - find(MessageType::Dataspace)?, - find(MessageType::DataLayout)?, - ) { - let dt = Datatype::parse(&dt)?.0; - let ds = Dataspace::parse(&ds, ls)?; - let dl = DataLayout::parse(&dl, os, ls)?; - let pl = find(MessageType::FilterPipeline)? - .map(|p| FilterPipeline::parse(&p)) - .transpose()?; - let data = read_full_with_fill_in( - &header.messages, - file, - &dl, - &ds, - dt.type_size() as usize, - os, - ls, - || read_raw_data_full_in(file, &dl, &ds, &dt, pl.as_ref(), os, ls), - ); - out.push_str(&format!("{addr}: {data:?}\n")); - if let Some(&d0) = ds.dimensions.first() { - let rank = ds.dimensions.len(); - let sel = Selection::Hyperslab { - start: vec![0; rank], - stride: vec![1; rank], - count: std::iter::once(d0.div_ceil(2)) - .chain(ds.dimensions[1..].iter().copied()) - .collect(), - block: vec![1; rank], - }; - let part = read_raw_data_selection_in( - file, - &dl, - &ds, - &dt, - pl.as_ref(), - os, - ls, - &sel, - ); - out.push_str(&format!("{addr} half: {part:?}\n")); - } - } - let children = group_v2::resolve_group_children_in(file, &sb, addr); - out.push_str(&format!("{addr} children: {children:?}\n")); - if let Ok(c) = children { - queue.extend(c.iter().map(|c| c.object_header_address)); - } - } - Ok(out) - }; + let everything = |file: &dyn Storage| read_everything(file, &sb); let want = everything(&hdf5); let counting = CountingStorage::new(hdf5.to_vec()); assert_eq!( @@ -947,6 +943,61 @@ fn misbehaving_storage_never_returns_wrong_data() { ); } +/// A storage that breaks `read_at`'s contract the other way: every read +/// comes back with 37 bytes more than asked for (junk past the range). +struct Overlong { + data: Vec, +} + +impl Storage for Overlong { + fn read_at(&self, offset: u64, len: usize) -> Result, FormatError> { + let mut v = self.data.as_slice().read_at(offset, len)?.into_owned(); + v.extend(std::iter::repeat_n(0xa5, 37)); + Ok(std::borrow::Cow::Owned(v)) + } + + fn len(&self) -> u64 { + self.data.len() as u64 + } +} + +/// Bytes past the range asked for are never used: every read through a +/// storage that returns more than asked gives exactly the in-memory result. +#[test] +fn overlong_reads_are_cut_to_the_range_asked_for() { + let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures"); + let mut files = Vec::new(); + hdf5_files(&dir, &mut files); + files.sort(); + let mut compared = 0; + for path in &files { + let Ok(bytes) = std::fs::read(path) else { + continue; + }; + let Ok((_, hdf5)) = split_user_block(&bytes) else { + continue; + }; + let Ok(sb) = Superblock::parse(hdf5, 0) else { + continue; + }; + let want = read_everything(&hdf5, &sb); + let got = read_everything( + &Overlong { + data: hdf5.to_vec(), + }, + &sb, + ); + assert_eq!( + format!("{got:?}"), + format!("{want:?}"), + "{}", + path.display() + ); + compared += 1; + } + assert!(compared > 40, "{compared}"); +} + /// A read_at-only storage that also counts `read_ranges` calls and ranges. struct BatchCounting { inner: CountingStorage, diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 191c614..a99373f 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -300,7 +300,7 @@ impl Storage for FileData { if len == 0 { return Ok(Cow::Borrowed(&[])); } - let bytes = self.remote()?.read_at(self.base + offset, len)?; + let bytes = cut_to(self.remote()?.read_at(self.base + offset, len)?, len); Ok(self.with_overlay(offset, bytes)) } @@ -323,8 +323,11 @@ impl Storage for FileData { let got = self.remote()?.read_ranges(&shifted)?; Ok(got .into_iter() - .zip(ranges) - .map(|(bytes, r)| self.with_overlay(r.start, bytes)) + .zip(ranges.iter().zip(&shifted)) + .map(|(bytes, (r, asked))| { + let bytes = cut_to(bytes, (asked.end - asked.start) as usize); + self.with_overlay(r.start, bytes) + }) .collect()) } @@ -334,6 +337,23 @@ impl Storage for FileData { } } +/// `bytes`, a backend's answer to a read of `len` bytes, without anything +/// past those `len` (a backend that returns more than asked breaks +/// [`Storage::read_at`]'s contract; the extra bytes are not the file's). A +/// short answer is passed on: the parsers' own checks refuse it. +fn cut_to(bytes: Cow<'_, [u8]>, len: usize) -> Cow<'_, [u8]> { + if bytes.len() <= len { + return bytes; + } + match bytes { + Cow::Borrowed(b) => Cow::Borrowed(&b[..len]), + Cow::Owned(mut v) => { + v.truncate(len); + Cow::Owned(v) + } + } +} + // --------------------------------------------------------------------------- // File // --------------------------------------------------------------------------- diff --git a/crates/clawhdf5/tests/storage_equivalence.rs b/crates/clawhdf5/tests/storage_equivalence.rs index 7a2749b..71768d2 100644 --- a/crates/clawhdf5/tests/storage_equivalence.rs +++ b/crates/clawhdf5/tests/storage_equivalence.rs @@ -432,3 +432,50 @@ fn storage_backed_files_keep_their_zero_copy_views_only_in_memory() { "as_bytes over a range storage must not answer" ); } + +/// A storage that returns 37 junk bytes more than every read asked for. +struct Overlong(Vec); + +impl clawhdf5::Storage for Overlong { + fn read_at(&self, offset: u64, len: usize) -> Result, FormatError> { + let mut v = clawhdf5::Storage::read_at(self.0.as_slice(), offset, len)?.into_owned(); + v.extend(std::iter::repeat_n(0xa5, 37)); + Ok(std::borrow::Cow::Owned(v)) + } + + fn len(&self) -> u64 { + self.0.len() as u64 + } +} + +/// Bytes a misbehaving storage returns past the range asked for are never +/// read as the file's: every fixture reads through it as through +/// `File::open`. +#[test] +fn overlong_storage_reads_identically() { + let root = Path::new(env!("CARGO_MANIFEST_DIR")); + let mut files = Vec::new(); + hdf5_files(&root.join("tests/fixtures"), &mut files); + hdf5_files(&root.join("../clawhdf5-format/tests/fixtures"), &mut files); + files.sort(); + let mut compared = 0; + for path in &files { + let Ok(bytes) = std::fs::read(path) else { + continue; + }; + let Ok(local) = File::open(path) else { + continue; + }; + let mut remote = File::open_storage(Arc::new(Overlong(bytes))) + .unwrap_or_else(|e| panic!("{}: {e}", path.display())); + remote.set_vds_resolver(sibling_resolver(path.parent().map(Path::to_path_buf))); + assert_eq!( + transcript(&local), + transcript(&remote), + "{}", + path.display() + ); + compared += 1; + } + assert!(compared >= 40, "{compared}"); +}