fix: int64 overflow in linear/log iterator reporting level (near INT64_MAX) - #149
filipecosta90 wants to merge 13 commits into
Conversation
Found by adversarial review during the ClusterFuzzLite effort. For a histogram whose highest_trackable_value is near INT64_MAX, the top bucket's lowest_equivalent_value + size_of_equivalent_value_range exceeds INT64_MAX, which is signed-overflow UB. This poisoned hdr_next_non_equivalent_value -> highest_equivalent_value (hence hdr_max, hdr_value_at_percentile, hdr_percentiles_print) and the move_next iterator step (hence hdr_stddev and any full iteration -- the all-values iterator visits the top bucket regardless of counts). All in-contract and reachable via the read path on a decoded histogram. Saturate both additions at INT64_MAX instead of overflowing. Verified under ASan+UBSan: hdr_init(1, INT64_MAX, 3) + record(INT64_MAX) then hdr_max / percentile / stddev / full iteration / percentiles_print no longer trap and return representable values. Added a regression test. Found-by: adversarial review during the ClusterFuzzLite fuzzing effort Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…variant) Adversarial review of the initial saturation found that deriving highest_equivalent_value as hdr_next_non_equivalent_value(v) - 1 yielded INT64_MAX-1 for the top bucket, so hdr_max of a histogram that recorded INT64_MAX returned a value BELOW it (violating value <= highest_equivalent), and disagreed with move_next() which saturates the iterator's highest_equivalent_value to INT64_MAX. Give highest_equivalent_value its own overflow-aware computation clamping to INT64_MAX so hdr_max / value_at_percentile and the iterator agree. Strengthen the regression test to pin the saturated values (hdr_max == INT64_MAX, hdr_next_non_equivalent_value == INT64_MAX -- the latter distinguishes the fix without a sanitizer, since the buggy code wraps to INT64_MIN). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cedc46a to
92a219f
Compare
49b2366 to
655ad34
Compare
Found by adversarial review during the ClusterFuzzLite effort. For a histogram whose highest_trackable_value is near INT64_MAX, the linear and logarithmic iterators advanced their reporting level with 'next += value_units_per_bucket' (iter_linear_next) and 'next *= (int64_t)log_base' (log_iter_next), both of which overflow int64 -> signed-overflow UB, then a negative left-shift when the wrapped value flows into lowest_equivalent_value. Saturate the reporting level at INT64_MAX when the next step would overflow. Crucially, also pin next_value_reporting_level_lowest_equivalent to INT64_MAX in that case: no bucket value ever equals INT64_MAX, so the 'iter->value >= level' emit test can no longer fire and the iterator drains its remaining buckets and terminates rather than re-reporting the saturated level forever. Verified under ASan+UBSan: linear (value_units_per_bucket=2^62) and log (base 2) over an INT64_MAX-range histogram no longer trap and terminate (2 and 64 steps); normal-range iteration is unchanged. Regression test added. Stacked on #148 (whose move_next fix the iterators depend on). 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 reporting-level saturation found two more iterator issues in the same functions: - next_value_greater_than_reporting_level_upper_bound peeked hdr_value_at_index(h, counts_index + 1); at the last bucket that reads one past counts[], and value_from_index shifts into the sign bit -> signed-shift UB for a near-INT64_MAX range. It was masked before this PR (the reporting level overflowed first); saturating that unmasked it. Require counts_index+1 to be in range before peeking. - Degenerate iterator parameters looped forever: value_units_per_bucket <= 0 (level never advances) and log_base <= 1 (cast to 0/1, *= never advances). Pin the level (and its lowest-equivalent) to INT64_MAX in those cases too so the emit test cannot re-fire and the iterator drains and terminates. The degenerate checks are ordered first so the guard arithmetic (INT64_MAX - vpb, INT64_MAX / base) can't itself overflow or divide by zero. Verified under ASan+UBSan: the peek repro (hdr_init(1, 1<<62, 1), record 1<<62, linear vpb=INT64_MAX-1 / log base 2) and the degenerate cases (vpb=0, log_base 1.0/0.5/1.5) all terminate with no trap; normal iteration unchanged. Regression tests extended. Found-by: adversarial review during the ClusterFuzzLite fuzzing effort Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
655ad34 to
e478997
Compare
|
🤖 Automated first-pass review — a human maintainer's review is still required before merge. The core saturation argument holds: bucket values are Four things worth a look before merging: The two
Degenerate init params now yield a truncated iteration instead of looping. These init functions return On verification: the per-PR ClusterFuzzLite gate is ASan only (UBSan is the weekly batch), the linear fuzz target clamps Minor: |
…ear INT64_MAX, guard non-positive log level
| while (hdr_iter_next(&iter)) | ||
| { | ||
| mu_assert("linear iterator must terminate", ++steps < 1000000); | ||
| } |
There was a problem hiding this comment.
Looks like iter.value_iterated_to remains at the previous reporting level after saturation (instead of being updated to INT64_MAX)
Should this test also assert that the final reported value is INT64_MAX?
If so, it should also apply for the log iterator test too
…reporting-level-overflow
# Conflicts: # test/hdr_histogram_test.c
…reporting-level-overflow
# Conflicts: # test/hdr_histogram_test.c
3 participants
Stacked on #148 (the iterators call
move_next, whose value-range overflow #148 fixes). Base this PR on #148; retarget tomainonce #148 merges.Bug (found by adversarial review during the ClusterFuzzLite effort)
For a histogram with
highest_trackable_valuenearINT64_MAX, the linear and log iterators overflow int64 when advancing the reporting level:iter_linear_next(:1162):next_value_reporting_level += value_units_per_bucketlog_iter_next(:1219):next_value_reporting_level *= (int64_t) log_baseBoth are in-contract (public iterators) and reachable via any full linear/log iteration of such a histogram; the wrapped negative level then triggers a negative left-shift in
lowest_equivalent_value.Fix
Saturate the reporting level at
INT64_MAXwhen the next step would overflow. Termination: naive saturation would re-report the top level forever (the emit branch doesn't advancecounts_index). Since no bucket value ever equalsINT64_MAX, pinningnext_value_reporting_level_lowest_equivalenttoINT64_MAXas well makes theiter->value >= leveltest stop firing, so the iterator drains remaining buckets and terminates.Verification (ASan+UBSan)
units=2^62) and log (base 2) over anINT64_MAXhistogram: no trap, terminate (2 and 64 steps). Normal-range iteration unchanged. Regression test added; full suite green.🤖 Generated with Claude Code