fix: remediate pre-release audit r5 — SIMD safety docs, fuzz hot paths, loader/Python hardening#40
Conversation
Audit r5 #5/#6: the four pub(crate) asymmetric scan kernels read packed docs via raw pointer arithmetic but stated no SAFETY invariant. Add the slice-length / query-length / divisibility contract to each, and document the scan_b4_asym_avx512 0xFF00 blend-mask per-lane semantics (lanes 0-7 <- chunk_lo, 8-15 <- chunk_hi, pairing with the shift vector). Comment-only; no behavioural change.
Audit r5 #1: all four loaders did file.metadata().map(|m| m.len()).unwrap_or(0). On a metadata race (NFS/procfs) that yields a bogus 0 that false-rejects a valid index. Propagate the io::Error via ? instead.
Audit r5 #7: k==0 arises when an empty index clamps k=min(requested,n)=0. Pin that maybe_insert is a no-op and finalize_into emits nothing, without panicking on the zero-length slots.
Audit r5 #11/#12 (and the Perplexity-deferred add/search fuzz): the four existing targets only cover the loaders. Add search_rankquant (add -> search_asymmetric, driving the SIMD scan kernels + TopK, k=0 included) and roundtrip_rankquant (write -> load round-trip, the write path the loader targets never reach). Validated: fuzz build green + ~14k/13k runs each with no crashes.
Audit r5 #14: the ci.yml deny job runs on push/PR only, so a new RUSTSEC advisory against an unchanged dependency goes unseen until the next PR. Add a scheduled (weekly) + on-demand advisories scan, read-only token, mirroring ci.yml's EmbarkStudios/cargo-deny-action@v2.
Review Summary by QodoAudit r5 remediation: SIMD safety docs, fuzz hot paths, loader hardening
WalkthroughsDescription• Propagate loader metadata errors instead of swallowing as 0 (TOCTOU false-reject fix) • Add comprehensive SAFETY documentation to all 4 SIMD scan kernels with slice/query invariants • Add per-method path-trust docstrings to all 8 write/load methods across Python bindings • Add two new fuzz targets: search_rankquant (compute hot path) and roundtrip_rankquant (write→load) • Add TopK::new(0) edge-case test and reword search_asymmetric_byte_lut error message • Add weekly scheduled cargo-deny advisories CI scan for supply-chain hygiene Diagramflowchart LR
A["Audit Findings"] --> B["SIMD Safety Docs"]
A --> C["Loader Hardening"]
A --> D["Python Path Trust"]
A --> E["Fuzz Coverage"]
A --> F["CI Advisories"]
B --> G["4 Scan Kernels"]
C --> H["Metadata Error Propagation"]
D --> I["8 Write/Load Methods"]
E --> J["search_rankquant Target"]
E --> K["roundtrip_rankquant Target"]
F --> L["Weekly Schedule"]
File Changes1. src/quant_kernels.rs
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Code Review
This pull request introduces two new fuzz targets, search_rankquant and roundtrip_rankquant, to test the compute path and serialization round-trip of the RankQuant index. It also improves IO robustness by propagating metadata errors during index loading and adds comprehensive safety documentation to the unsafe SIMD kernels. Additionally, a unit test was added to ensure the TopK utility correctly handles cases where k=0, and Python bindings were updated with documentation regarding path traversal. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
This PR applies the r5 pre-release audit hardening across the Rust core, SIMD docs, fuzzing coverage, Python bindings docs, and CI supply-chain scanning, without introducing new functionality or changing dependency manifests.
Changes:
- Harden loader correctness by propagating
metadata()errors instead of silently treating them as length0. - Add fuzz coverage for RankQuant hot paths (ingest+search) and write→load round-trips, plus a TopK
k=0edge-case regression test. - Expand SIMD-kernel safety documentation and add per-method Python path-trust disclosures; add a weekly scheduled
cargo-deny advisoriesworkflow.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util.rs | Adds a regression test covering TopK::new(0) inert behavior. |
| src/rank_io.rs | Propagates file metadata errors in all four loaders to avoid incorrect length checks. |
| src/quant_kernels.rs | Adds detailed SAFETY: invariants and AVX-512 blend-mask rationale to scan kernels. |
| ordvec-python/src/lib.rs | Documents path-trust expectations per write/load method; rewords a benchmark-only error message. |
| fuzz/fuzz_targets/search_rankquant.rs | New fuzz target exercising RankQuant ingest + asymmetric-search compute path. |
| fuzz/fuzz_targets/roundtrip_rankquant.rs | New fuzz target for RankQuant write→load round-trip. |
| fuzz/Cargo.toml | Registers the two new fuzz targets. |
| .github/workflows/audit.yml | Adds a weekly scheduled cargo-deny advisories scan workflow. |
Comments suppressed due to low confidence (3)
src/rank_io.rs:364
- Same issue as in
load_rank: the comment claims a swallowedmetadata()error would make the payload-vs-file check “pass … false-rejecting”, but in practicefile_len = 0makescheck_payload_matches_filereject any non-empty payload and can let empty-payload files bypass the trailing-bytes check. Please reword to match the real behavior.
// Propagate a metadata failure rather than swallowing it as `0`: a bogus
// `0` would make the payload-vs-file check pass for an empty corpus even on
// a TOCTOU-racy filesystem (NFS/procfs), false-rejecting a valid index.
let file_len = file.metadata()?.len();
src/rank_io.rs:493
- Same comment-rationale mismatch here: with
file_len = 0,check_payload_matches_filewill reject any non-empty bitmap file and will not detect trailing bytes whenn_vectors == 0. Consider rewording this duplicated comment block so it describes the actual failure modes.
// Propagate a metadata failure rather than swallowing it as `0`: a bogus
// `0` would make the payload-vs-file check pass for an empty corpus even on
// a TOCTOU-racy filesystem (NFS/procfs), false-rejecting a valid index.
let file_len = file.metadata()?.len();
src/rank_io.rs:612
- Same duplicated comment issue as the other loaders: swallowing
metadata()errors as0does not just cause “false-reject”; it can also defeat the trailing-bytes check for the empty-payload case. Please reword the rationale to match whatcheck_payload_matches_fileactually does whenfile_lenis bogus.
// Propagate a metadata failure rather than swallowing it as `0`: a bogus
// `0` would make the payload-vs-file check pass for an empty corpus even on
// a TOCTOU-racy filesystem (NFS/procfs), false-rejecting a valid index.
let file_len = file.metadata()?.len();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…4} text) The #9 reword in this PR dropped the '{2, 4}' dispatch detail from the search_asymmetric_byte_lut bits==1 error; test_search_asymmetric_byte_lut_rejects_b1 had pinned that substring via match='2, 4', so CI's pytest leg failed. Re-point it at the new benchmark-only phrasing. Verified locally: maturin develop + pytest = 155 passed.
copilot: the comment conflated 'passes for an empty corpus' with 'false-rejects a valid index'. Reword to the actual failure modes a bogus file_len=0 causes — false-reject of any non-empty index, and an empty-corpus pass that skips the trailing-byte check. Comment-only; all 4 loaders.
…or (#40 review) copilot: the target swallowed write() failures (hiding regressions) and wrote to an already-open NamedTempFile handle (reopen-by-path write can fail on Windows). Write to a fresh path in tempfile::tempdir() and treat a write failure as a crash, matching the stated round-trip contract. Validated: fuzz build + 7531 runs, no crashes.
|
Addressed the review (latest commits on the branch):
gemini had no findings. Both threads resolved. |
Review (Bug/Security): audit.yml runs UNATTENDED on a cron schedule, so a mutable @v4/@v2 tag could ship changed/compromised action code that auto-executes on the next tick with no PR in the loop. Pin actions/checkout to 34e1148 (v4.3.1) and EmbarkStudios/cargo-deny-action to a531616 (v2.0.19). ci.yml stays on tags by the team's existing policy — it only runs on human-triggered push/PR, a lower-risk surface.
Triage + remediation of the 5th external pre-release audit (Rust crate + Python bindings). The audit confirmed no memory-safety / CVE-class findings; everything is hardening, docs, or coverage. Audit was taken at an older commit, so each finding was re-verified against current
mainfirst.Fixed (9)
SAFETY+ AVX-5120xFF00blend-mask rationale on all 4 scan kernelsdocs: …SIMD scan-kernel safety// SAFETY:invariant on the AVX2/AVX-512 raw-pointer scansdocs(python): …path-trustsearch_asymmetric_byte_lutbits==1 error reworded benchmark-only (no{2,4}dispatch leak)metadata()?instead of swallowing as0(TOCTOU false-reject)fix: propagate loader metadata errorTopK::new(0)inert-edge testtest: cover TopK::new(0)cargo-deny advisoriesscanci: weekly …advisories scanroundtrip_rankquantfuzz target (write→load)test: fuzz …hot pathssearch_rankquantfuzz target (add→search, drives SIMD scan + TopK)Fuzz validated locally:
cargo +nightly fuzz buildgreen; ~14.7k / 12.8k executions each, no crashes. (Covers the Perplexity-deferred add/search fuzz too.)Verified already-satisfied (1)
python.ymlalready builds the abi3 wheel, installs the built wheel, and runs pytest across 3.9 (floor) + 3.13 (ceiling) plus 3.13 on arm64/macOS/windows. No change needed.Deferred to issues (low-value / higher-effort)
rank_transform_into,l2_normalise— thread-local scratch).safe_pathscanonicalize+prefix; the per-method disclosure — the audit’s minimum bar — landed here).unsafe— documented there).Declined (non-issues, with rationale)
write_*assert_eq!→debug_assert!): writes are not a hot path, and the hard assert is deliberate defense-in-depth — converting it would regress the fix: pre-public public-API contract hardening (release-readiness triage) #26 "debug_assertcompiles out in release" lesson.rank_norm(0)PythonValueError): deliberate Rust↔Python parity — the core is total there (defined output, no panic); a Python-onlyValueErrorwould diverge from the Rust API, exactly the call made forrank_to_bucket/bucket_centrein Pre-flip: remediate release-gate audit (8 findings) + full Python parity #23/fix: pre-public public-API contract hardening (release-readiness triage) #26.Gate
fmt · clippy (core +
-p ordvec-python) · test (default / no-default / experimental) ·fuzz build+ run — all green. No coreCargo.toml/lock change.Merges under strict protection (CI + 1 non-self code-owner approval).