Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
563cdd2178 |
@@ -0,0 +1,434 @@
|
||||
# Implementation Brief — clawhdf5 Performance/Security/Provenance Pass
|
||||
|
||||
**Research date:** 2026-08-16
|
||||
**Scope:** `crates/` only. Read against `ROADMAP.md`, `IMPROVEMENT_LOG.md`, `CLAUDE.md`, and
|
||||
`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.
|
||||
|
||||
`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.
|
||||
|
||||
Also checked and found clean (no INT items filed): `clawhdf5-migrate` (zero `unwrap()` outside
|
||||
`#[test]` code in `main.rs`; `sqlite_reader.rs`/`hdf5_writer.rs`/`validate.rs` are unwrap-free),
|
||||
`clawhdf5-cli`, `clawhdf5-napi` (zero `unwrap()` in `lib.rs`), `clawhdf5-accel` SIMD dispatch
|
||||
(`is_x86_feature_detected!`/runtime gating is correct — no illegal-instruction risk),
|
||||
`clawhdf5-filters` hot path (slice-based, no byte-by-byte loops of consequence), and TODO/FIXME
|
||||
grep across all crates (the only hits are test-fixture bytes literally named `b"XXXX"`, not
|
||||
real markers).
|
||||
|
||||
---
|
||||
|
||||
## Priority key
|
||||
- **P0** — correctness/security bug reachable from untrusted input (crafted file, external
|
||||
caller), should block release.
|
||||
- **P1** — real functional gap or measurable perf cost on a hot path.
|
||||
- **P2** — consistency/hardening/API-quality; safe to defer.
|
||||
|
||||
---
|
||||
|
||||
## Group A — `clawhdf5-agent`: provenance/security is unwired (headline finding)
|
||||
|
||||
### INT-01 — Wire `WriteAnomalyDetector` / `ProvenanceStore` into the actual write path [P0]
|
||||
**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`
|
||||
|
||||
**Problem:** `ROADMAP.md` Track 5 ("Memory Security & Provenance") is marked 🟢 Complete, listing
|
||||
source attribution, write anomaly detection, source isolation, and integrity verification as
|
||||
done. The types exist and are unit-tested in isolation — but `grep -rn
|
||||
"WriteAnomalyDetector\|ProvenanceStore\|SourceIsolation"` across every file in
|
||||
`clawhdf5-agent` *except* `provenance.rs`/`anomaly.rs` themselves returns nothing.
|
||||
`storage.rs` (the real save/delete/replay path) never imports or calls into either module.
|
||||
Nothing in `HDF5Memory::save`/`save_batch` populates a `ProvenanceStore`, runs a rate/pattern
|
||||
check, or routes through `SourceIsolation`. In its current state this is a library the crate
|
||||
ships but never uses on itself — every memory write today has **no** rate limiting, no pattern
|
||||
detection, and no provenance recorded, contrary to what the roadmap and any consumer relying on
|
||||
it would assume.
|
||||
|
||||
**Change:** In `storage.rs`'s save/save_batch entry point(s), construct/thread a
|
||||
`WriteAnomalyDetector` and `ProvenanceStore` (or accept them as constructor params on the
|
||||
memory-store struct so callers can configure `AnomalyConfig`), call `record_write` +
|
||||
`check_rate_anomaly`/`check_pattern_anomaly` before persisting each chunk, and call
|
||||
`ProvenanceStore::add` with the resulting `MemoryProvenance` alongside the write. Surface
|
||||
anomaly alerts through the existing error/result type rather than silently dropping them
|
||||
(decide via a config flag whether pattern/rate hits are hard-rejects or soft warnings — a hard
|
||||
reject changes public API behavior, a warning is additive). Add an integration test that writes
|
||||
a chunk containing one of the 15 suspicious patterns and asserts the alert actually fires
|
||||
through the public save path (not just the unit-level `WriteAnomalyDetector` test).
|
||||
|
||||
---
|
||||
|
||||
### INT-02 — `check_pattern_anomaly` is trivially bypassed substring matching [P1]
|
||||
**File:** `crates/clawhdf5-agent/src/anomaly.rs:192-211`
|
||||
|
||||
**Problem:**
|
||||
```rust
|
||||
let lower = chunk.to_lowercase();
|
||||
for pattern in &self.config.suspicious_patterns {
|
||||
if lower.contains(pattern.as_str()) { ... }
|
||||
}
|
||||
```
|
||||
Matching is raw case-folded substring containment against 15 fixed literals (`"ignore
|
||||
previous"`, `"system:"`, …). Trivially defeated by inserting extra whitespace/punctuation
|
||||
(`"ignore previous"`), splitting the phrase across two separate writes (checks are per-chunk,
|
||||
not per-session-buffer), or any non-ASCII obfuscation. As a poisoning-resistance control this
|
||||
currently only stops the laziest attacks.
|
||||
|
||||
**Change:** Normalize input before matching (collapse whitespace/strip zero-width and combining
|
||||
characters), and consider word-boundary-tolerant/regex matching instead of raw `contains`.
|
||||
Document the remaining limitation (this is a heuristic filter, not a guarantee) rather than
|
||||
implying full poisoning resistance.
|
||||
|
||||
---
|
||||
|
||||
### 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`
|
||||
|
||||
**Problem:** `session_counts: HashMap<String, u32>` is incremented on every `record_write` and
|
||||
never pruned — unlike `window` (which has a 60s sliding-window prune). A caller that creates
|
||||
many distinct `session_id` values (fully caller-controlled strings) grows this map without
|
||||
bound for the process lifetime. `check_rate_anomaly`'s session-level loop
|
||||
(`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.
|
||||
|
||||
**Change:** Bound `session_counts` with an LRU/TTL eviction policy, or track only counts within
|
||||
the same rolling window used for `window` (see INT-05, which is closely related — the
|
||||
session-level check has its own separate bug on top of this).
|
||||
|
||||
---
|
||||
|
||||
### INT-04 — Sliding-window prune only inspects the front of the deque [P1]
|
||||
**File:** `crates/clawhdf5-agent/src/anomaly.rs:134-139`
|
||||
|
||||
**Problem:**
|
||||
```rust
|
||||
self.window.push_back(event);
|
||||
let cutoff = self.last_timestamp - 60.0;
|
||||
while self.window.front().is_some_and(|e| e.timestamp < cutoff) {
|
||||
self.window.pop_front();
|
||||
}
|
||||
```
|
||||
`WriteEvent.timestamp` is caller-supplied (not sampled from a clock inside this type), so
|
||||
nothing prevents an out-of-order/backdated event from landing behind the front after a more
|
||||
recent one. Because eviction only ever looks at `front()`, a single out-of-order event
|
||||
permanently corrupts the window — old entries behind it are never pruned, so
|
||||
`check_rate_anomaly`'s window-length count over-reports forever (and can be intentionally
|
||||
inflated by a caller that varies timestamp ordering).
|
||||
|
||||
**Change:** Prune by retaining only entries `>= cutoff` across the whole deque
|
||||
(`self.window.retain(|e| e.timestamp >= cutoff)`), or reject/clamp non-monotonic timestamps in
|
||||
`record_write` and document that `WriteEvent.timestamp` must be non-decreasing per detector
|
||||
instance.
|
||||
|
||||
---
|
||||
|
||||
### 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`)
|
||||
|
||||
**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
|
||||
"is this session currently abusive" — any long-lived legitimate session (e.g. a persistent
|
||||
agent) permanently trips the alert once past the threshold regardless of pace, while a burst of
|
||||
writes in a brand-new session under the threshold is missed even if it's the real anomaly.
|
||||
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
### INT-06 — `bfs_neighbors` re-scans all relations on every queue pop [P1]
|
||||
**File:** `crates/clawhdf5-agent/src/knowledge.rs:339-378`, hot loop at 352-365
|
||||
|
||||
**Problem:**
|
||||
```rust
|
||||
let neighbours: Vec<u64> = self.relations.iter().filter_map(|r| { ... }).collect();
|
||||
```
|
||||
runs once per node dequeued during BFS, giving `O(visited_nodes × total_relations)` total cost.
|
||||
`get_subgraph` (`knowledge.rs:387-417`) calls `bfs_neighbors` once per seed node, multiplying
|
||||
the cost again. On a graph with a non-trivial relation count this is the dominant cost of any
|
||||
graph traversal query — the kind of memory-graph read the whole crate exists to serve
|
||||
efficiently.
|
||||
|
||||
**Change:** Build an adjacency `HashMap<u64, Vec<u64>>` once (either eagerly maintained on
|
||||
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