Fix overflow/underflow detection in join_seconds by derekmauro · Pull Request #354 · google/cctz

derekmauro · 2026-07-17T19:58:59Z

Improve the precision of overflow and underflow checks in the join_seconds template function.

Use int64_t for intermediate subsecond calculations to prevent premature overflow when the target representation type is small.
Refine the underflow check to correctly handle boundary cases near the minimum limit, avoiding unsafe subtraction.
Handle cases where the duration denominator is larger than the maximum limit of the representation type.

Fixes #199

Improve the precision of overflow and underflow checks in the `join_seconds` template function. Specifically: - Use `int64_t` for intermediate subsecond calculations to prevent premature overflow when the target representation type is small. - Refine the underflow check to correctly handle boundary cases near the minimum limit, avoiding unsafe subtraction. - Handle cases where the duration denominator is larger than the maximum limit of the representation type. Fixes google#199

devbww · 2026-07-22T18:37:11Z

@derekmauro ... Just a heads-up that I think I'll have some comments for this, but it might take me a day or two to formulate them. Thanks.

derekmauro · 2026-07-22T18:42:07Z

@devbww - No problem. If you haven't figured it out yet, thanks to Mythos we are all inundated with bugs, and apparently I am not the only person with access. Take your time.

devbww · 2026-07-22T18:48:05Z

If you haven't figured it out yet, thanks to Mythos we are all inundated with bugs, and apparently I am not the only person with access.

I had not figured it out, but I was beginning to wonder. Hopefully the pickings are getting slimmer. :-)

derekmauro · 2026-08-10T16:32:21Z

@devbww - Do you still want to review this one?

devbww · 2026-08-11T13:40:54Z

@devbww - Do you still want to review this one?

Yes. I'm sorry for the delay. That bout of recent PRs sucked up my free time. I'll respond today. Thanks.

devbww · 2026-08-12T04:47:15Z

-  *tpp += std::chrono::duration_cast<D>(fs);
+  // Use a 64-bit representation for intermediate subsecond calculations
+  // to avoid premature overflow if Rep is a smaller type (e.g., int8_t).
+  using D_check = std::chrono::duration<std::int64_t, std::ratio<1, Denom>>;


We should avoid std::int64_t. It isn't even guaranteed to exist. We don't use the fixed-size integer types anywhere in cctz except in these overflow tests (which seems necessary to make tight expectations, but which would be nice to avoid), the fuzz tests (which I disavow, but should probably be changed), and the Windows time-zone-name stuff (which I assume is required by the API).

So, std::int_fast64_t would be a better choice. That will also make count and sub have the same type, which looks like a feature.

That said, std::intmax_t is probably the best choice, and that is the type of Denom anyway (and it might be also be Rep).

And that said, Rep may also be a floating-point type, so we'll need to be careful there too (see below).

devbww · 2026-08-12T04:47:24Z

+  // Check for underflow.
+  // Equivalent to: count * Denom + sub < min, but safe from underflow.
+  // We cannot use "min - sub" directly as it would underflow.
+  const auto min_div = (std::numeric_limits<Rep>::min)() / Denom;


std::numeric_limits<Rep>::min() is not the "minimum" value of Rep when it is floating point. Rather, it is the smallest, positive, normalized value. So, we'll either have to avoid min() or treat floating point separately.

Note that we similarly use std::numeric_limits<Rep>::min() in a couple of the other existing join_seconds() overloads, so we should deal with them somehow too.

I don't think we can now simply exclude floating-point representations, despite them currently misbehaving on pre-epoch times.

Fixed to use std::numeric_limits<Rep>::lowest(). Added tag dispatch for floating point types.

devbww · 2026-08-12T04:47:27Z

+    const auto min_mod = (std::numeric_limits<Rep>::min)() % Denom;
+    if (sub < Denom + min_mod) return false;
+  }
+  *tpp = time_point<D>() + D{static_cast<Rep>(count * Denom + sub)};


Until C++20, there is no guarantee that the std::chrono::system_clock epoch is the same as the Unix epoch, so carefully checking that count * Denom + sub fits in Rep may all be for naught when chrono includes some additional offset in its calculations.

But that said, I'd be happy to place something along the lines of ...

#if __cplusplus < 202002L // Assert that the std::chrono::system_clock epoch is the Unix epoch. // This is required by C++20, but was also ubiquitous before that. // We use this in join_seconds() to simplify overflow detection. assert(std::chrono::system_clock::from_time_t(0).time_since_epoch() == std::chrono::system_clock::duration::zero()); #endif

