Skip to content

fix: int64 overflow in linear/log iterator reporting level (near INT64_MAX) - #149

Merged
fcostaoliveira merged 15 commits into
mainfrom
fix/iterator-reporting-level-overflow
Sep 29, 2026
Merged

fcostaoliveira merged 15 commits into
mainfrom
fix/iterator-reporting-level-overflow

Conversation

@filipecosta90

Copy link
Copy Markdown
Contributor

Stacked on #148 (the iterators call move_next, whose value-range overflow #148 fixes). Base this PR on #148; retarget to main once #148 merges.

Bug (found by adversarial review during the ClusterFuzzLite effort)

For a histogram with highest_trackable_value near INT64_MAX, the linear and log iterators overflow int64 when advancing the reporting level:

  • iter_linear_next (:1162): next_value_reporting_level += value_units_per_bucket
  • log_iter_next (:1219): next_value_reporting_level *= (int64_t) log_base

Both 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_MAX when the next step would overflow. Termination: naive saturation would re-report the top level forever (the emit branch doesn't advance counts_index). Since no bucket value ever equals INT64_MAX, pinning next_value_reporting_level_lowest_equivalent to INT64_MAX as well makes the iter->value >= level test stop firing, so the iterator drains remaining buckets and terminates.

Verification (ASan+UBSan)

  • Linear (units=2^62) and log (base 2) over an INT64_MAX histogram: no trap, terminate (2 and 64 steps). Normal-range iteration unchanged. Regression test added; full suite green.

🤖 Generated with Claude Code

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>
@fcostaoliveira
fcostaoliveira force-pushed the fix/top-bucket-value-range-overflow branch from cedc46a to 92a219f Compare August 28, 2026 09:54
@fcostaoliveira
fcostaoliveira force-pushed the fix/iterator-reporting-level-overflow branch from 49b2366 to 655ad34 Compare August 28, 2026 10:31
fcostaoliveira and others added 2 commits August 28, 2026 15:38
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>
@fcostaoliveira
fcostaoliveira force-pushed the fix/iterator-reporting-level-overflow branch from 655ad34 to e478997 Compare August 28, 2026 14:41
@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

The saturation logic looks right to me. No real bucket value can equal INT64_MAX, so pinning next_value_reporting_level_lowest_equivalent to INT64_MAX does stop the emit test from firing again, and the iterator then runs through the remaining buckets and stops. struct hdr_histogram and all public signatures are unchanged, so there is no SOVERSION question here. A few small points:

  • The new doc on hdr_iter_log_init says any base <= 1 "terminates after the first level". That isn't quite what the code does. A base <= 1.0 (and NaN/Inf, or a non-positive first bucket) is pinned at init, so it never emits a level. Only 1 < base < 2 emits the first level before saturating. The same applies to vpb <= 0 on the linear side. In both pinned cases you still get one final step with value_iterated_to == 0, from the existing !move_next → return true path, and not zero steps. That's fine for termination, but the wording should match.
  • peek_next_value_from_index now duplicates the index-decomposition logic of hdr_value_at_index. It would be less likely to drift if the overflow check lived in (or next to) a shared helper, rather than in a second copy.
  • test/hdr_histogram_test.c now has #include <math.h> twice.
  • Fractional log bases truncating toward zero is a real divergence from the Java LogarithmicIterator, which iterates in double. Documenting it and leaving the fix for a separate change that touches the ABI seems reasonable.
  • Per-PR ClusterFuzzLite is ASan-only. So for the UB fixes, the evidence is your local ASan+UBSan runs plus the weekly UBSan batch job. hdr_record_fuzzer.c does call the linear/log iterators, so the batch job should cover this path over time.

Comment thread test/hdr_histogram_test.c
Comment on lines +619 to +622
while (hdr_iter_next(&iter))
{
mu_assert("linear iterator must terminate", ++steps < 1000000);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fcostaoliveira
fcostaoliveira changed the base branch from fix/top-bucket-value-range-overflow to main September 18, 2026 10:00
@fcostaoliveira

Copy link
Copy Markdown
Contributor

Review round: 2026-09-29. Reviewed commit: bcf56f6380bc96b93a1372d533dd40114168d0ea.

Verdict: NEEDS WORK — fractional logarithmic-base semantics remain unresolved.

Paulo's terminal-boundary finding is fixed and pushed: linear/logarithmic iteration now emits INT64_MAX exactly once before terminating. Tests assert both that boundary and the bounded iteration counts. The newly introduced signed shifts were also replaced with unsigned shifts.

Fresh arm64 release CTest 5/5, ASan+UBSan+float-cast-overflow 5/5 and logging-disabled 4/4 pass; this head's CI and PR fuzz checks are green.

A remaining functional probe records value 100 and starts logarithmic iteration at 1 with base 1.5: it terminates after two steps with the last reported boundary still 1, despite counting the value 100. Base 2.5 produces the same boundaries as 2.0 because log_base is truncated to an integer. This integer truncation predates this PR, but the new termination guard still leaves misleading output for an accepted finite base greater than one. Resolve the supported-base contract and test it before treating this as complete logarithmic-iterator hardening. Adding floating-point iterator state would also require an explicit public-layout/ABI review; the terminal fix intentionally does not change that layout.

Atomic recording and offset access are unchanged. No performance claim is made. Review evidence, fractional probe and experiment history.

 review)

log_base is applied as level *= (int64_t) log_base, so a fractional base
truncates toward zero (2.5 -> 2) and any 1 < base < 2 truncates to 1 and
terminates after the first level. Document this on hdr_iter_log_init and pin
it: base 2.5 iterates in lock-step with base 2.0, and base 1.5 terminates.
No behavior/ABI change; fractional-base support would need a double reporting
level in the public struct (separate PR).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@fcostaoliveira

Copy link
Copy Markdown
Contributor

Review round: 2026-09-29 (follow-up). Reviewed commit: dcc2a32.

Verdict: the fractional-log-base item is resolved — scoped to (c)+(a).

Adversarial re-review (7 independent correctness lenses) plus a decision on the open contract question:

  • The INT64_MAX terminal/overflow fix is correct and stays as-is. hdr_iter_log_init
    already rejects base <= 1.0 (pins to the terminating state), and the advance-path
    guard pins (int64_t) base <= 1 before the multiply — so no infinite loop and no
    float-cast / signed-overflow UB.
  • The only residual was honesty about the base contract, not a code defect. The
    effective base is (int64_t) log_base (truncated toward zero): 2.5 behaves as 2.0,
    and any 1 < base < 2 truncates to 1 and terminates after the first level.
  • Fixed by documenting that on hdr_iter_log_init and pinning it with a test:
    test_log_iterator_integer_base_contract asserts base 2.5 iterates in lock-step with
    base 2.0 (identical boundaries) and that base 1.5 terminates quickly.

Full fractional-base support (matching the Java reference, which iterates in double)
would require changing the reporting level to double in the public iterator struct — a
layout/ABI + SOVERSION change out of scope for a terminal-overflow fix. Deliberately left
for a separate PR; this PR changes no struct layout.

Gates on dcc2a32: gcc RelWithDebInfo ctest 5/5; ASan+UBSan (-fno-sanitize-recover=all)
ctest 5/5, 27 unit tests (the new contract test runs green under UBSan). No behavior change
for integer bases; no ABI change.

@paulorsousa — this also closes out your original point: the terminal boundary is now
emitted exactly once (INT64_MAX) and asserted, and the fractional-base ambiguity is
documented and pinned rather than left implicit.

@paulorsousa paulorsousa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the fix locally and LGTM! 🙌 TY!!

@fcostaoliveira
fcostaoliveira merged commit 2bdcb0f into main Sep 29, 2026
32 checks passed
fcostaoliveira pushed a commit to fcostaoliveira/HdrHistogram_c that referenced this pull request Sep 29, 2026
fcostaoliveira pushed a commit to fcostaoliveira/HdrHistogram_c that referenced this pull request Sep 29, 2026
fcostaoliveira pushed a commit to fcostaoliveira/HdrHistogram_c that referenced this pull request Sep 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants