From 6e84f31ed606d1fbd6a537b572f2266a46a0fa34 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 19 Sep 2026 06:13:39 -0700 Subject: [PATCH] fix(format): overflow-checked sizes and fallible allocation on chunked reads Dataspace and chunk dimensions are untrusted 64-bit fields, but the chunked read paths computed `num_elements() as usize * elem_size` and `chunk_dims.product() * elem_size` with plain arithmetic and fed the result to `vec![0u8; n]`. A crafted file could wrap the product (under-sizing the output buffer that chunks are then copied into) or request an allocation large enough to abort the process. - Dataspace::checked_num_elements, checked_byte_len, checked_chunk_byte_len and alloc_output (try_reserve_exact) replace the plain products and vec![0; n] at every chunked read site, plus the VDS and hyperslab paths. Overflow and allocation failure are FormatError::Overflow. - Dataspace::num_elements saturates instead of wrapping. - A zero-element dataset returns early, which also keeps the stride products in range when another dimension is huge. - parallel_read.rs: the three `c_addr + size > len` bounds checks used a raw add; they now use checked_add like the rest of the crate. Co-Authored-By: Claude Fable 5.1 --- crates/clawhdf5-format/src/chunked_read.rs | 146 +++++++++++++++++--- crates/clawhdf5-format/src/data_read.rs | 16 ++- crates/clawhdf5-format/src/dataspace.rs | 30 +++- crates/clawhdf5-format/src/parallel_read.rs | 21 ++- 4 files changed, 181 insertions(+), 32 deletions(-) diff --git a/crates/clawhdf5-format/src/chunked_read.rs b/crates/clawhdf5-format/src/chunked_read.rs index 43184a3..edb1705 100644 --- a/crates/clawhdf5-format/src/chunked_read.rs +++ b/crates/clawhdf5-format/src/chunked_read.rs @@ -132,6 +132,47 @@ fn ensure_len(data: &[u8], offset: usize, needed: usize) -> Result<(), FormatErr Ok(()) } +/// `elements * elem_size` for sizes that come from the file. Dataspace and +/// chunk dimensions are untrusted 64-bit fields, so a crafted file can make +/// the plain product wrap to a small number (or to something enormous). +pub(crate) fn checked_byte_len(elements: u64, elem_size: usize) -> Result { + usize::try_from(elements) + .ok() + .and_then(|n| n.checked_mul(elem_size)) + .ok_or_else(|| { + FormatError::Overflow(format!( + "{elements} elements of {elem_size} bytes exceeds the addressable size" + )) + }) +} + +/// Product of chunk dimensions times the element size, overflow-checked. +pub(crate) fn checked_chunk_byte_len( + chunk_dims: &[usize], + elem_size: usize, +) -> Result { + chunk_dims + .iter() + .try_fold(elem_size, |acc, &d| acc.checked_mul(d)) + .ok_or_else(|| { + FormatError::Overflow(format!( + "chunk dimensions {chunk_dims:?} x {elem_size} bytes exceeds the addressable size" + )) + }) +} + +/// A zero-filled output buffer of `len` bytes. `vec![0; len]` aborts the +/// process when the allocation fails; a size taken from the file must surface +/// as an error instead. +pub(crate) fn alloc_output(len: usize) -> Result, FormatError> { + let mut out = Vec::new(); + out.try_reserve_exact(len).map_err(|_| { + FormatError::Overflow(format!("cannot allocate {len} bytes for dataset output")) + })?; + out.resize(len, 0); + Ok(out) +} + fn read_offset(data: &[u8], pos: usize, size: u8) -> Result { let s = size as usize; if pos.checked_add(s).is_none_or(|end| end > data.len()) { @@ -393,7 +434,7 @@ pub fn read_chunked_data( } (4, Some(1)) => { // Single chunk — one chunk covering the entire dataset - let chunk_byte_size: usize = chunk_dims.iter().product::() * elem_size; + let chunk_byte_size = checked_chunk_byte_len(&chunk_dims, elem_size)?; let (csize, fmask) = if let Some(fs) = single_filtered_size { (fs as u32, single_filter_mask.unwrap_or(0)) } else { @@ -454,9 +495,13 @@ pub fn read_chunked_data( }; // Assemble output - let total_elements = dataspace.num_elements() as usize; - let total_bytes = total_elements * elem_size; - let mut output = vec![0u8; total_bytes]; + let total_bytes = checked_byte_len(dataspace.checked_num_elements()?, elem_size)?; + if total_bytes == 0 { + // Also keeps the stride products below in range: with a zero-sized + // dimension the total is 0 even if other dimensions are huge. + return Ok(Vec::new()); + } + let mut output = alloc_output(total_bytes)?; let mut ds_strides = vec![1usize; rank]; for i in (0..rank.saturating_sub(1)).rev() { @@ -468,8 +513,7 @@ pub fn read_chunked_data( chunk_strides[i] = chunk_strides[i + 1] * chunk_dims[i + 1]; } - let chunk_total_elements: usize = chunk_dims.iter().product(); - let chunk_total_bytes = chunk_total_elements * elem_size; + let chunk_total_bytes = checked_chunk_byte_len(&chunk_dims, elem_size)?; // Fast path: no filters — copy directly from file_data without intermediate alloc if pipeline.is_none() { @@ -623,7 +667,7 @@ pub fn read_chunked_data_cached( let chunks = match (version, chunk_index_type) { (3, _) => collect_chunk_info(file_data, addr, ndims, offset_size, length_size)?, (4, Some(1)) => { - let chunk_byte_size: usize = chunk_dims.iter().product::() * elem_size; + let chunk_byte_size = checked_chunk_byte_len(&chunk_dims, elem_size)?; let (csize, fmask) = if let Some(fs) = single_filtered_size { (fs as u32, single_filter_mask.unwrap_or(0)) } else { @@ -689,9 +733,13 @@ pub fn read_chunked_data_cached( let chunks = cache.all_indexed_chunks().unwrap_or_default(); // Assemble output - let total_elements = dataspace.num_elements() as usize; - let total_bytes = total_elements * elem_size; - let mut output = vec![0u8; total_bytes]; + let total_bytes = checked_byte_len(dataspace.checked_num_elements()?, elem_size)?; + if total_bytes == 0 { + // Also keeps the stride products below in range: with a zero-sized + // dimension the total is 0 even if other dimensions are huge. + return Ok(Vec::new()); + } + let mut output = alloc_output(total_bytes)?; let mut ds_strides = vec![1usize; rank]; for i in (0..rank.saturating_sub(1)).rev() { @@ -703,8 +751,7 @@ pub fn read_chunked_data_cached( chunk_strides[i] = chunk_strides[i + 1] * chunk_dims[i + 1]; } - let chunk_total_elements: usize = chunk_dims.iter().product(); - let chunk_total_bytes = chunk_total_elements * elem_size; + let chunk_total_bytes = checked_chunk_byte_len(&chunk_dims, elem_size)?; for chunk_info in &chunks { let coord: Vec = chunk_info.offsets.iter().take(rank).copied().collect(); @@ -976,7 +1023,7 @@ pub fn read_chunked_data_sweep( let chunks = match (version, chunk_index_type) { (3, _) => collect_chunk_info(file_data, addr, ndims, offset_size, length_size)?, (4, Some(1)) => { - let chunk_byte_size: usize = chunk_dims.iter().product::() * elem_size; + let chunk_byte_size = checked_chunk_byte_len(&chunk_dims, elem_size)?; let (csize, fmask) = if let Some(fs) = single_filtered_size { (fs as u32, single_filter_mask.unwrap_or(0)) } else { @@ -1042,9 +1089,13 @@ pub fn read_chunked_data_sweep( let chunks = cache.all_indexed_chunks().unwrap_or_default(); // Assemble output - let total_elements = dataspace.num_elements() as usize; - let total_bytes = total_elements * elem_size; - let mut output = vec![0u8; total_bytes]; + let total_bytes = checked_byte_len(dataspace.checked_num_elements()?, elem_size)?; + if total_bytes == 0 { + // Also keeps the stride products below in range: with a zero-sized + // dimension the total is 0 even if other dimensions are huge. + return Ok(Vec::new()); + } + let mut output = alloc_output(total_bytes)?; let mut ds_strides = vec![1usize; rank]; for i in (0..rank.saturating_sub(1)).rev() { @@ -1056,8 +1107,7 @@ pub fn read_chunked_data_sweep( chunk_strides[i] = chunk_strides[i + 1] * chunk_dims[i + 1]; } - let chunk_total_elements: usize = chunk_dims.iter().product(); - let chunk_total_bytes = chunk_total_elements * elem_size; + let chunk_total_bytes = checked_chunk_byte_len(&chunk_dims, elem_size)?; for chunk_info in &chunks { let coord: Vec = chunk_info.offsets.iter().take(rank).copied().collect(); @@ -1199,7 +1249,7 @@ pub fn read_chunked_data_indexed( let chunks = match (version, chunk_index_type) { (3, _) => collect_chunk_info(file_data, addr, ndims, offset_size, length_size)?, (4, Some(1)) => { - let chunk_byte_size: usize = chunk_dims.iter().product::() * elem_size; + let chunk_byte_size = checked_chunk_byte_len(&chunk_dims, elem_size)?; let (csize, fmask) = if let Some(fs) = single_filtered_size { (fs as u32, single_filter_mask.unwrap_or(0)) } else { @@ -1463,6 +1513,64 @@ fn copy_chunk_to_output( mod tests { use super::*; + fn simple_space(dimensions: Vec) -> Dataspace { + Dataspace { + space_type: crate::dataspace::DataspaceType::Simple, + rank: dimensions.len() as u8, + dimensions, + max_dimensions: None, + } + } + + #[test] + fn crafted_dimensions_are_errors_not_wraparound() { + // 2^63 * 2 wraps to 0 with a plain product; 2^40 * 2^40 wraps too. + for dims in [ + vec![1u64 << 63, 2], + vec![1 << 40, 1 << 40], + vec![u64::MAX, u64::MAX], + ] { + let space = simple_space(dims.clone()); + assert!( + matches!(space.checked_num_elements(), Err(FormatError::Overflow(_))), + "{dims:?}" + ); + // The infallible accessor saturates instead of wrapping. + assert_eq!(space.num_elements(), u64::MAX, "{dims:?}"); + } + assert_eq!(simple_space(vec![3, 4]).checked_num_elements().unwrap(), 12); + // A zero-sized dimension makes the whole product 0, not an overflow. + assert_eq!( + simple_space(vec![0, 1 << 40, 1 << 40]) + .checked_num_elements() + .unwrap(), + 0 + ); + } + + #[test] + fn byte_length_helpers_check_overflow() { + assert_eq!(checked_byte_len(10, 8).unwrap(), 80); + assert!(matches!( + checked_byte_len(u64::MAX, 8), + Err(FormatError::Overflow(_)) + )); + assert_eq!(checked_chunk_byte_len(&[10, 10], 4).unwrap(), 400); + assert!(matches!( + checked_chunk_byte_len(&[usize::MAX, 2], 4), + Err(FormatError::Overflow(_)) + )); + } + + #[test] + fn unallocatable_output_is_an_error_not_an_abort() { + assert_eq!(alloc_output(16).unwrap(), vec![0u8; 16]); + assert!(matches!( + alloc_output(usize::MAX / 2), + Err(FormatError::Overflow(_)) + )); + } + fn write_offset(buf: &mut Vec, val: u64, size: u8) { match size { 4 => buf.extend_from_slice(&(val as u32).to_le_bytes()), diff --git a/crates/clawhdf5-format/src/data_read.rs b/crates/clawhdf5-format/src/data_read.rs index a469084..9f33175 100644 --- a/crates/clawhdf5-format/src/data_read.rs +++ b/crates/clawhdf5-format/src/data_read.rs @@ -475,8 +475,10 @@ fn read_virtual_data( use crate::selection::Selection; let elem_size = datatype.type_size() as usize; - let total_elems = dataspace.num_elements() as usize; - let mut out = vec![0u8; total_elems.saturating_mul(elem_size)]; + let mut out = crate::chunked_read::alloc_output(crate::chunked_read::checked_byte_len( + dataspace.checked_num_elements()?, + elem_size, + )?)?; let virtual_dims = &dataspace.dimensions; @@ -616,12 +618,14 @@ fn extract_selection_from_buffer( block, } => { let rank = dims.len(); - let output_elements: usize = count + let output_elements = count .iter() .zip(block.iter()) - .map(|(&c, &b)| (c * b) as usize) - .product(); - let mut output = vec![0u8; output_elements * elem_size]; + .try_fold(1u64, |acc, (&c, &b)| acc.checked_mul(c.checked_mul(b)?)) + .ok_or_else(|| FormatError::Overflow("hyperslab count x block overflows".into()))?; + let mut output = crate::chunked_read::alloc_output( + crate::chunked_read::checked_byte_len(output_elements, elem_size)?, + )?; // Compute dataset strides (row-major) let mut ds_strides = vec![1usize; rank]; diff --git a/crates/clawhdf5-format/src/dataspace.rs b/crates/clawhdf5-format/src/dataspace.rs index efece6d..a08ca9d 100644 --- a/crates/clawhdf5-format/src/dataspace.rs +++ b/crates/clawhdf5-format/src/dataspace.rs @@ -1,5 +1,7 @@ //! HDF5 Dataspace message parsing (message type 0x0001). +#[cfg(not(feature = "std"))] +use alloc::format; #[cfg(not(feature = "std"))] use alloc::vec::Vec; @@ -167,6 +169,27 @@ impl Dataspace { } } + /// [`Dataspace::num_elements`] with the product overflow-checked. The + /// dimensions are untrusted 64-bit fields; read paths that size a buffer + /// from them must use this one. + pub fn checked_num_elements(&self) -> Result { + match self.space_type { + DataspaceType::Null => Ok(0), + DataspaceType::Scalar => Ok(1), + DataspaceType::Simple if self.dimensions.is_empty() => Ok(0), + DataspaceType::Simple => self + .dimensions + .iter() + .try_fold(1u64, |acc, &d| acc.checked_mul(d)) + .ok_or_else(|| { + FormatError::Overflow(format!( + "dataspace dimensions {:?} overflow the element count", + self.dimensions + )) + }), + } + } + /// Total number of elements. Scalar = 1, Null = 0. pub fn num_elements(&self) -> u64 { match self.space_type { @@ -176,7 +199,12 @@ impl Dataspace { if self.dimensions.is_empty() { 0 } else { - self.dimensions.iter().product() + // Saturate rather than wrap: a wrapped product could + // under-size a buffer. Size-critical callers use + // `checked_num_elements`. + self.dimensions + .iter() + .fold(1u64, |acc, &d| acc.saturating_mul(d)) } } } diff --git a/crates/clawhdf5-format/src/parallel_read.rs b/crates/clawhdf5-format/src/parallel_read.rs index c294203..14f5613 100644 --- a/crates/clawhdf5-format/src/parallel_read.rs +++ b/crates/clawhdf5-format/src/parallel_read.rs @@ -73,9 +73,12 @@ pub fn decompress_chunks_lane_partitioned( let c_addr = chunk_info.address as usize; let size = chunk_info.chunk_size as usize; - if c_addr + size > file_data.len() { + if c_addr + .checked_add(size) + .is_none_or(|end| end > file_data.len()) + { return Err(FormatError::UnexpectedEof { - expected: c_addr + size, + expected: c_addr.saturating_add(size), available: file_data.len(), }); } @@ -144,9 +147,12 @@ pub fn decompress_chunks_parallel( .map(|(index, chunk_info)| { let c_addr = chunk_info.address as usize; let size = chunk_info.chunk_size as usize; - if c_addr + size > file_data.len() { + if c_addr + .checked_add(size) + .is_none_or(|end| end > file_data.len()) + { return Err(FormatError::UnexpectedEof { - expected: c_addr + size, + expected: c_addr.saturating_add(size), available: file_data.len(), }); } @@ -182,9 +188,12 @@ pub fn decompress_chunks_sequential( for chunk_info in chunks { let c_addr = chunk_info.address as usize; let size = chunk_info.chunk_size as usize; - if c_addr + size > file_data.len() { + if c_addr + .checked_add(size) + .is_none_or(|end| end > file_data.len()) + { return Err(FormatError::UnexpectedEof { - expected: c_addr + size, + expected: c_addr.saturating_add(size), available: file_data.len(), }); }