in, say, detail::parse() so that we might assume there is no such offset.

devbww · 2026-08-12T04:47:33Z

  const time_zone utc = utc_time_zone();

+  // Test with nanosecond resolution (typical 64-bit time_point).
  using D = chrono::duration<std::int64_t, std::nano>;


Is there any reason to test std::femto or beyond?

Added a bunch of tests.

devbww · 2026-08-12T04:47:35Z

            cctz::format(RFC3339_full, tp, utc));
-#if 0
-  // TODO(#199): Will fail until cctz::parse() properly detects overflow.
+  


Trailing whitespace.

devbww · 2026-08-17T07:20:42Z

    const time_point<seconds>& sec, const femtoseconds& fs,
-    time_point<std::chrono::duration<Rep, std::ratio<1, Denom>>>* tpp) {
+    time_point<std::chrono::duration<Rep, std::ratio<1, Denom>>>* tpp,
+    std::true_type /* is_integral */) {


Consider changing the comment here and below to /* is_integral<Rep> */ to make it clear what type we're talking about.

But, I think the next suggestion is better, and avoids this altogether.

devbww · 2026-08-17T07:20:57Z

+    std::true_type /* is_integral */) {
+  using D = std::chrono::duration<Rep, std::ratio<1, Denom>>;
+  using D_check = std::chrono::duration<std::intmax_t, std::ratio<1, Denom>>;
+  const auto count = static_cast<std::intmax_t>(sec.time_since_epoch().count());


It seems clearer and safer to do simple initialization with a named type rather than static_cast'ing into an auto. That is, ...

const std::intmax_t count = sec.time_since_epoch().count();

devbww · 2026-08-17T07:21:11Z

+    const time_point<seconds>& sec, const femtoseconds& fs,
+    time_point<std::chrono::duration<Rep, std::ratio<1, Denom>>>* tpp) {
+  return join_seconds(sec, fs, tpp, std::is_integral<Rep>());
+}


I would avoid this template that indirects to the other, now true/false_type-overloaded templates by simply making the original "1/Denom" template, and the new floating-point one, only accept the desired type.

That is, remove this helper and introduce std::enable_if<> into the others as ...

template <typename Rep, std::intmax_t Denom> typename std::enable_if<std::is_integral<Rep>::value, bool>::type join_seconds( const time_point<seconds>& sec, const femtoseconds& fs, time_point<std::chrono::duration<Rep, std::ratio<1, Denom>>>* tpp);

and

template <typename Rep, std::intmax_t Denom> typename std::enable_if<std::is_floating_point<Rep>::value, bool>::type join_seconds( const time_point<seconds>& sec, const femtoseconds& fs, time_point<std::chrono::duration<Rep, std::ratio<1, Denom>>>* tpp);

This also makes it very clear that the second one is for is_floating_point<Rep>, and not just !is_integral<Rep>, even if we might know from elsewhere that Rep has to be arithmetic.

But, also see below about generalizing the floating-point version.

devbww · 2026-08-17T07:21:25Z

