From 5404143dee89e7e4f338dfcfffca2446b249f633 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Thu, 24 Apr 2025 17:15:51 +0200 Subject: [PATCH] Ensure consistent mktime() DST behavior across different implementations There are inputs to mktime() where the behavior is not specified and there's also no single obviously correct behavior. In particular, this affects how auto-detection of whether DST is in effect is done when tm_isdst = -1 is set and the time specified does not exist at all or exists twice on that day. If different implementations are used within an Icinga 2 cluster, that can lead to inconsistent behavior because different nodes may interpret the same TimePeriod differently. This commit introduces a wrapper to mktime(), namely Utility::NormalizeTm() that implements the behavior provided by glibc. The choice for glibc's behavior is pretty arbitrary, it was simply picked because most systems that are officially/fully supported use it (with the only exception being Windows), so this should give the least possible amount of user-visible changes. As part of this commit, the closely related helper function mktime_const() is also moved to Utility::TmToTimestamp() and made a wrapper around the newly introduced NormalizeTm(). --- lib/base/datetime.cpp | 2 +- lib/base/utility.cpp | 48 +++++++++++++++++++ lib/base/utility.hpp | 3 ++ lib/icinga/legacytimeperiod.cpp | 45 +++++++----------- test/CMakeLists.txt | 1 + test/base-utility.cpp | 83 +++++++++++++++++++++++++++++++++ 6 files changed, 153 insertions(+), 29 deletions(-) diff --git a/lib/base/datetime.cpp b/lib/base/datetime.cpp index aa7b5e59e..3e443e613 100644 --- a/lib/base/datetime.cpp +++ b/lib/base/datetime.cpp @@ -35,7 +35,7 @@ DateTime::DateTime(const std::vector& args) tms.tm_isdst = -1; - m_Value = mktime(&tms); + m_Value = Utility::TmToTimestamp(&tms); } else if (args.size() == 1) m_Value = args[0]; else diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index a2e461e5c..2b4e32def 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -1958,3 +1958,51 @@ bool Utility::ComparePasswords(const String& enteredPassword, const String& actu return result; } + +/** + * Normalizes the given struct tm like mktime() from libc does with some exception for DST handling: If the given time + * exists twice on a day, the instance in the DST timezone is picked. If the time does not actually exist on a day, it's + * interpreted using the UTC offset of the standard timezone and then normalized. + * + * This is done in order to provide consistent behavior across operating systems. Historically, Icinga 2 just relied on + * whatever mktime() of the operating system did and this function mimics what glibc does as that's what most systems + * use. + * + * @param t tm struct to be normalized + * @return time_t representing the timestamp given by t + */ +time_t Utility::NormalizeTm(tm *t) +{ + // If tm_isdst already specifies the timezone (0 or 1), just use the mktime() behavior. + if (t->tm_isdst >= 0) { + return mktime(t); + } + + const tm copy = *t; + + t->tm_isdst = 1; + time_t result = mktime(t); + if (result != -1 && t->tm_isdst == 1) { + return result; + } + + // Restore the original input. mktime() can (and does) change more fields than just tm_isdst by converting from + // daylight saving time to standard time (it moves the contents by (typically) an hour, which can move across + // days/weeks/months/years changing all other fields). + *t = copy; + + t->tm_isdst = 0; + return mktime(t); +} + +/** + * Returns the same as NormalizeTm() but takes a const pointer as argument and thus does not modify it. + * + * @param t struct tm to convert to time_t + * @return time_t representing the timestamp given by t + */ +time_t Utility::TmToTimestamp(const tm *t) +{ + tm copy = *t; + return NormalizeTm(©); +} diff --git a/lib/base/utility.hpp b/lib/base/utility.hpp index 5132673ca..2a2fe0c1b 100644 --- a/lib/base/utility.hpp +++ b/lib/base/utility.hpp @@ -185,6 +185,9 @@ public: return in.SubStr(0, maxLength - sha1HexLength - strlen(trunc)) + trunc + SHA1(in); } + static time_t NormalizeTm(tm *t); + static time_t TmToTimestamp(const tm *t); + private: Utility(); diff --git a/lib/icinga/legacytimeperiod.cpp b/lib/icinga/legacytimeperiod.cpp index 55bc58ed3..6d2433758 100644 --- a/lib/icinga/legacytimeperiod.cpp +++ b/lib/icinga/legacytimeperiod.cpp @@ -13,23 +13,12 @@ using namespace icinga; REGISTER_FUNCTION_NONCONST(Internal, LegacyTimePeriod, &LegacyTimePeriod::ScriptFunc, "tp:begin:end"); -/** - * Returns the same as mktime() but does not modify its argument and takes a const pointer. - * - * @param t struct tm to convert to time_t - * @return time_t representing the timestamp given by t - */ -static time_t mktime_const(const tm *t) { - tm copy = *t; - return mktime(©); -} - bool LegacyTimePeriod::IsInTimeRange(const tm *begin, const tm *end, int stride, const tm *reference) { time_t tsbegin, tsend, tsref; - tsbegin = mktime_const(begin); - tsend = mktime_const(end); - tsref = mktime_const(reference); + tsbegin = Utility::TmToTimestamp(begin); + tsend = Utility::TmToTimestamp(end); + tsref = Utility::TmToTimestamp(reference); if (tsref < tsbegin || tsref >= tsend) return false; @@ -85,7 +74,7 @@ void LegacyTimePeriod::FindNthWeekday(int wday, int n, tm *reference) t.tm_sec = 0; t.tm_isdst = -1; - mktime(&t); + Utility::NormalizeTm(&t); if (t.tm_wday == wday) { seen++; @@ -398,8 +387,8 @@ bool LegacyTimePeriod::IsInDayDefinition(const String& daydef, const tm *referen ParseTimeRange(daydef, &begin, &end, &stride, reference); Log(LogDebug, "LegacyTimePeriod") - << "ParseTimeRange: '" << daydef << "' => " << mktime(&begin) - << " -> " << mktime(&end) << ", stride: " << stride; + << "ParseTimeRange: '" << daydef << "' => " << Utility::TmToTimestamp(&begin) + << " -> " << Utility::TmToTimestamp(&end) << ", stride: " << stride; return IsInTimeRange(&begin, &end, stride, reference); } @@ -448,8 +437,8 @@ Dictionary::Ptr LegacyTimePeriod::ProcessTimeRange(const String& timestamp, cons ProcessTimeRangeRaw(timestamp, reference, &begin, &end); return new Dictionary({ - { "begin", (long)mktime(&begin) }, - { "end", (long)mktime(&end) } + { "begin", (long)Utility::TmToTimestamp(&begin) }, + { "end", (long)Utility::TmToTimestamp(&end) } }); } @@ -480,13 +469,13 @@ Dictionary::Ptr LegacyTimePeriod::FindRunningSegment(const String& daydef, const time_t tsend, tsiter, tsref; int stride; - tsref = mktime_const(reference); + tsref = Utility::TmToTimestamp(reference); ParseTimeRange(daydef, &begin, &end, &stride, reference); iter = begin; - tsend = mktime(&end); + tsend = Utility::NormalizeTm(&end); do { if (IsInTimeRange(&begin, &end, stride, &iter)) { @@ -518,7 +507,7 @@ Dictionary::Ptr LegacyTimePeriod::FindRunningSegment(const String& daydef, const iter.tm_hour = 0; iter.tm_min = 0; iter.tm_sec = 0; - tsiter = mktime(&iter); + tsiter = Utility::NormalizeTm(&iter); } while (tsiter < tsend); return nullptr; @@ -538,13 +527,13 @@ Dictionary::Ptr LegacyTimePeriod::FindNextSegment(const String& daydef, const St ref.tm_mday++; } - tsref = mktime(&ref); + tsref = Utility::NormalizeTm(&ref); ParseTimeRange(daydef, &begin, &end, &stride, &ref); iter = begin; - tsend = mktime(&end); + tsend = Utility::NormalizeTm(&end); do { if (IsInTimeRange(&begin, &end, stride, &iter)) { @@ -575,7 +564,7 @@ Dictionary::Ptr LegacyTimePeriod::FindNextSegment(const String& daydef, const St iter.tm_hour = 0; iter.tm_min = 0; iter.tm_sec = 0; - tsiter = mktime(&iter); + tsiter = Utility::NormalizeTm(&iter); } while (tsiter < tsend); } @@ -607,17 +596,17 @@ Array::Ptr LegacyTimePeriod::ScriptFunc(const TimePeriod::Ptr& tp, double begin, t->tm_isdst = -1; // Normalize fields using mktime. - mktime(t); + Utility::NormalizeTm(t); // Reset tm_isdst so that future calls figure out the correct time zone after setting tm_hour/tm_min/tm_sec. t->tm_isdst = -1; }; - for (tm reference = tm_begin; mktime_const(&reference) <= end; advance_to_next_day(&reference)) { + for (tm reference = tm_begin; Utility::TmToTimestamp(&reference) <= end; advance_to_next_day(&reference)) { #ifdef I2_DEBUG Log(LogDebug, "LegacyTimePeriod") - << "Checking reference time " << mktime_const(&reference); + << "Checking reference time " << Utility::TmToTimestamp(&reference); #endif /* I2_DEBUG */ ObjectLock olock(ranges); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 8ed300c1f..0e2a2976e 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -186,6 +186,7 @@ add_boost_test(base base_utility/EscapeCreateProcessArg base_utility/TruncateUsingHash base_utility/FormatDateTime + base_utility/NormalizeTm base_value/scalar base_value/convert base_value/format diff --git a/test/base-utility.cpp b/test/base-utility.cpp index 2273e1b8c..0e6e0c645 100644 --- a/test/base-utility.cpp +++ b/test/base-utility.cpp @@ -1,6 +1,7 @@ /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ #include "base/utility.hpp" +#include "test/utils.hpp" #include #include @@ -230,4 +231,86 @@ BOOST_AUTO_TEST_CASE(FormatDateTime) { BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", positive_out_of_range), positive_overflow); } +BOOST_AUTO_TEST_CASE(NormalizeTm) +{ + GlobalTimezoneFixture tz(GlobalTimezoneFixture::TestTimezoneWithDST); + + auto normalize = [](const std::string_view& input) { + tm t = make_tm(std::string(input)); + return Utility::NormalizeTm(&t); + }; + + auto is_dst = [](const std::string_view& input) { + tm t = make_tm(std::string(input)); + Utility::NormalizeTm(&t); + BOOST_CHECK_GE(t.tm_isdst, 0); + return t.tm_isdst > 0; + }; + + // The whole day 2021-01-01 uses PST (24h day) + BOOST_CHECK(!is_dst("2021-01-01 10:00:00")); + BOOST_CHECK_EQUAL(normalize("2021-01-01 10:00:00"), 1609524000); + BOOST_CHECK_EQUAL(normalize("2021-01-01 10:00:00 PST"), 1609524000); + BOOST_CHECK_EQUAL(normalize("2021-01-01 11:00:00 PDT"), 1609524000); // normalized to 10:00 PST + BOOST_CHECK_EQUAL(normalize("2021-01-02 00:00:00") - normalize("2021-01-01 00:00:00"), 24*60*60); + + // The whole day 2021-07-01 uses PDT (24h day) + BOOST_CHECK(is_dst("2021-07-01 10:00:00")); + BOOST_CHECK_EQUAL(normalize("2021-07-01 10:00:00"), 1625158800); + BOOST_CHECK_EQUAL(normalize("2021-07-01 10:00:00 PDT"), 1625158800); + BOOST_CHECK_EQUAL(normalize("2021-07-01 09:00:00 PST"), 1625158800); // normalized to 10:00 PDT + BOOST_CHECK_EQUAL(normalize("2021-07-02 00:00:00") - normalize("2021-07-01 00:00:00"), 24*60*60); + + // On 2021-03-14, PST changes to PDT (23h day) + BOOST_CHECK(!is_dst("2021-03-14 00:00:00")); + BOOST_CHECK(is_dst("2021-03-14 23:59:59")); + BOOST_CHECK_EQUAL(normalize("2021-03-15 00:00:00") - normalize("2021-03-14 00:00:00"), 23*60*60); + + BOOST_CHECK_EQUAL(normalize("2021-03-14 01:59:59 PST"), 1615715999); + // The following three times do not exist on that day in that timezone. + // They are interpreted as UTC-8, which is the offset of PST. + BOOST_CHECK_EQUAL(normalize("2021-03-14 02:00:00 PST"), 1615716000); + BOOST_CHECK_EQUAL(normalize("2021-03-14 02:30:00 PST"), 1615717800); + BOOST_CHECK_EQUAL(normalize("2021-03-14 03:00:00 PST"), 1615719600); + + BOOST_CHECK_EQUAL(normalize("2021-03-14 03:00:00 PDT"), 1615716000); + // The following three times do not exist on that day in that timezone. + // They are interpreted as UTC-7, which is the offset of PDT. + BOOST_CHECK_EQUAL(normalize("2021-03-14 01:59:59 PDT"), 1615712399); + BOOST_CHECK_EQUAL(normalize("2021-03-14 02:00:00 PDT"), 1615712400); + BOOST_CHECK_EQUAL(normalize("2021-03-14 02:30:00 PDT"), 1615714200); + + BOOST_CHECK_EQUAL(normalize("2021-03-14 01:59:59"), 1615715999); + BOOST_CHECK_EQUAL(normalize("2021-03-14 03:00:00"), 1615716000); + // The following two times don't exist on that day, they are within the hour that is skipped. + // They are interpreted as UTC-8 (offset of PST) and then normalized to PDT. + BOOST_CHECK_EQUAL(normalize("2021-03-14 02:00:00"), 1615716000); + BOOST_CHECK_EQUAL(normalize("2021-03-14 02:30:00"), 1615717800); + + // On 2021-11-07, PDT changes to PST (25h day) + BOOST_CHECK(is_dst("2021-11-07 00:00:00")); + BOOST_CHECK(!is_dst("2021-11-07 23:59:59")); + BOOST_CHECK_EQUAL(normalize("2021-11-08 00:00:00") - normalize("2021-11-07 00:00:00"), 25*60*60); + + BOOST_CHECK_EQUAL(normalize("2021-11-07 00:59:59 PDT"), 1636271999); + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:00:00 PDT"), 1636272000); + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:30:00 PDT"), 1636273800); + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:59:59 PDT"), 1636275599); + // The following time does not exist on that day in that timezone, it's interpreted as 01:00:00 PST. + BOOST_CHECK_EQUAL(normalize("2021-11-07 02:00:00 PDT"), 1636275600); + + // The following time does not exist on that day in that timezone, it's interpreted as 01:59:59 PDT. + BOOST_CHECK_EQUAL(normalize("2021-11-07 00:59:59 PST"), 1636275599); + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:00:00 PST"), 1636275600); + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:30:00 PST"), 1636277400); + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:59:59 PST"), 1636279199); + BOOST_CHECK_EQUAL(normalize("2021-11-07 02:00:00 PST"), 1636279200); + + BOOST_CHECK_EQUAL(normalize("2021-11-07 00:59:59"), 1636271999); // unambiguous: PDT + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:00:00"), 1636272000); // exists twice, interpreted as PDT + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:30:00"), 1636273800); // exists twice, interpreted as PDT + BOOST_CHECK_EQUAL(normalize("2021-11-07 01:59:59"), 1636275599); // exists twice, interpreted as PDT + BOOST_CHECK_EQUAL(normalize("2021-11-07 02:00:00"), 1636279200); // unambiguous: PST +} + BOOST_AUTO_TEST_SUITE_END()