diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ec2f47..e4ea2a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,61 @@ # Changelog +## Unreleased + +### Correctness +- `clawhdf5-format`: **datasets indexed by an Extensible Array returned wrong + data beyond their first few dozen chunks.** One unlimited dimension gives a + dataset an Extensible Array chunk index, whose first elements (4 by default) + sit inline in the index block and whose rest live in data blocks sized by a + formula the reader got wrong. In the default layout everything through the + 36th chunk happened to line up and the 37th onwards did not: a 400-chunk + dataset silently returned wrong values from chunk 37, and datasets past + about a thousand chunks failed outright with "invalid Extensible Array data + block signature". **Reads were wrong, not + merely refused** — the caller got plausible numbers from the wrong chunks. + Four separate layout errors, each checked against files written by HDF5 2.0 + and against the library source: + - the number of data blocks in super block `u` is `2^(u/2)`, not `2^u`; + - each holds `2^((u+1)/2) * data_blk_min_elmts` elements, which doubles + every *other* level rather than every level; + - a super block carries a block-offset field before its data block + addresses, which was not skipped; + - the page-init bitmap belongs to the super block, one bit per page packed + across all its data blocks (MSB first), and was being read from inside the + data block instead; a paged data block also ends its prefix with a + checksum before the first page. + Covered now by interop tests at 4, 37, 400, 5 000 and 200 000 chunks (the + last large enough for paged data blocks), plus sparse, gzip-filtered and + 2-D cases. Writing is unaffected; this is a read-path bug. +- `clawhdf5-format`: the sibling Fixed Array index (fixed dimensions written + with `libver='latest'`) was checked against the same range and is correct, + including paged data blocks and sparse datasets — it really does keep its + page-init bitmap in the data block, where the Extensible Array does not. + It had no real-file coverage above the inline sizes either, so it now has + the same tests. + +### Security +- `clawhdf5-format`: **a crafted file could crash any reader through B-tree v2 + traversal.** Recursion was bounded only by the depth the file claimed (a + `u16`), and child addresses were never checked for sharing. A node listing + itself as its own child under a header claiming 65 535 levels — under 100 + bytes — overflowed the stack and **aborted the process** (SIGABRT, not a + catchable error). Levels whose children all point at one shared node below + reached it fan-out^depth times: 29.5 million records from ~5 KB, and one + more level would exhaust memory. Both are now errors, returned in under a + millisecond: depth is capped at 64 (as the fractal heap already was), and + traversal stops once it has produced more records than the file has bytes + to hold. Every B-tree v2 user goes through this path — dense attributes, + v2 groups, shared messages and chunk indexes. Valid files are unaffected, + including a depth-2 HDF5 2.0 chunk index with 40 000 records, now covered by + an interop test. + +### Documentation +- `clawhdf5-agent`: `BM25Index::search` claimed to use Block-Max WAND for early + termination. It never did; it scores every match exhaustively. It now says + so, and why no pruning would help the store: `hybrid_search` uses `scores()`, + since fusion normalises over every match. + ## v2.6.0 (2026-09-20) ### Upgrade Notes diff --git a/crates/clawhdf5-agent/src/bm25.rs b/crates/clawhdf5-agent/src/bm25.rs index 621a9f7..ba23cb0 100644 --- a/crates/clawhdf5-agent/src/bm25.rs +++ b/crates/clawhdf5-agent/src/bm25.rs @@ -88,8 +88,11 @@ impl BM25Index { /// Search the index for a query, returning the top `k` results /// as `(doc_id, score)` pairs sorted by score descending. /// - /// Uses Block-Max WAND for early termination when remaining documents - /// cannot beat the current top-k threshold. + /// Scores every matching document exhaustively, then keeps the top `k`. + /// There is no early termination (WAND, MaxScore): the store's hot path + /// is [`scores`](Self::scores), because score fusion normalises over the + /// whole matching set and so needs every score, which no pruning scheme + /// can skip. This method is for BM25-only callers. pub fn search(&self, query: &str, k: usize) -> Vec<(usize, f32)> { if k == 0 { return Vec::new(); @@ -561,8 +564,9 @@ mod tests { } #[test] - fn wand_returns_same_results_as_exhaustive() { - // WAND-style search should produce same scores as exhaustive + fn top_k_search_matches_ranking_every_score() { + // `search` must agree with ranking the full `scores` set — the + // bounded heap is an optimisation over sorting, not an approximation. let docs: Vec = (0..100) .map(|i| { if i % 3 == 0 { diff --git a/crates/clawhdf5-format/fuzz/.gitignore b/crates/clawhdf5-format/fuzz/.gitignore index 2f7896d..fe68c97 100644 --- a/crates/clawhdf5-format/fuzz/.gitignore +++ b/crates/clawhdf5-format/fuzz/.gitignore @@ -1 +1,4 @@ target/ +corpus/ +artifacts/ +coverage/ diff --git a/crates/clawhdf5-format/fuzz/fuzz_targets/fuzz_btree_v2.rs b/crates/clawhdf5-format/fuzz/fuzz_targets/fuzz_btree_v2.rs index 370b2d8..a33b4af 100644 --- a/crates/clawhdf5-format/fuzz/fuzz_targets/fuzz_btree_v2.rs +++ b/crates/clawhdf5-format/fuzz/fuzz_targets/fuzz_btree_v2.rs @@ -1,15 +1,36 @@ #![no_main] +use clawhdf5_format::btree_v2::{BTreeV2Header, collect_btree_v2_records}; use libfuzzer_sys::fuzz_target; fuzz_target!(|data: &[u8]| { for &offset_size in &[4u8, 8] { for &length_size in &[4u8, 8] { - let _ = clawhdf5_format::btree_v2::BTreeV2Header::parse( - data, - 0, - offset_size, - length_size, - ); + if let Ok(header) = BTreeV2Header::parse(data, 0, offset_size, length_size) { + let _ = collect_btree_v2_records(data, &header, offset_size, length_size); + } } } + + // Parsing a header requires a valid checksum, which random input almost + // never has, so the traversal behind it went unfuzzed — and that is where + // a node listing itself as its own child overflowed the stack. Take the + // header fields straight from the input instead and walk the rest. + let Some((fields, file)) = data.split_first_chunk::<20>() else { + return; + }; + let header = BTreeV2Header { + tree_type: fields[0], + node_size: u32::from_le_bytes([fields[1], fields[2], fields[3], fields[4]]), + record_size: u16::from_le_bytes([fields[5], fields[6]]), + depth: u16::from_le_bytes([fields[7], fields[8]]), + root_node_address: u64::from(u32::from_le_bytes([ + fields[9], fields[10], fields[11], fields[12], + ])), + num_records_in_root: u16::from_le_bytes([fields[13], fields[14]]), + total_records: u64::from(u32::from_le_bytes([ + fields[15], fields[16], fields[17], fields[18], + ])), + }; + let offset_size = if fields[19] & 1 == 0 { 4 } else { 8 }; + let _ = collect_btree_v2_records(file, &header, offset_size, 8); }); diff --git a/crates/clawhdf5-format/src/btree_v2.rs b/crates/clawhdf5-format/src/btree_v2.rs index ad58aa0..43fd7b0 100644 --- a/crates/clawhdf5-format/src/btree_v2.rs +++ b/crates/clawhdf5-format/src/btree_v2.rs @@ -172,6 +172,17 @@ fn max_records_leaf(node_size: u32, record_size: u16) -> u64 { ((node_size - overhead) / record_size as u32) as u64 } +/// Deepest B-tree v2 accepted. See [`collect_btree_v2_records`]. +const MAX_DEPTH: u16 = 64; + +/// Take `n` records from the traversal's budget, or refuse the tree. +fn spend(budget: &mut usize, n: usize) -> Result<(), FormatError> { + *budget = budget + .checked_sub(n) + .ok_or(FormatError::NestingDepthExceeded)?; + Ok(()) +} + /// Collect all records from a B-tree v2 by traversing from the root. pub fn collect_btree_v2_records( file_data: &[u8], @@ -182,6 +193,22 @@ pub fn collect_btree_v2_records( if header.total_records == 0 || header.num_records_in_root == 0 { return Ok(Vec::new()); } + // Recursion is one frame per level, and the depth is read from the file: + // a crafted header claiming 65 535 levels over a node that is its own + // child overflowed the stack. 64 matches the fractal heap's guard, and no + // real tree comes close — even at the minimum fan-out of two it would + // hold more than 2^64 records. + if header.depth > MAX_DEPTH { + return Err(FormatError::NestingDepthExceeded); + } + // A valid tree stores each record once, in its own bytes, so it cannot + // hold more records than the file has room for. Children are addresses, + // though, and nothing makes them distinct: levels whose children all + // point at one shared node below reach it fan-out^depth times, which is + // millions of records from a few kilobytes. Counting against what the + // file could physically contain bounds that without trusting the + // header's own `total_records`. + let mut budget = file_data.len() / usize::from(header.record_size.max(1)); let max_leaf_nrec = max_records_leaf(header.node_size, header.record_size); @@ -206,6 +233,7 @@ pub fn collect_btree_v2_records( offset_size, length_size, max_leaf_nrec, + &mut budget, &mut records, )?; Ok(records) @@ -273,6 +301,7 @@ fn collect_internal_records( offset_size: u8, length_size: u8, max_leaf_nrec: u64, + budget: &mut usize, out: &mut Vec, ) -> Result<(), FormatError> { // signature(4) + version(1) + type(1) = 6 @@ -350,6 +379,8 @@ fn collect_internal_records( // We collect child[0] records, then record[0], then child[1], etc. for (i, &(child_addr, child_nrec)) in children.iter().enumerate() { if child_depth == 0 { + // Before parsing, so a refused tree is not also a large allocation. + spend(budget, usize::from(child_nrec))?; let leaf_recs = parse_leaf_records(file_data, child_addr as usize, child_nrec, record_size)?; out.extend(leaf_recs); @@ -364,6 +395,7 @@ fn collect_internal_records( offset_size, length_size, max_leaf_nrec, + budget, out, )?; } @@ -393,6 +425,7 @@ fn collect_internal_records( available: file_data.len(), }); } + spend(budget, 1)?; out.push(BTreeV2Record { data: file_data[rec_start..rec_end].to_vec(), }); @@ -466,6 +499,124 @@ mod tests { buf } + /// An internal node laid out exactly as `collect_internal_records` will + /// read it at `depth`: `records` zeroed records, then `children` pointers, + /// all to `child_addr` claiming `child_nrec` records. + fn internal_node( + depth: u16, + node_size: u32, + record_size: u16, + records: usize, + children: usize, + child_addr: u64, + child_nrec: u64, + ) -> Vec { + let max_leaf = max_records_leaf(node_size, record_size); + let nrec_width = bytes_for_max_records(if depth == 1 { max_leaf } else { max_leaf * 2 }); + let total_width = if depth > 1 { + bytes_for_max_records(header_max_total_records(max_leaf, depth - 1)) + } else { + 0 + }; + let mut buf = b"BTIN".to_vec(); + buf.extend_from_slice(&[0, 5]); + buf.resize(buf.len() + records * record_size as usize, 0); + for _ in 0..children { + buf.extend_from_slice(&child_addr.to_le_bytes()); + buf.extend_from_slice(&child_nrec.to_le_bytes()[..nrec_width]); + buf.resize(buf.len() + total_width, 0); + } + buf + } + + fn header(depth: u16, root: u64, root_nrec: u16, total: u64) -> BTreeV2Header { + BTreeV2Header { + tree_type: 5, + node_size: 512, + record_size: 8, + depth, + root_node_address: root, + num_records_in_root: root_nrec, + total_records: total, + } + } + + #[test] + fn a_node_that_is_its_own_child_is_rejected_not_recursed() { + // One internal node whose two children are itself, under a header + // claiming the deepest tree a u16 allows. The layout stops depending + // on depth once the subtree-total width saturates, so every level + // parses cleanly and recursion runs ~65 000 frames deep: before the + // cap this overflowed the stack and aborted the process, from a file + // of under 100 bytes. + let mut data = internal_node(u16::MAX, 512, 8, 1, 2, 0, 1); + data.resize(4096, 0); + let result = collect_btree_v2_records(&data, &header(u16::MAX, 0, 1, 1), 8, 8); + assert!(result.is_err(), "{result:?}"); + } + + #[test] + fn a_shared_subtree_cannot_multiply_the_work() { + // A chain of distinct levels, each node's children all pointing at the + // single node below, ending in a real leaf. Every node parses and + // nothing is cyclic, yet the leaf is reached fan-out^depth times: 62 + // children over 4 levels is ~15 million leaf visits from a few + // kilobytes. A valid tree cannot hold more records than the file has + // room for, so that bounds the traversal instead. + let (node_size, record_size) = (512u32, 8u16); + let fanout = 62usize; + let depth = 4u16; + let leaf = build_leaf_node(5, &[&[0u8; 8][..]]); + + // Lay out root first, then each lower level, then the leaf. + let mut nodes: Vec> = Vec::new(); + let mut addrs = Vec::new(); + let mut at = 0u64; + let mut sizes = Vec::new(); + for d in (1..=depth).rev() { + let n = internal_node(d, node_size, record_size, fanout - 1, fanout, 0, 0); + sizes.push(n.len()); + } + for size in &sizes { + addrs.push(at); + at += *size as u64; + } + let leaf_addr = at; + for (i, d) in (1..=depth).rev().enumerate() { + let (child, child_nrec) = if d == 1 { + (leaf_addr, 1) + } else { + (addrs[i + 1], fanout as u64 - 1) + }; + nodes.push(internal_node( + d, + node_size, + record_size, + fanout - 1, + fanout, + child, + child_nrec, + )); + } + let mut data: Vec = nodes.concat(); + data.extend_from_slice(&leaf); + data.resize(data.len() + 64, 0); + + let started = std::time::Instant::now(); + let result = + collect_btree_v2_records(&data, &header(depth, 0, fanout as u16 - 1, u64::MAX), 8, 8); + assert!( + result.is_err(), + "expected a refusal, got {} records", + result.map_or(0, |r| r.len()) + ); + assert!( + started.elapsed() < std::time::Duration::from_secs(2), + "took {:?}", + started.elapsed() + ); + } + #[test] fn parse_header() { let data = build_btree_v2_header(5, 512, 11, 0, 0x1000, 3, 3, 8, 8); diff --git a/crates/clawhdf5-format/src/extensible_array.rs b/crates/clawhdf5-format/src/extensible_array.rs index 84add79..c3665d2 100644 --- a/crates/clawhdf5-format/src/extensible_array.rs +++ b/crates/clawhdf5-format/src/extensible_array.rs @@ -270,6 +270,40 @@ fn index_to_chunk_offsets( /// Collect elements from a data block at the given offset. #[allow(clippy::too_many_arguments)] +/// Layout of super block `u`, per the HDF5 spec: the number of data blocks it +/// owns and how many elements each of them holds. +/// +/// `ndblks` and `dblk_nelmts` each double every *other* level, a half-step +/// apart, so the blocks grow as 1x16, 1x32, 2x32, 2x64, 4x64 ... for a +/// 16-element minimum. Treating either as doubling every level (the previous +/// implementation) puts every element after the first data block at the wrong +/// index. +fn sblk_info(u: usize, data_blk_min_elmts: usize) -> Option<(usize, usize)> { + let ndblks = 1usize.checked_shl((u / 2) as u32)?; + let dblk_nelmts = 1usize + .checked_shl(u.div_ceil(2) as u32)? + .checked_mul(data_blk_min_elmts)?; + Some((ndblks, dblk_nelmts)) +} + +/// Width of the "offset of the block in the array" field carried by super and +/// data blocks (`hdr->arr_off_size`). +fn arr_off_size(header: &ExtensibleArrayHeader) -> usize { + (header.max_nelmts_bits as usize).div_ceil(8) +} + +/// Elements per data block page, once a data block is large enough to be paged. +fn page_nelmts(header: &ExtensibleArrayHeader) -> Option { + 1usize.checked_shl(u32::from(header.max_dblk_nelmts_bits)) +} + +/// Read the elements of one data block (EADB). +/// +/// `page_init` is the owning super block's page-init bitmap and `first_page` +/// this block's first bit in it; both are only consulted when the block is +/// paged. The bitmap lives in the super block, not here — a paged data block +/// stores only its prefix, then one slot per page. +#[allow(clippy::too_many_arguments)] fn read_data_block_elements( file_data: &[u8], db_offset: usize, @@ -280,117 +314,86 @@ fn read_data_block_elements( start_index: usize, num_chunks_per_dim: &[u64], chunk_dimensions: &[u32], + page_init: &[u8], + first_page: usize, ) -> Result, FormatError> { - // AEDB: signature(4) + version(1) + client_id(1) + header_address(offset_size) - let db_header_size = 4 + 1 + 1 + offset_size as usize; + // EADB: signature(4) + version(1) + client_id(1) + header_address(offset_size) + // + block offset(arr_off_size) + let db_header_size = 4 + 1 + 1 + offset_size as usize + arr_off_size(header); ensure_len(file_data, db_offset, db_header_size)?; - let d = &file_data[db_offset..]; - if &d[0..4] != b"EADB" { + if &file_data[db_offset..db_offset + 4] != b"EADB" { return Err(FormatError::ChunkedReadError( "invalid Extensible Array data block signature".into(), )); } - // Skip version(1) + client_id(1) + header_address(offset_size) + block_offset - // Block offset is encoded in ceil(max_nelmts_bits/8) bytes - let blk_off_size = (header.max_nelmts_bits as usize).div_ceil(8); - let mut pos = db_offset + db_header_size + blk_off_size; - // Check if paged - if header.max_nelmts_bits >= usize::BITS as u8 { - return Err(FormatError::Overflow( - "max_nelmts_bits exceeds usize bit width".into(), - )); - } - let page_nelmts = 1usize << header.max_nelmts_bits; - let is_paged = nelmts > page_nelmts; + let mut pos = db_offset + db_header_size; + let page = page_nelmts(header).ok_or_else(|| { + FormatError::Overflow("Extensible Array page element count overflows usize".into()) + })?; let mut chunks = Vec::new(); - - if !is_paged { - for i in 0..nelmts { + let read_run = |from: usize, + count: usize, + first_index: usize, + chunks: &mut Vec| + -> Result { + let mut p = from; + for i in 0..count { let (info, consumed) = read_element( file_data, - pos, + p, header.client_id, header.element_size, offset_size, chunk_byte_size, - start_index + i, + first_index + i, num_chunks_per_dim, chunk_dimensions, )?; if let Some(ci) = info { chunks.push(ci); } - pos += consumed; + p += consumed; } + Ok(p) + }; + + if nelmts <= page { + read_run(pos, nelmts, start_index, &mut chunks)?; + return Ok(chunks); + } + + // Paged: the prefix ends with its own checksum, then one slot per page, + // each holding `page` elements followed by a checksum. Pages whose bit is + // clear were never written; their slot still occupies the file, so stride + // over it rather than reading zeros as addresses. + pos += 4; + let elem_bytes = if header.client_id == 0 { + offset_size as usize } else { - // Paged: elements are split into pages of page_nelmts. - // After the data block header comes a page bitmap, then each page - // has page_nelmts elements followed by a 4-byte checksum. - let npages = nelmts.div_ceil(page_nelmts); - // Page bitmap: ceil(npages / 8) bytes - let bitmap_size = npages.div_ceil(8); - // Read bitmap - if pos + bitmap_size > file_data.len() { - return Err(FormatError::UnexpectedEof { - expected: pos + bitmap_size, - available: file_data.len(), - }); - } - let bitmap = &file_data[pos..pos + bitmap_size]; - pos += bitmap_size; - - let elem_bytes = if header.client_id == 0 { - offset_size as usize - } else { - header.element_size as usize - }; - - let mut global_idx = start_index; - for page_idx in 0..npages { - let byte_idx = page_idx / 8; - let bit_idx = page_idx % 8; - let page_has_data = (bitmap[byte_idx] >> bit_idx) & 1 != 0; - - let elems_this_page = if page_idx == npages - 1 { - let remainder = nelmts % page_nelmts; - if remainder == 0 { - page_nelmts - } else { - remainder - } - } else { - page_nelmts - }; - - if page_has_data { - for i in 0..elems_this_page { - let (info, consumed) = read_element( - file_data, - pos, - header.client_id, - header.element_size, - offset_size, - chunk_byte_size, - global_idx + i, - num_chunks_per_dim, - chunk_dimensions, - )?; - if let Some(ci) = info { - chunks.push(ci); - } - pos += consumed; - } - // Skip page checksum (4 bytes) - pos += 4; - } else { - // Empty page: skip all elements + checksum - pos += elems_this_page * elem_bytes + 4; - } - global_idx += elems_this_page; + header.element_size as usize + }; + let page_stride = page + .checked_mul(elem_bytes) + .and_then(|b| b.checked_add(4)) + .ok_or_else(|| FormatError::Overflow("Extensible Array page stride".into()))?; + let npages = nelmts.div_ceil(page); + for p in 0..npages { + // One bit per page across the whole super block, packed contiguously + // and MSB-first within each byte, as H5VM_bit_get reads it. + let bit = first_page + p; + let initialised = page_init + .get(bit / 8) + .is_some_and(|byte| byte & (0x80 >> (bit % 8)) != 0); + if initialised { + let count = core::cmp::min(page, nelmts - p * page); + read_run(pos, count, start_index + p * page, &mut chunks)?; } + pos = pos + .checked_add(page_stride) + .ok_or_else(|| FormatError::Overflow("Extensible Array page offset".into()))?; } Ok(chunks) @@ -427,30 +430,26 @@ pub fn read_extensible_array_chunks( let chunk_byte_size: u64 = chunk_dimensions.iter().map(|&d| d as u64).product::() * element_size as u64; - // Parse index block (AEIB) + // Parse index block (EAIB): signature(4) + version(1) + client_id(1) + // + header address(offset_size), then the inline elements, then the + // direct data block addresses, then the super block addresses. let ib_offset = header.index_block_address as usize; - let ib_header_size = 4 + 1 + 1 + offset_size as usize; // sig + ver + client + hdr_addr + let ib_header_size = 4 + 1 + 1 + os; ensure_len(file_data, ib_offset, ib_header_size)?; - let ib = &file_data[ib_offset..]; - if &ib[0..4] != b"EAIB" { + if &file_data[ib_offset..ib_offset + 4] != b"EAIB" { return Err(FormatError::ChunkedReadError( "invalid Extensible Array index block signature".into(), )); } - // Skip version(1) + client_id(1) + header_address(offset_size) let mut pos = ib_offset + ib_header_size; let mut chunks = Vec::new(); - let mut global_index = 0usize; let total_elements = header.num_elements as usize; - // 1. Read inline elements in index block - let n_inline = header.idx_blk_elmts as usize; + // 1. Elements stored inline in the index block. + let n_inline = (header.idx_blk_elmts as usize).min(total_elements); for i in 0..n_inline { - if global_index + i >= total_elements { - break; - } let (info, consumed) = read_element( file_data, pos, @@ -458,7 +457,7 @@ pub fn read_extensible_array_chunks( header.element_size, offset_size, chunk_byte_size, - global_index + i, + i, &num_chunks_per_dim, chunk_dimensions, )?; @@ -467,154 +466,128 @@ pub fn read_extensible_array_chunks( } pos += consumed; } - global_index += n_inline.min(total_elements); - - // If all elements were inline, we're done + let mut global_index = n_inline; if global_index >= total_elements { return Ok(chunks); } - // Compute data block and super block counts - let min_dblk = header.min_dblk_nelmts as usize; - let sblk_min = header.super_blk_min_nelmts as usize; + let dmin = header.min_dblk_nelmts as usize; + if dmin == 0 || !dmin.is_power_of_two() { + return Err(FormatError::ChunkedReadError( + "Extensible Array data block minimum is not a power of two".into(), + )); + } + // nsblks = 1 + (max_nelmts_bits - log2(data_blk_min_elmts)), and the index + // block holds 2 * (sup_blk_min_data_ptrs - 1) data block addresses. + let log2_dmin = dmin.trailing_zeros() as usize; + let nsblks = 1 + (header.max_nelmts_bits as usize).saturating_sub(log2_dmin); + let ndblk_addrs = 2 * (header.super_blk_min_nelmts as usize).saturating_sub(1); - // The first sblk_min super block levels have their data blocks listed directly - // in the index block. Compute their sizes. - let mut n_direct_dblks = 0usize; - let mut dblk_sizes: Vec = Vec::new(); - { - let mut nelmts = min_dblk; - for sb_level in 0..sblk_min { - if sb_level >= usize::BITS as usize { - return Err(FormatError::Overflow( - "sb_level exceeds usize bit width".into(), + // 2. Data blocks listed directly in the index block: the first + // `ndblk_addrs` data blocks in super-block order, each sized by the + // level it belongs to. + let mut direct: Vec = Vec::with_capacity(ndblk_addrs); + let mut level = 0usize; + while direct.len() < ndblk_addrs { + let (ndblks, dblk_nelmts) = sblk_info(level, dmin).ok_or_else(|| { + FormatError::Overflow("Extensible Array super block layout overflows usize".into()) + })?; + if level >= nsblks { + return Err(FormatError::ChunkedReadError( + "Extensible Array index block claims more data blocks than the array has".into(), + )); + } + for _ in 0..ndblks { + direct.push(dblk_nelmts); + } + level += 1; + } + if direct.len() != ndblk_addrs { + // A partial level in the index block is not a layout HDF5 produces, + // and guessing where the super blocks resume would misplace elements. + return Err(FormatError::ChunkedReadError( + "Extensible Array index block ends mid super block".into(), + )); + } + + for &dblk_nelmts in &direct { + if global_index >= total_elements { + return Ok(chunks); + } + ensure_len(file_data, pos, os)?; + let addr = read_offset(file_data, pos, offset_size)?; + pos += os; + if !is_undefined_addr(addr, offset_size) { + if dblk_nelmts > page_nelmts(header).unwrap_or(usize::MAX) { + // Would need a page-init bitmap, which only a super block + // carries. HDF5 never pages these small early blocks. + return Err(FormatError::ChunkedReadError( + "Extensible Array index block references a paged data block".into(), )); } - let ndblks = 1usize << sb_level; - for _ in 0..ndblks { - dblk_sizes.push(nelmts); - n_direct_dblks += 1; - } - if sb_level > 0 { - nelmts *= 2; - } + chunks.extend(read_data_block_elements( + file_data, + addr as usize, + dblk_nelmts, + header, + offset_size, + chunk_byte_size, + global_index, + &num_chunks_per_dim, + chunk_dimensions, + &[], + 0, + )?); } + global_index += dblk_nelmts; } - // Read direct data block addresses from index block - let mut dblk_addrs: Vec = Vec::with_capacity(n_direct_dblks); - for _ in 0..n_direct_dblks { - if pos + os > file_data.len() { + // 3. Everything else lives in super blocks, one address per remaining + // level, starting at the level after the direct data blocks. + for u in level..nsblks { + if global_index >= total_elements { break; } - let addr = read_offset(file_data, pos, offset_size)?; - dblk_addrs.push(addr); + ensure_len(file_data, pos, os)?; + let sb_addr = read_offset(file_data, pos, offset_size)?; pos += os; - } - - // Read elements from direct data blocks - for (i, &addr) in dblk_addrs.iter().enumerate() { - if i >= dblk_sizes.len() { - break; + let (ndblks, dblk_nelmts) = sblk_info(u, dmin).ok_or_else(|| { + FormatError::Overflow("Extensible Array super block layout overflows usize".into()) + })?; + if !is_undefined_addr(sb_addr, offset_size) { + chunks.extend(read_super_block( + file_data, + sb_addr as usize, + ndblks, + dblk_nelmts, + header, + offset_size, + chunk_byte_size, + global_index, + &num_chunks_per_dim, + chunk_dimensions, + )?); } - let nelmts = dblk_sizes[i]; - if is_undefined_addr(addr, offset_size) { - global_index += nelmts; - continue; - } - let block_chunks = read_data_block_elements( - file_data, - addr as usize, - nelmts, - header, - offset_size, - chunk_byte_size, - global_index, - &num_chunks_per_dim, - chunk_dimensions, - )?; - chunks.extend(block_chunks); - global_index += nelmts; - } - - // Remaining elements are in super blocks - let total_in_ib_and_direct: usize = n_inline + dblk_sizes.iter().sum::(); - if total_elements <= total_in_ib_and_direct { - return Ok(chunks); - } - let remaining_elements = total_elements - total_in_ib_and_direct; - - // Compute super block layout - let mut sb_addrs: Vec = Vec::new(); - let mut sb_infos: Vec<(usize, usize)> = Vec::new(); - { - let mut covered = 0usize; - let mut sb_level = sblk_min; - let mut nelmts_per_dblk = min_dblk; - for lev in 0..sblk_min { - if lev > 0 { - nelmts_per_dblk *= 2; - } - } - - while covered < remaining_elements { - if sb_level >= usize::BITS as usize { - return Err(FormatError::Overflow( - "sb_level exceeds usize bit width".into(), - )); - } - let ndblks = 1usize << sb_level; - nelmts_per_dblk *= 2; - let total_in_sb = ndblks * nelmts_per_dblk; - sb_infos.push((ndblks, nelmts_per_dblk)); - covered += total_in_sb; - sb_level += 1; - } - } - - // Read super block addresses from index block - for _ in 0..sb_infos.len() { - if pos + os > file_data.len() { - break; - } - let addr = read_offset(file_data, pos, offset_size)?; - sb_addrs.push(addr); - pos += os; - } - - // Process each super block - for (sb_idx, &sb_addr) in sb_addrs.iter().enumerate() { - let (ndblks, nelmts_per_dblk) = sb_infos[sb_idx]; - if is_undefined_addr(sb_addr, offset_size) { - global_index += ndblks * nelmts_per_dblk; - continue; - } - let sb_chunks = read_super_block( - file_data, - sb_addr as usize, - ndblks, - nelmts_per_dblk, - header, - offset_size, - chunk_byte_size, - global_index, - &num_chunks_per_dim, - chunk_dimensions, - )?; - chunks.extend(sb_chunks); - global_index += ndblks * nelmts_per_dblk; + global_index = + global_index.saturating_add(ndblks.checked_mul(dblk_nelmts).ok_or_else(|| { + FormatError::Overflow("Extensible Array super block span".into()) + })?); } Ok(chunks) } -/// Read a super block (AESB) and its data blocks. +/// Read a super block (EASB) and the data blocks it owns. +/// +/// On disk: signature(4) + version(1) + client_id(1) + header address +/// + block offset + the page-init bitmap for every data block it owns +/// + one address per data block + checksum. #[allow(clippy::too_many_arguments)] fn read_super_block( file_data: &[u8], sb_offset: usize, ndblks: usize, - nelmts_per_dblk: usize, + dblk_nelmts: usize, header: &ExtensibleArrayHeader, offset_size: u8, chunk_byte_size: u64, @@ -623,9 +596,7 @@ fn read_super_block( chunk_dimensions: &[u32], ) -> Result, FormatError> { let os = offset_size as usize; - - // AESB: signature(4) + version(1) + client_id(1) + header_address(offset_size) - let sb_header_size = 4 + 1 + 1 + os; + let sb_header_size = 4 + 1 + 1 + os + arr_off_size(header); ensure_len(file_data, sb_offset, sb_header_size)?; if &file_data[sb_offset..sb_offset + 4] != b"EASB" { @@ -634,43 +605,50 @@ fn read_super_block( )); } - let mut pos = sb_offset + sb_header_size; - - // Read data block addresses - let mut dblk_addrs: Vec = Vec::with_capacity(ndblks); - for _ in 0..ndblks { - if pos + os > file_data.len() { - return Err(FormatError::UnexpectedEof { - expected: pos + os, - available: file_data.len(), - }); - } - let addr = read_offset(file_data, pos, offset_size)?; - dblk_addrs.push(addr); - pos += os; - } + // Page-init bitmap: one bit per page, `npages` bits per data block, packed + // contiguously. HDF5 sizes the buffer `ndblks * ceil(npages / 8)`, which + // is bigger than the bits need when `npages` is not a multiple of eight. + // Zero-sized unless this level's data blocks are paged. + let page = page_nelmts(header).ok_or_else(|| { + FormatError::Overflow("Extensible Array page element count overflows usize".into()) + })?; + let npages = if dblk_nelmts > page { + dblk_nelmts / page + } else { + 0 + }; + let per_dblk_bitmap = npages.div_ceil(8); + let bitmap_bytes = per_dblk_bitmap + .checked_mul(ndblks) + .ok_or_else(|| FormatError::Overflow("Extensible Array page bitmap size".into()))?; + let bitmap_start = sb_offset + sb_header_size; + ensure_len(file_data, bitmap_start, bitmap_bytes)?; + let bitmap = &file_data[bitmap_start..bitmap_start + bitmap_bytes]; + let mut pos = bitmap_start + bitmap_bytes; let mut chunks = Vec::new(); let mut global_idx = start_index; - for &addr in &dblk_addrs { - if is_undefined_addr(addr, offset_size) { - global_idx += nelmts_per_dblk; - continue; + for i in 0..ndblks { + ensure_len(file_data, pos, os)?; + let addr = read_offset(file_data, pos, offset_size)?; + pos += os; + if !is_undefined_addr(addr, offset_size) { + chunks.extend(read_data_block_elements( + file_data, + addr as usize, + dblk_nelmts, + header, + offset_size, + chunk_byte_size, + global_idx, + num_chunks_per_dim, + chunk_dimensions, + bitmap, + i * npages, + )?); } - let block_chunks = read_data_block_elements( - file_data, - addr as usize, - nelmts_per_dblk, - header, - offset_size, - chunk_byte_size, - global_idx, - num_chunks_per_dim, - chunk_dimensions, - )?; - chunks.extend(block_chunks); - global_idx += nelmts_per_dblk; + global_idx += dblk_nelmts; } Ok(chunks) diff --git a/crates/clawhdf5/tests/h5py_interop_tests.rs b/crates/clawhdf5/tests/h5py_interop_tests.rs index c7ab11c..7082a06 100644 --- a/crates/clawhdf5/tests/h5py_interop_tests.rs +++ b/crates/clawhdf5/tests/h5py_interop_tests.rs @@ -1047,3 +1047,274 @@ with h5py.File("{path_str}", "r") as f: data[start..start + cols as usize] ); } + +#[test] +fn h5py_deep_btree_v2_chunk_index_clawhdf5_reads() { + // Two unlimited dimensions give a B-tree v2 chunk index, and 2x2 chunks + // over 400x400 give 40 000 index records — enough for HDF5 to build a + // tree of depth 2. Small h5py files only ever produce depth-0 trees, so + // this is the one fixture that walks internal nodes: the path where the + // traversal's record budget (the guard against crafted shared-subtree + // trees) is spent, which must never refuse a real file. + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("deep_btree.h5"); + let path_str = path.display().to_string(); + + let script = format!( + r#" +import h5py, numpy as np +with h5py.File("{path_str}", "w", libver="latest") as f: + d = f.create_dataset("x", shape=(400, 400), maxshape=(None, None), + chunks=(2, 2), dtype="i4") + d[...] = np.arange(160000, dtype="i4").reshape(400, 400) +"# + ); + run_python(&script); + + // The fixture is only meaningful if HDF5 really built internal nodes. + let bytes = std::fs::read(&path).unwrap(); + let at = bytes + .windows(4) + .position(|w| w == b"BTHD") + .expect("expected a B-tree v2 chunk index"); + let depth = u16::from_le_bytes([bytes[at + 12], bytes[at + 13]]); + assert!( + depth >= 1, + "fixture tree has depth {depth}; it tests nothing" + ); + + let file = File::open(&path).unwrap(); + let values = file.dataset("x").unwrap().read_i32().unwrap(); + assert_eq!(values.len(), 160_000); + for (i, &v) in values.iter().enumerate() { + assert_eq!(v, i as i32, "element {i}"); + } +} + +#[test] +fn h5py_extensible_array_chunk_index_clawhdf5_reads() { + // One unlimited dimension means an Extensible Array chunk index. Only its + // first few elements live inline in the index block (4 by default), and + // every other fixture here is small enough to stop there — which is how + // the data block and super block layouts came to be wrong without a test + // noticing. The counts below step over each boundary in turn: + // 4 inline elements only + // 37 past the first direct data block + // 400 into the first super block + // 5000 several super block levels + // 200000 data blocks large enough to be paged + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + + for n in [4usize, 37, 400, 5_000, 200_000] { + let path = dir.path().join(format!("ea_{n}.h5")); + let path_str = path.display().to_string(); + run_python(&format!( + r#" +import h5py, numpy as np +with h5py.File("{path_str}", "w", libver="latest") as f: + d = f.create_dataset("x", shape=({n},), maxshape=(None,), chunks=(1,), dtype="i4") + d[...] = np.arange({n}, dtype="i4") +"# + )); + + let bytes = std::fs::read(&path).unwrap(); + assert!( + bytes.windows(4).any(|w| w == b"EAHD"), + "n={n}: fixture is not indexed by an Extensible Array" + ); + + let file = File::open(&path).unwrap(); + let values = file.dataset("x").unwrap().read_i32().unwrap(); + assert_eq!(values.len(), n, "n={n}"); + let wrong = values + .iter() + .enumerate() + .filter(|&(i, &v)| v != i as i32) + .count(); + assert_eq!(wrong, 0, "n={n}: {wrong} of {n} elements read back wrong"); + } +} + +#[test] +fn h5py_sparse_extensible_array_leaves_pages_uninitialised() { + // Writing a scattered subset leaves whole pages of a paged data block + // never initialised. Those pages still occupy their slot on disk, so the + // reader has to skip them by stride and take the fill value instead — + // driven by the page-init bitmap, which is packed one bit per page across + // the whole super block, MSB first. + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("ea_sparse.h5"); + let path_str = path.display().to_string(); + let n = 200_000usize; + let step = 997usize; + + run_python(&format!( + r#" +import h5py +with h5py.File("{path_str}", "w", libver="latest") as f: + d = f.create_dataset("x", shape=({n},), maxshape=(None,), chunks=(1,), + dtype="i4", fillvalue=-1) + for i in list(range(0, {n}, {step})) + list(range(0, 40)): + d[i] = i +"# + )); + + let file = File::open(&path).unwrap(); + let values = file.dataset("x").unwrap().read_i32().unwrap(); + assert_eq!(values.len(), n); + let wrong = values + .iter() + .enumerate() + .filter(|&(i, &v)| { + let expected = if i % step == 0 || i < 40 { + i as i32 + } else { + -1 + }; + v != expected + }) + .count(); + assert_eq!(wrong, 0, "{wrong} of {n} elements read back wrong"); +} + +#[test] +fn h5py_filtered_and_2d_extensible_array_clawhdf5_reads() { + // Filtered elements carry a size and filter mask beside the address, and + // a second (fixed) dimension changes how a linear index maps back to + // chunk offsets. Both run through the same traversal. + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + + let gz = dir.path().join("ea_gzip.h5"); + let gz_str = gz.display().to_string(); + run_python(&format!( + r#" +import h5py, numpy as np +with h5py.File("{gz_str}", "w", libver="latest") as f: + d = f.create_dataset("x", shape=(5000,), maxshape=(None,), chunks=(1,), + dtype="i4", compression="gzip", compression_opts=4) + d[...] = np.arange(5000, dtype="i4") +"# + )); + let values = File::open(&gz) + .unwrap() + .dataset("x") + .unwrap() + .read_i32() + .unwrap(); + assert_eq!(values.len(), 5000); + assert_eq!( + values + .iter() + .enumerate() + .filter(|&(i, &v)| v != i as i32) + .count(), + 0 + ); + + let two_d = dir.path().join("ea_2d.h5"); + let two_d_str = two_d.display().to_string(); + run_python(&format!( + r#" +import h5py, numpy as np +with h5py.File("{two_d_str}", "w", libver="latest") as f: + d = f.create_dataset("x", shape=(3000, 4), maxshape=(None, 4), chunks=(1, 4), dtype="i4") + d[...] = np.arange(12000, dtype="i4").reshape(3000, 4) +"# + )); + let values = File::open(&two_d) + .unwrap() + .dataset("x") + .unwrap() + .read_i32() + .unwrap(); + assert_eq!(values.len(), 12_000); + assert_eq!( + values + .iter() + .enumerate() + .filter(|&(i, &v)| v != i as i32) + .count(), + 0 + ); +} + +#[test] +fn h5py_fixed_array_chunk_index_clawhdf5_reads() { + // Fixed dimensions plus libver='latest' give a Fixed Array chunk index. + // Its data blocks are paged above 2^page_bits elements (1024 by default), + // and unlike the Extensible Array it keeps the page-init bitmap in the + // data block itself — a difference worth pinning down, since assuming + // otherwise is exactly what made the Extensible Array reader wrong. The + // sparse case leaves whole pages uninitialised so the bitmap is actually + // consulted rather than being all ones. + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + + for n in [100usize, 5_000, 200_000] { + let path = dir.path().join(format!("fa_{n}.h5")); + let path_str = path.display().to_string(); + run_python(&format!( + r#" +import h5py, numpy as np +with h5py.File("{path_str}", "w", libver="latest") as f: + d = f.create_dataset("x", shape=({n},), chunks=(1,), dtype="i4") + d[...] = np.arange({n}, dtype="i4") +"# + )); + let bytes = std::fs::read(&path).unwrap(); + assert!( + bytes.windows(4).any(|w| w == b"FAHD"), + "n={n}: fixture is not indexed by a Fixed Array" + ); + let values = File::open(&path) + .unwrap() + .dataset("x") + .unwrap() + .read_i32() + .unwrap(); + assert_eq!(values.len(), n, "n={n}"); + let wrong = values + .iter() + .enumerate() + .filter(|&(i, &v)| v != i as i32) + .count(); + assert_eq!(wrong, 0, "n={n}: {wrong} elements read back wrong"); + } + + let sparse = dir.path().join("fa_sparse.h5"); + let sparse_str = sparse.display().to_string(); + let (n, step) = (200_000usize, 997usize); + run_python(&format!( + r#" +import h5py +with h5py.File("{sparse_str}", "w", libver="latest") as f: + d = f.create_dataset("x", shape=({n},), chunks=(1,), dtype="i4", fillvalue=-1) + for i in list(range(0, {n}, {step})) + list(range(0, 40)): + d[i] = i +"# + )); + let values = File::open(&sparse) + .unwrap() + .dataset("x") + .unwrap() + .read_i32() + .unwrap(); + assert_eq!(values.len(), n); + let wrong = values + .iter() + .enumerate() + .filter(|&(i, &v)| { + let expected = if i % step == 0 || i < 40 { + i as i32 + } else { + -1 + }; + v != expected + }) + .count(); + assert_eq!(wrong, 0, "sparse: {wrong} of {n} elements read back wrong"); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index ad06ab0..57aa18e 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -169,3 +169,61 @@ python3 -m venv .venv && .venv/bin/pip install h5py numpy netCDF4 Set `CLAWHDF5_REQUIRE_INTEROP=1` in any automated runner so a missing interpreter is a failure rather than a skip. + + +--- + +## Crafted B-tree v2 structures crash or exhaust the reader + +**Status:** fixed on `main` (2026-09-20), after v2.6.0. **Every release up to +and including v2.6.0 is affected.** + +B-tree v2 traversal (`clawhdf5-format`, `btree_v2::collect_btree_v2_records`) +recursed one frame per level with the depth taken from the file, and followed +child addresses without checking whether they were shared. Two consequences +for anyone reading untrusted files: + +- A node that is its own child, under a header claiming 65 535 levels, overflows + the stack and aborts the process. The file is under 100 bytes. +- Levels whose children all point at one node below make the traversal visit it + fan-out^depth times: ~30 million records from ~5 KB, and memory exhaustion one + level deeper. + +B-tree v2 backs dense attribute storage, v2 groups, shared object header +messages and chunk indexes, so opening an object that uses any of them is +enough. Both are now errors: depth is capped at 64, and traversal stops once it +has produced more records than the file could physically hold. + +--- + +## Extensible Array chunk indexes read back wrong data past the inline elements + +**Status:** fixed on `main` (2026-09-20), after v2.6.0. **Every release up to +and including v2.6.0 is affected.** + +A dataset created with exactly one unlimited dimension (`maxshape=(None, ...)`, +the usual append-only/resizable case) is indexed by an Extensible Array. Its +index block holds the first `idx_blk_elmts` chunk entries inline — 4 by +default — and everything after that lives in data blocks and super blocks whose +layout `clawhdf5-format` computed incorrectly. + +Consequences, by dataset size (1 chunk per element): + +| chunks | result before the fix | +|---|---| +| <= 36 | correct (inline, plus two data blocks that happened to line up) | +| 37 | 1 element wrong | +| 400 | 364 elements wrong | +| >= ~1000 | `invalid Extensible Array data block signature` | + +The dangerous case is the middle one: values were returned from the wrong +chunks rather than an error being raised. Any reader that accepted the data at +face value saw plausible but incorrect numbers. + +The root causes were the super block sizing formulas (`ndblks` and +`dblk_nelmts` each double every *other* level, a half-step apart), a missing +block-offset field in the super block, and a page-init bitmap read from the +wrong structure. All four are fixed and covered by interop tests against +HDF5 2.0 at sizes that cross each boundary, including paged data blocks. + +Files written by this crate are unaffected — this was purely a read-path bug.