fix: signed-shift overflow in hdr_calculate_bucket_config (UBSan, found by fuzzing)

[filipecosta90]

Found by fuzzing

The new weekly ClusterFuzzLite batch run (UBSan) flagged undefined behaviour in hdr_calculate_bucket_config:

src/hdr_histogram.c:419:72: runtime error: left shift of 255 by 56 places cannot be represented in type 'int64_t'
  hdr_calculate_bucket_config -> hdr_init -> hdr_decode_compressed_v0 -> hdr_log_read_entry -> hdr_log_read -> log_reader_fuzzer

Root cause

sub_bucket_mask was computed as ((int64_t) sub_bucket_count - 1) << unit_magnitude before the guard if (unit_magnitude + sub_bucket_half_count_magnitude > 61) return EINVAL;. A crafted, decoded log entry can produce lowest_discernible_value ≈ 2^56 → unit_magnitude == 56, sub_bucket_count == 256, so the code shifts 255 << 56, which overflows int64_t — signed-left-shift UB — before the config is rejected.

Fix

Move the > 61 guard above the sub_bucket_mask shift, so an out-of-range config returns EINVAL without ever performing the overflowing shift. The threshold is unchanged and correct: (sub_bucket_count - 1) occupies (sub_bucket_half_count_magnitude + 1) bits, so the shift's top bit is unit_magnitude + sub_bucket_half_count_magnitude; keeping that ≤ 61 leaves a representable, non-negative int64_t. No behavior change for valid inputs.

Verification

• Reproduced the exact UBSan error locally on the crash input, confirmed resolved after the fix (input now takes the EINVAL path).
• Full unit suite: 5/5 pass.

🤖 Generated with Claude Code

[github-actions]

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

The core fix looks right. Moving the > 61 guard above the sub_bucket_mask shift removes the UB without changing anything for valid configs, and lowest_discernible_value > highest_trackable_value / 2 is equivalent to the old lowest * 2 > highest for every input that reaches it (lowest >= 1 is already checked above it, and C's truncation toward zero makes the two agree for negative highest_trackable_value as well). Worth noting the 61 threshold is load-bearing in a second place: buckets_needed_to_cover_value() computes ((int64_t) sub_bucket_count) << unit_magnitude, whose top bit is unit_magnitude + sub_bucket_half_count_magnitude + 1, so 61 is exactly what keeps that at bit 62 and representable — it shouldn't be relaxed. No ABI surface is touched here (no struct hdr_histogram layout change, no public signature change), so no SOVERSION step is implicated.

The three hdr_free(h) → hdr_close(h) changes in the aggregate branch of hdr_decode_compressed_v0/v1/v2 are real leaks of the counts array, and they're the same bug #122 fixed on the error paths of those same three functions while missing the success path — the cleanup block already read hdr_close(h) two branches up. That's a user-visible fix in the log-decode path, and the title and description mention only the shift overflow; it should be called out explicitly, since it's the part a downstream consumer would care about in a release note.

Two smaller things. memset(cfg, 0, sizeof(*cfg)) establishes a new guarantee about a public out-param on the EINVAL path, and test_bucket_config_reject_defines_cfg now pins that guarantee, but nothing in hdr_histogram.h states it — either document it next to hdr_calculate_bucket_config or drop it, since no in-tree caller reads cfg after a non-zero return and hdr_init returns immediately. Separately, the new free(buffer) / free(data) calls in the log tests are freeing memory that hdr_encode_compressed and hdr_log_encode allocated via hdr_malloc/hdr_calloc; that's consistent with what those tests already did and harmless with the default allocator, but it's a mismatched pair for anyone building with a custom HDR_MALLOC_INCLUDE.

The sanitizers job is the most useful part of this PR beyond the fix itself: the per-PR fuzzing gate runs ASan only and UBSan lands only in the weekly batch, which is precisely why this class of bug surfaced late. Be clear about what it doesn't cover, though — it's Linux/x64/gcc only, so it does nothing for the BSD/musl/32-bit build breakages that are historically this project's more frequent failure mode. The .hlog reproducer needs no build-script change since .clusterfuzzlite/build.sh already globs test/*.hlog into the seed corpus, but the deterministic per-PR coverage here really comes from test_bucket_config_shift_overflow, not from the corpus file.

I read the sources to check the arithmetic equivalence and the recorder ownership — hdr_interval_recorder_destroy does close both active and inactive, and the leftover sample1 in reset_histogram_on_sample_and_recycle genuinely isn't held by the recorder, so the added hdr_close calls don't introduce a double free. I have not built or run the suite under ASan+UBSan myself, so the "clean under both sanitizers" claim rests on the author's own verification and on the new job.

[paulorsousa]

LGTM 👍