avoid signed overflow computing 400-year shift in BreakTime

[rajath201]

BreakTime()'s extended-zone branch shifts a far-future time back through the 400-year cycle with shift = diff / kSecsPer400Years + 1, then forms seconds(shift * kSecsPer400Years) in signed int64. When a zone is extended by a POSIX DST footer and its last transition sits low in positive unix time (explicit transitions before ~1795, e.g. a 1700 transition that extends to 2101), a lookup near time_point<seconds>::max() drives shift past seconds::max()/kSecsPer400Years and that multiply overflows. UBSan trips at time_zone_info.cc:979 on 730692562 * 12622780800.

TimeLocal() already clamps this same product, but BreakTime() didn't. Clamping shift to seconds::max().count() / kSecsPer400Years fixes it: a cycle is exactly 400 years, so the recursive call absorbs whatever the clamp trims and every in-range lookup returns the same result. The regression test extends a 1700 zone to 2101 and looks up the maximum time.

[devbww]

Reducing by only seconds::max().count() / kSecsPer400Years cycles and handling the residual recursively is certainly valid. But so would reducing by one cycle (albeit with unacceptable recursion depth).

So it seems to me that it would be clearer if we directly split shift into two cycle-sized pieces, N and 1, and handled them both in the same call. That is, the shift = ... + 1 is the real problem. Therefore, ...

      const year_t shift = diff / kSecsPer400Years;
      time_zone::absolute_lookup al = BreakTime(
          tp - seconds(shift * kSecsPer400Years) - seconds(kSecsPer400Years));
      al.cs = YearShift(al.cs, (shift + 1) * 400);

That said, I think we would instead be best off if we simply extended the restriction introduced in #357 such that the 400-year cycle has to start in non-negative territory rather than end there. This would guarantee that (diff / kSecsPer400Years + 1) / kSecsPer400Years is representable.

--- a/src/time_zone_info.cc
+++ b/src/time_zone_info.cc
@@ -342,6 +342,14 @@ bool TimeZoneInfo::ExtendTransitions() {
     return EquivTransitions(transitions_.back().type_index, dst_ti);
   }
 
+  // We require that zoneinfo data with a rule for future transitions
+  // ends with a non-negative transition.  This removes the need to add
+  // any "second-half" transition to ensure differences between adjacent
+  // transitions are always representable, while also guaranteeing that
+  // the arithmetic used to shift between 400-year cycles never overflows.
+  // All valid zones easily meet this requirement.
+  if (transitions_.back().unix_time < 0) return false;
+
   // Extend the transitions for an additional 401 years using the future
   // specification. Years beyond those can be handled by mapping back to
   // a cycle-equivalent year within that range. Note that we need 401
@@ -850,7 +858,7 @@ bool TimeZoneInfo::Load(ZoneInfoSource* zip) {
   // previous transition is always representable, without overflow.
   const Transition& last(transitions_.back());
   if (last.unix_time < 0) {
-    if (extended_) return false;
+    assert(!extended_);
     const std::uint_fast8_t type_index = last.type_index;
     Transition& tr(*transitions_.emplace(transitions_.end()));
     tr.unix_time = 2147483647;  // 2038-01-19T03:14:07+00:00

[devbww]

The suggested change breaks this assertion as the zone will no longer load.

The same will be true of the recently added "test:extended_dst", so we'll have to adjust that case.

[devbww]

If some form of this test survives, I would formulate it like all the similar existing ones by using ExpectTime().

  auto tp_max = time_point<cctz::seconds>::max();
  ExpectTime(tp_max, tz, 292277026596, 12, 4, 10, 30, 7, -5 * 3600, false,
             "EST");
  EXPECT_STREQ("EST", tz.lookup(tp_max).abbr);