Merge pull request #19075 from keszybz/calendarspec-loop

Fix infinite loop in calendarspec calculation when timezone has negative DST save value

3 files changed, 72 insertions, 46 deletions

diff --git a/src/shared/calendarspec.c b/src/shared/calendarspec.c
index 4f68a570b5..bf24d8d5bb 100644
--- a/src/shared/calendarspec.c
+++ b/src/shared/calendarspec.c
@@ -1101,7 +1101,7 @@ int calendar_spec_from_string(const char *p, CalendarSpec **spec) {
         return 0;
 }
 
-static int find_end_of_month(struct tm *tm, bool utc, int day) {
+static int find_end_of_month(const struct tm *tm, bool utc, int day) {
         struct tm t = *tm;
 
         t.tm_mon++;
@@ -1114,28 +1114,39 @@ static int find_end_of_month(struct tm *tm, bool utc, int day) {
         return t.tm_mday;
 }
 
-static int find_matching_component(const CalendarSpec *spec, const CalendarComponent *c,
-                                   struct tm *tm, int *val) {
-        const CalendarComponent *p = c;
-        int start, stop, d = -1;
+static int find_matching_component(
+                const CalendarSpec *spec,
+                const CalendarComponent *c,
+                const struct tm *tm,           /* tm is only used for end-of-month calculations */
+                int *val) {
+
+        int d = -1, r;
         bool d_set = false;
-        int r;
 
         assert(val);
 
+        /* Finds the *earliest* matching time specified by one of the CalendarCompoment items in chain c.
+         * If no matches can be found, returns -ENOENT.
+         * Otherwise, updates *val to the matching time. 1 is returned if *val was changed, 0 otherwise.
+         */
+
         if (!c)
                 return 0;
 
+        bool end_of_month = spec->end_of_month && c == spec->day;
+
         while (c) {
-                start = c->start;
-                stop = c->stop;
+                int start, stop;
 
-                if (spec->end_of_month && p == spec->day) {
-                        start = find_end_of_month(tm, spec->utc, start);
-                        stop = find_end_of_month(tm, spec->utc, stop);
+                if (end_of_month) {
+                        start = find_end_of_month(tm, spec->utc, c->start);
+                        stop = find_end_of_month(tm, spec->utc, c->stop);
 
                         if (stop > 0)
                                 SWAP_TWO(start, stop);
+                } else {
+                        start = c->start;
+                        stop = c->stop;
                 }
 
                 if (start >= *val) {
@@ -1184,15 +1195,18 @@ static int tm_within_bounds(struct tm *tm, bool utc) {
                 return negative_errno();
 
         /* Did any normalization take place? If so, it was out of bounds before */
-        bool good = t.tm_year == tm->tm_year &&
-                    t.tm_mon  == tm->tm_mon  &&
-                    t.tm_mday == tm->tm_mday &&
-                    t.tm_hour == tm->tm_hour &&
-                    t.tm_min  == tm->tm_min  &&
-                    t.tm_sec  == tm->tm_sec;
-        if (!good)
+        int cmp = CMP(t.tm_year, tm->tm_year) ?:
+                  CMP(t.tm_mon, tm->tm_mon) ?:
+                  CMP(t.tm_mday, tm->tm_mday) ?:
+                  CMP(t.tm_hour, tm->tm_hour) ?:
+                  CMP(t.tm_min, tm->tm_min) ?:
+                  CMP(t.tm_sec, tm->tm_sec);
+
+        if (cmp < 0)
+                return -EDEADLK; /* Refuse to go backward */
+        if (cmp > 0)
                 *tm = t;
-        return good;
+        return cmp == 0;
 }
 
 static bool matches_weekday(int weekdays_bits, const struct tm *tm, bool utc) {
@@ -1210,6 +1224,10 @@ static bool matches_weekday(int weekdays_bits, const struct tm *tm, bool utc) {
         return (weekdays_bits & (1 << k));
 }
 
+/* A safety valve: if we get stuck in the calculation, return an error.
+ * C.f. https://bugzilla.redhat.com/show_bug.cgi?id=1941335. */
+#define MAX_CALENDAR_ITERATIONS 1000
+
 static int find_next(const CalendarSpec *spec, struct tm *tm, usec_t *usec) {
         struct tm c;
         int tm_usec;
@@ -1223,7 +1241,7 @@ static int find_next(const CalendarSpec *spec, struct tm *tm, usec_t *usec) {
         c = *tm;
         tm_usec = *usec;
 
-        for (;;) {
+        for (unsigned iteration = 0; iteration < MAX_CALENDAR_ITERATIONS; iteration++) {
                 /* Normalize the current date */
                 (void) mktime_or_timegm(&c, spec->utc);
                 c.tm_isdst = spec->dst;
@@ -1320,6 +1338,14 @@ static int find_next(const CalendarSpec *spec, struct tm *tm, usec_t *usec) {
                 *usec = tm_usec;
                 return 0;
         }
+
+        /* It seems we entered an infinite loop. Let's gracefully return an error instead of hanging or
+         * aborting. This code is also exercised when timers.target is brought up during early boot, so
+         * aborting here is problematic and hard to diagnose for users. */
+        _cleanup_free_ char *s = NULL;
+        (void) calendar_spec_to_string(spec, &s);
+        return log_warning_errno(SYNTHETIC_ERRNO(EDEADLK),
+                                 "Infinite loop in calendar calculation: %s", strna(s));
 }
 
 static int calendar_spec_next_usec_impl(const CalendarSpec *spec, usec_t usec, usec_t *ret_next) {
diff --git a/src/test/test-calendarspec.c b/src/test/test-calendarspec.c
index 01ec7f8770..4f1d0f64d5 100644
--- a/src/test/test-calendarspec.c
+++ b/src/test/test-calendarspec.c
@@ -2,11 +2,11 @@
 
 #include "alloc-util.h"
 #include "calendarspec.h"
+#include "env-util.h"
 #include "errno-util.h"
 #include "string-util.h"
-#include "util.h"
 
-static void test_one(const char *input, const char *output) {
+static void _test_one(int line, const char *input, const char *output) {
         CalendarSpec *c;
         _cleanup_free_ char *p = NULL, *q = NULL;
         usec_t u;
@@ -16,13 +16,13 @@ static void test_one(const char *input, const char *output) {
         assert_se(calendar_spec_from_string(input, &c) >= 0);
 
         assert_se(calendar_spec_to_string(c, &p) >= 0);
-        printf("\"%s\" → \"%s\"\n", input, p);
+        log_info("line %d: \"%s\" → \"%s\"", line, input, p);
 
         assert_se(streq(p, output));
 
         u = now(CLOCK_REALTIME);
         r = calendar_spec_next_usec(c, u, &u);
-        printf("Next: %s\n", r < 0 ? strerror_safe(r) : format_timestamp(buf, sizeof(buf), u));
+        log_info("Next: %s", r < 0 ? strerror_safe(r) : format_timestamp(buf, sizeof buf, u));
         calendar_spec_free(c);
 
         assert_se(calendar_spec_from_string(p, &c) >= 0);
@@ -31,8 +31,9 @@ static void test_one(const char *input, const char *output) {
 
         assert_se(streq(q, p));
 }
+#define test_one(input, output) _test_one(__LINE__, input, output)
 
-static void test_next(const char *input, const char *new_tz, usec_t after, usec_t expect) {
+static void _test_next(int line, const char *input, const char *new_tz, usec_t after, usec_t expect) {
         CalendarSpec *c;
         usec_t u;
         char *old_tz;
@@ -43,22 +44,19 @@ static void test_next(const char *input, const char *new_tz, usec_t after, usec_
         if (old_tz)
                 old_tz = strdupa(old_tz);
 
-        if (new_tz) {
-                char *colon_tz;
+        if (!isempty(new_tz))
+                new_tz = strjoina(":", new_tz);
 
-                colon_tz = strjoina(":", new_tz);
-                assert_se(setenv("TZ", colon_tz, 1) >= 0);
-        } else
-                assert_se(unsetenv("TZ") >= 0);
+        assert_se(set_unset_env("TZ", new_tz, true) == 0);
         tzset();
 
         assert_se(calendar_spec_from_string(input, &c) >= 0);
 
-        printf("\"%s\"\n", input);
+        log_info("line %d: \"%s\" new_tz=%s", line, input, strnull(new_tz));
 
         u = after;
         r = calendar_spec_next_usec(c, after, &u);
-        printf("At: %s\n", r < 0 ? strerror_safe(r) : format_timestamp_style(buf, sizeof buf, u, TIMESTAMP_US));
+        log_info("At: %s", r < 0 ? strerror_safe(r) : format_timestamp_style(buf, sizeof buf, u, TIMESTAMP_US));
         if (expect != USEC_INFINITY)
                 assert_se(r >= 0 && u == expect);
         else
@@ -66,12 +64,10 @@ static void test_next(const char *input, const char *new_tz, usec_t after, usec_
 
         calendar_spec_free(c);
 
-        if (old_tz)
-                assert_se(setenv("TZ", old_tz, 1) >= 0);
-        else
-                assert_se(unsetenv("TZ") >= 0);
+        assert_se(set_unset_env("TZ", old_tz, true) == 0);
         tzset();
 }
+#define test_next(input, new_tz, after, expect) _test_next(__LINE__, input,new_tz,after,expect)
 
 static void test_timestamp(void) {
         char buf[FORMAT_TIMESTAMP_MAX];
@@ -83,12 +79,12 @@ static void test_timestamp(void) {
 
         x = now(CLOCK_REALTIME);
 
-        assert_se(format_timestamp_style(buf, sizeof(buf), x, TIMESTAMP_US));
-        printf("%s\n", buf);
+        assert_se(format_timestamp_style(buf, sizeof buf, x, TIMESTAMP_US));
+        log_info("%s", buf);
         assert_se(calendar_spec_from_string(buf, &c) >= 0);
         assert_se(calendar_spec_to_string(c, &t) >= 0);
         calendar_spec_free(c);
-        printf("%s\n", t);
+        log_info("%s", t);
 
         assert_se(parse_timestamp(t, &y) >= 0);
         assert_se(y == x);
@@ -104,11 +100,11 @@ static void test_hourly_bug_4031(void) {
         n = now(CLOCK_REALTIME);
         assert_se((r = calendar_spec_next_usec(c, n, &u)) >= 0);
 
-        printf("Now: %s (%"PRIu64")\n", format_timestamp_style(buf, sizeof buf, n, TIMESTAMP_US), n);
-        printf("Next hourly: %s (%"PRIu64")\n", r < 0 ? strerror_safe(r) : format_timestamp_style(buf, sizeof buf, u, TIMESTAMP_US), u);
+        log_info("Now: %s (%"PRIu64")", format_timestamp_style(buf, sizeof buf, n, TIMESTAMP_US), n);
+        log_info("Next hourly: %s (%"PRIu64")", r < 0 ? strerror_safe(r) : format_timestamp_style(buf, sizeof buf, u, TIMESTAMP_US), u);
 
         assert_se((r = calendar_spec_next_usec(c, u, &w)) >= 0);
-        printf("Next hourly: %s (%"PRIu64")\n", r < 0 ? strerror_safe(r) : format_timestamp_style(zaf, sizeof zaf, w, TIMESTAMP_US), w);
+        log_info("Next hourly: %s (%"PRIu64")", r < 0 ? strerror_safe(r) : format_timestamp_style(zaf, sizeof zaf, w, TIMESTAMP_US), w);
 
         assert_se(n < u);
         assert_se(u <= n + USEC_PER_HOUR);
@@ -209,15 +205,18 @@ int main(int argc, char* argv[]) {
         test_next("2017-08-06 9..17/2:00 UTC", "", 1502029800000000, 1502031600000000);
         test_next("2016-12-* 3..21/6:00 UTC", "", 1482613200000001, 1482634800000000);
         test_next("2017-09-24 03:30:00 Pacific/Auckland", "", 12345, 1506177000000000);
-        // Due to daylight saving time - 2017-09-24 02:30:00 does not exist
+        /* Due to daylight saving time - 2017-09-24 02:30:00 does not exist */
         test_next("2017-09-24 02:30:00 Pacific/Auckland", "", 12345, -1);
         test_next("2017-04-02 02:30:00 Pacific/Auckland", "", 12345, 1491053400000000);
-        // Confirm that even though it's a time change here (backward) 02:30 happens only once
+        /* Confirm that even though it's a time change here (backward) 02:30 happens only once */
         test_next("2017-04-02 02:30:00 Pacific/Auckland", "", 1491053400000000, -1);
         test_next("2017-04-02 03:30:00 Pacific/Auckland", "", 12345, 1491060600000000);
-        // Confirm that timezones in the Spec work regardless of current timezone
+        /* Confirm that timezones in the Spec work regardless of current timezone */
         test_next("2017-09-09 20:42:00 Pacific/Auckland", "", 12345, 1504946520000000);
         test_next("2017-09-09 20:42:00 Pacific/Auckland", "EET", 12345, 1504946520000000);
+        /* Check that we don't start looping if mktime() moves us backwards */
+        test_next("Sun *-*-* 01:00:00 Europe/Dublin", "", 1616412478000000, 1617494400000000);
+        test_next("Sun *-*-* 01:00:00 Europe/Dublin", "IST", 1616412478000000, 1617494400000000);
 
         assert_se(calendar_spec_from_string("test", &c) < 0);
         assert_se(calendar_spec_from_string(" utc", &c) < 0);
diff --git a/test/test-functions b/test/test-functions
index d7f7967e2f..6b94058fd3 100644
--- a/test/test-functions
+++ b/test/test-functions
@@ -1340,6 +1340,7 @@ install_zoneinfo() {
     inst_any /usr/share/zoneinfo/Asia/Vladivostok
     inst_any /usr/share/zoneinfo/Australia/Sydney
     inst_any /usr/share/zoneinfo/Europe/Berlin
+    inst_any /usr/share/zoneinfo/Europe/Dublin
     inst_any /usr/share/zoneinfo/Europe/Kiev
     inst_any /usr/share/zoneinfo/Pacific/Auckland
     inst_any /usr/share/zoneinfo/Pacific/Honolulu