From 7846f008a288c3913d7c30f22781544cde84b0b3 Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Fri, 15 Mar 2024 16:44:07 -0600 Subject: [PATCH] Fix date parsing from TAL cachefiles The code was writing dates in Zulu format, which was fine. But then, to read them back, it was loading them with mktime(), which is a local timezone function. The effects of this bug depend on the time zone. Files would expire from the cache up to 12 hours too early (UTC+) or late (UTC-), or be updated up to 12 hours late (UTC-). (There's no such thing as "updating too early" in this context, since Fort cannot refresh before the present.) I fixed this by swapping mktime() for timegm(), which is not great, because the latter is still a nonstandard function. But it'll have to do, because the other options are worse. Below is my thought process on timegm() and the other options: ================================================== Problem: 1. I want to store timestamps as human-readable strings. 2. Converting a timestamp to Zulu string is trivial: time_t -> gmtime_r() -> struct tm (GMT) -> strftime() -> char*. 3. But converting a string to timestamp relies on timegm(), which is nonstandard: char* -> strptime() -> struct tm (GMT) -> timegm() -> time_t. Brainstorm: 1. Store the dates in local time. Hideous option, but not ENTIRELY insane. Storing in local time will render the dates up to 24 hours off, I think. But this only happens when the cache changes timezone, which isn't really often. But it's still pretty clumsy, and also not future-proof, as new date fields would have to be constrained by the same limitation. 2/10 at best. 2. Ditch time_t, use struct tm in UTC for everything. So I would rely on gmtime_r() only to get out of time_t, and never need timegm() for anything. Comparing field by field would be a pain, but it's interesting to note that Fort is actually already doing it somewhere: tm_cmp(). I guess I have to admire the audaciousness of past me. What mainly scares me is that mktime() seems to be the only standard function that is contractually obligated to normalize the tm, which means I would have to keep mktime()ing every time I need to compare them. And mktime() is... a local time function. Probably wouldn't actually work. 4/10. I hate this API. 3. Bite the bullet: Go modern POSIX; assume time_t is integer seconds since the 1970 epoch. I'm fantasizing about an early environment assertion that checks the gmtime_r() for a known time_t, that shows people the door if the implementation isn't sane. Would work even in POSIX-adjacent systems like most Linuxes. This would have other benefits, like the ability to ditch difftime(), and perform arithmetic operations on time_t's. All the officially supported platforms get this aspect of POSIX right, so what's stopping me? Well, it would mean I would have to store the date as a seconds-since- epoch integer, which is not as human-friendly and defeats the whole point of the JSON format. And yet... this feels so right, I might end up doing it even regardless of the JSON data type and timegm(). But not today. 7/10. 4. Bite the bullet: Use timegm(). Interesting. timegm() might be added to C23: > Changes integrated into the latest working draft of C23 are listed > below. > (...) > - Add timegm() function in to convert time structure into > calendar time value - similar to function in glibc and musl libraries. https://en.wikipedia.org/wiki/C23_(C_standard_revision) So this has some probability of not needing future tweaking. Also, it's very clean. (Except for the feature test macros.) 8/10. --- src/Makefile.am | 6 +----- src/cache/local_cache.c | 4 ++-- src/json_util.c | 44 +++++++++++++++++++++---------------- src/json_util.h | 2 +- test/Makefile.am | 10 ++++++--- test/json_util_test.c | 48 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 84 insertions(+), 30 deletions(-) create mode 100644 test/json_util_test.c diff --git a/src/Makefile.am b/src/Makefile.am index 25ee95af..f9b7d451 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -114,9 +114,8 @@ include asn1/asn1c/Makefile.include fort_SOURCES += $(ASN_MODULE_SRCS) $(ASN_MODULE_HDRS) fort_CFLAGS = -Wall -Wpedantic -# Feel free to temporarily remove this one if you're not using gcc 7.3.0. #fort_CFLAGS += $(GCC_WARNS) -fort_CFLAGS += -std=c99 -D_POSIX_C_SOURCE=200809 -D_XOPEN_SOURCE=700 +fort_CFLAGS += -std=c99 -D_DEFAULT_SOURCE=1 -D_XOPEN_SOURCE=700 -D_BSD_SOURCE=1 fort_CFLAGS += -O2 -g $(FORT_FLAGS) ${XML2_CFLAGS} if BACKTRACE_ENABLED fort_CFLAGS += -DBACKTRACE_ENABLED @@ -127,9 +126,6 @@ fort_LDADD = ${JANSSON_LIBS} ${CURL_LIBS} ${XML2_LIBS} # I'm tired of scrolling up, but feel free to comment this out. GCC_WARNS = -fmax-errors=1 -# Please ready some arguments if you want to permanently remove some of these -# flags. I gave a quick read to the documentation of all of these warnings, and -# generally liked each of them. GCC_WARNS += -pedantic-errors -Waddress -Walloc-zero -Walloca GCC_WARNS += -Wno-aggressive-loop-optimizations -Warray-bounds=2 -Wbool-compare GCC_WARNS += -Wbool-operation -Wno-builtin-declaration-mismatch -Wcast-align diff --git a/src/cache/local_cache.c b/src/cache/local_cache.c index 4550ecb6..6670c3d9 100644 --- a/src/cache/local_cache.c +++ b/src/cache/local_cache.c @@ -392,12 +392,12 @@ node2json(struct cache_node *node) if (uri_is_notif(node->url)) if (json_add_bool(json, TAGNAME_IS_NOTIF, true)) goto cancel; - if (json_add_date(json, TAGNAME_ATTEMPT_TS, node->attempt.ts)) + if (json_add_ts(json, TAGNAME_ATTEMPT_TS, node->attempt.ts)) goto cancel; if (json_add_int(json, TAGNAME_ATTEMPT_ERR, node->attempt.result)) goto cancel; if (node->success.happened) - if (json_add_date(json, TAGNAME_SUCCESS_TS, node->success.ts)) + if (json_add_ts(json, TAGNAME_SUCCESS_TS, node->success.ts)) goto cancel; return json; diff --git a/src/json_util.c b/src/json_util.c index 870ecadd..be912401 100644 --- a/src/json_util.c +++ b/src/json_util.c @@ -10,6 +10,7 @@ * documented in the Linux man page are not actually portable. */ #define JSON_TS_FORMAT "%Y-%m-%dT%H:%M:%SZ" +#define JSON_TS_LEN 21 /* strlen("YYYY-mm-ddTHH:MM:SSZ") + 1 */ int json_get_str(json_t *parent, char const *name, char const **result) @@ -105,36 +106,43 @@ json_get_u32(json_t *parent, char const *name, uint32_t *result) return 0; } -int -json_get_ts(json_t *parent, char const *name, time_t *result) +static int +str2tt(char const *str, time_t *tt) { - char const *str, *consumed; + char const *consumed; struct tm tm; time_t time; int error; - *result = 0; - - error = json_get_str(parent, name, &str); - if (error) - return error; - memset(&tm, 0, sizeof(tm)); consumed = strptime(str, JSON_TS_FORMAT, &tm); if (consumed == NULL || (*consumed) != 0) return pr_op_err("String '%s' does not appear to be a timestamp.", str); - time = mktime(&tm); + time = timegm(&tm); if (time == ((time_t) -1)) { error = errno; return pr_op_err("String '%s' does not appear to be a timestamp: %s", str, strerror(error)); } - *result = time; + *tt = time; return 0; } +int +json_get_ts(json_t *parent, char const *name, time_t *result) +{ + char const *str; + int error; + + error = json_get_str(parent, name, &str); + if (error) + return error; + + return str2tt(str, result); +} + int json_get_array(json_t *parent, char const *name, json_t **array) { @@ -220,36 +228,34 @@ json_add_str(json_t *parent, char const *name, char const *value) } static int -tt2json(time_t tt, json_t **result) +tt2str(time_t tt, char *str) { - char str[32]; struct tm tmbuffer, *tm; memset(&tmbuffer, 0, sizeof(tmbuffer)); tm = gmtime_r(&tt, &tmbuffer); if (tm == NULL) return errno; - if (strftime(str, sizeof(str) - 1, JSON_TS_FORMAT, tm) == 0) + if (strftime(str, JSON_TS_LEN, JSON_TS_FORMAT, tm) == 0) return ENOSPC; - *result = json_string(str); return 0; } int -json_add_date(json_t *parent, char const *name, time_t value) +json_add_ts(json_t *parent, char const *name, time_t value) { - json_t *date = NULL; + char str[JSON_TS_LEN]; int error; - error = tt2json(value, &date); + error = tt2str(value, str); if (error) { pr_op_err("Cannot convert timestamp '%s' to json: %s", name, strerror(error)); return error; } - if (json_object_set_new(parent, name, date)) + if (json_object_set_new(parent, name, json_string(str))) return pr_op_err( "Cannot convert timestamp '%s' to json; unknown cause.", name diff --git a/src/json_util.h b/src/json_util.h index 5c6dc959..bb2df3ff 100644 --- a/src/json_util.h +++ b/src/json_util.h @@ -37,6 +37,6 @@ bool json_valid_members_count(json_t *, size_t); int json_add_bool(json_t *, char const *, bool); int json_add_int(json_t *, char const *, int); int json_add_str(json_t *, char const *, char const *); -int json_add_date(json_t *, char const *, time_t); +int json_add_ts(json_t *, char const *, time_t); #endif /* SRC_JSON_UTIL_H_ */ diff --git a/test/Makefile.am b/test/Makefile.am index 6b528020..f1d7af94 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -14,18 +14,19 @@ if USE_TESTS # mumble_mumble_CFLAGS = ${AM_CFLAGS} flag1 flag2 flag3 ... AM_CFLAGS = -pedantic -Wall #AM_CFLAGS += -Wno-unused -AM_CFLAGS += -std=c99 -D_POSIX_C_SOURCE=200809 -D_XOPEN_SOURCE=700 -AM_CFLAGS += -I../src -DUNIT_TESTING ${CHECK_CFLAGS} ${XML2_CFLAGS} +AM_CFLAGS += -std=c99 -D_DEFAULT_SOURCE=1 -D_XOPEN_SOURCE=700 -D_BSD_SOURCE=1 +AM_CFLAGS += -I../src -DUNIT_TESTING ${CHECK_CFLAGS} ${XML2_CFLAGS} ${JANSSON_CFLAGS} # Reminder: As opposed to AM_CFLAGS, "AM_LDADD" is not idiomatic automake, and # autotools will even reprehend us if we declare it. Therefore, I came up with # "my" own "ldadd". Unlike AM_CFLAGS, it needs to be manually added to every # target. -MY_LDADD = ${CHECK_LIBS} +MY_LDADD = ${CHECK_LIBS} ${JANSSON_LIBS} check_PROGRAMS = address.test check_PROGRAMS += cache.test check_PROGRAMS += db_table.test check_PROGRAMS += deltas_array.test +check_PROGRAMS += json.test check_PROGRAMS += line_file.test check_PROGRAMS += pb.test check_PROGRAMS += pdu_handler.test @@ -53,6 +54,9 @@ db_table_test_LDADD = ${MY_LDADD} deltas_array_test_SOURCES = rtr/db/deltas_array_test.c deltas_array_test_LDADD = ${MY_LDADD} +json_test_SOURCES = json_util_test.c +json_test_LDADD = ${MY_LDADD} + line_file_test_SOURCES = line_file_test.c line_file_test_LDADD = ${MY_LDADD} diff --git a/test/json_util_test.c b/test/json_util_test.c new file mode 100644 index 00000000..fbdcd8ba --- /dev/null +++ b/test/json_util_test.c @@ -0,0 +1,48 @@ +#include "json_util.c" + +#include + +#include "mock.c" + +START_TEST(test_tt) +{ + char str[JSON_TS_LEN + 1]; + time_t tt; + + ck_assert_int_eq(0, str2tt("2024-03-14T17:51:16Z", &tt)); + + memset(str, 'f', sizeof(str)); + ck_assert_int_eq(0, tt2str(tt, str)); + ck_assert_str_eq("2024-03-14T17:51:16Z", str); + ck_assert_int_eq('f', str[JSON_TS_LEN]); /* Tests JSON_TS_LEN. */ +} +END_TEST + +static Suite *json_load_suite(void) +{ + Suite *suite; + TCase *core; + + core = tcase_create("utils"); + tcase_add_test(core, test_tt); + + suite = suite_create("JSON util"); + suite_add_tcase(suite, core); + return suite; +} + +int main(void) +{ + Suite *suite; + SRunner *runner; + int tests_failed; + + suite = json_load_suite(); + + runner = srunner_create(suite); + srunner_run_all(runner, CK_NORMAL); + tests_failed = srunner_ntests_failed(runner); + srunner_free(runner); + + return (tests_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE; +}