+template <typename Rep, std::intmax_t Denom>
+bool join_seconds(
+    const time_point<seconds>& sec, const femtoseconds& fs,
+    time_point<std::chrono::duration<Rep, std::ratio<1, Denom>>>* tpp,


We don't want to limit this new floating-point version to only representations of sub-seconds, however. So, ...

template <typename Rep, std::intmax_t Num, std::intmax_t Denom> typename std::enable_if<std::is_floating_point<Rep>::value, bool>::type join_seconds( const time_point<seconds>& sec, const femtoseconds& fs, time_point<std::chrono::duration<Rep, std::ratio<Num, Denom>>>* tpp) { using D = std::chrono::duration<Rep, std::ratio<Num, Denom>>; *tpp = std::chrono::time_point_cast<D>(sec); *tpp += std::chrono::duration_cast<D>(fs); return true; }

Methinks all the other join_seconds() templates should then be restricted to is_integral<Rep>.

devbww · 2026-08-17T07:21:44Z

 bool join_seconds(
    const time_point<seconds>& sec, const femtoseconds& fs,
-    time_point<std::chrono::duration<Rep, std::ratio<1, Denom>>>* tpp) {
+    time_point<std::chrono::duration<Rep, std::ratio<1, Denom>>>* tpp,


Aside: We should probably do something using std::ratio<>::type to ensure we're dealing with the lowest terms by the time we reach for these templates.

Here is the idea to replace the existing cctz::parse():

template <typename Rep, typename Period> bool parse(const std::string& fmt, const std::string& input, const time_zone& tz, time_point<std::chrono::duration<Rep, Period>>* tpp) { time_point<seconds> sec; detail::femtoseconds fs; if (!detail::parse(fmt, input, tz, &sec, &fs)) return false; time_point<std::chrono::duration<Rep, typename Period::type>> tp; if (!detail::join_seconds(sec, fs, &tp)) return false; *tpp = tp; return true; }

And a test that previously would not have compiled could be something like:

TEST(Parse, RatioNonLowestTerms) { const time_zone utc = utc_time_zone(); using D = chrono::duration<std::int64_t, std::ratio<2, 2>>; time_point<D> tp; EXPECT_TRUE(parse(RFC3339_full, "2026-08-17T11:42:16-04:00", utc, &tp)); ExpectTime(tp, utc, 2026, 8, 17, 15, 42, 16, 0, false, "UTC"); }

devbww · 2026-08-17T07:21:53Z

+  using D_check = std::chrono::duration<std::intmax_t, std::ratio<1, Denom>>;
+  const auto count = static_cast<std::intmax_t>(sec.time_since_epoch().count());
+  const auto sub =
+      std::chrono::duration_cast<D_check>(fs).count();  // [0, Denom)


Aside: Hopefully chrono is smart enough to avoid unnecessary overflow when converting to a ratio with a very large Denom.

devbww · 2026-08-17T07:22:13Z

+  EXPECT_FALSE(parse(RFC3339_full, "1969-12-31T23:59:50+00:00", utc, &atp));
+
+  // Test with 1-second resolution using int8_t (very narrow range: [-128, 127] seconds).
  using DS = chrono::duration<std::int8_t, chrono::seconds::period>;


Elsewhere we omit the Period when it is the default std::ratio<1, 1> (e.g., in the definition of cctz::seconds). My suggestion would be to keep doing that here.

devbww · 2026-08-17T07:22:23Z

  EXPECT_FALSE(parse(RFC3339_full, "1969-12-31T23:57:51.9+00:00", utc, &stp));

+  // Test with 1-minute resolution using int8_t (range: [-128, 127] minutes).
  using DM = chrono::duration<std::int8_t, chrono::minutes::period>;


We must use a named ratio like std::femto when there is no matching chrono::duration to extract a period from, so I would suggest always using ratios directly and saying std::ratio<60> here (which requires #include <ratio>). But, it's a small point.

devbww · 2026-08-17T07:22:37Z

@@ -1628,52 +1628,144 @@ TEST(Parse, MaxRange) {
 TEST(Parse, TimePointOverflow) {


All these cases really have the same form, so I'd make them look as similar as possible.

I'd also nest each case so we don't have to conjure up new type and variable names each time. (Perhaps they could even be separate tests.)

Anyway, to avoid you having to guess everything, here is what I mean (despite its length):

TEST(Parse, TimePointOverflow) { const time_zone utc = utc_time_zone(); { // Test with nanosecond resolution (typical 64-bit time_point). using D = chrono::duration<std::int64_t, std::nano>; time_point<D> tp; // Max time_point<D> is 2262-04-11T23:47:16.854775807+00:00. EXPECT_TRUE( parse(RFC3339_full, "2262-04-11T23:47:16.8547758079+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::max()); EXPECT_EQ("2262-04-11T23:47:16.854775807+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 nanosecond beyond max should fail. EXPECT_FALSE( parse(RFC3339_full, "2262-04-11T23:47:16.8547758080+00:00", utc, &tp)); // Min time_point<D> is 1677-09-21T00:12:43.145224192+00:00. EXPECT_TRUE( parse(RFC3339_full, "1677-09-21T00:12:43.1452241920+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::min()); EXPECT_EQ("1677-09-21T00:12:43.145224192+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 nanosecond below min should fail. EXPECT_FALSE( parse(RFC3339_full, "1677-09-21T00:12:43.1452241919+00:00", utc, &tp)); } { // Test with femtosecond resolution. using D = chrono::duration<std::int64_t, std::femto>; time_point<D> tp; // Max time_point<D> is 1970-01-01T02:33:43.372036854775807+00:00. EXPECT_TRUE(parse(RFC3339_full, "1970-01-01T02:33:43.3720368547758079+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::max()); EXPECT_EQ("1970-01-01T02:33:43.372036854775807+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 femtosecond beyond max should fail. EXPECT_FALSE(parse(RFC3339_full, "1970-01-01T02:33:43.3720368547758080+00:00", utc, &tp)); // Min time_point<D> is 1969-12-31T21:26:16.627963145224192+00:00. EXPECT_TRUE(parse(RFC3339_full, "1969-12-31T21:26:16.6279631452241920+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::min()); EXPECT_EQ("1969-12-31T21:26:16.627963145224192+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 femtosecond below min should fail. EXPECT_FALSE(parse(RFC3339_full, "1969-12-31T21:26:16.6279631452241919+00:00", utc, &tp)); } { // Test with attosecond resolution. using D = chrono::duration<std::int64_t, std::atto>; time_point<D> tp; // Max time_point<D> is 1970-01-01T00:00:09.223372036854775807+00:00, // but cctz::parse() truncates (towards zero) to a femtosecond boundary, // so the last three decimal places are lost. EXPECT_TRUE(parse(RFC3339_full, "1970-01-01T00:00:09.223372036854775807+00:00", utc, &tp)); EXPECT_EQ(tp.time_since_epoch(), D{9223372036854775000LL}); EXPECT_EQ("1970-01-01T00:00:09.223372036854775+00:00", cctz::format(RFC3339_full, tp, utc)); // Moving into the next femtosecond should fail. EXPECT_FALSE(parse(RFC3339_full, "1970-01-01T00:00:09.223372036854776000+00:00", utc, &tp)); // Min time_point<D> is 1969-12-31T23:59:50.776627963145224192+00:00, // but cctz::parse() truncates (towards zero) to a femtosecond boundary, // so we must round up. EXPECT_TRUE(parse(RFC3339_full, "1969-12-31T23:59:50.776627963145225000+00:00", utc, &tp)); EXPECT_EQ(tp.time_since_epoch(), D{-9223372036854775000LL}); EXPECT_EQ("1969-12-31T23:59:50.776627963145225+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 attosecond below min should fail. EXPECT_FALSE(parse(RFC3339_full, "1969-12-31T23:59:50.776627963145224999+00:00", utc, &tp)); } { // Test with 1-second resolution using int8_t ([-128, 127] seconds). using D = chrono::duration<std::int8_t>; time_point<D> tp; // Max time_point<D> is 1970-01-01T00:02:07+00:00. EXPECT_TRUE(parse(RFC3339_full, "1970-01-01T00:02:07.9+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::max()); EXPECT_EQ("1970-01-01T00:02:07+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 second beyond max should fail. EXPECT_FALSE(parse(RFC3339_full, "1970-01-01T00:02:08+00:00", utc, &tp)); // Min time_point<D> is 1969-12-31T23:57:52+00:00. EXPECT_TRUE(parse(RFC3339_full, "1969-12-31T23:57:52+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::min()); EXPECT_EQ("1969-12-31T23:57:52+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 second below min should fail. EXPECT_FALSE(parse(RFC3339_full, "1969-12-31T23:57:51+00:00", utc, &tp)); } { // Test with 1-minute resolution using int8_t ([-128, 127] minutes). using D = chrono::duration<std::int8_t, std::ratio<60>>; time_point<D> tp; // Max time_point<D> is 1970-01-01T02:07:00+00:00. EXPECT_TRUE(parse(RFC3339_full, "1970-01-01T02:07:00+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::max()); EXPECT_EQ("1970-01-01T02:07:00+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 minute beyond max should fail. EXPECT_FALSE(parse(RFC3339_full, "1970-01-01T02:08:00+00:00", utc, &tp)); // Min time_point<D> is 1969-12-31T21:52:00+00:00. EXPECT_TRUE(parse(RFC3339_full, "1969-12-31T21:52:00+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::min()); EXPECT_EQ("1969-12-31T21:52:00+00:00", cctz::format(RFC3339_full, tp, utc)); // 1 minute below min should fail. EXPECT_FALSE(parse(RFC3339_full, "1969-12-31T21:51:00+00:00", utc, &tp)); } { // Test with millisecond resolution using int8_t ([-128, 127] milliseconds). // This tests the case where Denom (1000) is larger than Rep max/min. using D = chrono::duration<std::int8_t, std::milli>; time_point<D> tp; // Max time_point<D> is 1970-01-01T00:00:00.127+00:00. EXPECT_TRUE(parse(RFC3339_full, "1970-01-01T00:00:00.127+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::max()); // 1 millisecond beyond max should fail (tests sub > max check). EXPECT_FALSE( parse(RFC3339_full, "1970-01-01T00:00:00.128+00:00", utc, &tp)); // Min time_point<D> is 1969-12-31T23:59:59.872+00:00. EXPECT_TRUE(parse(RFC3339_full, "1969-12-31T23:59:59.872+00:00", utc, &tp)); EXPECT_EQ(tp, time_point<D>::min()); // 1 millisecond below min should fail (tests min with Denom > -min). EXPECT_FALSE( parse(RFC3339_full, "1969-12-31T23:59:59.871+00:00", utc, &tp)); } }

devbww · 2026-08-17T07:22:48Z

+  EXPECT_TRUE(parse(RFC3339_full, "1970-01-01T00:00:00.5+00:00", utc, &mdtp));
+  EXPECT_EQ(mdtp.time_since_epoch().count(), 500.0);
+  EXPECT_TRUE(parse(RFC3339_full, "1969-12-31T23:59:59.5+00:00", utc, &mdtp));
+  EXPECT_EQ(mdtp.time_since_epoch().count(), -500.0);


These don't have anything to do with "TimePointOverflow", so I'd place them in a separate test.

It also seems like we should be testing fractional values and periods larger than 1 second (which would catch the join_second() issue mentioned above).

So, ...

TEST(Parse, TimePointFloatingPoint) { const time_zone utc = utc_time_zone(); { using D = chrono::duration<double>; time_point<D> tp; EXPECT_TRUE(parse(RFC3339_full, "1970-01-01T00:00:59.25+00:00", utc, &tp)); EXPECT_EQ(tp.time_since_epoch(), D{59.25}); EXPECT_TRUE(parse(RFC3339_full, "1969-12-31T23:59:00.75+00:00", utc, &tp)); EXPECT_EQ(tp.time_since_epoch(), D{-59.25}); } { using D = chrono::duration<double, std::milli>; time_point<D> tp; EXPECT_TRUE( parse(RFC3339_full, "1970-01-01T00:00:00.015625+00:00", utc, &tp)); EXPECT_EQ(tp.time_since_epoch(), D{15.625}); EXPECT_TRUE( parse(RFC3339_full, "1969-12-31T23:59:59.984375+00:00", utc, &tp)); EXPECT_EQ(tp.time_since_epoch(), D{-15.625}); } { using D = chrono::duration<double, std::ratio<60>>; time_point<D> tp; EXPECT_TRUE(parse(RFC3339_full, "1970-01-01T00:01:15+00:00", utc, &tp)); EXPECT_EQ(tp.time_since_epoch(), D{1.25}); EXPECT_TRUE(parse(RFC3339_full, "1969-12-31T23:58:45+00:00", utc, &tp)); EXPECT_EQ(tp.time_since_epoch(), D{-1.25}); } }

devbww · 2026-08-17T07:27:57Z

  if (count > (std::numeric_limits<Rep>::max)()) return false;
-  if (count < (std::numeric_limits<Rep>::min)()) return false;
+  if (count < (std::numeric_limits<Rep>::lowest)()) return false;
  *tpp = time_point<D>() + D{static_cast<Rep>(count)};


If I was writing this again, I'd say ...

*tpp = std::chrono::time_point_cast<D>(sec);

Perhaps now is a good time to change it.

devbww · 2026-08-17T07:28:02Z

  if (count > (std::numeric_limits<Rep>::max)()) return false;
-  if (count < (std::numeric_limits<Rep>::min)()) return false;
+  if (count < (std::numeric_limits<Rep>::lowest)()) return false;
  *tpp = time_point<D>() + D{static_cast<Rep>(count)};


mkruskal-google assigned devbww Jul 23, 2026

devbww reviewed Aug 12, 2026

View reviewed changes

derekmauro added 2 commits August 12, 2026 15:05

Merge branch 'master' into b529539419

b12ebee

Changes from code review

e7c5649

derekmauro force-pushed the b529539419 branch from e8cdefa to e7c5649 Compare August 12, 2026 19:52

Fix clang-format-created inconsistencies

86193b2

devbww reviewed Aug 17, 2026

View reviewed changes

		@@ -1628,52 +1628,144 @@ TEST(Parse, MaxRange) {
		TEST(Parse, TimePointOverflow) {

Uh oh!

Conversation

derekmauro commented Jul 17, 2026

Uh oh!

devbww commented Jul 22, 2026

Uh oh!

derekmauro commented Jul 22, 2026

Uh oh!

devbww commented Jul 22, 2026

Uh oh!

derekmauro commented Aug 10, 2026

Uh oh!

devbww commented Aug 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekmauro Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

derekmauro Aug 12, 2026 •

edited

Loading