fix: hdr_reset_internal_counters ignored normalizing_index_offset (wrong min/max on decoded shifted logs)

[fcostaoliveira]

Problem

hdr_reset_internal_counters scans counts[] with counts_get_direct(h, i) — a raw
storage index — but then passes the winning max_index / min_non_zero_index to
hdr_value_at_index, which decomposes its argument as a logical index:

if ((count_at_index = counts_get_direct(h, i)) > 0)   /* storage index */
...
int64_t max_value = hdr_value_at_index(h, max_index); /* treated as logical */

When normalizing_index_offset is non-zero the two differ, so min_value and max_value
come out wrong.

This is reachable from the log reader, not just by hand: hdr_decode_compressed_v1 and
_v2 both assign h->normalizing_index_offset from the payload and call this function on
the very next line (hdr_histogram_log.c:555-557 and :671-673). So any decoded log
carrying a non-zero offset — a Java-encoded shifted log — reports wrong min/max.
total_count is unaffected, because summing every slot is rotation-invariant.

Rotating a histogram's storage without changing what it logically holds:

	min	max	total
before rotation	1000	100031	3
after rotation, on main	43974656	3995074559	3
after rotation, with this change	1000	100031	3

Change

Use counts_get_normalised so the count and the value share the logical index. One line.

Parity with the Java reference

This was the open question when the bug was first spotted, so to settle it rather than
assert it — AbstractHistogram.establishInternalTackingValues() reads via
getCountAtIndex(index), and in Histogram.java that is:

long getCountAtIndex(final int index) {
    return counts[normalizeIndex(index, normalizingIndexOffset, countsArrayLength)];
}

long getCountAtNormalizedIndex(final int index) {
    return counts[index];
}

So Java's is the normalising accessor, and it pairs it with valueFromIndex(maxIndex) —
exactly what this change does. The raw accessor is the other one. C was the outlier here,
and this restores parity.

It is also the only counts_get_direct call in hdr_histogram.c outside
counts_get_normalised itself; hdr_count_at_index and hdr_count_at_value already
normalise.

Cost

None on the hot paths — this runs on decode and manual import only, never on
hdr_record_value or hdr_value_at_percentile. normalize_index early-returns when the
offset is zero, so the unrotated case is one predictable branch per slot on a cold path.

Test

test_reset_internal_counters_honours_offset rotates a histogram's storage and asserts the
reported min/max/total are unchanged. On unpatched code it fails with
min_value wrong after rotation.

It closes the histogram before asserting: a failing assert would otherwise leak it, and
LeakSanitizer _exit()s before stdio flushes, so the assertion message is swallowed and all
you see is a leak trace. Worth knowing for any test added to this suite now that the
sanitizers job enables LSan.

Verification

gate	result
ctest gcc ASan+UBSan+LSan, -fno-sanitize-recover=all	5/5
ctest clang ASan+UBSan+float-cast-overflow	5/5
ctest RelWithDebInfo, gcc and clang	5/5 each
ctest -DHDR_LOG_REQUIRED=DISABLED	4/4
compiler warnings on hdr_histogram.c, -Wall -Wextra	0, same as main

Steps to reproduce

cmake -E make_directory _build && cmake -E chdir _build cmake .. \
  -DCMAKE_BUILD_TYPE=Debug -DHDR_LOG_REQUIRED=ON
cmake --build _build && cmake -E chdir _build ctest --output-on-failure

Note

A dedicated .hlog fixture with a non-zero normalizing_index_offset would exercise this
end-to-end through hdr_log_read, but producing one needs the Java encoder (the C encoder
always writes offset 0). The unit test covers the same state the decoder hands to this
function. Happy to add a fixture if you can point me at a shifted log.

🤖 Generated with Claude Code

[github-actions]

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

Reads correct to me. hdr_value_at_index (src/hdr_histogram.c:246) decomposes its argument into bucket/sub-bucket, so it takes a logical index, and everything else in the file that pairs a count with a value already goes through counts_get_normalised — the iterators at :993, plus hdr_count_at_index and hdr_count_at_value. This loop was the one place pairing a raw slot with a logical decode, so the one-line change just lines it up with the rest of the file. normalize_index also stays in [0, counts_len) here given the decoder reduces the offset modulo counts_len first (hdr_histogram_log.c:563 and :687), so the new call can't index out of range.

One thing the description doesn't mention but which the same change fixes: the i != 0 guard was wrong for the same reason. Under a non-zero offset it skipped storage slot 0 — which is logical index offset, a real bucket — rather than the logical zero bucket, so min_value could be suppressed for a genuine count as well as computed from the wrong index.

On Java parity, the quoted getCountAtIndex / getCountAtNormalizedIndex split matches my reading of the reference, and pairing the normalising accessor with valueFromIndex(maxIndex) is what establishInternalTackingValues() does — but I haven't run the Java side myself, so that's worth confirming against upstream rather than taking from the PR body, since that comparison is the usual standard for min/max and percentile questions here.

Reachability looks as stated: there's no value-shifting API in the C port, so the log decoder is the only thing that sets a non-zero offset, and the v0 path (hdr_histogram_log.c:442) calls this with the offset still 0 from hdr_init, so it's unchanged. No struct layout or public signature change either, so no SOVERSION action.

