harden(ingest): validate inbound CMS/CountSketch wire dimensions (defensive) - #344
Merged
Merged
Conversation
…rix reconstruct Defensive, non-breaking hardening on the modified-OTLP frequency-sketch ingest path. Before reconstructing a CMS / CountSketch matrix from the wire-declared `(rows, cols)`, validate the dims so a malformed / hostile payload fails gracefully instead of risking a degenerate matrix or a huge allocation. New shared `validate_sketch_dims` helper (in count_min_sketch_accumulator, reused by count_sketch_accumulator) rejects: - `rows < 1` or `cols < 1` (degenerate, no cells); - narrow-hash-budget violation `rows * ceil(log2(cols)) > 64` — the point at which the cross-language Packed64 wire hasher's per-row column slices overflow / alias the 64-bit word and the matrix-cell layout degrades (mirrors sketchlib's `MatrixFastHash::assert_compatible`); checking the column-index bits alone keeps the realistic configs 5x2048 / 5x4096 / 5x2000 valid; - obviously-oversized dims `rows * cols > 8M cells` to cap allocation. On rejection the decoder returns `Err` (never panics); the existing OTLP ingest caller already skips that data point and increments `decoded_failed`. That call site now logs dim-validation rejections at WARN (ordinary decode fallbacks stay at DEBUG to avoid spam). Valid configs decode unchanged. Unit tests added to both accumulators: a malformed-dim data point is skipped (Err, no panic), a valid one still decodes, plus zero/budget/cap/ extreme-value (saturating) cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Optional, non-breaking defense-in-depth: validate the
rows/colsdeclared on inbound modified-OTLP CMS/CountSketch data points before reconstructing the matrix, so a malformed/adversarial payload is skipped (logged) rather than panicking or producing a degenerate sketch. Companion to ProjectASAP/ASAPCollector#451 — per that compatibility analysis the backend needs no required change; this is hardening only.What
New
validate_sketch_dims()(shared by both accumulators), wired intofrom_sketchlib_proto_bytes:rows < 1orcols < 1;rows * ceil(log2(cols)) > 64;rows * cols > 8Micells (all productssaturating_mul, so no overflow/panic).On rejection it returns
Err(never panics); the existing ingest caller already skips the data point (decoded_failed += 1; continue), now logged atwarn!for dim rejections (ordinary decode fallbacks staydebug!).Files:
data_plane/src/precompute_engine/operators/count_min_sketch_accumulator.rs(validator + wiring + 3 tests)data_plane/src/precompute_engine/operators/count_sketch_accumulator.rs(calls shared validator + 2 tests)data_plane/src/drivers/ingest/otel.rs(warn-log dim rejections at the skip site)On the budget formula
The naive
floor(log2(cols))+2 > 64would wrongly reject the canonical5×2048and5×4096configs. The correct producer-side condition — verified againstasap_sketchlib'sMatrixFastHash::assert_compatible(matrix_storage.rs: the Packed64 wire hasher readsceil(log2(cols))column-index bits per row from one 64-bit word) — isrows * ceil(log2(cols)) > 64, which admits5×2048/5×4096/5×2000/4×1000and rejects degenerate dims. This matches the edge-side guard added in ASAPCollector#451, so producer and consumer agree.Verification / caveat (honest)
Validation logic verified standalone (rustc) for all required-valid + reject cases (no panic; correct messages); unit tests mirror it. Full
cargo test -p data_planecould not run locally due to a pre-existing, unrelated dependency skew: the path-dep'dasap-precompute-rsin the localASAPCollectorcheckout is stale against the currentasap_sketchlib(d7ca0ce, which renamedCountMinSketch→CountMinetc. and added proto fields) — confirmed it fails identically without any of my files, whileasap_sketchlib(my actual dependency) builds and exposes every API used. Should build cleanly in CI with pinned deps.🤖 Generated with Claude Code