reject zones whose extended transitions are out of order - #365
Conversation
| // its own, so the two years' generated transitions overlap. | ||
| return std::unique_ptr<ZoneInfoSource>(new StringZoneInfoSource( | ||
| MakeExtendedTzif(0, -5 * 3600, std::string{"STD", 4}, | ||
| "STD-12:00:00DST12:00:00,358/100:00:00,1/-139:00:00"))); |
There was a problem hiding this comment.
Civil-to-absolute conversion in CCTZ is premised on the civil time of transitions being increasing. While this POSIX rule is expressible (with the extended zic rules), I would assert that it not practical given that it loops civil times back on themselves outside of a single, REPEATED transition. No real zone would ever behave that way.
So, much like other recent changes, I think the best tactic here is to reject such rules and cause the load_time_zone() to fail. That is, ...
--- a/src/time_zone_info.cc
+++ b/src/time_zone_info.cc
@@ -753,11 +753,6 @@ bool TimeZoneInfo::Load(ZoneInfoSource* zip) {
if (transitions_[i].unix_time < -(1LL << 59) ||
transitions_[i].unix_time > (1LL << 59))
return false; // out of range
- if (i != 0) {
- // Check that the transitions are ordered by time (as zic guarantees).
- if (!Transition::ByUnixTime()(transitions_[i - 1], transitions_[i]))
- return false; // out of order
- }
}
bool seen_type_0 = false;
for (std::size_t i = 0; i != hdr.timecnt; ++i) {
@@ -880,11 +875,13 @@ bool TimeZoneInfo::Load(ZoneInfoSource* zip) {
ttp = &transition_types_[tr.type_index];
tr.civil_sec = LocalTime(tr.unix_time, *ttp).cs;
if (i != 0) {
- // Check that the transitions are ordered by civil time. Essentially
- // this means that an offset change cannot cross another such change.
- // No one does this in practice, and we depend on it in MakeTime().
- if (!Transition::ByCivilTime()(transitions_[i - 1], tr))
+ // Check that offset changes don't cross each other. No one
+ // does this is practice, and we depend on increasing absolute
+ // and civil times in BreakTime() and MakeTime() respectively.
+ if (!Transition::ByUnixTime()(transitions_[i - 1], tr) ||
+ !Transition::ByCivilTime()(transitions_[i - 1], tr)) {
return false; // out of order
+ }
}
}
There was a problem hiding this comment.
Makes sense. Applied your diff (with the small "does this is practice" typo fixed) and flipped the test to expect load_time_zone() to fail. All four suites pass, and every zone in testdata/zoneinfo still loads since the consolidated check sees the same loaded transitions as before plus the extended ones.
There was a problem hiding this comment.
Looks good to me. Thanks. I'll now pass this along to @derekmauro.
One optional thing you could do in the meantime, if you like, is unify the ordering of the if (name == "test:Foo") statements with the TEST(TimeZoneEdgeCase, Foo) cases. They might as well be the same. There could even be some logical order.
A POSIX transition time may carry a day offset of up to +/-167 hours, so the pair of transitions ExtendTransitions() generates for one year can overlap the pair generated for the next, leaving transitions_ out of order in unix time. BreakTime() binary searches that vector, so a lookup on such a zone depends on what was looked up before it. No real zone loops civil times back on themselves like this, so fold the unix-time ordering check into the loop that already checks the civil-time ordering of every transition, and fail the load instead.
2936f3d to
772c6ca
Compare
rajath201
changed the title
| // its own, so the two years' generated transitions overlap. | ||
| return std::unique_ptr<ZoneInfoSource>(new StringZoneInfoSource( | ||
| MakeExtendedTzif(0, -5 * 3600, std::string{"STD", 4}, | ||
| "STD-12:00:00DST12:00:00,358/100:00:00,1/-139:00:00"))); |
There was a problem hiding this comment.
Looks good to me. Thanks. I'll now pass this along to @derekmauro.
One optional thing you could do in the meantime, if you like, is unify the ordering of the if (name == "test:Foo") statements with the TEST(TimeZoneEdgeCase, Foo) cases. They might as well be the same. There could even be some logical order.
3 participants
ExtendTransitions() generates the future transitions a year at a time and guards each generated pair with
last_time, the unix time of the last transition loaded from the file. That bound never moves as transitions are pushed, so nothing keeps one year's pair ahead of the next year's. A transition time may carry a day offset of up to +/-167 hours, which is more slack than the day or so separating adjacent years, so a footer whose daylight time starts several days after its nominal date while the following year's ends several days before its own leaves transitions_ out of order in unix time. WithSTD-12:00:00DST12:00:00,358/100:00:00,1/-139:00:00(ordinary +/-12h offsets) the same instant 1970-12-27T17:00:00Z resolves to offset 43200 or -18000 on the same time_zone, depending only on which instant was looked up before it.No real zone loops civil times back on themselves this way, so reject such rules and fail the load: fold the unix-time ordering check into the loop that already checks the civil-time ordering of every transition, which covers the extended transitions as well as the loaded ones. Every zone in testdata/zoneinfo still loads.