From cf2b408a63bd7ea630875183ea82506e56add14b Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 12:57:08 -0500 Subject: [PATCH] format: read fixed array chunk indexes over Storage FixedArrayHeader::parse_in reads the header in one read, and read_fixed_array_chunks_in reads the data block's prefix and then the whole block, paged or not, as one window; checksums and elements are checked in it, with bounds errors reported as the whole-file checks did (also in builds without the checksum feature, where the per-element checks are the only ones). The open-ended &file_data[offset..] slices are gone. The &[u8] functions are wrappers. New test: non-paged and paged, filtered and unfiltered arrays, cut through the data block and with damaged bytes, read identically through a read_at-only CountingStorage. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/fixed_array.rs | 280 ++++++++++++++++------ 1 file changed, 208 insertions(+), 72 deletions(-) diff --git a/crates/clawhdf5-format/src/fixed_array.rs b/crates/clawhdf5-format/src/fixed_array.rs index 8546224..de936a7 100644 --- a/crates/clawhdf5-format/src/fixed_array.rs +++ b/crates/clawhdf5-format/src/fixed_array.rs @@ -9,16 +9,19 @@ use alloc::{format, vec, vec::Vec}; use crate::chunk_grid::ChunkGrid; use crate::chunked_read::ChunkInfo; use crate::error::FormatError; +use crate::storage::{Storage, Window, len_usize, read_exact_at}; /// Verify the Jenkins lookup3 checksum stored immediately after -/// `data[start..end]`, as every Fixed Array structure carries one. +/// `data[start..end]`, as every Fixed Array structure carries one. `w` is +/// a window of the file and `start`/`end` are relative to it. /// /// A corrupt chunk index silently yields addresses pointing at the wrong /// bytes, so a mismatch has to be an error rather than a shrug: without this /// the damage surfaces as plausible-looking data from the wrong chunk. #[cfg(feature = "checksum")] -fn verify_checksum(data: &[u8], start: usize, end: usize) -> Result<(), FormatError> { - ensure_len(data, end, 4)?; +fn verify_checksum(w: &Window<'_>, start: usize, end: usize) -> Result<(), FormatError> { + w.ensure(end, 4)?; + let data = &w.bytes; let stored = u32::from_le_bytes([data[end], data[end + 1], data[end + 2], data[end + 3]]); let computed = crate::checksum::jenkins_lookup3(&data[start..end]); if computed != stored { @@ -31,7 +34,7 @@ fn verify_checksum(data: &[u8], start: usize, end: usize) -> Result<(), FormatEr } #[cfg(not(feature = "checksum"))] -fn verify_checksum(_data: &[u8], _start: usize, _end: usize) -> Result<(), FormatError> { +fn verify_checksum(_w: &Window<'_>, _start: usize, _end: usize) -> Result<(), FormatError> { Ok(()) } @@ -73,19 +76,6 @@ fn read_length(data: &[u8], pos: usize, size: u8) -> Result { read_offset(data, pos, size) } -fn ensure_len(data: &[u8], offset: usize, needed: usize) -> Result<(), FormatError> { - if offset - .checked_add(needed) - .is_none_or(|end| end > data.len()) - { - return Err(FormatError::UnexpectedEof { - expected: offset.saturating_add(needed), - available: data.len(), - }); - } - Ok(()) -} - fn is_undefined(data: &[u8], pos: usize, size: u8) -> bool { let s = size as usize; if pos + s > data.len() { @@ -101,13 +91,24 @@ impl FixedArrayHeader { offset: usize, offset_size: u8, length_size: u8, + ) -> Result { + Self::parse_in(&file_data, offset as u64, offset_size, length_size) + } + + /// [`Self::parse`] over any [`Storage`]: one read of the header. + pub fn parse_in( + file: &dyn Storage, + offset: u64, + offset_size: u8, + length_size: u8, ) -> Result { // FAHD signature(4) + version(1) + client_id(1) + element_size(1) + // max_nelmts_bits(1) + num_elements(length_size) + data_block_addr(offset_size) + checksum(4) let min_size = 4 + 1 + 1 + 1 + 1 + length_size as usize + offset_size as usize + 4; - ensure_len(file_data, offset, min_size)?; + let w = Window::read(file, offset, min_size)?; + w.ensure(0, min_size)?; - let d = &file_data[offset..]; + let d: &[u8] = &w.bytes; if &d[0..4] != b"FAHD" { return Err(FormatError::ChunkedReadError( "invalid Fixed Array header signature".into(), @@ -130,7 +131,7 @@ impl FixedArrayHeader { pos += length_size as usize; let data_block_address = read_offset(d, pos, offset_size)?; pos += offset_size as usize; - verify_checksum(file_data, offset, offset + pos)?; + verify_checksum(&w, 0, pos)?; Ok(FixedArrayHeader { client_id, @@ -156,15 +157,39 @@ pub fn read_fixed_array_chunks( chunk_dimensions: &[u32], element_size: u32, offset_size: u8, + length_size: u8, +) -> Result, FormatError> { + read_fixed_array_chunks_in( + &file_data, + header, + dataset_dims, + max_dims, + chunk_dimensions, + element_size, + offset_size, + length_size, + ) +} + +/// [`read_fixed_array_chunks`] over any [`Storage`]: one read of the data +/// block's prefix, one of the whole data block (pages included). +#[allow(clippy::too_many_arguments)] +pub fn read_fixed_array_chunks_in( + file: &dyn Storage, + header: &FixedArrayHeader, + dataset_dims: &[u64], + max_dims: Option<&[u64]>, + chunk_dimensions: &[u32], + element_size: u32, + offset_size: u8, _length_size: u8, ) -> Result, FormatError> { + let file_len = len_usize(file); let db_offset = header.data_block_address as usize; // Parse data block header: FADB(4) + version(1) + client_id(1) + header_address(offset_size) let db_header_size = 4 + 1 + 1 + offset_size as usize; - ensure_len(file_data, db_offset, db_header_size)?; - - let d = &file_data[db_offset..]; + let d = read_exact_at(file, db_offset as u64, db_header_size)?; if &d[0..4] != b"FADB" { return Err(FormatError::ChunkedReadError( "invalid Fixed Array data block signature".into(), @@ -178,7 +203,7 @@ pub fn read_fixed_array_chunks( // A chunk index cannot describe more elements than the file has bytes (each // element occupies at least `offset_size` bytes). Reject a corrupt count // before it can drive a huge loop or overflow an offset computation. - if num_elements > file_data.len() { + if num_elements > file_len { return Err(FormatError::ChunkedReadError( "Fixed Array element count exceeds file size".into(), )); @@ -208,30 +233,34 @@ pub fn read_fixed_array_chunks( chunk_dimensions.iter().map(|&d| d as u64).product::() * element_size as u64; let mut chunks = Vec::new(); - let push_element = - |i: usize, abs: usize, chunks: &mut Vec| -> Result<(), FormatError> { - if let Some((address, chunk_size, filter_mask)) = parse_fa_element( - file_data, - abs, - header.client_id, - offset_size, - header.element_size, - chunk_byte_size, - )? { - // A slot beyond the current extent is ignored, as the - // library does. - let Some(offsets) = grid.offsets(i as u64) else { - return Ok(()); - }; - chunks.push(ChunkInfo { - chunk_size, - filter_mask, - offsets, - address, - }); - } - Ok(()) - }; + // `rel` is relative to the data block, whose bytes are in `w`. + let push_element = |w: &Window<'_>, + i: usize, + rel: usize, + chunks: &mut Vec| + -> Result<(), FormatError> { + if let Some((address, chunk_size, filter_mask)) = parse_fa_element( + w, + rel, + header.client_id, + offset_size, + header.element_size, + chunk_byte_size, + )? { + // A slot beyond the current extent is ignored, as the + // library does. + let Some(offsets) = grid.offsets(i as u64) else { + return Ok(()); + }; + chunks.push(ChunkInfo { + chunk_size, + filter_mask, + offsets, + address, + }); + } + Ok(()) + }; // A data block is paged when it holds more elements than fit in one page. // `max_nelmts_bits` is an untrusted u8; a shift >= the pointer width would @@ -246,10 +275,13 @@ pub fn read_fixed_array_chunks( if !is_paged { // Non-paged: prefix, then `num_elements` elements packed directly, - // then a checksum over both. - verify_checksum(file_data, db_offset, elem_at(elements_start, num_elements)?)?; + // then a checksum over both. One window holds all of it (or ends at + // the end of the file), so its bounds checks are the whole-file ones. + let end = elem_at(elements_start, num_elements)?; + let w = Window::read(file, db_offset as u64, end.saturating_add(4) - db_offset)?; + verify_checksum(&w, 0, end - db_offset)?; for i in 0..num_elements { - push_element(i, elem_at(elements_start, i)?, &mut chunks)?; + push_element(&w, i, elem_at(elements_start, i)? - db_offset, &mut chunks)?; } return Ok(chunks); } @@ -272,22 +304,27 @@ pub fn read_fixed_array_chunks( .and_then(|x| x.checked_add(4)) .ok_or_else(stride_overflow)?; - if bitmap_start + bitmap_size > file_data.len() { + if bitmap_start + bitmap_size > file_len { return Err(FormatError::UnexpectedEof { expected: bitmap_start + bitmap_size, - available: file_data.len(), + available: file_len, }); } + // The whole data block in one window: every page slot is at most + // `page_stride` bytes, so every position checked below lies inside it + // (or past the end of the file). + let block_len = (pages_start - db_offset).saturating_add(npages.saturating_mul(page_stride)); + let w = Window::read(file, db_offset as u64, block_len)?; // The prefix and page bitmap are covered by their own checksum, and each // initialised page by one of its own. - verify_checksum(file_data, db_offset, bitmap_start + bitmap_size)?; + verify_checksum(&w, 0, bitmap_start + bitmap_size - db_offset)?; for p in 0..npages { let page_first = p * page_nelmts; // < num_elements, cannot overflow let page_count = core::cmp::min(page_nelmts, num_elements - page_first); // Check the page-init bit (MSB-first within each byte). - let bit_byte = file_data[bitmap_start + p / 8]; + let bit_byte = w.bytes[bitmap_start + p / 8 - db_offset]; let bit_mask = 1u8 << (7 - (p % 8)); if bit_byte & bit_mask == 0 { continue; // entire page unallocated @@ -297,21 +334,30 @@ pub fn read_fixed_array_chunks( .checked_mul(page_stride) .and_then(|o| pages_start.checked_add(o)) .ok_or_else(stride_overflow)?; - verify_checksum(file_data, page_off, elem_at(page_off, page_count)?)?; + verify_checksum( + &w, + page_off - db_offset, + elem_at(page_off, page_count)? - db_offset, + )?; for e in 0..page_count { - push_element(page_first + e, elem_at(page_off, e)?, &mut chunks)?; + push_element( + &w, + page_first + e, + elem_at(page_off, e)? - db_offset, + &mut chunks, + )?; } } Ok(chunks) } -/// Parse a single Fixed Array element at absolute file offset `abs`. +/// Parse a single Fixed Array element at offset `abs` of the window `w`. /// /// Returns `Some((address, chunk_size, filter_mask))` for an allocated chunk, or /// `None` if the element is undefined (an unallocated chunk, address all-`0xFF`). fn parse_fa_element( - file_data: &[u8], + w: &Window<'_>, abs: usize, client_id: u8, offset_size: u8, @@ -321,12 +367,8 @@ fn parse_fa_element( let os = offset_size as usize; if client_id == 0 { // Non-filtered: element is just the chunk address. - if abs + os > file_data.len() { - return Err(FormatError::UnexpectedEof { - expected: abs + os, - available: file_data.len(), - }); - } + w.ensure(abs, os)?; + let file_data: &[u8] = &w.bytes; if is_undefined(file_data, abs, offset_size) { return Ok(None); } @@ -341,17 +383,14 @@ fn parse_fa_element( )); } let chunk_size_bytes = es - os - 4; - if abs + es > file_data.len() { - return Err(FormatError::UnexpectedEof { - expected: abs + es, - available: file_data.len(), - }); - } + w.ensure(abs, es)?; + let file_data: &[u8] = &w.bytes; if is_undefined(file_data, abs, offset_size) { return Ok(None); } let address = read_offset(file_data, abs, offset_size)?; - let chunk_size = read_variable_length(&file_data[abs + os..], chunk_size_bytes)?; + let chunk_size = + read_variable_length(&file_data[abs + os..abs + es - 4], chunk_size_bytes)?; let fm_off = abs + os + chunk_size_bytes; let filter_mask = u32::from_le_bytes([ file_data[fm_off], @@ -813,4 +852,101 @@ mod tests { .collect(); assert_eq!(got, expect); } + + /// A fixed array (header at 0x100, data block at 0x200) of `n` chunks, + /// filtered or not, paged when `n` exceeds `1 << page_bits`; every + /// page initialised except page 1. + fn build_fixed_array(n: usize, filtered: bool, page_bits: u8) -> Vec { + let os = 8usize; + let es = if filtered { os + 4 + 4 } else { os }; + let (fahd, db) = (0x100usize, 0x200usize); + let mut f = vec![0u8; 0x2000]; + f[fahd..fahd + 4].copy_from_slice(b"FAHD"); + f[fahd + 5] = u8::from(filtered); + f[fahd + 6] = es as u8; + f[fahd + 7] = page_bits; + f[fahd + 8..fahd + 16].copy_from_slice(&(n as u64).to_le_bytes()); + f[fahd + 16..fahd + 24].copy_from_slice(&(db as u64).to_le_bytes()); + stamp_checksum(&mut f, fahd, fahd + 24); + f[db..db + 4].copy_from_slice(b"FADB"); + f[db + 5] = u8::from(filtered); + f[db + 6..db + 14].copy_from_slice(&(fahd as u64).to_le_bytes()); + let elems = db + 6 + os; + let write = |f: &mut Vec, at: usize, i: usize| { + let addr = if i == 2 { + u64::MAX + } else { + 0x1000 + i as u64 * 0x100 + }; + f[at..at + os].copy_from_slice(&addr.to_le_bytes()); + if filtered { + f[at + os..at + os + 4].copy_from_slice(&(100 + i as u32).to_le_bytes()); + f[at + os + 4..at + os + 8].copy_from_slice(&(i as u32 & 1).to_le_bytes()); + } + }; + let page = 1usize << page_bits; + if n <= page { + for i in 0..n { + write(&mut f, elems + i * es, i); + } + stamp_checksum(&mut f, db, elems + n * es); + } else { + let npages = n.div_ceil(page); + let bitmap = npages.div_ceil(8); + for p in 0..npages { + if p != 1 { + f[elems + p / 8] |= 0x80 >> (p % 8); + } + } + stamp_checksum(&mut f, db, elems + bitmap); + let pages_start = elems + bitmap + 4; + for p in (0..npages).filter(|&p| p != 1) { + let at = pages_start + p * (page * es + 4); + let count = page.min(n - p * page); + for e in 0..count { + write(&mut f, at + e * es, p * page + e); + } + stamp_checksum(&mut f, at, at + count * es); + } + } + f + } + + /// Non-paged and paged, filtered and unfiltered arrays, cut at every + /// length through the data block and with a damaged byte, read + /// identically through a `read_at`-only storage. + #[test] + fn storage_reads_match_slice_reads() { + use crate::storage::CountingStorage; + for (n, filtered, bits) in [(3, false, 10), (3, true, 10), (11, false, 2), (11, true, 2)] { + let full = build_fixed_array(n, filtered, bits); + let es = if filtered { 16 } else { 8 }; + let dims = [n as u64 * 20]; + let h = FixedArrayHeader::parse(&full, 0x100, 8, 8).unwrap(); + let chunks = read_fixed_array_chunks(&full, &h, &dims, None, &[20], 8, 8, 8).unwrap(); + // Chunk 2 is unallocated, and so is page 1 of a paged array. + let expect = if n > 4 { n - 1 - 4 } else { n - 1 }; + assert_eq!(chunks.len(), expect); + let mut files = Vec::new(); + for cut in (0x100..0x200 + 40 + n * (es + 4) + 16).step_by(3) { + files.push(full[..cut].to_vec()); + } + for at in [0x104, 0x210, 0x21a, 0x230] { + let mut damaged = full.clone(); + damaged[at] ^= 1; + files.push(damaged); + } + files.push(full); + for f in files { + let storage = CountingStorage::new(f.clone()); + let want = FixedArrayHeader::parse(&f, 0x100, 8, 8); + let got = FixedArrayHeader::parse_in(&storage, 0x100, 8, 8); + assert_eq!(format!("{got:?}"), format!("{want:?}")); + let Ok(h) = want else { continue }; + let want = read_fixed_array_chunks(&f, &h, &dims, None, &[20], 8, 8, 8); + let got = read_fixed_array_chunks_in(&storage, &h, &dims, None, &[20], 8, 8, 8); + assert_eq!(format!("{got:?}"), format!("{want:?}"), "{} bytes", f.len()); + } + } + } }