Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset
I think the 2099 limitation is there because 2100 is not a leap year, and the code has over-simplified computation there (it counts every 4th year as a leap year, which is not generally true for century boundaries, except for 2000 because it is divisible by 400).
From the log message of ecee9d9e79 ([PATCH] Do date parsing by hand..., 2005-04-30), I think the 1970 limitation may be about avoiding non-60-minute DST (though it is not clear to me how the function cares about dst either way, as it operates on UTC). You can't represent times before 1970 in our code base anyway, because our epoch timestamp is unsigned.
So yes, we could do better than that function. I worked on a series to handle negative timestamps a while ago, and as part of that, I had to replace this function. You can see the full series at the "jk/time-negative-wip" branch of https://github.com/peff/git, but I'll reproduce the commit in question below for the list archive.
The resulting series worked fine for me, but IIRC there was some issue on Windows. I think its gmtime() did not understand negative timestamps, maybe? And we'd need the reverse equivalent of the patch below (which we could take from the same source).
There are some overflow checks already in date.c (grep for "overflow") that might cover us, but I won't be surprised if there's a case that's missing (although we store the timestamps as uintmax_t, so you're unlikely to overflow that in practice). Underflow is more likely, though if we finally support negative timestamps, then it becomes a non-issue (and we'd actually want to revert the patch you're working on).
If you're interested in picking up the negative timestamp work, I'd be happy to discuss it. It's been on my todo list for a long time, but I never quite get around to it.
Anyway, here's the tm_to_time_t() cleanup.
-- >8 -- date: make tm_to_time_t() less restricted
We've been using a hand-rolled tm_to_time_t() conversion since ecee9d9e79 ([PATCH] Do date parsing by hand..., 2005-04-30). It works fine for recent dates, but it doesn't work before 1970 nor after 2099.
Let's lift those restrictions by using a more robust algorithm. The days_from_civil() code here is lifted from Howard Hinnant's date library, which can be found at:
https://github.com/HowardHinnant/date
The code is in the year_month_day::to_days() function, but I also relied on his excellent explanation in:
http://howardhinnant.github.io/date_algorithms.html
The library is under the MIT license, which is GPL compatible. I've also left a comment in the source file marking the licensing (the code is substantially similar to the original; I only expanded the variable names to make it easier to follow).
Ideally we could just rely on a system function to do this, but there isn't one. POSIX specifies mktime(), but it uses the local timezone. There's a timegm() function available in glibc and some BSD systems, as well as a similar function on Windows (with a different name). However, it's likely that there are still going to be some platforms without it, so we'd need a fallback anyway. We may as well just use that fallback everywhere to make sure that Git behaves consistently (and get more complete test coverage).
Note that some callers, like parse_date_basic(), rely on tm_to_time_t() complaining when we have negative values in any field. So we'll continue to detect and complain about this case (the alternative would be to turn "month -1" into "month 11" of the previous year, and so on).
Signed-off-by: Jeff King <peff@peff.net>
---
date.c | 58 ++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 42 insertions(+), 16 deletions(-)
Show changes to date.c +42 −16
diff --git a/date.c b/date.c
index 4337e18abd..df6c414c78 100644
--- a/date.c
+++ b/date.c
@@ -10,29 +10,55 @@
#include "pager.h"
#include "strbuf.h"
+/*
+ * Convert a year-month-day time into a number of days since 1970 (possibly
+ * negative). Note that "year" is the full year (not offset from 1900), and
+ * "month" is in the range 1-12 (unlike a "struct tm" 0-11).
+ *
+ * Working in time_t is overkill (since it usually represents seconds), but
+ * this makes sure we don't hit any integer range issues.
+ *
+ * This function is taken from https://github.com/HowardHinnant/date,
+ * which is released under an MIT license.
+ */
+static time_t days_from_civil(int year, int month, int day)
+{
+ time_t era;
+ unsigned year_of_era, day_of_year, day_of_era;
+
+ year -= month <= 2;
+ era = (year >= 0 ? year : year - 399) / 400;
+ year_of_era = year - era * 400;
+ day_of_year = (153 * (month + (month > 2 ? -3 : 9)) + 2) / 5 + day - 1;
+ day_of_era = year_of_era * 365 + year_of_era / 4 - year_of_era / 100
+ + day_of_year;
+ return era * 146097 + (time_t)day_of_era - 719468;
+}
+
/*
* This is like mktime, but without normalization of tm_wday and tm_yday.
* It also always operates in UTC, not the local timezone.
*/
time_t tm_to_time_t(const struct tm *tm)
{
- static const int mdays[] = {
- 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334
- };
- int year = tm->tm_year - 70;
- int month = tm->tm_mon;
- int day = tm->tm_mday;
-
- if (year < 0 || year > 129) /* algo only works for 1970-2099 */
- return -1;
- if (month < 0 || month > 11) /* array bounds */
- return -1;
- if (month < 2 || (year + 2) % 4)
- day--;
- if (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_sec < 0)
+ time_t days, hours, minutes, seconds;
+
+ if (tm->tm_year < 0 ||
+ tm->tm_mon < 0 ||
+ tm->tm_mday < 0 ||
+ tm->tm_hour < 0 ||
+ tm->tm_min < 0 ||
+ tm->tm_sec < 0)
return -1;
- return (year * 365 + (year + 1) / 4 + mdays[month] + day) * 24*60*60UL +
- tm->tm_hour * 60*60 + tm->tm_min * 60 + tm->tm_sec;
+
+ days = days_from_civil(tm->tm_year + 1900,
+ tm->tm_mon + 1,
+ tm->tm_mday);
+ hours = 24 * days + tm->tm_hour;
+ minutes = 60 * hours + tm->tm_min;
+ seconds = 60 * minutes + tm->tm_sec;
+
+ return seconds;
}
static const char *month_names[] = {
darcy via GitGitGadget
