Skip to content

fix unchecked float to integral conversion in detail::duration_cast - #4985

Open
aizu-m wants to merge 3 commits into
fmtlib:mainfrom
aizu-m:chrono-mixed-duration-cast
Open

aizu-m wants to merge 3 commits into
fmtlib:mainfrom
aizu-m:chrono-mixed-duration-cast

Conversation

@aizu-m

@aizu-m aizu-m commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

UBSan, clang -fsanitize=undefined, when formatting a time point with a floating-point rep:

__chrono/duration.h:75:63: runtime error: 1e+300 is outside the range of representable values of type 'long'
__chrono/duration.h:75:63: runtime error: nan is outside the range of representable values of type 'long long'

Repro:

using fsec = std::chrono::duration<double>;
fmt::format("{:%T}", fmt::sys_time<fsec>(fsec(1e300)));
fmt::format("{:%T}", fmt::sys_time<fsec>(fsec(NAN)));  // prints "00:00:00an"

The mixed integer <-> float overload of detail::duration_cast delegates to std::chrono::duration_cast, so to_time_t and the sys_time / local_time formatters convert the seconds count with an unchecked static_cast. Only the float to integral direction can overflow, so the overload is split: integer to float keeps the plain cast, and float to integral range-checks the count before casting (which also rejects NaN) and throws like the other checked overloads, with the same boundary handling as to_nonnegative_int (#4802). is_similar_arithmetic_type had no remaining users and is removed.

@aizu-m
aizu-m requested a review from vitaut as a code owner October 14, 2026 17:16

@vitaut vitaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Floating-point-to-integer conversion truncates toward zero, so values slightly below the minimum are also valid. For example, converting -0.5 to unsigned produces 0, but this check rejects it. Could we check std::trunc(count) instead and add a regression test?

@aizu-m

aizu-m commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Good catch. The check was on the raw count, so -0.5 to unsigned threw even though the cast gives 0. Same at the signed end: INT_MIN - 0.5 was rejected but truncates to INT_MIN.

Switched it to check std::trunc(count) in c9b6642. NaN still fails the comparison, so that path is unchanged. Added chrono_test.float_to_integral_duration_cast for the -0.5 to unsigned, INT_MIN - 0.5 and INT_MAX + 0.5 cases, plus -1 and INT_MIN - 1 still throwing. It fails on the previous revision and passes now, with the rest of chrono-test clean under UBSan.

@vitaut vitaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One remaining concern: the change removes support for custom duration representations. The previous overload handled any combination of representation types that wasn't integral-to-integral or floating-point-to-floating-point. The new overloads only handle built-in arithmetic types.

I think we should preserve the original fallback for other representation types.

@aizu-m

aizu-m commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Right, the split dropped every rep that isn't a built-in arithmetic type. A duration<custom_rep> to seconds cast stopped compiling with "no matching function for call to 'duration_cast'".

Restored the original fallback in f501601. It now covers everything that isn't similar-arithmetic and isn't float to integral, so custom reps and integer to float go through std::chrono::duration_cast as before. Only float to integral takes the range-checked path. is_similar_arithmetic_type is back.

Added chrono_test.custom_rep_duration_cast. It fails to compile on the previous revision and passes now. chrono-test is clean under UBSan.

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.

2 participants