fix: harden query path — hdr_mean overflow + count accessor OOB reads - #147
Open
filipecosta90 wants to merge 2 commits into
Open
fix: harden query path — hdr_mean overflow + count accessor OOB reads#147filipecosta90 wants to merge 2 commits into
filipecosta90 wants to merge 2 commits into
Conversation
Two more issues found by adversarial review during the ClusterFuzzLite effort: - hdr_mean summed iter.count * hdr_median_equivalent_value in an int64 running total. For a histogram holding values near 2^62 this overflows INT64_MAX (signed-overflow UB) and yields a garbage/negative mean. Reachable via the fuzz surface (log_reader_fuzzer calls hdr_mean on every decoded histogram). Accumulate in double, as hdr_stddev already does. - hdr_count_at_value indexed counts[] via counts_index_for() with no range check, so a value beyond highest_trackable_value read out of bounds. Mirror hdr_record_values' guard and return 0 for out-of-range values. Verified under ASan+UBSan: main aborts (UBSan signed overflow in hdr_mean; ASan OOB read in hdr_count_at_value); both return clean results after the fix. Added regression tests. Found-by: adversarial review during the ClusterFuzzLite fuzzing effort Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review of the hdr_count_at_value fix flagged its index-based twin hdr_count_at_index as having the identical unchecked out-of-bounds read (raw caller index -> counts_get_normalised -> h->counts[index]). Guard it the same way (return 0 for an out-of-range index; the unsigned compare also catches negatives). All in-tree callers pass in-range indices. Also strengthen the regression tests per review: pin hdr_mean to a plausible magnitude band (not just sign/finiteness), assert in-range and the value==highest boundary for hdr_count_at_value (catchable without a sanitizer), and add an out-of-range test for hdr_count_at_index. (Note: hdr_value_at_index's shift UB at an extreme index is left as a documented precondition -- it performs no counts[] access and guarding it would change the internal peek at index==counts_len; tracked separately.) Found-by: adversarial review during the ClusterFuzzLite fuzzing effort Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
filipecosta90
changed the title
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
2 participants
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.
Two more fuzzing-surfaced bugs in the query path (found by adversarial review during the ClusterFuzzLite effort).
1.
hdr_mean— signed int64 overflow (UBSan), fuzz-reachabletotal += iter.count * hdr_median_equivalent_value(...)accumulated inint64. For a histogram holding values near2^62, a few samples overflowINT64_MAX→ signed-overflow UB and a wrapped/negative mean.log_reader_fuzzercallshdr_meanon every decoded histogram, so a crafted large-range log reaches it. Fix: accumulate indouble(exactly whathdr_stddevalready does).2.
hdr_count_at_value— out-of-bounds read (ASan)It indexed
counts[]viacounts_index_for()with no range check, so querying a value beyondhighest_trackable_valueread past the array. Fix: mirrorhdr_record_values' guard (valuerange +(uint32_t)index >= (uint32_t)counts_len) and return0— an untracked value has count 0.Verification (local, ASan+UBSan)
hdr_mean→runtime error: signed integer overflow ... at hdr_histogram.c:832;hdr_count_at_value(h, INT64_MAX)→ ASan heap-buffer-overflow READ.hdr_meanreturns a positive finite value;hdr_count_at_valuereturns 0. Added regression tests; full suite passes under ASan+UBSan.🤖 Generated with Claude Code