From: Ben Harris <bjh21@bjh21.me.uk>
Date: Mon, 11 Nov 2024 22:19:54 +0000 (+0000)
Subject: Make the processing of SOURCE_DATE_EPOCH safe
X-Git-Tag: bedstead-3.246~39
X-Git-Url: https://www.chiark.greenend.org.uk/ucgi/~bjharris/git?a=commitdiff_plain;h=b8abf30fcd0beec4ec958e583e123bb94ffe5410;p=bedstead.git

Make the processing of SOURCE_DATE_EPOCH safe

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

diff --git a/bedstead.c b/bedstead.c
index 104983b..71f77bf 100644
--- a/bedstead.c
+++ b/bedstead.c
@@ -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) {