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

@@ -35,7 +35,7 @@ DateTime::DateTime(const std::vector<Value>& args)
 
 		tms.tm_isdst = -1;
 
-		m_Value = mktime(&tms);
+		m_Value = Utility::TmToTimestamp(&tms);
 	} else if (args.size() == 1)
 		m_Value = args[0];
 	else

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(&copy);
+}

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();
 

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(&copy);
-}
-
 bool LegacyTimePeriod::IsInTimeRange(const tm *begin, const tm *end, int stride, const tm *reference)
 {
 	time_t tsbegin, tsend, tsref;
-	tsbegin = mktime_const(begin);
+	tsbegin = Utility::TmToTimestamp(begin);
-	tsend = mktime_const(end);
+	tsend = Utility::TmToTimestamp(end);
-	tsref = mktime_const(reference);
+	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)
+		<< "ParseTimeRange: '" << daydef << "' => " << Utility::TmToTimestamp(&begin)
-		<< " -> " << mktime(&end) << ", stride: " << stride;
+		<< " -> " << 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) },
+		{ "begin", (long)Utility::TmToTimestamp(&begin) },
-		{ "end", (long)mktime(&end) }
+		{ "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);

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

test/base-utility.cpp

@@ -1,6 +1,7 @@
 /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */
 
 #include "base/utility.hpp"
+#include "test/utils.hpp"
 #include <chrono>
 #include <BoostTestTargetConfig.h>
 
@@ -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()