fix: out-of-range double->int conversion in hdr_timespec_from_double (UBSan)

[fcostaoliveira]

Follow-up to #153, and the second half of what has been failing the weekly
ClusterFuzzLite batch fuzzing workflow.

Problem

hdr_log_read_header feeds the #[StartTime: %lf field straight from the file into
hdr_timespec_from_double, which converted it with:

int seconds = (int) value;

Converting a double that is out of int range — or non-finite — to an integer is
undefined behaviour. So any log whose StartTime is outside int is UB during header parse.
A millisecond epoch written where seconds were expected is enough, which is a realistic
mistake rather than a purely hostile input:

src/hdr_time.c:92:19: runtime error: 1.40348e+12 is outside the range of
  representable values of type 'int'

The narrow accumulator cascades two more UB sites for such a value: the (int) round(...)
of the resulting huge fraction, and milliseconds * 1000000. gcc's UBSan reports that last
one first, clang reports the cast — same root cause, and worth knowing when reading the two
CI configurations.

int was also too narrow regardless of the UB: it truncates every timestamp past 2038 even
on platforms whose tv_sec is 64-bit.

Change

Range-check before converting, and widen the result to what tv_sec can actually hold. The
bound is derived from sizeof(tv_sec), so this is correct on LP64, LLP64 (where tv_sec is
a 32-bit long) and 32-bit alike — 2^(bits-1) is exactly representable as a double, so
the comparison is exact rather than approximate.

A value tv_sec cannot hold, or a non-finite one, yields a defined 0/0 instead of a trap.
Both fields are always written, so a caller that passes an uninitialized hdr_timespec
cannot read indeterminate values on the reject path — the same concern as the last commit
of #145. isfinite is load-bearing here: NaN compares false against both bounds, so without
it NaN would fall straight through to the cast.

Millisecond resolution is unchanged, as hdr_time.h documents.

input	before	after (64-bit tv_sec)
1403476110.183	1403476110 / 183000000	unchanged
1403476110183.0 (ms epoch)	UB	1403476110183 / 0
1e300, ±INFINITY, NAN	UB	0 / 0
-2.0	-2 / 0	unchanged

On a 32-bit tv_sec platform the ms-epoch row rejects to 0/0 instead, which the test
asserts explicitly per sizeof(tv_sec).

Tests

• test_timespec_from_double in hdr_histogram_test.c covers a normal value, a negative
  value, a value beyond int that a 64-bit tv_sec holds, ±1e300, ±INFINITY and NAN.
  It lives in the always-built suite, so it runs in the HDR_LOG_REQUIRED=DISABLED legs too.
• handle_wide_start_time reads regression-log-start-time-overflow.hlog through
  hdr_log_read_header, covering the actual reachable path rather than only the function.
