From 169615c9a8cdc54d748d4dfc8279be9b3c2bec44 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Sun, 21 Mar 2021 20:59:32 +0100
Subject: [PATCH 1/5] shared/calendarspec: abort calculation after 1000
iterations
We have a bug where we seem to enter an infinite loop when running in the
Europe/Dublin timezone. The timezone is "special" because it has negative SAVE
values. The handling of this should obviously be fixed, but let's use a
belt-and-suspenders approach, and gracefully fail if we fail to find an answer
within a specific number of attempts. The code in this function is rather
complex, and it's hard to rule out another bug in the future.
---
src/shared/calendarspec.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/shared/calendarspec.c b/src/shared/calendarspec.c
index 4f68a570b52..feb43efdcda 100644
--- a/src/shared/calendarspec.c
+++ b/src/shared/calendarspec.c
@@ -1210,6 +1210,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 +1227,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 +1324,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) {
From 462f15d92d35f812d7d77edd486ca63236cffe83 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Mon, 22 Mar 2021 09:20:47 +0100
Subject: [PATCH 2/5] shared/calendarspec: constify parameter and simplify
assignments to variable
The scope of start & stop is narrowed down, and they are assigned only once.
No functional change, but I think the code is easier to read this way.
Also add a comment to make the code easier to read.
---
src/shared/calendarspec.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/shared/calendarspec.c b/src/shared/calendarspec.c
index feb43efdcda..5c666412946 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) {
From f035bb1b7a5900439640f267db881c60d042e450 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Mon, 22 Mar 2021 11:10:22 +0100
Subject: [PATCH 3/5] test-calendarspec: print offending line in output
The output is rather long at this makes it easier to jump to the right place.
Also use normal output routines and set_unset_env() to make things more
compact.
---
src/test/test-calendarspec.c | 48 +++++++++++++++++-------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/src/test/test-calendarspec.c b/src/test/test-calendarspec.c
index 01ec7f87704..152ce879f8a 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 (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,13 +205,13 @@ 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);
From 47b0b65766229a18921a3ce831ef708ef408a34c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Mon, 22 Mar 2021 11:29:35 +0100
Subject: [PATCH 4/5] test-calendarspec: do not convert timezone "" to ":"
I *think* it doesn't actually make any difference, because ":" will be ignored.
437f48a471f51ac9dd2697ee3b848a71b4f101df added prefixing with ":", but didn't
take into account the fact that we also use "" with a different meaning than
NULL here. But let's restore the original behaviour of specifying the empty
string.
---
src/test/test-calendarspec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/test-calendarspec.c b/src/test/test-calendarspec.c
index 152ce879f8a..c62e6860cf9 100644
--- a/src/test/test-calendarspec.c
+++ b/src/test/test-calendarspec.c
@@ -44,7 +44,7 @@ static void _test_next(int line, const char *input, const char *new_tz, usec_t a
if (old_tz)
old_tz = strdupa(old_tz);
- if (new_tz)
+ if (!isempty(new_tz))
new_tz = strjoina(":", new_tz);
assert_se(set_unset_env("TZ", new_tz, true) == 0);
From 129cb6e249bef30dc33e08f98f0b27a6de976f6f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Mon, 22 Mar 2021 12:51:47 +0100
Subject: [PATCH 5/5] shared/calendarspec: when mktime() moves us backwards,
jump forward
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When trying to calculate the next firing of 'Sun *-*-* 01:00:00', we'd fall
into an infinite loop, because mktime() moves us "backwards":
Before this patch:
tm_within_bounds: good=0 2021-03-29 01:00:00 → 2021-03-29 00:00:00
tm_within_bounds: good=0 2021-03-29 01:00:00 → 2021-03-29 00:00:00
tm_within_bounds: good=0 2021-03-29 01:00:00 → 2021-03-29 00:00:00
...
We rely on mktime() normalizing the time. The man page does not say that it'll
move the time forward, but our algorithm relies on this. So let's catch this
case explicitly.
With this patch:
$ TZ=Europe/Dublin faketime 2021-03-21 build/systemd-analyze calendar --iterations=5 'Sun *-*-* 01:00:00'
Normalized form: Sun *-*-* 01:00:00
Next elapse: Sun 2021-03-21 01:00:00 GMT
(in UTC): Sun 2021-03-21 01:00:00 UTC
From now: 59min left
Iter. #2: Sun 2021-04-04 01:00:00 IST
(in UTC): Sun 2021-04-04 00:00:00 UTC
From now: 1 weeks 6 days left <---- note the 2 week jump here
Iter. #3: Sun 2021-04-11 01:00:00 IST
(in UTC): Sun 2021-04-11 00:00:00 UTC
From now: 2 weeks 6 days left
Iter. #4: Sun 2021-04-18 01:00:00 IST
(in UTC): Sun 2021-04-18 00:00:00 UTC
From now: 3 weeks 6 days left
Iter. #5: Sun 2021-04-25 01:00:00 IST
(in UTC): Sun 2021-04-25 00:00:00 UTC
From now: 1 months 4 days left
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1941335.
---
src/shared/calendarspec.c | 19 +++++++++++--------
src/test/test-calendarspec.c | 3 +++
test/test-functions | 1 +
3 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/shared/calendarspec.c b/src/shared/calendarspec.c
index 5c666412946..bf24d8d5bbb 100644
--- a/src/shared/calendarspec.c
+++ b/src/shared/calendarspec.c
@@ -1195,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) {
diff --git a/src/test/test-calendarspec.c b/src/test/test-calendarspec.c
index c62e6860cf9..4f1d0f64d57 100644
--- a/src/test/test-calendarspec.c
+++ b/src/test/test-calendarspec.c
@@ -214,6 +214,9 @@ int main(int argc, char* argv[]) {
/* 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 d7f7967e2ff..6b94058fd36 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