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.

The fix looks like the right shape. The bound is exact where the obvious version wouldn't be: ldexp(1.0, bits-1) is exactly representable, whereas comparing against (double) LONG_MAX rounds up to 2^63 and would let the bad value straight through to the cast. Both fields are written on the reject path, and the fractional part bounds the remaining (int) round(...) and * 1000000, so the two cascading sites go with it. No struct layout or signature change, so nothing to do for SOVERSION.

Two things worth a look. The bound is keyed to sizeof(long), but on POSIX hdr_timespec is struct timespec and tv_sec is time_t. On a 32-bit target with 64-bit time_t — musl ≥ 1.2, or glibc with _TIME_BITS=64, i.e. current Alpine — long is 32 bits, so a StartTime past 2038 zeroes instead of parsing, in a field that could have held it. Not a regression, since the old int cast was UB there anyway, and the commit message is straight about the tradeoff; but that's a platform class ci.yml has no runner for (no 32-bit Linux, no musl), so neither revision gets checked there. hdr_time.h already splits tv_sec on _WIN32 || _WIN64 || __CYGWIN__, and keying the bound and the cast off that same branch — long there, time_t elsewhere — would drop the compromise without bringing C4244 back. Happy to be told the simpler version is preferred.

Second, the reject is silent: hdr_log_read_header still returns 0 and hands back 0/0, so a log with an out-of-range StartTime parses "successfully" as having started at the epoch. That's a behaviour choice rather than a bug, but the reference point is what Java's HistogramLogReader does with the same header — Java's double→long narrowing is defined and saturating, so it never had to make this call, and I haven't verified what it actually lands on. Worth comparing against the Java reader before settling on zero rather than an error.

Pre-existing rather than introduced here, and the same family as the tv_nsec == 1000000000 case you've deferred: a negative fractional StartTime (-2.5) gives tv_nsec = -500000000. The test covers -2.0, which sidesteps it — worth folding into that follow-up.

One note on verification: the per-PR ClusterFuzzLite job is ASan only, UBSan runs in the weekly batch, so nothing in this PR's own checks re-confirms the UB is gone. The local clang float-cast-overflow run in the description is the real evidence, which is the usual situation for this repo.

[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.