chiark / gitweb /
Make the processing of SOURCE_DATE_EPOCH safe
authorBen Harris <bjh21@bjh21.me.uk>
Mon, 11 Nov 2024 22:19:54 +0000 (22:19 +0000)
committerBen Harris <bjh21@bjh21.me.uk>
Thu, 14 Nov 2024 22:27:18 +0000 (22:27 +0000)
And explain in excruciating detail why it's safe.  It's only actually
safe on systems (like POSIX ones) where time_t is an integer type, but
I think that's good enough for me.

bedstead.c

index 104983b5e950f19ee2c49840ea948c5618ca2ddb..71f77bff037b77869a9b23b79d94670ed99def00 100644 (file)
@@ -99,6 +99,7 @@
 #include <assert.h>
 #include <ctype.h>
 #include <errno.h>
+#include <inttypes.h>
 #include <limits.h>
 #include <stdbool.h>
 #include <stdint.h>
@@ -2730,29 +2731,68 @@ fullname_to_fontname(char const *fullname)
 static char *
 time_for_ttx(void)
 {
-       time_t now;
-       long long epochll;
+       time_t now = 0;
+       uintmax_t epochumax;
        struct tm *timeptr;
        char *epochstr, *endptr, *timestr;
 
        /* Work out what timestamp to use. */
        if ((epochstr = getenv("SOURCE_DATE_EPOCH")) != NULL) {
                /*
-                * Assume that SOURCE_DATE_EPOCH is set only on
-                * systems where time_t is also in seconds since the
-                * epoch.
+                * Correctly handling SOURCE_DATE_EPOCH is
+                * surprisingly fiddly.  To make life slightly easy,
+                * we assume that we're on a POSIX system, where
+                * time_t is an integer type, and that we can reject
+                * negative values out of hand.
+                *
+                * Even given that, time_t might be any integer type.
+                * It might be signed or unsigned, and it might be a
+                * standard or an extended type.
+                *
+                * If time_t is unsigned, it's always safe to convert
+                * something to it.  If the value is within the range
+                * of time_t it will remain unchanged, and if it
+                * isn't, it'll be reduced to be within the range (C11
+                * 6.3.1.3).
+                *
+                * While it's well-known that signed integer overflow
+                * in C causes undefined behaviour, this doesn't apply
+                * to conversion.  According to C11 6.3.1.3, if a
+                * conversion overflows "either the result is
+                * implementation-defined or an implementation-defined
+                * signal is raised."  I can't find any clear
+                * definition of what might happen when such a signal
+                * is raised, but I think it must either cause the
+                * process to exit with an error or do nothing (and
+                * presumably leave the destination untouched).
+                *
+                * So having initialised the destination to 0, we can
+                * assume that after the assignment it will either
+                * have the correct value or have a different value
+                * within the range of time_t.  Then we just have to
+                * check it.
                 */
-               epochll = strtoll(epochstr, &endptr, 10);
-               if (endptr == epochstr || *endptr != '\0' ||
-                   epochll == LLONG_MAX) {
+               errno = 0;
+               epochumax = strtoumax(epochstr, &endptr, 10);
+               if (!isdigit((unsigned char)epochstr[0]) ||
+                   endptr == epochstr || *endptr != '\0' ||
+                   errno == ERANGE) {
                        fprintf(stderr, "Invalid SOURCE_DATE_EPOCH\n");
                        return NULL;
                }
+               now = epochumax;
                /*
-                * I can't find a way to range-check this assignment
-                * in standard C or even in POSIX.
+                * Check if the value fitted into time_t.  If "now" is
+                * negative then it obviously didn't.  If it's
+                * non-negative then converting back into a uintmax_t
+                * will not change the value, since all non-negative
+                * numbers that can be represented in any integer type
+                * can be represented in a uintmax_t.
                 */
-               now = epochll;
+               if (now < 0 || (uintmax_t)now != epochumax) {
+                       fprintf(stderr, "Invalid SOURCE_DATE_EPOCH\n");
+                       return NULL;
+               }
        } else {
                now = time(NULL);
                if (now == (time_t)-1) {