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 <time.h> 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.

diff --git a/src/Makefile.am b/src/Makefile.am
index 25ee95af6cea9129796a6aa43a319cf51ff1ba1e..f9b7d451306b37b020e0b696707404472fe418ab 100644 (file)
--- 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 4550ecb6a965a926b19481ef85c64dcbe32e6189..6670c3d94d545bf2456d2d347e9e8a86f38c846e 100644 (file)
--- 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 870ecadd5b4354a33cad2f30f4be9ca418bb42ea..be91240118d602b29103b5cf371bdb816010a5de 100644 (file)
--- 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 5c6dc959968d846998491d4b87958d124a7a8491..bb2df3fff3f948306885501ffa3b0d0e3f6bd65e 100644 (file)
--- 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 6b528020f3d29d7141a146c523d54d07d57e6491..f1d7af946a625d2385ef985c9919a6f6a4cd3991 100644 (file)
--- 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 (file)
index 0000000..fbdcd8b
--- /dev/null
+++ b/test/json_util_test.c
@@ -0,0 +1,48 @@
+#include "json_util.c"
+
+#include <check.h>
+
+#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;
+}