• That fixture also joins the log_reader_fuzzer seed corpus automatically via
  .clusterfuzzlite/build.sh (test/*.hlog), same as the reproducer added in fix: signed-shift overflow in hdr_calculate_bucket_config (UBSan, found by fuzzing) #145.

Both tests abort under UBSan on the pre-fix code — reverting just the src/hdr_time.c hunk
reproduces the CI message verbatim at hdr_time.c:92:19, so these are real regression tests
rather than assertions that merely happen to pass.

Verification

gate	result
ctest gcc ASan+UBSan, -fno-sanitize-recover=all (the sanitizers job's flags)	5/5
ctest clang ASan+UBSan+float-cast-overflow (the check the fuzzing build uses)	5/5
ctest RelWithDebInfo, gcc and clang	5/5 each
ctest -DHDR_LOG_REQUIRED=DISABLED	4/4
compiler warnings, -Wall -Wextra -Wconversion -Wsign-conversion	identical to main (2 pre-existing in hdr_timespec_as_double, none added)
the original ClusterFuzzLite crash artifact, replayed	clean

MSVC reasoning: isfinite, trunc and ldexp are all C99 <math.h>, and this file already
calls round/modf, so it does not move the toolchain floor; CHAR_BIT and int64_t come
from the two added includes.

Combined with #153

Since both PRs fix findings from the same fuzzer, I validated them stacked as well: with
#153 and this change applied together, ctest is 5/5 under both sanitizer configurations
and 4/4 with logging disabled, and a 600-second log_reader_fuzzer session
(ASan+UBSan+float-cast-overflow, seeded from test/*.hlog) ran 5.68 M executions with
no crash and no UB, at higher coverage than before the fixes. That is the pair that
should take the weekly batch run green.

They touch adjacent lines in test/CMakeLists.txt and test/hdr_histogram_log_test.c, so
whichever lands second needs a trivial additive resolution (keep both fixture entries, keep
both mu_run_test lines) — happy to rebase whenever suits.

Steps to reproduce

cmake -E make_directory _build && cmake -E chdir _build cmake .. \
  -DCMAKE_BUILD_TYPE=Debug -DHDR_LOG_REQUIRED=ON \
  -DCMAKE_C_COMPILER=clang \
  -DCMAKE_C_FLAGS="-fsanitize=address,undefined,float-cast-overflow -fno-sanitize-recover=all -g"
cmake --build _build && cmake -E chdir _build ctest --output-on-failure

Not included

hdr_timespec_from_double can still round the fraction up to a full second and emit
tv_nsec = 1000000000 (e.g. 1.9996 gives tv_sec=1, tv_nsec=1e9), which is a malformed
hdr_timespec but not UB. It is a normalization bug rather than this defect class, and
fixing it changes output for inputs that are currently accepted, so I left it out to keep
this PR to one purpose. Happy to send it separately if you want it.

🤖 Generated with Claude Code

[github-actions]

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

Looks right to me. The bound comes from sizeof(t->tv_sec), the value is truncated before it's compared, and isfinite catches NaN before the cast. milliseconds * 1000000 now stays within ±1e9, so the int multiply can't overflow. The per-platform cast matches the header's hdr_timespec split. There are no public signature or struct changes, so nothing touches the ABI.

One small thing, no need to act on it: a StartTime that tv_sec can't hold now quietly becomes 0 while hdr_log_read_header still returns success. That's the best a void function can do without changing its signature. The 32-bit time_t case is also only checked by the authors' own reasoning, since CI has no 32-bit Linux leg.

[fcostaoliveira]

Confirmed and fixed — thanks, this was a real one.

I checked the Windows build logs rather than reasoning about it, and C4244 is exactly what
the x86 legs emit on the line this PR added:

hdr_time.c(109,27): warning C4244: '=': conversion from 'int64_t' to 'long',
  possible loss of data

Both x86 legs, both the static and shared targets. And the diagnosis of why my warning
comparison missed it is right: it ran on LP64, where int64_t and long are the same type,
so the narrowing could not appear there.

Fixed in 113e566 by keying both the bound and the cast to long, so they agree by
construction and neither narrows. That also matches hdr_gettime's Windows branch just
above, which already does (long) integral. Using sizeof(long) rather than
sizeof(tv_sec) is never narrower than the int it replaces, so no platform loses range
against the pre-fix code — the one configuration where my previous revision was more
permissive was a 32-bit target with a 64-bit time_t, which is also precisely where it
warned. stdint.h is no longer needed. The test's guard moved to sizeof(long) too,
otherwise it would have asserted the wide value parses on a 32-bit platform whose tv_sec
is 64-bit but whose long is not.

Re-verified: ctest 5/5 under gcc ASan+UBSan and under clang
ASan+UBSan+float-cast-overflow, hdr_time.c back to main's 2 pre-existing warnings, and a
boundary sweep still accepts up to 2^63-1024 and exactly -2^63 while rejecting one past
either end.

On the negative non-integral case — you're right that -2.5 yields tv_sec=-2,
tv_nsec=-500000000, and that it is pre-existing and equally malformed. I've kept it out
for the same reason as the tv_nsec = 1000000000 carry: both are normalization rather than
UB, and both change output for inputs that are accepted today. They want one PR together,
with a decision on whether a negative timespec should normalize toward tv_nsec >= 0 or be
rejected outright.

On the Java HistogramLogReader comparison neither of us has made: noted, and I'd rather
leave that to @mikeb01 than assert a compatibility claim I haven't verified.

[fcostaoliveira]

Review round: 2026-09-29. Reviewed commit: 426f5ff0f5b821cd939d40d734e865f878d0c024.

Verdict: MERGE-READY for this finite/range-checked time conversion.

The destination-width correction is pushed. The range check now follows tv_sec, with the POSIX conversion using time_t, rather than assuming long has the same width. The exact September 28 weekly UBSan crash (SHA1 63bd43e82ce8833d9cc40e175407eba57f04467f) aborts on main and replays cleanly here.

Fresh arm64 release CTest 5/5, ASan+UBSan+float-cast-overflow 5/5, and logging-disabled 4/4 pass. Linux/Windows/macOS CI, the sanitizer job, and PR fuzzing are green on this exact head. Atomic recording, offsets and the histogram layout are unchanged; no recording/scan performance claim is made.

Merge #154 before #156, or use #156 as the combined fix: #156 includes normalization and the same width correction. Negative fractions remain unnormalized in this intentionally narrower PR. This is a scoped PR verdict, not certification of every input or of the entire release.

Review evidence and experiment history.

[paulorsousa]

We're "silently" mapping invalid or out of range input values to zero (tbh, I'm not totally aware of the impact)
Maybe we could return an error instead and propagate it above (?)
The down-side is that the function returns void, so we'd need an API change, or a separate checked conversion function

[fcostaoliveira]

Good question — I dug into this rather than hand-wave it. Recommendation: merge #154 as-is; keep the silent clamp, and add a checked variant only later as a separate, additive follow-up if a caller actually needs it.

Reasoning:

1. Signature/ABI. hdr_timespec_from_double is a published void symbol in hdr/hdr_time.h. Changing it to return int is a source + ABI break that would force a SOVERSION bump — disproportionate for a UBSan fix and against the one-PR-one-purpose norm here.
2. No caller can consume an error. The only in-tree caller is scan_start_time (src/hdr_histogram_log.c), which is itself void, and its parent header-scan chain propagates no per-field error. An absent StartTime already degrades to 0 via hdr_log_reader_init. So invalid input mapping to 0/0 lands exactly on the pre-existing "unknown start time" sentinel — consistent behavior, not a new silent failure mode. Even if the function returned an error today, every caller would discard it.
3. The PR's job is to remove UB. 0/0 is defined behavior; the out-of-range double→int cast was UB. Replacing UB with a defined sentinel is the fix. Bolting on error-return machinery is a separate design change (same reason the tv_nsec == 1e9 carry and negative-fraction normalization were split into fix: hdr_timespec_from_double could emit a malformed hdr_timespec (tv_nsec == 1e9, or negative) #156).

If a checked path is wanted later, the minimal non-breaking shape is additive and belongs in its own PR, not here:

/* returns 0 on success, -EINVAL on non-finite, -ERANGE on out-of-tv_sec-range input */
int hdr_timespec_from_double_checked(hdr_timespec* t, double value);

with the existing void function kept as a thin wrapper that calls it and zeroes on failure — so no caller and no ABI break, and encode/decode callers that care could then propagate the error.

(Adversarial re-review of the current head was 7/7 green through all correctness lenses; this was the only open thread.)

[paulorsousa]

Sound good!!
Approving

[fcostaoliveira]

Review round: 2026-09-29 (adversarial re-verification). Reviewed commit: 426f5ff.

7-lens adversarial pass on the current head — 7/7 GREEN; the finite/range-checked
double→hdr_timespec conversion is UB-free at both tv_sec widths and value-preserving.

The one open thread — @paulorsousa's question on silently mapping invalid/out-of-range input
to 0/0 — is answered inline: recommend merging as-is (public void ABI; the only caller
discards errors; 0/0 coincides with the pre-existing "unknown start time" sentinel; the PR's
job is UB→defined). An optional additive hdr_timespec_from_double_checked() can follow in a
separate PR if a caller ever needs to propagate the error.

Verdict: MERGE-READY (merge #154 before #156, or use #156 as the combined fix).

[fcostaoliveira]

Published the optional checked variant as #161 — hdr_timespec_from_double_checked() returning 0 / -EINVAL / -ERANGE, with the void form kept as a thin wrapper (no behavior/ABI change). @paulorsousa this closes out your question here.