Two minor notes, neither blocking:

• hdr_reset_internal_counters is the subject of the still-open INTEGER_OVERFLOW in src/hdr_histogram.c #118 (the int64_t summation into observed_total_count is unchecked). This change neither worsens nor addresses it, but it is the same loop, so worth noting it stays open.
• The "Should allocate rotated counts" assert leaks h on failure — the exact thing the later close-before-assert is there to avoid. calloc failing at that size is unlikely enough that it's probably not worth another commit.

On the fixture question: the .hlog files in test/ are all jHiccup output, which won't carry a shifted offset — producing one would need a Java Histogram with shiftValuesLeft/shiftValuesRight applied before encoding. The unit test covers the state the decoder actually hands this function, so I wouldn't hold the fix for an end-to-end fixture. Note also that the per-PR ClusterFuzzLite job is ASan-only, and this is a wrong-value bug rather than a memory-safety one, so no CI gate here would have caught it — the added regression test is what covers it.

[fcostaoliveira]

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

Verdict: MERGE-READY for this offset/min-max fix.

Fresh arm64 validation passes: release CTest 5/5, ASan+UBSan+float-cast-overflow CTest 5/5, and logging-disabled CTest 4/4. An additional 50,000 deterministic V1/V2 decode cases cover positive/negative/wrapping offsets and check counts, percentiles, min and max under sanitizers. The reviewed head's Linux/Windows/macOS CI and PR fuzz checks are green.

The normalizing count accessor now pairs with the logical index used to reconstruct extrema. Atomic recording, public layout and the recording/percentile hot paths are unchanged. No new performance claim is made.

Scope: issue #118's separate dense total-count overflow remains open; this verdict does not certify the entire release or close that issue. Recheck after conflict resolution if the reviewed code changes.

Review evidence and experiment history.

[fcostaoliveira]

Review round: 2026-09-29 (adversarial re-verification, no code change). Reviewed commit: b420563.

Ran a 7-lens adversarial pass on the current head. One lens raised a plausible OOB counts[] read: hdr_reset_internal_counters now reads via counts_get_normalised(h, i), and normalize_index applies only a single ±counts_len wrap, so an offset with |offset| >= counts_len from a crafted V1/V2 log would index out of bounds.

Validated as a false positive. Both compressed-decode paths reduce the decoded offset into (-counts_len, counts_len) before calling hdr_reset_internal_counters:

• V1 decode (hdr_histogram_log.c): h->normalizing_index_offset %= h->counts_len;
• V2 decode: same %= counts_len reduction

That guard is pre-existing in main (this PR's git diff against upstream/main touches only src/hdr_histogram.c — hdr_histogram_log.c is byte-identical), so |offset| < counts_len always holds at the read and the single-wrap normalize_index is sufficient. The only three callers of hdr_reset_internal_counters are the V0/V1/V2 decoders; V0 runs with offset == 0. The same counts_get_normalised is already used on other decode-reachable read paths in main, so the change is consistent with existing behavior — it does not widen the reachable index range.

Verdict: MERGE-READY confirmed. A1/A2/A3/A4/A5/A6 all clean; the offset/min-max recompute is correct on decoded shifted logs. No code change.

[paulorsousa]

LGTM!
Also confirmed the bug and the fix locally

C code

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <inttypes.h>
#include <hdr/hdr_histogram.h>

static void show(const char *label, struct hdr_histogram *h) {
    printf("%-26s total=%lld min=%lld us max=%lld us counts(1000)=%lld counts(100000)=%lld\n",
        label, (long long)h->total_count, (long long)hdr_min(h), (long long)hdr_max(h),
        (long long)hdr_count_at_value(h, 1000), (long long)hdr_count_at_value(h, 100000));
}

int main(void) {
    struct hdr_histogram *h = NULL;

    if (hdr_init(1, 3600000000LL, 3, &h)) {
        return 2;
    }

    hdr_record_value(h, 1000);
    hdr_record_value(h, 1000);
    hdr_record_value(h, 100000);

    int64_t expected_min = hdr_min(h), expected_max = hdr_max(h);
    show("Original samples", h);

    int32_t offset = h->counts_len / 3;
    int64_t *rotated = calloc((size_t)h->counts_len, sizeof(*rotated));
    if (!rotated) {
        hdr_close(h);
        return 2;
    }

    for (int32_t i = 0; i < h->counts_len; ++i) {
        int32_t slot = i - offset;
        if (slot < 0) {
            slot += h->counts_len;
        }
        rotated[slot] = h->counts[i];
    }

    memcpy(h->counts, rotated, (size_t)h->counts_len * sizeof(*rotated));
    free(rotated);
    h->normalizing_index_offset = offset;

    printf("Storage offset: %d slots (array size: %d)\n", offset, h->counts_len);
    show("After storage rotation", h);

    /* V1/V2 decoders call this after loading counts and their offset. */
    hdr_reset_internal_counters(h);
    show("After rebuilding counters", h);

    int correct = hdr_min(h) == expected_min && hdr_max(h) == expected_max && h->total_count == 3;
    printf("Result: %s\n", correct ? "PASS: same samples, same min/max" : "BUG: unchanged samples, corrupted min/max");

    hdr_close(h);
    return correct ? 0 : 1;
}