Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
563cdd2178 |
@@ -8,28 +8,7 @@
|
|||||||
//! - Sorted posting lists by doc_id for cache-friendly access
|
//! - Sorted posting lists by doc_id for cache-friendly access
|
||||||
//! - Block-Max WAND early termination
|
//! - Block-Max WAND early termination
|
||||||
|
|
||||||
use std::cmp::Reverse;
|
use std::collections::HashMap;
|
||||||
use std::collections::{BinaryHeap, HashMap};
|
|
||||||
|
|
||||||
/// `f32` wrapper providing a total order (via `total_cmp`) so BM25 scores can
|
|
||||||
/// be kept in a `BinaryHeap`. Scores are always finite in practice (no NaN
|
|
||||||
/// inputs reach this path), so `total_cmp`'s NaN ordering is never exercised.
|
|
||||||
#[derive(Debug, Clone, Copy, PartialEq)]
|
|
||||||
struct HeapScore(f32);
|
|
||||||
|
|
||||||
impl Eq for HeapScore {}
|
|
||||||
|
|
||||||
impl PartialOrd for HeapScore {
|
|
||||||
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
|
|
||||||
Some(self.cmp(other))
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
impl Ord for HeapScore {
|
|
||||||
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
|
|
||||||
self.0.total_cmp(&other.0)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
/// Default BM25 term-frequency saturation parameter.
|
/// Default BM25 term-frequency saturation parameter.
|
||||||
const DEFAULT_K1: f32 = 1.2;
|
const DEFAULT_K1: f32 = 1.2;
|
||||||
@@ -118,11 +97,9 @@ impl BM25Index {
|
|||||||
|
|
||||||
let total_max_contribution: f32 = max_tf_score.iter().sum();
|
let total_max_contribution: f32 = max_tf_score.iter().sum();
|
||||||
|
|
||||||
// Threshold for WAND early termination. `top_k_heap` is a min-heap of
|
// Threshold for WAND early termination
|
||||||
// size k (worst-of-the-top-k at the head) so it can be maintained in
|
|
||||||
// O(log k) per update instead of re-sorting the whole buffer.
|
|
||||||
let mut threshold = 0.0f32;
|
let mut threshold = 0.0f32;
|
||||||
let mut top_k_heap: BinaryHeap<Reverse<HeapScore>> = BinaryHeap::with_capacity(k);
|
let mut top_k_scores: Vec<f32> = Vec::with_capacity(k);
|
||||||
|
|
||||||
for (term_idx, (_, idf, postings)) in query_terms.iter().enumerate() {
|
for (term_idx, (_, idf, postings)) in query_terms.iter().enumerate() {
|
||||||
for &(doc_id, freq) in *postings {
|
for &(doc_id, freq) in *postings {
|
||||||
@@ -141,17 +118,24 @@ impl BM25Index {
|
|||||||
if term_idx == query_terms.len() - 1 {
|
if term_idx == query_terms.len() - 1 {
|
||||||
// Last term: check if this doc beats threshold
|
// Last term: check if this doc beats threshold
|
||||||
let final_score = *entry;
|
let final_score = *entry;
|
||||||
if top_k_heap.len() >= k {
|
if final_score > threshold && top_k_scores.len() >= k {
|
||||||
if final_score > threshold {
|
// Update threshold
|
||||||
// Replace the current worst-of-top-k.
|
top_k_scores
|
||||||
top_k_heap.pop();
|
.sort_by(|a, b| b.partial_cmp(a).unwrap_or(std::cmp::Ordering::Equal));
|
||||||
top_k_heap.push(Reverse(HeapScore(final_score)));
|
if final_score > top_k_scores[k - 1] {
|
||||||
threshold = top_k_heap.peek().map(|Reverse(s)| s.0).unwrap_or(0.0);
|
top_k_scores[k - 1] = final_score;
|
||||||
|
top_k_scores.sort_by(|a, b| {
|
||||||
|
b.partial_cmp(a).unwrap_or(std::cmp::Ordering::Equal)
|
||||||
|
});
|
||||||
|
threshold = top_k_scores[k - 1];
|
||||||
}
|
}
|
||||||
} else {
|
} else if top_k_scores.len() < k {
|
||||||
top_k_heap.push(Reverse(HeapScore(final_score)));
|
top_k_scores.push(final_score);
|
||||||
if top_k_heap.len() == k {
|
if top_k_scores.len() == k {
|
||||||
threshold = top_k_heap.peek().map(|Reverse(s)| s.0).unwrap_or(0.0);
|
top_k_scores.sort_by(|a, b| {
|
||||||
|
b.partial_cmp(a).unwrap_or(std::cmp::Ordering::Equal)
|
||||||
|
});
|
||||||
|
threshold = top_k_scores[k - 1];
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -118,7 +118,7 @@ impl ImportanceScorer {
|
|||||||
|
|
||||||
/// Novelty score: 1.0 − max cosine similarity against all existing records.
|
/// Novelty score: 1.0 − max cosine similarity against all existing records.
|
||||||
/// Returns 1.0 when there are no existing memories.
|
/// Returns 1.0 when there are no existing memories.
|
||||||
pub fn score_surprise(embedding: &[f32], existing_memories: &[&MemoryRecord]) -> f32 {
|
pub fn score_surprise(embedding: &[f32], existing_memories: &[MemoryRecord]) -> f32 {
|
||||||
if existing_memories.is_empty() {
|
if existing_memories.is_empty() {
|
||||||
return 1.0;
|
return 1.0;
|
||||||
}
|
}
|
||||||
@@ -209,10 +209,11 @@ impl ConsolidationEngine {
|
|||||||
source: MemorySource,
|
source: MemorySource,
|
||||||
now: f64,
|
now: f64,
|
||||||
) -> u64 {
|
) -> u64 {
|
||||||
let working: Vec<&MemoryRecord> = self
|
let working: Vec<MemoryRecord> = self
|
||||||
.records
|
.records
|
||||||
.iter()
|
.iter()
|
||||||
.filter(|r| r.tier == MemoryTier::Working)
|
.filter(|r| r.tier == MemoryTier::Working)
|
||||||
|
.cloned()
|
||||||
.collect();
|
.collect();
|
||||||
|
|
||||||
let surprise = ImportanceScorer::score_surprise(&embedding, &working);
|
let surprise = ImportanceScorer::score_surprise(&embedding, &working);
|
||||||
@@ -280,7 +281,7 @@ impl ConsolidationEngine {
|
|||||||
if working_count > capacity {
|
if working_count > capacity {
|
||||||
let evict_n = working_count - capacity;
|
let evict_n = working_count - capacity;
|
||||||
// Collect the ids of the records to evict (lowest decay = first in sorted list).
|
// Collect the ids of the records to evict (lowest decay = first in sorted list).
|
||||||
let evict_ids: std::collections::HashSet<u64> = working_indices[..evict_n]
|
let evict_ids: Vec<u64> = working_indices[..evict_n]
|
||||||
.iter()
|
.iter()
|
||||||
.map(|&i| self.records[i].id)
|
.map(|&i| self.records[i].id)
|
||||||
.collect();
|
.collect();
|
||||||
@@ -341,7 +342,7 @@ impl ConsolidationEngine {
|
|||||||
});
|
});
|
||||||
|
|
||||||
let evict_n = episodic_count - episodic_capacity;
|
let evict_n = episodic_count - episodic_capacity;
|
||||||
let evict_ids: std::collections::HashSet<u64> = episodic_indices[..evict_n]
|
let evict_ids: Vec<u64> = episodic_indices[..evict_n]
|
||||||
.iter()
|
.iter()
|
||||||
.map(|&i| self.records[i].id)
|
.map(|&i| self.records[i].id)
|
||||||
.collect();
|
.collect();
|
||||||
@@ -463,8 +464,7 @@ mod tests {
|
|||||||
created_at: 0.0,
|
created_at: 0.0,
|
||||||
source: MemorySource::User,
|
source: MemorySource::User,
|
||||||
}];
|
}];
|
||||||
let existing_refs: Vec<&MemoryRecord> = existing.iter().collect();
|
let score = ImportanceScorer::score_surprise(&emb, &existing);
|
||||||
let score = ImportanceScorer::score_surprise(&emb, &existing_refs);
|
|
||||||
assert!(score < 0.01, "expected ~0.0, got {score}");
|
assert!(score < 0.01, "expected ~0.0, got {score}");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -204,25 +204,11 @@ fn read_uint(data: &[u8], offset: usize, nbytes: usize) -> Result<u64, FormatErr
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Maximum recursion depth for nested datatypes (Compound/Enumeration/
|
|
||||||
/// VariableLength/Array). A crafted file can nest a message-size-capped
|
|
||||||
/// (65535 byte) datatype message ~8000 levels deep, which would blow the
|
|
||||||
/// stack — especially on the project's no_std/embedded targets where
|
|
||||||
/// available stack is a few KB.
|
|
||||||
const MAX_DATATYPE_DEPTH: u16 = 64;
|
|
||||||
|
|
||||||
impl Datatype {
|
impl Datatype {
|
||||||
/// Parse a datatype message from raw bytes.
|
/// Parse a datatype message from raw bytes.
|
||||||
///
|
///
|
||||||
/// Returns `(Datatype, bytes_consumed)` for recursive parsing.
|
/// Returns `(Datatype, bytes_consumed)` for recursive parsing.
|
||||||
pub fn parse(data: &[u8]) -> Result<(Datatype, usize), FormatError> {
|
pub fn parse(data: &[u8]) -> Result<(Datatype, usize), FormatError> {
|
||||||
Self::parse_with_depth(data, 0)
|
|
||||||
}
|
|
||||||
|
|
||||||
fn parse_with_depth(data: &[u8], depth: u16) -> Result<(Datatype, usize), FormatError> {
|
|
||||||
if depth >= MAX_DATATYPE_DEPTH {
|
|
||||||
return Err(FormatError::NestingDepthExceeded);
|
|
||||||
}
|
|
||||||
// Minimum header: 4 bytes (class_and_version + 3 bytes bit field) + 4 bytes size = 8
|
// Minimum header: 4 bytes (class_and_version + 3 bytes bit field) + 4 bytes size = 8
|
||||||
ensure_len(data, 0, 8)?;
|
ensure_len(data, 0, 8)?;
|
||||||
|
|
||||||
@@ -372,7 +358,7 @@ impl Datatype {
|
|||||||
pos += name_len;
|
pos += name_len;
|
||||||
let byte_offset = read_uint(data, pos, ob)?;
|
let byte_offset = read_uint(data, pos, ob)?;
|
||||||
pos += ob;
|
pos += ob;
|
||||||
let (member_dt, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?;
|
let (member_dt, consumed) = Datatype::parse(&data[pos..])?;
|
||||||
pos += consumed;
|
pos += consumed;
|
||||||
members.push(CompoundMember {
|
members.push(CompoundMember {
|
||||||
name,
|
name,
|
||||||
@@ -398,7 +384,7 @@ impl Datatype {
|
|||||||
// dimensionality(1) + reserved(3) + dim_perm(4) + 4 dim slots(16) = 24
|
// dimensionality(1) + reserved(3) + dim_perm(4) + 4 dim slots(16) = 24
|
||||||
ensure_len(data, pos, 24)?;
|
ensure_len(data, pos, 24)?;
|
||||||
pos += 24;
|
pos += 24;
|
||||||
let (member_dt, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?;
|
let (member_dt, consumed) = Datatype::parse(&data[pos..])?;
|
||||||
pos += consumed;
|
pos += consumed;
|
||||||
members.push(CompoundMember {
|
members.push(CompoundMember {
|
||||||
name,
|
name,
|
||||||
@@ -429,7 +415,7 @@ impl Datatype {
|
|||||||
// Enumeration
|
// Enumeration
|
||||||
let num_members = (bf0 as u16) | ((bf1 as u16) << 8);
|
let num_members = (bf0 as u16) | ((bf1 as u16) << 8);
|
||||||
// Parse base type
|
// Parse base type
|
||||||
let (base_type, base_consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?;
|
let (base_type, base_consumed) = Datatype::parse(&data[pos..])?;
|
||||||
pos += base_consumed;
|
pos += base_consumed;
|
||||||
let base_size = base_type.type_size();
|
let base_size = base_type.type_size();
|
||||||
let mut members = Vec::with_capacity(num_members as usize);
|
let mut members = Vec::with_capacity(num_members as usize);
|
||||||
@@ -482,7 +468,7 @@ impl Datatype {
|
|||||||
} else {
|
} else {
|
||||||
None
|
None
|
||||||
};
|
};
|
||||||
let (base_type, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?;
|
let (base_type, consumed) = Datatype::parse(&data[pos..])?;
|
||||||
pos += consumed;
|
pos += consumed;
|
||||||
Ok((
|
Ok((
|
||||||
Datatype::VariableLength {
|
Datatype::VariableLength {
|
||||||
@@ -508,7 +494,7 @@ impl Datatype {
|
|||||||
}
|
}
|
||||||
// skip permutation indices
|
// skip permutation indices
|
||||||
pos += ndims * 4;
|
pos += ndims * 4;
|
||||||
let (base_type, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?;
|
let (base_type, consumed) = Datatype::parse(&data[pos..])?;
|
||||||
pos += consumed;
|
pos += consumed;
|
||||||
Ok((
|
Ok((
|
||||||
Datatype::Array {
|
Datatype::Array {
|
||||||
@@ -529,7 +515,7 @@ impl Datatype {
|
|||||||
dimensions.push(LittleEndian::read_u32(&data[pos..pos + 4]));
|
dimensions.push(LittleEndian::read_u32(&data[pos..pos + 4]));
|
||||||
pos += 4;
|
pos += 4;
|
||||||
}
|
}
|
||||||
let (base_type, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?;
|
let (base_type, consumed) = Datatype::parse(&data[pos..])?;
|
||||||
pos += consumed;
|
pos += consumed;
|
||||||
Ok((
|
Ok((
|
||||||
Datatype::Array {
|
Datatype::Array {
|
||||||
@@ -559,7 +545,7 @@ impl Datatype {
|
|||||||
pos += name_len;
|
pos += name_len;
|
||||||
let byte_offset = read_uint(data, pos, ob)?;
|
let byte_offset = read_uint(data, pos, ob)?;
|
||||||
pos += ob;
|
pos += ob;
|
||||||
let (member_dt, consumed) = Self::parse_with_depth(&data[pos..], depth + 1)?;
|
let (member_dt, consumed) = Datatype::parse(&data[pos..])?;
|
||||||
pos += consumed;
|
pos += consumed;
|
||||||
members.push(CompoundMember {
|
members.push(CompoundMember {
|
||||||
name,
|
name,
|
||||||
@@ -828,39 +814,6 @@ mod tests {
|
|||||||
buf
|
buf
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A crafted datatype message nesting Variable-Length wrappers deeper
|
|
||||||
/// than `MAX_DATATYPE_DEPTH` must return `NestingDepthExceeded`
|
|
||||||
/// instead of overflowing the stack.
|
|
||||||
#[test]
|
|
||||||
fn nested_variable_length_exceeds_depth_limit() {
|
|
||||||
// Each VL level is just an 8-byte header (class 9, vl_type=0 =>
|
|
||||||
// sequence, no padding/charset fields) immediately followed by the
|
|
||||||
// next level's bytes, terminated by a fixed-point base type.
|
|
||||||
let levels = MAX_DATATYPE_DEPTH as usize + 10;
|
|
||||||
let mut data = Vec::new();
|
|
||||||
for _ in 0..levels {
|
|
||||||
data.extend_from_slice(&build_dt_header(9, 3, [0, 0, 0], 0));
|
|
||||||
}
|
|
||||||
data.extend_from_slice(&build_fixed_point(4, false, false, 0, 32));
|
|
||||||
|
|
||||||
let result = Datatype::parse(&data);
|
|
||||||
assert!(matches!(result, Err(FormatError::NestingDepthExceeded)));
|
|
||||||
}
|
|
||||||
|
|
||||||
/// A datatype nested just within the depth limit must still parse fine.
|
|
||||||
#[test]
|
|
||||||
fn nested_variable_length_within_depth_limit_ok() {
|
|
||||||
let levels = MAX_DATATYPE_DEPTH as usize - 1;
|
|
||||||
let mut data = Vec::new();
|
|
||||||
for _ in 0..levels {
|
|
||||||
data.extend_from_slice(&build_dt_header(9, 3, [0, 0, 0], 0));
|
|
||||||
}
|
|
||||||
data.extend_from_slice(&build_fixed_point(4, false, false, 0, 32));
|
|
||||||
|
|
||||||
let result = Datatype::parse(&data);
|
|
||||||
assert!(result.is_ok());
|
|
||||||
}
|
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_fixed_point_u8() {
|
fn test_fixed_point_u8() {
|
||||||
let data = build_fixed_point(1, false, false, 0, 8);
|
let data = build_fixed_point(1, false, false, 0, 8);
|
||||||
|
|||||||
@@ -54,19 +54,6 @@ fn read_offset(data: &[u8], pos: usize, size: u8) -> Result<u64, FormatError> {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
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_addr(addr: u64, offset_size: u8) -> bool {
|
fn is_undefined_addr(addr: u64, offset_size: u8) -> bool {
|
||||||
match offset_size {
|
match offset_size {
|
||||||
2 => addr == 0xFFFF,
|
2 => addr == 0xFFFF,
|
||||||
@@ -111,7 +98,12 @@ impl ExtensibleArrayHeader {
|
|||||||
// 6 stats fields (each length_size) + index_block_address(offset_size) + checksum(4)
|
// 6 stats fields (each length_size) + index_block_address(offset_size) + checksum(4)
|
||||||
let min_size =
|
let min_size =
|
||||||
4 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 6 * length_size as usize + offset_size as usize + 4;
|
4 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 6 * length_size as usize + offset_size as usize + 4;
|
||||||
ensure_len(file_data, offset, min_size)?;
|
if offset + min_size > file_data.len() {
|
||||||
|
return Err(FormatError::UnexpectedEof {
|
||||||
|
expected: offset + min_size,
|
||||||
|
available: file_data.len(),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
let d = &file_data[offset..];
|
let d = &file_data[offset..];
|
||||||
if &d[0..4] != b"EAHD" {
|
if &d[0..4] != b"EAHD" {
|
||||||
@@ -283,7 +275,12 @@ fn read_data_block_elements(
|
|||||||
) -> Result<Vec<ChunkInfo>, FormatError> {
|
) -> Result<Vec<ChunkInfo>, FormatError> {
|
||||||
// AEDB: signature(4) + version(1) + client_id(1) + header_address(offset_size)
|
// AEDB: signature(4) + version(1) + client_id(1) + header_address(offset_size)
|
||||||
let db_header_size = 4 + 1 + 1 + offset_size as usize;
|
let db_header_size = 4 + 1 + 1 + offset_size as usize;
|
||||||
ensure_len(file_data, db_offset, db_header_size)?;
|
if db_offset + db_header_size > file_data.len() {
|
||||||
|
return Err(FormatError::UnexpectedEof {
|
||||||
|
expected: db_offset + db_header_size,
|
||||||
|
available: file_data.len(),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
let d = &file_data[db_offset..];
|
let d = &file_data[db_offset..];
|
||||||
if &d[0..4] != b"EADB" {
|
if &d[0..4] != b"EADB" {
|
||||||
@@ -430,7 +427,12 @@ pub fn read_extensible_array_chunks(
|
|||||||
// Parse index block (AEIB)
|
// Parse index block (AEIB)
|
||||||
let ib_offset = header.index_block_address as usize;
|
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 + offset_size as usize; // sig + ver + client + hdr_addr
|
||||||
ensure_len(file_data, ib_offset, ib_header_size)?;
|
if ib_offset + ib_header_size > file_data.len() {
|
||||||
|
return Err(FormatError::UnexpectedEof {
|
||||||
|
expected: ib_offset + ib_header_size,
|
||||||
|
available: file_data.len(),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
let ib = &file_data[ib_offset..];
|
let ib = &file_data[ib_offset..];
|
||||||
if &ib[0..4] != b"EAIB" {
|
if &ib[0..4] != b"EAIB" {
|
||||||
@@ -626,7 +628,12 @@ fn read_super_block(
|
|||||||
|
|
||||||
// AESB: signature(4) + version(1) + client_id(1) + header_address(offset_size)
|
// 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;
|
||||||
ensure_len(file_data, sb_offset, sb_header_size)?;
|
if sb_offset + sb_header_size > file_data.len() {
|
||||||
|
return Err(FormatError::UnexpectedEof {
|
||||||
|
expected: sb_offset + sb_header_size,
|
||||||
|
available: file_data.len(),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
if &file_data[sb_offset..sb_offset + 4] != b"EASB" {
|
if &file_data[sb_offset..sb_offset + 4] != b"EASB" {
|
||||||
return Err(FormatError::ChunkedReadError(
|
return Err(FormatError::ChunkedReadError(
|
||||||
@@ -752,33 +759,6 @@ mod tests {
|
|||||||
assert!(result.is_err());
|
assert!(result.is_err());
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A near-`usize::MAX` offset must error cleanly, not overflow/panic.
|
|
||||||
#[test]
|
|
||||||
fn parse_rejects_offset_overflow() {
|
|
||||||
let buf = vec![0u8; 64];
|
|
||||||
let result = ExtensibleArrayHeader::parse(&buf, usize::MAX - 4, 8, 8);
|
|
||||||
assert!(result.is_err());
|
|
||||||
}
|
|
||||||
|
|
||||||
/// A near-`usize::MAX` index block address must error cleanly, not overflow/panic.
|
|
||||||
#[test]
|
|
||||||
fn read_rejects_index_block_offset_overflow() {
|
|
||||||
let header = ExtensibleArrayHeader {
|
|
||||||
client_id: 0,
|
|
||||||
element_size: 8,
|
|
||||||
max_nelmts_bits: 10,
|
|
||||||
idx_blk_elmts: 2,
|
|
||||||
min_dblk_nelmts: 4,
|
|
||||||
super_blk_min_nelmts: 2,
|
|
||||||
max_dblk_nelmts_bits: 8,
|
|
||||||
num_elements: 5,
|
|
||||||
index_block_address: (usize::MAX - 4) as u64,
|
|
||||||
};
|
|
||||||
let buf = vec![0u8; 64];
|
|
||||||
let r = read_extensible_array_chunks(&buf, &header, &[100], &[20], 8, 8, 8);
|
|
||||||
assert!(r.is_err());
|
|
||||||
}
|
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn parse_header_invalid_version() {
|
fn parse_header_invalid_version() {
|
||||||
let mut buf = vec![0u8; 256];
|
let mut buf = vec![0u8; 256];
|
||||||
|
|||||||
@@ -47,19 +47,6 @@ fn read_length(data: &[u8], pos: usize, size: u8) -> Result<u64, FormatError> {
|
|||||||
read_offset(data, pos, size)
|
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 {
|
fn is_undefined(data: &[u8], pos: usize, size: u8) -> bool {
|
||||||
let s = size as usize;
|
let s = size as usize;
|
||||||
if pos + s > data.len() {
|
if pos + s > data.len() {
|
||||||
@@ -79,7 +66,12 @@ impl FixedArrayHeader {
|
|||||||
// FAHD signature(4) + version(1) + client_id(1) + element_size(1) +
|
// 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)
|
// 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;
|
let min_size = 4 + 1 + 1 + 1 + 1 + length_size as usize + offset_size as usize + 4;
|
||||||
ensure_len(file_data, offset, min_size)?;
|
if offset + min_size > file_data.len() {
|
||||||
|
return Err(FormatError::UnexpectedEof {
|
||||||
|
expected: offset + min_size,
|
||||||
|
available: file_data.len(),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
let d = &file_data[offset..];
|
let d = &file_data[offset..];
|
||||||
if &d[0..4] != b"FAHD" {
|
if &d[0..4] != b"FAHD" {
|
||||||
@@ -134,7 +126,12 @@ pub fn read_fixed_array_chunks(
|
|||||||
|
|
||||||
// Parse data block header: FADB(4) + version(1) + client_id(1) + header_address(offset_size)
|
// 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;
|
let db_header_size = 4 + 1 + 1 + offset_size as usize;
|
||||||
ensure_len(file_data, db_offset, db_header_size)?;
|
if db_offset + db_header_size > file_data.len() {
|
||||||
|
return Err(FormatError::UnexpectedEof {
|
||||||
|
expected: db_offset + db_header_size,
|
||||||
|
available: file_data.len(),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
let d = &file_data[db_offset..];
|
let d = &file_data[db_offset..];
|
||||||
if &d[0..4] != b"FADB" {
|
if &d[0..4] != b"FADB" {
|
||||||
@@ -492,29 +489,6 @@ mod tests {
|
|||||||
assert!(r.is_err());
|
assert!(r.is_err());
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A near-`usize::MAX` offset must error cleanly, not overflow/panic.
|
|
||||||
#[test]
|
|
||||||
fn parse_rejects_offset_overflow() {
|
|
||||||
let buf = vec![0u8; 64];
|
|
||||||
let result = FixedArrayHeader::parse(&buf, usize::MAX - 4, 8, 8);
|
|
||||||
assert!(result.is_err());
|
|
||||||
}
|
|
||||||
|
|
||||||
/// A near-`usize::MAX` data block address must error cleanly, not overflow/panic.
|
|
||||||
#[test]
|
|
||||||
fn read_rejects_data_block_offset_overflow() {
|
|
||||||
let header = FixedArrayHeader {
|
|
||||||
client_id: 0,
|
|
||||||
element_size: 8,
|
|
||||||
max_nelmts_bits: 10,
|
|
||||||
num_elements: 1,
|
|
||||||
data_block_address: (usize::MAX - 4) as u64,
|
|
||||||
};
|
|
||||||
let buf = vec![0u8; 64];
|
|
||||||
let r = read_fixed_array_chunks(&buf, &header, &[100], &[20], 8, 8, 8);
|
|
||||||
assert!(r.is_err());
|
|
||||||
}
|
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn parse_fixed_array_header_invalid_version() {
|
fn parse_fixed_array_header_invalid_version() {
|
||||||
let mut buf = vec![0u8; 256];
|
let mut buf = vec![0u8; 256];
|
||||||
|
|||||||
@@ -80,9 +80,9 @@ impl SymbolTableNode {
|
|||||||
offset_size: u8,
|
offset_size: u8,
|
||||||
) -> Result<SymbolTableNode, FormatError> {
|
) -> Result<SymbolTableNode, FormatError> {
|
||||||
// signature(4) + version(1) + reserved(1) + number_of_symbols(2) = 8
|
// signature(4) + version(1) + reserved(1) + number_of_symbols(2) = 8
|
||||||
if offset.checked_add(8).is_none_or(|end| end > file_data.len()) {
|
if offset + 8 > file_data.len() {
|
||||||
return Err(FormatError::UnexpectedEof {
|
return Err(FormatError::UnexpectedEof {
|
||||||
expected: offset.saturating_add(8),
|
expected: offset + 8,
|
||||||
available: file_data.len(),
|
available: file_data.len(),
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
@@ -103,12 +103,7 @@ impl SymbolTableNode {
|
|||||||
// Each entry: link_name_offset(os) + obj_hdr_addr(os) + cache_type(4) + reserved(4) + scratch(16)
|
// Each entry: link_name_offset(os) + obj_hdr_addr(os) + cache_type(4) + reserved(4) + scratch(16)
|
||||||
let entry_size = os + os + 4 + 4 + 16;
|
let entry_size = os + os + 4 + 4 + 16;
|
||||||
let entries_start = offset + 8;
|
let entries_start = offset + 8;
|
||||||
let needed = entries_start
|
let needed = entries_start + num_symbols * entry_size;
|
||||||
.checked_add(num_symbols * entry_size)
|
|
||||||
.ok_or(FormatError::UnexpectedEof {
|
|
||||||
expected: usize::MAX,
|
|
||||||
available: file_data.len(),
|
|
||||||
})?;
|
|
||||||
if needed > file_data.len() {
|
if needed > file_data.len() {
|
||||||
return Err(FormatError::UnexpectedEof {
|
return Err(FormatError::UnexpectedEof {
|
||||||
expected: needed,
|
expected: needed,
|
||||||
@@ -233,24 +228,4 @@ mod tests {
|
|||||||
let err = SymbolTableNode::parse(&data, 0, 8).unwrap_err();
|
let err = SymbolTableNode::parse(&data, 0, 8).unwrap_err();
|
||||||
assert_eq!(err, FormatError::InvalidSymbolTableNodeVersion(2));
|
assert_eq!(err, FormatError::InvalidSymbolTableNodeVersion(2));
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A near-`usize::MAX` SNOD offset must error cleanly, not overflow/panic.
|
|
||||||
#[test]
|
|
||||||
fn parse_snod_rejects_offset_overflow() {
|
|
||||||
let data = build_snod(&[], 8);
|
|
||||||
let result = SymbolTableNode::parse(&data, usize::MAX - 4, 8);
|
|
||||||
assert!(result.is_err());
|
|
||||||
}
|
|
||||||
|
|
||||||
/// A huge symbol count combined with a large entries_start must not
|
|
||||||
/// overflow the `needed` size computation.
|
|
||||||
#[test]
|
|
||||||
fn parse_snod_rejects_entries_size_overflow() {
|
|
||||||
let mut data = build_snod(&[], 8);
|
|
||||||
// num_symbols at offset 6..8 — set to max to blow up entries_start + num_symbols*entry_size
|
|
||||||
data[6] = 0xFF;
|
|
||||||
data[7] = 0xFF;
|
|
||||||
let result = SymbolTableNode::parse(&data, usize::MAX / 2, 8);
|
|
||||||
assert!(result.is_err());
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -144,8 +144,7 @@ fn truncate(s: &str) -> String {
|
|||||||
if s.len() <= 40 {
|
if s.len() <= 40 {
|
||||||
s.to_string()
|
s.to_string()
|
||||||
} else {
|
} else {
|
||||||
let cut = s.char_indices().nth(40).map(|(i, _)| i).unwrap_or(s.len());
|
format!("{}…", &s[..40])
|
||||||
format!("{}…", &s[..cut])
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -162,31 +161,3 @@ fn sample_indices(n: usize, full: bool) -> Vec<usize> {
|
|||||||
idx.dedup();
|
idx.dedup();
|
||||||
idx
|
idx
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
|
||||||
mod tests {
|
|
||||||
use super::*;
|
|
||||||
|
|
||||||
#[test]
|
|
||||||
fn truncate_short_string_unchanged() {
|
|
||||||
assert_eq!(truncate("hello"), "hello");
|
|
||||||
}
|
|
||||||
|
|
||||||
/// A multi-byte character straddling byte offset 40 must not panic a
|
|
||||||
/// byte-index slice — this is arbitrary UTF-8 chunk text from an
|
|
||||||
/// untrusted source database, not test-only input.
|
|
||||||
#[test]
|
|
||||||
fn truncate_multibyte_char_at_boundary_does_not_panic() {
|
|
||||||
// 39 ASCII bytes then a 4-byte emoji straddling the byte-40 cut point.
|
|
||||||
let s = format!("{}{}", "a".repeat(39), "😀".repeat(5));
|
|
||||||
let result = truncate(&s);
|
|
||||||
assert!(result.ends_with('…'));
|
|
||||||
assert!(result.chars().count() < s.chars().count());
|
|
||||||
}
|
|
||||||
|
|
||||||
#[test]
|
|
||||||
fn truncate_exactly_at_limit_unchanged() {
|
|
||||||
let s = "a".repeat(40);
|
|
||||||
assert_eq!(truncate(&s), s);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
+396
-307
@@ -1,345 +1,434 @@
|
|||||||
# Implementation Brief — Performance, Security & Provenance
|
# Implementation Brief — clawhdf5 Performance/Security/Provenance Pass
|
||||||
|
|
||||||
**Phase:** Research
|
**Research date:** 2026-08-16
|
||||||
**Date:** 2026-08-17
|
**Scope:** `crates/` only. Read against `ROADMAP.md`, `IMPROVEMENT_LOG.md`, `CLAUDE.md`, and
|
||||||
**Scope:** `clawhdf5` Rust workspace (`/mission/repo`)
|
`CHANGELOG.md` first — those documents record a genuinely large amount of prior hardening
|
||||||
|
(WAL CRC32, `chunked_read.rs`/`data_read.rs`/`local_heap.rs`/`btree_v1.rs` bounds audits,
|
||||||
|
Android JNI bounds checks, pyo3 bump, HNSW `prune_connections` rayon parallelism, bounded
|
||||||
|
decompression, no_std fixes, `cargo-audit`-clean dependency tree). None of that is
|
||||||
|
re-proposed here. Every item below was independently verified by reading the current source
|
||||||
|
(file path + line numbers cited), not inferred from docs.
|
||||||
|
|
||||||
## Method
|
`cargo audit` was run against the current lockfile: **zero vulnerability advisories**, three
|
||||||
|
"unmaintained" warnings (`custom_derive` via `mpi`→`conv`, `number_prefix` via `tokenizers`→
|
||||||
|
`indicatif`, `paste`) — all transitive through optional deps (`mpi-io` feature, `tokenizers`),
|
||||||
|
no upstream fix available, not actionable as a code change. Not filed as an INT item.
|
||||||
|
|
||||||
Read `ROADMAP.md`, `IMPROVEMENT_LOG.md`, `CLAUDE.md`, `CHANGELOG.md`, and recent
|
Also checked and found clean (no INT items filed): `clawhdf5-migrate` (zero `unwrap()` outside
|
||||||
`git log` before scoping this brief, to avoid re-proposing work already merged.
|
`#[test]` code in `main.rs`; `sqlite_reader.rs`/`hdf5_writer.rs`/`validate.rs` are unwrap-free),
|
||||||
The repo has already been through several hardening passes (Tier 1–4, see
|
`clawhdf5-cli`, `clawhdf5-napi` (zero `unwrap()` in `lib.rs`), `clawhdf5-accel` SIMD dispatch
|
||||||
`CHANGELOG.md` "Unreleased" section and the `git log` entries tagged
|
(`is_x86_feature_detected!`/runtime gating is correct — no illegal-instruction risk),
|
||||||
`security:`/`perf:`): bounds-check audits on `chunked_read.rs`/`data_read.rs`/
|
`clawhdf5-filters` hot path (slice-based, no byte-by-byte loops of consequence), and TODO/FIXME
|
||||||
`local_heap.rs`/`btree_v1.rs`, `MAX_DECOMPRESS_SIZE` output caps, WAL v2
|
grep across all crates (the only hits are test-fixture bytes literally named `b"XXXX"`, not
|
||||||
per-entry CRC32, Android JNI length validation, pyo3 bump, O(1) chunk-cache
|
real markers).
|
||||||
lookup with `Arc`-shared buffers, and optional rayon parallelism for HNSW
|
|
||||||
`prune_connections`. None of that is re-proposed here.
|
|
||||||
|
|
||||||
Four focused audits were run against the areas those passes did **not**
|
|
||||||
cover: (1) the HDF5 binary parser files outside the already-audited set, plus
|
|
||||||
`clawhdf5-accel`/`clawhdf5-gpu` unsafe code; (2) `clawhdf5-agent`'s
|
|
||||||
query-time hot paths (search/rerank/consolidation/knowledge graph); (3) the
|
|
||||||
provenance/anomaly-detection subsystem end-to-end; (4) error handling in
|
|
||||||
`clawhdf5-io`, `clawhdf5-migrate`, `clawhdf5-py`, and the `clawhdf5` facade.
|
|
||||||
|
|
||||||
`clawhdf5-accel` (SIMD dispatch), `clawhdf5-gpu` (no unsafe code, wgpu-mediated),
|
|
||||||
`clawhdf5-io`, `clawhdf5-py`, and the `clawhdf5` facade crate were all found
|
|
||||||
already sound for the failure modes investigated — no items proposed for
|
|
||||||
those beyond what's listed below. Say so once here rather than padding the
|
|
||||||
list with manufactured items.
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Section A — Parser crash safety (crafted-file DoS)
|
## Priority key
|
||||||
|
- **P0** — correctness/security bug reachable from untrusted input (crafted file, external
|
||||||
These three files use raw `offset + N > file_data.len()` arithmetic instead
|
caller), should block release.
|
||||||
of the `checked_add`-based `ensure_len` helper that every other parser in
|
- **P1** — real functional gap or measurable perf cost on a hot path.
|
||||||
`clawhdf5-format` already uses (established pattern: `btree_v2.rs`,
|
- **P2** — consistency/hardening/API-quality; safe to defer.
|
||||||
`global_heap.rs`, `fractal_heap.rs`, `shared_message.rs`, `local_heap.rs`'s
|
|
||||||
own `ensure_len`, etc.). On a crafted file with an address field close to
|
|
||||||
`u64::MAX`, the addition overflows — panicking in debug builds, silently
|
|
||||||
wrapping in the release profile (no `overflow-checks` set anywhere in the
|
|
||||||
workspace `Cargo.toml`), after which the bounds check passes falsely and the
|
|
||||||
next slice operation panics anyway. Net effect either way: a crafted file
|
|
||||||
crashes the parser instead of returning `Err`.
|
|
||||||
|
|
||||||
### INT-01 — `crates/clawhdf5-format/src/fixed_array.rs`, `crates/clawhdf5-format/src/extensible_array.rs`
|
|
||||||
**Problem:** Six unguarded-addition bounds checks: `FixedArrayHeader::parse`
|
|
||||||
(fixed_array.rs:69), the data-block header check in
|
|
||||||
`read_fixed_array_chunks` (fixed_array.rs:129), `ExtensibleArrayHeader::parse`
|
|
||||||
(extensible_array.rs:101), `read_extensible_array_data_block`
|
|
||||||
(extensible_array.rs:278), the index-block parse (extensible_array.rs:429),
|
|
||||||
and the super-block parse (extensible_array.rs:630). The offending offsets
|
|
||||||
(`data_block_address`/`index_block_address`) come from `DataLayout::parse`
|
|
||||||
(`data_layout.rs`, chunk_index_type 3/4 branches, ~lines 460–470), which only
|
|
||||||
special-cases the exact all-`0xFF` sentinel via `is_undefined` — any other
|
|
||||||
near-max value passes through unchanged.
|
|
||||||
**Change:** Replace every raw `offset + N > file_data.len()` in both files
|
|
||||||
with the `checked_add`-based `ensure_len` pattern already used elsewhere in
|
|
||||||
the crate (e.g. mirror `local_heap.rs`'s `ensure_len`).
|
|
||||||
|
|
||||||
### INT-02 — `crates/clawhdf5-format/src/symbol_table.rs`
|
|
||||||
**Problem:** `SymbolTableNode::parse` (line 83) uses raw
|
|
||||||
`offset + 8 > file_data.len()`, unlike `read_offset` in the same file which
|
|
||||||
already uses `checked_add`. `offset` is a SNOD address taken verbatim from a
|
|
||||||
v1 B-tree leaf entry and passed straight through by `group_v1.rs:49` with no
|
|
||||||
sentinel/range check — a crafted v1-group B-tree leaf with a near-`u64::MAX`
|
|
||||||
child pointer overflows the check the same way as INT-01.
|
|
||||||
**Change:** Use `offset.checked_add(8)` (`ensure_len` pattern) at line 83.
|
|
||||||
Note: the `entries_start + num_symbols * entry_size` addition at line 106 has
|
|
||||||
the same raw-arithmetic style, but `num_symbols` is `u16` so the multiply
|
|
||||||
itself can't overflow — lower priority, but worth fixing for consistency in
|
|
||||||
the same pass.
|
|
||||||
|
|
||||||
### INT-03 — `crates/clawhdf5-format/src/datatype.rs`
|
|
||||||
**Problem:** `Datatype::parse` recurses into itself with no depth counter
|
|
||||||
(`grep -n "depth" datatype.rs` — zero hits) for Compound members (lines 361,
|
|
||||||
387), Enumeration base type (line 418), VariableLength base type (line 471),
|
|
||||||
and Array base type (lines 497, 518). A message data size is capped at
|
|
||||||
`u16::MAX` (65535 bytes; see `object_header.rs:141` v1, `object_header.rs:411`
|
|
||||||
v2), so a crafted Compound-of-Compound-of-Compound... datatype message can
|
|
||||||
nest ~8000 levels deep — enough to blow the stack, and materially worse on
|
|
||||||
the project's documented no_std/embedded targets (`thumbv7em-none-eabihf`,
|
|
||||||
per `CHANGELOG.md`) where available stack is a few KB. The changelog records
|
|
||||||
this exact class of bug already fixed for the N-Bit filter's type tree, but
|
|
||||||
that fix was never applied to the general `Datatype::parse` reader used for
|
|
||||||
every Dataspace/Attribute/Dataset datatype message.
|
|
||||||
**Change:** Thread a `depth: u16` counter through `Datatype::parse`'s
|
|
||||||
recursive call sites (mirror `object_header.rs`'s continuation-depth guards)
|
|
||||||
and return a new `FormatError::NestingDepthExceeded` past a fixed limit
|
|
||||||
(suggest 64).
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Section B — Provenance & anomaly detection
|
## Group A — `clawhdf5-agent`: provenance/security is unwired (headline finding)
|
||||||
|
|
||||||
The most significant finding of this brief: **the provenance/anomaly
|
### INT-01 — Wire `WriteAnomalyDetector` / `ProvenanceStore` into the actual write path [P0]
|
||||||
subsystem exists and is tested, but is never invoked from the real save/load
|
**Files:** `crates/clawhdf5-agent/src/storage.rs` (save/save_batch path), `crates/clawhdf5-agent/src/provenance.rs`, `crates/clawhdf5-agent/src/anomaly.rs`, `crates/clawhdf5-agent/src/lib.rs`
|
||||||
path.** It's a fully-built, unused API surface, not an active control.
|
|
||||||
|
|
||||||
### INT-04 — `crates/clawhdf5-agent/src/provenance.rs`, `crates/clawhdf5-agent/src/anomaly.rs`, `crates/clawhdf5-agent/src/lib.rs`
|
**Problem:** `ROADMAP.md` Track 5 ("Memory Security & Provenance") is marked 🟢 Complete, listing
|
||||||
**Problem:** `ProvenanceStore`, `MemoryProvenance::new`, `verify_integrity`,
|
source attribution, write anomaly detection, source isolation, and integrity verification as
|
||||||
`mark_verified`, `WriteAnomalyDetector`, `record_write`,
|
done. The types exist and are unit-tested in isolation — but `grep -rn
|
||||||
`check_pattern_anomaly`, `check_rate_anomaly`, `check_source_anomaly` have
|
"WriteAnomalyDetector\|ProvenanceStore\|SourceIsolation"` across every file in
|
||||||
zero callers outside their own module/tests. `lib.rs` only declares
|
`clawhdf5-agent` *except* `provenance.rs`/`anomaly.rs` themselves returns nothing.
|
||||||
`pub mod provenance;` / `pub mod anomaly;` (lines 22, 33) — neither is
|
`storage.rs` (the real save/delete/replay path) never imports or calls into either module.
|
||||||
referenced from `HDF5Memory::save_or_update` (~line 495) or the WAL replay
|
Nothing in `HDF5Memory::save`/`save_batch` populates a `ProvenanceStore`, runs a rate/pattern
|
||||||
path (`wal.rs::replay_into_cache`, line 311). Concretely: the 15
|
check, or routes through `SourceIsolation`. In its current state this is a library the crate
|
||||||
injection-pattern checks, rate limiting, and content-hash integrity
|
ships but never uses on itself — every memory write today has **no** rate limiting, no pattern
|
||||||
verification described as shipped in `ROADMAP.md` Track 5 never execute
|
detection, and no provenance recorded, contrary to what the roadmap and any consumer relying on
|
||||||
during normal library usage today.
|
it would assume.
|
||||||
**Change:** Call `ProvenanceStore::add` and
|
|
||||||
`WriteAnomalyDetector::record_write` + the `check_*` methods from
|
|
||||||
`HDF5Memory::save_or_update`, and call `verify_integrity` from the
|
|
||||||
open/load path (surfacing a mismatch to the caller, not panicking). If the
|
|
||||||
intent is genuinely opt-in-only, that's a legitimate design choice, but it
|
|
||||||
must be documented prominently at the crate root / in `CLAUDE.md` — right
|
|
||||||
now it reads as an active control and isn't one.
|
|
||||||
|
|
||||||
### INT-05 — `crates/clawhdf5-agent/src/lib.rs` (`MemoryEntry.source_channel`, ~line 167), `crates/clawhdf5-agent/src/consolidation.rs` (`ConsolidationEngine::add_memory`, ~line 205)
|
**Change:** In `storage.rs`'s save/save_batch entry point(s), construct/thread a
|
||||||
**Problem:** `source_channel: String` is free text set entirely by the
|
`WriteAnomalyDetector` and `ProvenanceStore` (or accept them as constructor params on the
|
||||||
caller of `save`/`save_or_update` — nothing validates it against an
|
memory-store struct so callers can configure `AnomalyConfig`), call `record_write` +
|
||||||
allowlist, so a write can claim `source_channel = "system"` or any other
|
`check_rate_anomaly`/`check_pattern_anomaly` before persisting each chunk, and call
|
||||||
privileged-looking label. Separately, `add_memory` takes `source:
|
`ProvenanceStore::add` with the resulting `MemoryProvenance` alongside the write. Surface
|
||||||
MemorySource` (User/System/Tool/Retrieval/Correction) as a plain parameter;
|
anomaly alerts through the existing error/result type rather than silently dropping them
|
||||||
`MemorySource::Correction`/`System` get elevated importance weighting in
|
(decide via a config flag whether pattern/rate hits are hard-rejects or soft warnings — a hard
|
||||||
`score_correction` (~line 133), so any caller can claim a trust level the
|
reject changes public API behavior, a warning is additive). Add an integration test that writes
|
||||||
content doesn't warrant.
|
a chunk containing one of the 15 suspicious patterns and asserts the alert actually fires
|
||||||
**Change:** Derive `MemorySource`/`source_channel` at the actual trust
|
through the public save path (not just the unit-level `WriteAnomalyDetector` test).
|
||||||
boundary (the ingestion layer that knows the true origin), not as a
|
|
||||||
caller-supplied argument to the storage API. At minimum, gate
|
|
||||||
`MemorySource::System`/`Correction` construction behind a distinct
|
|
||||||
constructor not exposed to the same call path as untrusted content.
|
|
||||||
|
|
||||||
### INT-06 — `crates/clawhdf5-agent/src/anomaly.rs` (`check_pattern_anomaly`, ~lines 192–195)
|
|
||||||
**Problem:** Matching is `chunk.to_lowercase().contains(pattern.as_str())` —
|
|
||||||
plain literal-substring test after case folding only. Inserting any
|
|
||||||
character inside a pattern (extra whitespace, a zero-width character, `.`
|
|
||||||
between letters) or substituting a homoglyph for one Latin letter defeats
|
|
||||||
every one of the 15 injection patterns; there's no Unicode
|
|
||||||
confusable-normalization or punctuation/whitespace stripping.
|
|
||||||
**Change:** Normalize input before matching (strip zero-width characters and
|
|
||||||
punctuation, apply NFKC + confusable-folding) or switch to fuzzy/token-based
|
|
||||||
detection instead of raw `contains`.
|
|
||||||
|
|
||||||
### INT-07 — `crates/clawhdf5-agent/src/anomaly.rs` (`check_rate_anomaly`, ~lines 149–151)
|
|
||||||
**Problem:** The per-minute rate check uses a single global sliding window
|
|
||||||
(`self.window.len()`) across all sessions/sources combined. One noisy
|
|
||||||
session can trip the shared window without the alert naming the offending
|
|
||||||
session (unlike the separate cumulative `max_writes_per_session` check,
|
|
||||||
which does name it); conversely, many distinct low-volume sessions can
|
|
||||||
jointly flood the shared window without any individual one tripping its own
|
|
||||||
per-session limit.
|
|
||||||
**Change:** Key the sliding window by session/source (or add a per-source
|
|
||||||
rolling count) so the rate check attributes to, and can throttle, the actual
|
|
||||||
offender.
|
|
||||||
|
|
||||||
### INT-08 — `crates/clawhdf5-format/src/provenance.rs` (`verify_dataset`, ~line 126)
|
|
||||||
**Problem:** The SHA-256 content hash is written automatically on save when
|
|
||||||
`db.provenance` is set (`file_writer.rs` ~1061–1068, gated on the
|
|
||||||
`provenance` feature), but `verify_dataset` is only ever called from test
|
|
||||||
files — no reader/open path in `clawhdf5-io` or the `clawhdf5` facade calls
|
|
||||||
it. A corrupted dataset is silently readable with no automatic integrity
|
|
||||||
check; the write-side machinery exists but nothing consumes it. (Note:
|
|
||||||
`CHANGELOG.md` already documents that this hash is unkeyed/tamper-*evident*
|
|
||||||
not tamper-*proof* — that's accepted and not re-flagged here; this item is
|
|
||||||
about it never being invoked at all, not about its cryptographic strength.)
|
|
||||||
**Change:** Optionally call `verify_dataset` on dataset open (behind the
|
|
||||||
`provenance` feature) and surface a mismatch as a typed error/warning to the
|
|
||||||
caller instead of leaving verification purely opt-in/manual.
|
|
||||||
|
|
||||||
### INT-09 — `crates/clawhdf5-agent/src/wal.rs` (`WalFile::read_entries`, ~lines 219–272)
|
|
||||||
**Problem:** Two related gaps. (a) WAL v2's per-entry CRC32 covers only each
|
|
||||||
entry's own bytes — there's no sequence number or entry-chaining, so entries
|
|
||||||
could be reordered, duplicated, or spliced (e.g. a `Tombstone` moved
|
|
||||||
before/after its target `Save`) while every individual entry still passes
|
|
||||||
its own CRC check, silently changing replayed cache state. (b) The
|
|
||||||
`WAL_VERSION_LEGACY_NO_CRC` branch (~lines 260–266) does no CRC verification
|
|
||||||
at all, and the version byte itself is a single unauthenticated byte — since
|
|
||||||
`read_entries` is a public standalone API (not just reached via `open()`'s
|
|
||||||
one-time migrate-on-read), flipping that byte from `2` to `1` silently
|
|
||||||
downgrades every subsequent entry in the file to the fully-unverified
|
|
||||||
pre-hardening parser.
|
|
||||||
**Change:** Add a monotonic sequence number or entry-chaining (CRC/hash
|
|
||||||
including the previous entry's CRC) to detect reordering/splicing. Restrict
|
|
||||||
the legacy-no-CRC branch to the `open()` migration path only, or emit a
|
|
||||||
warning when `read_entries` falls back to it via any other entry point.
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Section C — Correctness bug (panic on valid, untrusted input)
|
### INT-02 — `check_pattern_anomaly` is trivially bypassed substring matching [P1]
|
||||||
|
**File:** `crates/clawhdf5-agent/src/anomaly.rs:192-211`
|
||||||
|
|
||||||
### INT-10 — `crates/clawhdf5-migrate/src/validate.rs` (`truncate`, lines 143–149)
|
|
||||||
**Problem:**
|
**Problem:**
|
||||||
```rust
|
```rust
|
||||||
fn truncate(s: &str) -> String {
|
let lower = chunk.to_lowercase();
|
||||||
if s.len() <= 40 {
|
for pattern in &self.config.suspicious_patterns {
|
||||||
s.to_string()
|
if lower.contains(pattern.as_str()) { ... }
|
||||||
} else {
|
|
||||||
format!("{}…", &s[..40]) // byte-index slice, not char-boundary safe
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
`s` is `source.chunk` — arbitrary UTF-8 text read from the source SQLite
|
Matching is raw case-folded substring containment against 15 fixed literals (`"ignore
|
||||||
database, called from the chunk-text mismatch branch of `validate_hdf5`
|
previous"`, `"system:"`, …). Trivially defeated by inserting extra whitespace/punctuation
|
||||||
(~line 58) whenever migrated text doesn't exactly match the source. This is
|
(`"ignore previous"`), splitting the phrase across two separate writes (checks are per-chunk,
|
||||||
the default (non-`--dry-run`) validation path, not test-only code — the file
|
not per-session-buffer), or any non-ASCII obfuscation. As a poisoning-resistance control this
|
||||||
has no `#[cfg(test)]` block. If a multi-byte character (emoji, accented
|
currently only stops the laziest attacks.
|
||||||
letter, CJK, etc.) straddles byte offset 40, `&s[..40]` panics with "byte
|
|
||||||
index 40 is not a char boundary" instead of producing the diagnostic the
|
**Change:** Normalize input before matching (collapse whitespace/strip zero-width and combining
|
||||||
code exists to report.
|
characters), and consider word-boundary-tolerant/regex matching instead of raw `contains`.
|
||||||
**Change:** Truncate on a char boundary, e.g.
|
Document the remaining limitation (this is a heuristic filter, not a guarantee) rather than
|
||||||
`let cut = s.char_indices().nth(40).map(|(i, _)| i).unwrap_or(s.len()); format!("{}…", &s[..cut])`.
|
implying full poisoning resistance.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Section D — Performance (query-time hot paths, `clawhdf5-agent`)
|
### INT-03 — `session_counts` grows unbounded and is fully rescanned on every rate check [P1]
|
||||||
|
**File:** `crates/clawhdf5-agent/src/anomaly.rs:109, 126-133, 170-181`
|
||||||
|
|
||||||
`search.rs`, `vector_search.rs`, `hybrid.rs`, `reranker.rs`, `confidence.rs`,
|
**Problem:** `session_counts: HashMap<String, u32>` is incremented on every `record_write` and
|
||||||
`temporal.rs`, `ivf.rs`, `pq.rs`, and `gpu_search.rs` were reviewed and found
|
never pruned — unlike `window` (which has a 60s sliding-window prune). A caller that creates
|
||||||
already efficient (temporal index uses `partition_point` binary search,
|
many distinct `session_id` values (fully caller-controlled strings) grows this map without
|
||||||
hybrid merge uses `HashMap` accumulation not nested loops, no gratuitous
|
bound for the process lifetime. `check_rate_anomaly`'s session-level loop
|
||||||
clones in the batch vector paths) — no items proposed there.
|
(`for (session, &count) in &self.session_counts`) then scans the *entire* historical map on
|
||||||
|
every single check call, so per-write cost grows with total lifetime session count, not
|
||||||
|
current activity.
|
||||||
|
|
||||||
### INT-11 — `crates/clawhdf5-agent/src/bm25.rs` (`BM25Index::search`, ~lines 118–141)
|
**Change:** Bound `session_counts` with an LRU/TTL eviction policy, or track only counts within
|
||||||
**Problem:** The WAND top-k threshold update calls
|
the same rolling window used for `window` (see INT-05, which is closely related — the
|
||||||
`top_k_scores.sort_by(...)` over the full `k`-sized buffer for every matching
|
session-level check has its own separate bug on top of this).
|
||||||
document that beats the running threshold (twice in the `>= k` branch), plus
|
|
||||||
another full sort on reaching exactly `k` results. For `m` matching
|
|
||||||
documents this is `O(m·k log k)` where a heap gives `O(m log k)`.
|
|
||||||
**Change:** Replace `top_k_scores: Vec<f32>` with a min-heap
|
|
||||||
(`BinaryHeap<Reverse<f32>>`) of size `k`; pop/push instead of sort-and-index.
|
|
||||||
|
|
||||||
### INT-12 — `crates/clawhdf5-agent/src/knowledge.rs` (`KnowledgeCache::resolve_or_create`, lines 304–330)
|
---
|
||||||
**Problem:** `self.entities.iter().map(|e| levenshtein(&lower_name,
|
|
||||||
&e.name.to_lowercase()))` allocates a fresh lowercased `String` for every
|
|
||||||
entity on every resolution call (this runs per extracted mention during
|
|
||||||
entity/relation extraction) and never short-circuits even on an exact
|
|
||||||
`dist == 0` match — it scores every remaining entity regardless.
|
|
||||||
**Change:** Cache a lowercased name on `Entity` to avoid the
|
|
||||||
per-call allocation, and break out of the scan as soon as a `dist == 0`
|
|
||||||
match is found.
|
|
||||||
|
|
||||||
### INT-13 — `crates/clawhdf5-agent/src/knowledge.rs` (`bfs_neighbors` lines 339–378, `spreading_activation` lines 435–495, `get_relations_from`/`get_relations_to` lines 247–254)
|
### INT-04 — Sliding-window prune only inspects the front of the deque [P1]
|
||||||
**Problem:** All four functions filter/scan the *entire* `self.relations`
|
**File:** `crates/clawhdf5-agent/src/anomaly.rs:134-139`
|
||||||
list per node processed (`O(V·E)` for BFS instead of `O(V+E)`;
|
|
||||||
`O(max_steps · active_nodes · relations)` for spreading activation), and
|
|
||||||
`bfs_neighbors` additionally calls `self.get_entity(neighbour_id)` per
|
|
||||||
discovered neighbor, itself an `O(n)` linear `.find()` over `self.entities`.
|
|
||||||
**Change:** Build (or maintain incrementally on `add_entity`/`add_relation`)
|
|
||||||
a `HashMap<u64, Vec<usize>>` adjacency index and a `HashMap<u64, usize>`
|
|
||||||
id→index map, shared across all four functions, replacing the linear scans
|
|
||||||
with O(1)/O(degree) lookups.
|
|
||||||
|
|
||||||
### INT-14 — `crates/clawhdf5-agent/src/consolidation.rs` (`ConsolidationEngine::add_memory`, lines 212–217)
|
|
||||||
**Problem:**
|
**Problem:**
|
||||||
```rust
|
```rust
|
||||||
let working: Vec<MemoryRecord> = self.records.iter()
|
self.window.push_back(event);
|
||||||
.filter(|r| r.tier == MemoryTier::Working)
|
let cutoff = self.last_timestamp - 60.0;
|
||||||
.cloned()
|
while self.window.front().is_some_and(|e| e.timestamp < cutoff) {
|
||||||
.collect();
|
self.window.pop_front();
|
||||||
|
}
|
||||||
```
|
```
|
||||||
`score_surprise` (the only consumer) only reads `r.embedding` by reference —
|
`WriteEvent.timestamp` is caller-supplied (not sampled from a clock inside this type), so
|
||||||
the full clone (chunk text + embedding `Vec<f32>`) of every working-tier
|
nothing prevents an out-of-order/backdated event from landing behind the front after a more
|
||||||
record is discarded immediately after use.
|
recent one. Because eviction only ever looks at `front()`, a single out-of-order event
|
||||||
**Change:** Collect `Vec<&MemoryRecord>` (or iterate the filtered
|
permanently corrupts the window — old entries behind it are never pruned, so
|
||||||
`self.records` directly, passing an iterator of `&[f32]`) instead of
|
`check_rate_anomaly`'s window-length count over-reports forever (and can be intentionally
|
||||||
`.cloned()`.
|
inflated by a caller that varies timestamp ordering).
|
||||||
|
|
||||||
### INT-15 — `crates/clawhdf5-agent/src/consolidation.rs` (`consolidate`, lines 284–291 and 345–351)
|
**Change:** Prune by retaining only entries `>= cutoff` across the whole deque
|
||||||
**Problem:** `self.records.retain(|r| !evict_ids.contains(&r.id))` where
|
(`self.window.retain(|e| e.timestamp >= cutoff)`), or reject/clamp non-monotonic timestamps in
|
||||||
`evict_ids: Vec<u64>` — `retain` calls `.contains()` (linear scan) for every
|
`record_write` and document that `WriteEvent.timestamp` must be non-decreasing per detector
|
||||||
record in `self.records`, giving `O(n·m)` cost (n = records, m = eviction
|
instance.
|
||||||
count) on both the Working-tier eviction (line 289) and Episodic-tier
|
|
||||||
eviction (line 350), on every consolidation tick.
|
|
||||||
**Change:** Build `evict_ids` as a `HashSet<u64>` for O(1) membership checks.
|
|
||||||
|
|
||||||
### INT-16 — `crates/clawhdf5-agent/src/blas_search.rs` (`blas_cosine_batch`, lines 30–39), `crates/clawhdf5-agent/src/accelerate_search.rs` (`accelerate_cosine_batch_vecs`, lines 164–173)
|
|
||||||
**Problem:** `cache.embeddings` is stored as `Vec<Vec<f32>>`; both functions
|
|
||||||
re-flatten the entire corpus into a fresh `Vec<f32>`
|
|
||||||
(`flat.extend_from_slice(&vectors[i])` per non-tombstoned vector) on *every
|
|
||||||
single query* before running the actual BLAS/Accelerate matmul — an
|
|
||||||
`O(N·dim)` copy paid per query when the `fast-math` feature is enabled. The
|
|
||||||
fix pattern already exists in-file: `blas_cosine_batch_flat` (same file,
|
|
||||||
lines 89–142) has an `all_active` fast path that skips this copy when
|
|
||||||
reading from a pre-flattened buffer directly — it's just not used for the
|
|
||||||
`Vec<Vec<f32>>` call sites.
|
|
||||||
**Change:** Maintain a persistent flat embedding buffer alongside
|
|
||||||
`cache.embeddings` (updated incrementally on insert/delete) and call
|
|
||||||
`blas_cosine_batch_flat` instead of `blas_cosine_batch` from both files'
|
|
||||||
query paths.
|
|
||||||
|
|
||||||
### INT-17 — `crates/clawhdf5-agent/src/entity_extract.rs` (`dedup_overlapping`, lines 302–313)
|
|
||||||
**Problem:** `result.iter().any(|existing| ...)` checks every candidate
|
|
||||||
entity against all already-accepted entities — `O(n²)` in
|
|
||||||
entities-per-extraction-call. This runs at ingestion time (every memory
|
|
||||||
save), not query time, and is bounded by entities-per-chunk (typically
|
|
||||||
small), so it's lower priority than INT-11 through INT-16.
|
|
||||||
**Change:** If profiling shows this matters in practice (large chunks with
|
|
||||||
many extracted entities), replace with a spatial/interval-based overlap
|
|
||||||
index; otherwise leave as-is — flagging for completeness, not urgency.
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Summary table
|
### INT-05 — Session-level rate check uses a lifetime cumulative counter, not a rate [P1]
|
||||||
|
**File:** `crates/clawhdf5-agent/src/anomaly.rs:170-181` (`check_rate_anomaly`)
|
||||||
|
|
||||||
| INT | Area | File(s) | Category |
|
**Problem:** `max_writes_per_session` is compared against `session_counts[session]`, which is
|
||||||
|-----|------|---------|----------|
|
incremented forever and never reset (see INT-03). This measures "how old is this session," not
|
||||||
| INT-01 | Parser crash safety | `fixed_array.rs`, `extensible_array.rs` | Security |
|
"is this session currently abusive" — any long-lived legitimate session (e.g. a persistent
|
||||||
| INT-02 | Parser crash safety | `symbol_table.rs` | Security |
|
agent) permanently trips the alert once past the threshold regardless of pace, while a burst of
|
||||||
| INT-03 | Parser crash safety | `datatype.rs` | Security |
|
writes in a brand-new session under the threshold is missed even if it's the real anomaly.
|
||||||
| INT-04 | Provenance wiring | `provenance.rs`, `anomaly.rs`, `lib.rs` | Provenance |
|
|
||||||
| INT-05 | Source trust boundary | `lib.rs`, `consolidation.rs` | Provenance |
|
|
||||||
| INT-06 | Anomaly pattern bypass | `anomaly.rs` | Provenance |
|
|
||||||
| INT-07 | Rate-limit attribution | `anomaly.rs` | Provenance |
|
|
||||||
| INT-08 | Integrity verification unwired | `clawhdf5-format/provenance.rs` | Provenance |
|
|
||||||
| INT-09 | WAL ordering/legacy fallback | `wal.rs` | Provenance |
|
|
||||||
| INT-10 | Char-boundary panic | `clawhdf5-migrate/validate.rs` | Correctness |
|
|
||||||
| INT-11 | WAND top-k re-sort | `bm25.rs` | Performance |
|
|
||||||
| INT-12 | Entity resolution scan | `knowledge.rs` | Performance |
|
|
||||||
| INT-13 | Graph traversal scan | `knowledge.rs` | Performance |
|
|
||||||
| INT-14 | Unneeded clone | `consolidation.rs` | Performance |
|
|
||||||
| INT-15 | O(n·m) eviction | `consolidation.rs` | Performance |
|
|
||||||
| INT-16 | Per-query re-flatten | `blas_search.rs`, `accelerate_search.rs` | Performance |
|
|
||||||
| INT-17 | O(n²) dedup (low priority) | `entity_extract.rs` | Performance |
|
|
||||||
|
|
||||||
## Follow-ups for the coding phase
|
**Change:** Make this a rate — either measure session writes within the existing 60s rolling
|
||||||
|
window (reuse `window`, filtered by `session_id`) or add a separate per-session rolling window,
|
||||||
|
rather than an unbounded lifetime total.
|
||||||
|
|
||||||
TASK: INT-01 — Fix unchecked-overflow bounds checks in fixed_array.rs/extensible_array.rs
|
---
|
||||||
TASK: INT-02 — Fix unchecked-overflow bounds check in symbol_table.rs
|
|
||||||
TASK: INT-03 — Add recursion-depth guard to Datatype::parse
|
### INT-06 — `bfs_neighbors` re-scans all relations on every queue pop [P1]
|
||||||
TASK: INT-04 — Wire provenance.rs/anomaly.rs into save/load path
|
**File:** `crates/clawhdf5-agent/src/knowledge.rs:339-378`, hot loop at 352-365
|
||||||
TASK: INT-05 — Enforce source-of-truth for MemorySource/source_channel at trust boundary
|
|
||||||
TASK: INT-06 — Harden anomaly pattern matching against whitespace/homoglyph bypass
|
**Problem:**
|
||||||
TASK: INT-07 — Make anomaly rate-limit window per-source
|
```rust
|
||||||
TASK: INT-08 — Wire clawhdf5-format provenance verify_dataset into read path
|
let neighbours: Vec<u64> = self.relations.iter().filter_map(|r| { ... }).collect();
|
||||||
TASK: INT-09 — Add WAL entry ordering protection and restrict legacy no-CRC fallback
|
```
|
||||||
TASK: INT-10 — Fix byte-index slice panic in clawhdf5-migrate validate.rs truncate()
|
runs once per node dequeued during BFS, giving `O(visited_nodes × total_relations)` total cost.
|
||||||
TASK: INT-11 — Replace BM25 top-k re-sort with a min-heap
|
`get_subgraph` (`knowledge.rs:387-417`) calls `bfs_neighbors` once per seed node, multiplying
|
||||||
TASK: INT-12 — Cache lowercased entity names and early-exit in resolve_or_create
|
the cost again. On a graph with a non-trivial relation count this is the dominant cost of any
|
||||||
TASK: INT-13 — Add adjacency index for knowledge graph traversal functions
|
graph traversal query — the kind of memory-graph read the whole crate exists to serve
|
||||||
TASK: INT-14 — Avoid cloning working-tier records in consolidation add_memory
|
efficiently.
|
||||||
TASK: INT-15 — Use HashSet for eviction ID membership checks in consolidation
|
|
||||||
TASK: INT-16 — Use persistent flat embedding buffer in blas_search/accelerate_search
|
**Change:** Build an adjacency `HashMap<u64, Vec<u64>>` once (either eagerly maintained on
|
||||||
TASK: INT-17 — (optional/low-priority) revisit entity_extract dedup_overlapping if profiling shows it matters
|
insert/delete, or lazily built and cached with invalidation on mutation) instead of
|
||||||
|
linear-scanning `self.relations` per hop.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-07 — Quadratic eviction via `Vec::contains` inside `retain` [P1]
|
||||||
|
**File:** `crates/clawhdf5-agent/src/consolidation.rs:345-350`
|
||||||
|
|
||||||
|
**Problem:**
|
||||||
|
```rust
|
||||||
|
let evict_ids: Vec<u64> = episodic_indices[..evict_n].iter().map(|&i| self.records[i].id).collect();
|
||||||
|
self.records.retain(|r| !evict_ids.contains(&r.id));
|
||||||
|
```
|
||||||
|
`retain` invokes the closure once per record; `Vec::contains` is `O(m)`. Worst case this is
|
||||||
|
`O(n·m)` per consolidation pass, run periodically over the full record set.
|
||||||
|
|
||||||
|
**Change:** Collect `evict_ids` into a `HashSet<u64>` before the `retain` call — `O(n)` lookup
|
||||||
|
per record instead of `O(m)`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-08 — `MediaRef.checksum` is unkeyed FNV-1a but named/documented as a checksum [P2]
|
||||||
|
**File:** `crates/clawhdf5-agent/src/multimodal.rs:96-97, 104, 116, 127`; compare
|
||||||
|
`crates/clawhdf5-agent/src/provenance.rs:16-19`
|
||||||
|
|
||||||
|
**Problem:** `provenance.rs` already carries an explicit doc comment (and the CHANGELOG has a
|
||||||
|
dedicated "doc-only" entry) clarifying that its FNV-1a content hash is unkeyed and detects only
|
||||||
|
accidental corruption, not tampering. `multimodal.rs`'s `MediaRef.checksum` field uses the same
|
||||||
|
FNV-1a hash for the same purpose but has no equivalent caveat, and the field name "checksum"
|
||||||
|
(vs. "hash") reads as an integrity guarantee to a downstream consumer (e.g. something in
|
||||||
|
ZeroClaw deciding whether to trust/reuse a cached media reference).
|
||||||
|
|
||||||
|
**Change:** Either rename the field (e.g. `content_fingerprint`) or add the same
|
||||||
|
non-tamper-evidence doc comment already used in `provenance.rs`, so the two unkeyed-hash usages
|
||||||
|
in the crate are consistently documented.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Group B — `clawhdf5-format` / `clawhdf5-io`: untrusted-file parsing gaps
|
||||||
|
|
||||||
|
The 2026-08-05 hardening pass (see CHANGELOG "Security" section) already covers
|
||||||
|
`chunked_read.rs`/`data_read.rs`/`local_heap.rs`/`btree_v1.rs` with `ensure_len`-style overflow
|
||||||
|
guards, a B-tree recursion-depth guard, and a `fuzz_dataset_read` target. ROADMAP.md explicitly
|
||||||
|
flags "a full manual audit of every indexing site is still open" as unfinished — the following
|
||||||
|
are concrete gaps found in that follow-up, in files/paths the prior pass did not touch.
|
||||||
|
|
||||||
|
### INT-09 — `btree_v2.rs` recursive tree-walk has no depth cap (stack-overflow DoS) [P0]
|
||||||
|
**File:** `crates/clawhdf5-format/src/btree_v2.rs:264-403` (`collect_internal_records`), entry
|
||||||
|
at `176-213` (`collect_btree_v2_records`)
|
||||||
|
|
||||||
|
**Problem:** `BTreeV2Header.depth: u16` (defined at line 21) is parsed straight from file bytes
|
||||||
|
with no upper bound. `collect_internal_records` recurses with `child_depth = depth - 1` (line
|
||||||
|
299) down to 0 with no depth-remaining cap — unlike the cyclic/self-referencing-index guards
|
||||||
|
already added elsewhere in this hardening cycle (`fractal_heap.rs`, `object_header.rs`'s
|
||||||
|
`depth_remaining` params, `filters.rs`'s `NBIT_MAX_DEPTH`). A crafted v2 B-tree header claiming
|
||||||
|
`depth = 65535` (paired with a matching on-disk `"BTIN"` internal-node chain, or even a node
|
||||||
|
that points back into itself since nothing here detects cycles either) drives ~65k stack frames
|
||||||
|
of native recursion — an abort/crash from a small crafted file. This is reachable from real
|
||||||
|
parse paths: `group_v2.rs:82`, `shared_message.rs:368`, `attribute.rs:384` (dense group/dense
|
||||||
|
attribute listings — a realistic file feature, not an obscure one).
|
||||||
|
|
||||||
|
**Change:** Thread a `depth_remaining: u16` (or similar) cap through
|
||||||
|
`collect_btree_v2_records`/`collect_internal_records`, capped at some sane bound (e.g. 64,
|
||||||
|
consistent with `NBIT_MAX_DEPTH`'s style elsewhere in this codebase), returning a `FormatError`
|
||||||
|
instead of recursing past it.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-10 — `fuzz_btree_v2` never exercises the recursive traversal where INT-09 lives [P1]
|
||||||
|
**File:** `crates/clawhdf5-format/fuzz/fuzz_targets/fuzz_btree_v2.rs` (or wherever this target
|
||||||
|
lives under `crates/clawhdf5-format/fuzz/`)
|
||||||
|
|
||||||
|
**Problem:** The existing target only calls `BTreeV2Header::parse` — it never calls
|
||||||
|
`collect_btree_v2_records`, so the actual tree-walk (the code path with the depth-recursion bug
|
||||||
|
in INT-09) has zero fuzz coverage today, despite the file being in scope for a target already
|
||||||
|
named after it.
|
||||||
|
|
||||||
|
**Change:** Extend `fuzz_btree_v2` to also invoke `collect_btree_v2_records` on the parsed
|
||||||
|
header against the fuzz input, so the recursive traversal gets the same adversarial coverage the
|
||||||
|
header parse already has. Land this alongside INT-09 so the fix is locked in by the fuzzer, not
|
||||||
|
just a manual patch.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-11 — Unchecked multiplication of file-derived sizes in fractal-heap size math [P0]
|
||||||
|
**File:** `crates/clawhdf5-format/src/fractal_heap.rs:479-496` (`block_size_for_row`,
|
||||||
|
`indirect_block_heap_size`)
|
||||||
|
|
||||||
|
**Problem:**
|
||||||
|
```rust
|
||||||
|
sbs * (1u64 << (row - 1)) // line ~484
|
||||||
|
total += self.block_size_for_row(row) * tw // line ~493
|
||||||
|
```
|
||||||
|
use plain `*` on `starting_block_size`/`table_width`, both read from the FRHP header with no
|
||||||
|
upper-bound validation. A crafted large `starting_block_size` combined with enough rows/columns
|
||||||
|
overflows `u64`; under `overflow-checks` (on for debug/fuzz builds, and optionally enabled in
|
||||||
|
release) this panics — a DoS abort from a malformed fractal heap, the same bug class the
|
||||||
|
2026-08-05 pass already fixed in sibling files.
|
||||||
|
|
||||||
|
**Change:** Replace with `checked_mul`/`saturating_mul` and propagate a `FormatError` on
|
||||||
|
overflow, matching the `ensure_len`/checked-arithmetic idiom already used in
|
||||||
|
`chunked_read.rs`/`local_heap.rs`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-12 — Unbounded allocation from an unvalidated length before any data is read [P0]
|
||||||
|
**Files:**
|
||||||
|
- `crates/clawhdf5-io/src/subfiling.rs:206-210` (`SubfileManager::read_at`) —
|
||||||
|
`Vec::with_capacity(length as usize)` where `length: u64` is caller/layout-supplied with no
|
||||||
|
cap tied to actual dataset or file size.
|
||||||
|
- `crates/clawhdf5-io/src/async_read.rs:84` (`AsyncFileReader::open`) —
|
||||||
|
`Vec::with_capacity(len as usize)` sized directly from `file.metadata().len()`, no cap.
|
||||||
|
|
||||||
|
**Problem:** Both allocate a buffer sized from an untrusted/unvalidated length *before*
|
||||||
|
validating it against anything (declared dataset size, actual readable bytes, or a configured
|
||||||
|
ceiling). A crafted layout-metadata value reaching `subfiling.rs`, or a crafted/sparse file
|
||||||
|
opened via `async_read.rs`, can trigger a multi-gigabyte-to-exabyte allocation attempt and an
|
||||||
|
OOM abort — the same "bounded allocation" concern the CHANGELOG's `MAX_DECOMPRESS_SIZE` fix
|
||||||
|
already addressed for the decompression path, just not yet for these two read paths.
|
||||||
|
|
||||||
|
**Change:** Cap the length against a known-sane bound (file size, or a configurable ceiling
|
||||||
|
similar in spirit to `MAX_DECOMPRESS_SIZE`/`MAX_WAL_FIELD_LEN`) before calling
|
||||||
|
`Vec::with_capacity`, or use `try_reserve` and return a clean error on failure instead of
|
||||||
|
aborting.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-13 — `symbol_table.rs` size arithmetic doesn't use the `checked_*`/`ensure_len` idiom used elsewhere [P2]
|
||||||
|
**File:** `crates/clawhdf5-format/src/symbol_table.rs:99-107` (`SymbolTableNode::parse`)
|
||||||
|
|
||||||
|
**Problem:** `let needed = entries_start + num_symbols * entry_size;` uses plain arithmetic.
|
||||||
|
Not exploitable to overflow on 64-bit today (`num_symbols` is bounded by its `u16` source
|
||||||
|
field), but it's inconsistent with the rest of the audited codebase and becomes a real risk if
|
||||||
|
either operand's type widens later.
|
||||||
|
|
||||||
|
**Change:** Route through `checked_mul`/`checked_add` + `ensure_len`, matching the pattern used
|
||||||
|
throughout `chunked_read.rs`/`data_read.rs`/`local_heap.rs`/`btree_v1.rs`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-14 — Filter bit-packing decode loops have no direct fuzz coverage [P2]
|
||||||
|
**File:** `crates/clawhdf5-format/src/filters.rs` (scale-offset unpack ~150-270, N-Bit type-tree
|
||||||
|
walk ~396-510); fuzz target `fuzz_filter_pipeline`
|
||||||
|
|
||||||
|
**Problem:** `fuzz_filter_pipeline` only fuzzes `FilterPipeline::parse` — the filter-pipeline
|
||||||
|
*metadata* message — not the actual decode functions in `filters.rs` that unpack
|
||||||
|
attacker-influenced compressed bytes bit-by-bit (scale-offset, N-Bit). This is the most
|
||||||
|
bit-twiddling-heavy code in the crate and, per the CHANGELOG, has already had real bugs found
|
||||||
|
there in the initial hardening pass (`1 << minbits` overflow, `bit_offset + precision`
|
||||||
|
overflow); it's exactly the kind of code that benefits most from fuzzing but currently gets none
|
||||||
|
directly.
|
||||||
|
|
||||||
|
**Change:** Add a `fuzz_filter_decode` target that feeds arbitrary bytes through the
|
||||||
|
scale-offset and N-Bit decode entry points directly (not just pipeline metadata parsing).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Group C — `clawhdf5-ann` (HNSW): hot-path performance
|
||||||
|
|
||||||
|
`prune_connections` rayon parallelism (already shipped) is out of scope. The outer
|
||||||
|
insert/build loop is intentionally left sequential per ROADMAP's own design note — not
|
||||||
|
re-proposed here.
|
||||||
|
|
||||||
|
### INT-15 — `compute_distance` is scalar-only; `clawhdf5-accel`'s SIMD path is never used [P1]
|
||||||
|
**Files:** `crates/clawhdf5-ann/src/hnsw.rs:47-74` (`compute_distance`);
|
||||||
|
`crates/clawhdf5-accel/src/lib.rs:125` (`cosine_similarity`), `:173` (`l2_distance`)
|
||||||
|
|
||||||
|
**Problem:** `clawhdf5-ann`'s `Cargo.toml` has no dependency on `clawhdf5-accel` at all.
|
||||||
|
`compute_distance` is a hand-written scalar loop for both L2 and cosine, called from every
|
||||||
|
candidate-expansion step in `greedy_closest`, `search_layer`, and `prune_connections` — i.e.
|
||||||
|
the entire build/insert/search hot path. `clawhdf5-accel` already provides
|
||||||
|
runtime-feature-detected, SIMD-accelerated equivalents (AVX2/AVX-512/NEON, correctly gated per
|
||||||
|
INT survey — see clean bill of health above) that go completely unused here.
|
||||||
|
|
||||||
|
**Change:** Add a `clawhdf5-accel` dependency to `clawhdf5-ann` and route `compute_distance`
|
||||||
|
through `l2_distance`/`cosine_similarity`. This is a drop-in replacement for the scalar
|
||||||
|
arithmetic, not a semantic change.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-16 — Best-entry-point distance is discarded and immediately recomputed [P1]
|
||||||
|
**File:** `crates/clawhdf5-ann/src/hnsw.rs` — `greedy_closest` (749-772) computes
|
||||||
|
`best_dist` at line 756 but returns only the `usize` node id; callers
|
||||||
|
(`build_with_metric` 251-253, `insert` 381-389, `search` 504-506) immediately recompute
|
||||||
|
`compute_distance(query, &vectors[ep], metric)` for that same `(query, ep)` pair before calling
|
||||||
|
`search_layer` (which itself recomputes it again at line 783).
|
||||||
|
|
||||||
|
**Problem:** Every layer transition during insert/search throws away a distance value it just
|
||||||
|
computed and recomputes the identical value at least once more. For an L-layer index this wastes
|
||||||
|
up to L redundant distance computations per insert/search call — pure waste on what is already
|
||||||
|
the hottest path in the crate (compounded by INT-15 if that's not yet fixed).
|
||||||
|
|
||||||
|
**Change:** Change `greedy_closest`'s return type to `(usize, f32)` (node id + its distance) and
|
||||||
|
thread that value into the next `greedy_closest`/`search_layer` call instead of recomputing.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-17 — `search_layer`'s visited-set uses `HashSet<usize>` instead of a dense bitset [P1]
|
||||||
|
**File:** `crates/clawhdf5-ann/src/hnsw.rs:799, 809-812`
|
||||||
|
|
||||||
|
**Problem:** `let mut visited = HashSet::new();` with `.contains(&neighbor)`/`.insert(neighbor)`
|
||||||
|
in the innermost per-candidate-expansion loop, run on every insert and search call. Node ids are
|
||||||
|
dense `0..n` integers — a `Vec<bool>` (or bitset) indexed directly by id gives O(1) lookup
|
||||||
|
without SipHash overhead, which matters when this loop dominates search cost.
|
||||||
|
|
||||||
|
**Change:** Replace with `vec![false; vectors.len()]` indexed by node id (reset/reused per
|
||||||
|
call), or a proper bitset if allocation-per-call cost matters.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### INT-18 — `compact()` clones every surviving vector twice [P1]
|
||||||
|
**File:** `crates/clawhdf5-ann/src/hnsw.rs:463-478` (`compact`), `:305`
|
||||||
|
(`build_with_metric`'s `vectors: vectors.to_vec()`)
|
||||||
|
|
||||||
|
**Problem:** `compact()` builds an owned `Vec<Vec<f32>>` via `surviving.push(v.clone())` (line
|
||||||
|
469), then passes `&surviving` into `build_with_metric`, whose first action clones it again via
|
||||||
|
`.to_vec()`. For a large index this doubles the memory-copy cost of an already-`O(n)` rebuild
|
||||||
|
operation.
|
||||||
|
|
||||||
|
**Change:** Give `build_with_metric` (or a private variant) an owned-`Vec<Vec<f32>>` entry point
|
||||||
|
so `compact` can move `surviving` in directly instead of cloning twice.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Group D — `clawhdf5-py`: Mutex poisoning bricks write-mode objects
|
||||||
|
|
||||||
|
### INT-19 — Pervasive `state.lock().unwrap()` on a shared `Mutex` reachable from Python calls [P1]
|
||||||
|
**Files:** `crates/clawhdf5-py/src/group.rs` (6 sites, e.g. `:115, :161, :184, :200, :217`),
|
||||||
|
`crates/clawhdf5-py/src/attrs.rs` (4 sites, e.g. `:56, :77, :92, :99`),
|
||||||
|
`crates/clawhdf5-py/src/file.rs` (1 site, `:282`)
|
||||||
|
|
||||||
|
**Problem:** `PyGroup`/`PyAttrs`/write-mode file state hold a `Mutex<...>` and every method that
|
||||||
|
touches it does `state.lock().unwrap()`. If any single call panics while holding the lock (a
|
||||||
|
future edge case in `extract_numpy_data`, an allocation failure, anything) the `Mutex` becomes
|
||||||
|
permanently poisoned. Every subsequent method call on that same Python object — for the rest of
|
||||||
|
its lifetime — then also panics via the same `.unwrap()`, instead of the object cleanly
|
||||||
|
returning a `PyErr` and remaining usable. This turns one transient panic into a permanently
|
||||||
|
broken object from the caller's perspective, which is a worse failure mode than a single
|
||||||
|
raised-and-handled Python exception.
|
||||||
|
|
||||||
|
**Change:** Replace `lock().unwrap()` with a helper that converts a poison error into a
|
||||||
|
`PyResult` `PyErr` (e.g. `state.lock().map_err(|_| PyErr::new::<PyRuntimeError, _>("internal state poisoned"))?`,
|
||||||
|
or use `parking_lot::Mutex` which doesn't have poisoning at all — likely the simpler fix given
|
||||||
|
`clawhdf5-py` doesn't appear to rely on poisoning semantics anywhere). Apply consistently across
|
||||||
|
all ~11 call sites.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Group E — noted, not proposed (checked and found low-priority/out-of-scope)
|
||||||
|
|
||||||
|
- **`clawhdf5-derive`'s generated `from_bytes`** (`crates/clawhdf5-derive/src/lib.rs:106-119`)
|
||||||
|
does `assert!(_data.len() >= _required, ...)` before any field-slicing, so it's a documented,
|
||||||
|
guarded panic (`# Panics` doc comment already present) rather than an unguarded OOB — and
|
||||||
|
`#[derive(H5Type)]` is currently used only in `crates/clawhdf5-format/tests/derive_tests.rs`,
|
||||||
|
not in any production code path. Making `from_bytes` return `Result` instead of asserting
|
||||||
|
would be a reasonable future API-ergonomics improvement for downstream users of the macro, but
|
||||||
|
it's not fixing a reachable bug today — left out as not worth an INT slot this pass.
|
||||||
|
- **`cargo audit` unmaintained warnings** (`custom_derive`, `number_prefix`, `paste`) — all
|
||||||
|
transitive through optional features (`mpi-io`, and whatever pulls in `tokenizers`), zero
|
||||||
|
actual vulnerabilities, no code-level fix available in this repo. FYI only.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Suggested implementation order for the coding phase
|
||||||
|
|
||||||
|
1. **INT-01** first — it's the load-bearing gap (provenance/anomaly detection is currently
|
||||||
|
inert), and INT-02/03/04/05 are bug fixes *inside* the code INT-01 wires up, so fixing them
|
||||||
|
before or during the wiring avoids shipping newly-live bugs.
|
||||||
|
2. **INT-09 + INT-10 together** (P0, security) and **INT-11, INT-12** (P0, security) — these are
|
||||||
|
independent of each other and of Group A, safe to parallelize.
|
||||||
|
3. **INT-15/16/17/18** (Group C, HNSW perf) — independent of A/B, safe to parallelize.
|
||||||
|
4. **INT-19** (Group D) — independent, small, safe to parallelize.
|
||||||
|
5. **INT-06, INT-07, INT-08, INT-13, INT-14** — lower urgency, pick up as time allows.
|
||||||
|
|
||||||
|
All items should land with `cargo test --workspace` (and `cargo clippy --workspace -- -D
|
||||||
|
warnings`, per this repo's established gate) passing before being considered done.
|
||||||
|
|||||||
Reference in New Issue
Block a user