Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime

Jeff King [off-list ref] writes:

quoted

9999999999999999 Sat Jan 25 10:46:39 316889355 -0700
99999999999999999 Wed Sep 6 02:46:39 -1126091476 -0700
999999999999999999 Thu Oct 24 18:46:39 1623969404 -0700

Thanks. Given the value where it fails, it kind of looks like there is
some signed 32-bit value at work (~300 million years is OK, but 10 times
that, rather than yielding ~3 billion, gets us -1 billion). Perhaps
tm.tm_year is 32-bit.

That is what it looks like to me, too.  It makes me wonder if some
other platforms may have similar breakage using 16-bit signed
tm_year and how such a breakage would look like, though ;-)

So what do we want to do? I think the options are:

  1. Try to guess when we have a bogus timestamp value with an arbitrary
     cutoff like "greater than 1 million years from now" (and enforce it
     via time_t seconds, and avoid gmtime entirely). That is made-up and
     arbitrary, but it also is sufficiently far that it won't ever
     matter, and sufficiently close that any gmtime should behave
     sensibly with it.

I think that two assumptions here are that

 (1) we would be able to find a single insane value (like 3 billion
     years from now expressed in time_t seconds) the test script
     would be able to feed and expect it to fail on all platforms we
     care about, even though how exactly that value causes various
     platforms fail may be different (glibc may give us a NULL from
     gmtime, FreeBSD may leave their own sentinel in gmtime we can
     recognize, and some others may simply wrap around the years);
     and that

 (2) the only other class of failure mode we haven't considered
     bevore Charles's report is tm_year wrapping around 32-bit
     signed int.

Offhand, the three possible failure modes this thread identified
sounds to me like the only plausible ones, and I think the best way
forward might be to

 - teach the "is the result sane, even though we may have got a
   non-NULL from gmtime?  otherwise let's signal a failure by
   replacing it with a known sentinel value" codepath the new
   failure mode Charles's report suggests---if we feed a positive
   timestamp and gmtime gave us back a tm_year+1900 < 0, that is
   certainly an overflow; and

 - Use that 3-billion years timestamp from Charles's report in the
   test and make sure we convert it to the known sentinel value.

perhaps?

  2. Accept that we can't guess at every broken gmtime's output, and
     just loosen the test to make sure we don't segfault.

Of course that is a simpler option, but we may have identified all
plausible failure modes, in which case we can afford to go with your
original plan to validate that we not just avoid segfaulting on one
of the three failure modes from gmtime, but also cover the other two
failure modes and signal any of them with a sentinel.  That way may
allow us to identify the fourth failure mode we haven't anticipated.

On Fri, Mar 28, 2014 at 09:41:53AM -0700, Junio C Hamano wrote:

Offhand, the three possible failure modes this thread identified
sounds to me like the only plausible ones, and I think the best way
forward might be to

 - teach the "is the result sane, even though we may have got a
   non-NULL from gmtime?  otherwise let's signal a failure by
   replacing it with a known sentinel value" codepath the new
   failure mode Charles's report suggests---if we feed a positive
   timestamp and gmtime gave us back a tm_year+1900 < 0, that is
   certainly an overflow; and

I don't think we can analyze the output from gmtime. If it wraps the
year at N, then won't N+2014 look like a valid value?

If we are going to do something trustworthy I think it has to be before
we hand off to gmtime. Like:

diff --git a/date.c b/date.c
index e1a2cee..e0c43c4 100644
--- a/date.c
+++ b/date.c

@@ -57,6 +57,8 @@ static time_t gm_time_t(unsigned long time, int tz)
 static struct tm *time_to_tm(unsigned long time, int tz)
 {
 	time_t t = gm_time_t(time, tz);
+	if (t > 9999999999999999)
+		return NULL;
 	return gmtime(&t);
 }

I suspect that would handle the FreeBSD case, as well.

By the way, I have a suspicion that the gm_time_t above can overflow if
you specially craft a value at the edge of what time_t can handle (we
check that our value will not overflow time_t earlier, but now we might
be adding up to 86400 seconds to it). <sigh>

-Peff