patch, 5 partshandle bogus commit dates

This series improves our handling of commit dates that are numbers, but are ridiculously large. The most critical one is the final commit, which fixes a segfault, but the others clean up various corner cases.

  [1/5]: t4212: test bogus timestamps with git-log
  [2/5]: fsck: report integer overflow in author timestamps
  [3/5]: date: check date overflow against time_t
  [4/5]: pretty: handle integer overflow in timestamps
  [5/5]: log: do not segfault on gmtime errors

-Peff

When t4212 was originally added by 9dbe7c3d (pretty: handle broken commit headers gracefully, 2013-04-17), it tested our handling of commits with broken ident lines in which the timestamps could not be parsed. It does so using a bogus line like "Name <email>-<> 1234 -0000", because that simulates an error that was seen in the wild.

Later, 03818a4 (split_ident: parse timestamp from end of line, 2013-10-14) made our parser smart enough to actually find the timestamp on such a line, and t4212 was adjusted to match. While it's nice that we handle this real-world case, this meant that we were not actually testing the bogus-timestamp case anymore.

This patch adds a test with a totally incomprehensible timestamp to make sure we are testing the code path.

Note that the behavior is slightly different between regular log output and "--format=%ad". In the former case, we produce a sentinel value and in the latter, we produce an empty string. While at first this seems unnecessarily inconsistent, it matches the original behavior given by 9dbe7c3d.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4212-log-corrupt.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

When we check commit objects, we complain if commit->date is ULONG_MAX, which is an indication that we saw integer overflow when parsing it. However, we do not do any check at all for author lines, which also contain a timestamp.

Let's actually check the timestamps on each ident line with strtoul. This catches both author and committer lines, and we can get rid of the now-redundant commit->date check.

Note that like the existing check, we compare only against ULONG_MAX. Now that we are calling strtoul at the site of the check, we could be slightly more careful and also check that errno is set to ERANGE. However, this will make further refactoring in future patches a little harder, and it doesn't really matter in practice.

For 32-bit systems, one would have to create a commit at the exact wrong second in 2038. But by the time we get close to that, all systems will hopefully have moved to 64-bit (and if they haven't, they have a real problem one second later).

For 64-bit systems, by the time we get close to ULONG_MAX, all systems will hopefully have been consumed in the fiery wrath of our expanding Sun.

Signed-off-by: Jeff King <peff@peff.net>
---
Note that tags don't get checked here, because we do not feed their
ident lines to fsck_ident at all. This is still a step forward, though,
as if we ever teach them to check ident lines, they'll get this new
check automatically.

 fsck.c          | 12 ++++++------
 t/t1450-fsck.sh | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

When we check whether a timestamp has overflowed, we check only against ULONG_MAX, meaning that strtoul has overflowed. However, we also feed these timestamps to system functions like gmtime, which expect a time_t. On many systems, time_t is actually smaller than "unsigned long" (e.g., because it is signed), and we would overflow when using these functions. We don't know the actual size or signedness of time_t, but we can easily check for truncation with a simple assignment.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |  1 +
 date.c  | 17 +++++++++++++++++
 fsck.c  |  2 +-
 3 files changed, 19 insertions(+), 1 deletion(-)

If an ident line has a ridiculous date value like (2^64)+1, we currently just pass ULONG_MAX along to the date code, which can produce nonsensical dates.

On systems with a signed long time_t (e.g., 64-bit glibc systems), this actually doesn't end up too bad. The ULONG_MAX is converted to -1, we apply the timezone field to that, and the result ends up somewhere between Dec 31, 1969 and Jan 1, 1970.

However, there is still a few good reasons to detect the overflow explicitly:

  1. On systems where "unsigned long" is smaller than
     time_t, we get a nonsensical date in the future.

  2. Even where it would produce "Dec 31, 1969", it's easier
     to recognize "midnight Jan 1" as a consistent sentinel
     value for "we could not parse this".

  3.  Values which do not overflow strtoul but do overflow a
      signed time_t produce nonsensical values in the past.
      For example, on a 64-bit system with a signed long
      time_t, a timestamp of 18446744073000000000 produces a
      date in 1947.

We also recognize overflow in the timezone field, which could produce nonsensical results. In this case we show the parsed date, but in UTC.

Signed-off-by: Jeff King <peff@peff.net>
---
A note on these tests. They are designed for 64-bit systems, but should
run fine on 32-bit systems (they are both just overflow, and the second
test is not doing anything novel that the first is not).

However, the second test relies on finding a value that fits into an "unsigned long" but does not fit into a time_t. For systems with a 64-bit signed time_t, that's fine. But if a system has an unsigned 64-bit time_t, the test will fail (it will actually produce some value 300 billion years from now).

I'm inclined to include it as-is, as I do not know of any such system (and it would be kind of lame, since it could not represent dates before 1970). If somebody comes up with such a system, we can allow either output (we could do it preemptively, but somebody would have to calculate the exact date/time billions of years in the future; we would be just as likely to disagree with the system's gmtime about something silly like leapseconds).

 pretty.c               | 10 ++++++++--
 t/t4212-log-corrupt.sh | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

Jeff King <peff@peff.net> writes:

Both are good measures to fallback to sanity, but why is that if/else? In other words...

... don't we want to fix an input having a bogus timestamp and also a bogus tz recorded in it?

On Mon, Feb 24, 2014 at 11:50:00AM -0800, Junio C Hamano wrote:

If there is a bogus timestamp, then we do not want to look at tz at all. We leave it at "0", so that we get a true sentinel:

  Thu Jan 1 00:00:00 1970 +0000

and not:

  Wed Dec 31 19:00:00 1969 -0500

If the timestamp is valid, then we bother to parse the zone, and handle overflow there.

-Peff

Jeff King <peff@peff.net> writes:

Ah, OK, I missed the initialization to 0 at the beginning.

It might have been more clear if "int tz" declaration were left uninitialized, and the variable were explicitly cleared to 0 in the "date-overflows" error codepath, but it is not a big deal.

Thanks for clarification.

On Mon, Feb 24, 2014 at 12:21:33PM -0800, Junio C Hamano wrote:

It might be, but I think it would end up cumbersome. The initialization was already there from the previous version, which was hitting the else for "ident->tz_begin". Without fallback initializations, you end up with:

  if (ident->date_begin && ident->date_end) {
          date = strtoul(ident->date_begin, NULL, 10);
          if (date_overflows(date)) {
                  date = 0;
                  tz = 0;
          } else {
                  if (ident->tz_begin && ident->tz_end) {
                          tz = strtol(ident->tz_begin, NULL, 10);
                          if (tz == LONG_MAX || tz == LONG_MIN)
                                  tz = 0;
                  } else
                          tz = 0;
          }
  } else {
          date = 0;
          tz = 0;
  }

which I think is much more confusing (and hard to verify that the variables are always set). Checking !date as an error condition would make it a little more readable:

  if (ident->date_begin && ident->date_end) {
          date = strtoul(ident->date_begin, NULL, 10);
          if (date_overflows(date))
                  date = 0;
  } else
          date = 0;

  if (date) {
          if (ident->tz_begin && ident->tz_end) {
                  tz = strtol(ident->tz_begin, NULL, 10);
                  if (tz == LONG_MAX || tz == LONG_MIN)
                          tz = 0;
          } else
                  tz = 0;
  } else
          tz = 0;

but then we treat date==0 as a sentinel, and can never correctly parse dates on Jan 1, 1970.

So I'd be in favor of keeping it as-is, but feel free to mark it up if you feel strongly.

-Peff

Jeff King <peff@peff.net> writes:

I'd be in favor of keeping it as-is.

Many code paths assume that show_date and show_ident_date cannot return NULL. For the most part, we handle missing or corrupt timestamps by showing the epoch time t=0.

However, we might still return NULL if gmtime rejects the time_t we feed it, resulting in a segfault. Let's catch this case and just format t=0.

Signed-off-by: Jeff King <peff@peff.net>
---
This test is of questionable portability, since we are depending on
gmtime's arbitrary point to decide that our input is crazy and return
NULL. The value is sufficiently large that I'd expect most to do so,
though, so it may be safe.

On 32-bit systems, of course, the test does nothing (it is just hitting the integer overflow code path). But that's OK, since the output is the same for both cases.

 date.c                 | 6 ++++--
 t/t4212-log-corrupt.sh | 8 ++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

Am 24.02.2014 08:49, schrieb Jeff King:

Just came around to testing on FreeBSD 10 amd64; the new test in t4212 fails there:

--- expect 2014-03-22 08:29:44.000000000 +0000 +++ actual 2014-03-22 08:29:44.000000000 +0000

On Sat, Mar 22, 2014 at 10:32:37AM +0100, René Scharfe wrote:

Thanks for the report. I don't think the problem is in the test here, but rather that we should do a more thorough job of detecting gmtime's "I don't know what to do with this" response.

That feels like a bit of a maintenance headache. Right now we do not internally represent times prior to the epoch, since we mostly use "unsigned long" through the code. So anything < 70 has to be an error. But it would be nice one day to consistently use a 64-bit signed time_t throughout git, and represent even ancient timestamps (e.g., for people doing historical projects, like importing laws into git). This would set quite a trap for people working later on the code.

If the result is all-zeroes, can we check for that case instead? I suppose that will eventually create a "trap" at midnight on January 1st of the year 0 (though I am not sure such a date is even meaningful, given the history of our calendars).

-Peff

Am 24.03.2014 22:33, schrieb Jeff King:

Makes sense. That would be "Sun Jan 0 00:00:00 1900", however -- days are 1-based and years 1900-based in struct tm. Since a zeroth day is invalid, would this suffice:

	if (!tm || !tm->tm_mday) {

(Yes, I'm lazy. :)

René

On Mon, Mar 24, 2014 at 11:03:42PM +0100, René Scharfe wrote:

Oh right, I was stupidly forgetting about being 1900-based.

That looks perfect (and I like that it is quick to check, too, since this case should be extremely rare).

-Peff

On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote:

Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 162396404 -0700. I don't know if this is correct for the given input but the test fails.

Charles.

On Wed, Mar 26, 2014 at 11:05:59AM +0000, Charles Bailey wrote:

Ick. I am not sure that dates at that range are even meaningful (will October really exist in the year 162396404? Did they account for all the leap-seconds between then and now?). But at the same time, I cannot fault their gmtime for just extrapolating the current rules out as far as we ask them to.

Unlike the FreeBSD thing that René brought up, this is not a problem in the code, but just in the test. So I think our options are basically:

  1. Scrap the test as unportable.

  2. Hard-code a few expected values. I'd be unsurprised if some other
     system comes up with a slightly different date in 162396404, so we
     may end up needing several of these.

I think I'd lean towards (2), just because it is testing an actual feature of the code, and I'd like to continue doing so. And while we may end up with a handful of values, there's probably not _that_ many independent implementations of gmtime in the wild.

-Peff

One of the tests in t4212 checks our behavior when we feed gmtime a date so far in the future that it gives up and returns NULL.

But some gmtime implementations just refuse to quit. They soldier on, giving us a glimpse of a chilly October evening some 160 million years in the future (and presumably filled with our leather-clad horseback-riding ape descendants).

Let's allow the test to match either the sentinel value (i.e., what we want when gmtime gives up) or any reasonable value returned by known implementations.

Reported-by: Charles Bailey <cbailey32@bloomberg.net>
Signed-off-by: Jeff King <peff@peff.net>
---
On top of jk/commit-dates-parsing-fix (though the test is already in
v1.9.1).

 t/t4212-log-corrupt.sh | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Jeff King <peff@peff.net> writes:

Style ;-)

It actually may be easier to understand if you write a trivial case statement at the sole calling site of this helper function, though.

In any case, would it really be essential to make sure that the output shows a phony (or a seemingly real) datestamp for this test?

The primary thing you wanted to achieve by the "gmtime gave us NULL, let's substitute it with an arbitrary value to avoid dereferencing the NULL" change was *not* that we see that same arbitrary value comes out of the system, but that we do not die by attempting to reference the NULL, I think. Not dying is the primary thing we want to (and we already do) test, no?

On Wed, Mar 26, 2014 at 12:18:25PM -0700, Junio C Hamano wrote:

Yeah, perhaps. I wanted to build on test_cmp because it has nice output for the failure case, but it is probably simple enough to do:

  output=$(cat actual)
  case "$output" in
  ...
  *) echo >&2 "unrecognized date: $output"

I think there are really two separate behaviors we are testing here (and in the surrounding tests):

  1. Don't segfault if gmtime returns NULL.

  2. Whenever we cannot process a date (either because gmtime fails, or
     because we fail before even getting the value to gmtime),
     consistently return the sentinel date (so the reader can easily
     know it's bogus).

Having the test be particular about its output helped us find a case where FreeBSD did not trigger (1), but did trigger (2), by returning a blanked "struct tm".

I'm open to the argument that (2) is not worth worrying about that much if it is a hassle to test. But I don't think it is that much hassle (yet, anyway).

-Peff

On Wed, Mar 26, 2014 at 03:25:36PM -0400, Jeff King wrote:

That being said, is the AIX value actually right? I did not look closely at first, but just assumed that it was vaguely right. But:

  999999999999999999 / (86400 * 365)

is something like 31 billion years in the future, not 160 million. A real date calculation will have a few tweaks (leap years, etc), but that is orders of magnitude off.

So I am not sure that AIX is not actually just giving us utter crap. In that case, the test is not wrong; it's pickiness is actually finding a real problem. But I am not sure it is a problem worth solving. I do not want to get into heuristics deciding whether a particular platform's gmtime output is crap or not. That pushes this into the realm of "it's not worth testing", and we should stick to just testing that we did not segfault.

-Peff

On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote:

Assuming my math is right, then here is the most sensible patch, IMHO.

-- >8 --
Subject: t4212: loosen far-in-future test for AIX

One of the tests in t4212 checks our behavior when we feed gmtime a date so far in the future that it gives up and returns NULL. Some implementations, like AIX, may actually just provide us a bogus result instead.

It's not worth it for us to come up with heuristics that guess whether the return value is sensible or not. On good platforms where gmtime reports the problem to us with NULL, we will print the epoch value. On bad platforms, we will print garbage. But our test should be written for the lowest common denominator so that it passes everywhere.

Reported-by: Charles Bailey <cbailey32@bloomberg.net>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4212-log-corrupt.sh | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

On Wed, Mar 26, 2014 at 03:40:43PM -0400, Jeff King wrote:

Perhaps hold onto this one for a little while longer. Splitting things out from the test is giving me some inconsistent results, there may be something else going wrong in our environment here.

Charles.

On Wed, Mar 26, 2014 at 08:36:18PM +0000, Charles Bailey wrote:

By the way, can you confirm that this is a 64-bit system? On a 32-bit system, we should be triggering different code paths (we fail at the strtoul level). Those should be checked by the previous tests, but I'd like to make sure.

-Peff

On Wed, Mar 26, 2014 at 04:38:30PM -0400, Jeff King wrote:

Yes, we're only building 64-bit at the moment.

On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote:

Well, this is embarrassing, while moving this through the corporate firewall (aka typing on one machine while looking at another), I munged the date. It still doesn't seem right but at least you can now see the actual data.

I stopped the test with --immediate and found the dangling commit that the test created and dumped it with the previous version of Git (well a 1.8.5.5 build)

  ibm: trash directory.t4212-log-corrupt $ git log -1 --pretty=raw 1fc17e734e4487c31bdfe05bb3d15618b69c4dca
  commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca
  tree 64fd3796c57084e7b8cbae358ce37970b8e954f6
  author A U Thor <author@example.com> 999999999999999999 -0700
  committer C O Mitter <committer@example.com> 1112911993 -0700

      foo

  ibm: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca
  commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca
  Author: A U Thor <author@example.com>
  Date:   Thu Oct 24 18:46:39 1623969404 -0700

      foo

Same commit but dumped from a linux machine:

  linux: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca
  commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca
  Author: A U Thor <author@example.com>
  Date:   (null)

      foo

Charles.

On Wed, Mar 26, 2014 at 09:22:27PM +0000, Charles Bailey wrote:

Hmm, so the year you got is actually: 1623969404. That still seems off to me by a factor 20. I don't know if this is really worth digging into that much further, but I wonder what you would get for timestamps of:

  99999999999999999
  9999999999999999
  999999999999999
  etc.

Do we start generating weird values at some particular size? Or is AIX gmtime really more clever than I am, and is accounting for wobble of the Earth or something over the next billion years?

-Peff

On Wed, Mar 26, 2014 at 05:57:41PM -0400, Jeff King wrote:

AIX goes negative at about the same time Linux and Solaris segfault:

9999999 Sun Apr 26 10:46:39 1970 -0700 99999999 Sat Mar 3 02:46:39 1973 -0700 999999999 Sat Sep 8 18:46:39 2001 -0700 9999999999 Sat Nov 20 10:46:39 2286 -0700 99999999999 Wed Nov 16 02:46:39 5138 -0700 999999999999 Thu Sep 26 18:46:39 33658 -0700 9999999999999 Sun May 20 10:46:39 318857 -0700 99999999999999 Sat Nov 7 02:46:39 3170843 -0700 999999999999999 Sat Jul 4 18:46:39 31690708 -0700 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

So, very bogus values.

Charles.

On Wed, Mar 26, 2014 at 10:46:16PM +0000, Charles Bailey wrote:

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.

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.

  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.

-Peff

Jeff King <peff@peff.net> writes:

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 ;-)

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?

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:

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:

Jeff King <peff@peff.net> writes:

Yes, but I was hoping that there are small number of possible N's ;-)

Yuck.  Let's not go there.

Thanks.

On Fri, Mar 28, 2014 at 12:02:46PM -0700, Junio C Hamano wrote:

I'm not sure I understand. Even if we know N, we've lost information during the truncation done by time_t (we cannot distingiuish true M from N+M).

Do you mean "let's not worry about the absurdly specific overflow case", or "let's not do this gross time_to_tm hack?"

This (non-)issue has consumed a lot more brain power than it is probably worth. I'd like to figure out which patch to go with and be done. :)

-Peff

Jeff King <peff@peff.net> writes:

Let's just deal with a simple known cases (like FreeBSD) in the real code that everybody exercises at runtime, and have the new test only check we do not segfault on a value we used to segfault upon seeing.

On Fri, Mar 28, 2014 at 12:30:02PM -0700, Junio C Hamano wrote:

OK. Here it is, with the other option as an "alt" patch for reference.

  [1/2]: date: recognize bogus FreeBSD gmtime output
  [2/2]: t4212: loosen far-in-future test for AIX
  [2alt/2]: work around unreliable gmtime errors on AIX

-Peff

Most gmtime implementations return a NULL value when they encounter an error (and this behavior is specified by ANSI C and POSIX). FreeBSD's implementation, however, will indicate an error by returning a pointer to a "struct tm" with all fields set to zero. Let's also recognize this and convert it to a NULL (with this patch, t4212 should pass on FreeBSD).

Reported-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
There are actually a few callers to gmtime and gmtime_r, so I pushed
this fix up into a compat wrapper rather than in time_to_tm to get them
all. It's possible that localtime() would want to receive the same
treatment, too.  It's not strictly necessary to make the wrapper
conditional, but it was easy to do so. We could also just run this code
all the time.

I don't have a FreeBSD VM handy to test this, so confirmation that it passes the test would be nice.

 Makefile          |  8 ++++++++
 compat/gmtime.c   | 26 ++++++++++++++++++++++++++
 config.mak.uname  |  1 +
 git-compat-util.h |  7 +++++++
 4 files changed, 42 insertions(+)
 create mode 100644 compat/gmtime.c

Am 01.04.2014 09:42, schrieb Jeff King:

http://pubs.opengroup.org/onlinepubs/009695399/functions/gmtime.html says that errno shall be set on error and only mentions EOVERFLOW as a possible error code.

René

René Scharfe <l.s.r@web.de> writes:

So are you saying we should set EOVERFLOW ourselves, or does FreeBSD set EOVERFLOW for us in this case and we do not have to worry?

Am 01.04.2014 21:08, schrieb Junio C Hamano:

If we correct the return value then we should correct errno as well. gmtime() on FreeBSD 10 leaves errno untouched when it encounters an overflow.

While testing this again I just noticed that FreeBSD doesn't strictly return a pointer to a cleared struct tm. It simply leaves its static buffer untouched. We should probably clear it (memset in git_gmtime_r).

Demo program:

-- >8 -- #include <stdio.h> #include <time.h>

const static time_t epoch;

int main(int argc, char **argv)
{
         time_t t = 99999999999999999;

         printf("%s", ctime(&t));
         printf("%s", asctime(gmtime(&t)));

         printf("%s", ctime(&t));
         gmtime(&epoch);
         printf("%s", asctime(gmtime(&t)));

         return 0;
}
-- 8< --

Results on FreeBSD 10:

	Wed Sep  6 11:46:39     -1126091476
	Wed Sep  6 11:46:39     -1126091476
	Wed Sep  6 11:46:39     -1126091476
	Thu Jan  1 00:00:00 1970

René

On Tue, Apr 01, 2014 at 11:17:14PM +0200, René Scharfe wrote:

Thanks, that all makes sense (we do not ever care about gmtime's errno in our code, but it does not hurt to be thorough to avoid surprising any new callers). Here's a replacement for patch 1.

-- >8 --
Subject: date: recognize bogus FreeBSD gmtime output

Most gmtime implementations return a NULL value when they encounter an error (and this behavior is specified by ANSI C and POSIX). FreeBSD's implementation, however, will simply leave the "struct tm" untouched. Let's also recognize this and convert it to a NULL (with this patch, t4212 should pass on FreeBSD).

Reported-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile          |  8 ++++++++
 compat/gmtime.c   | 29 +++++++++++++++++++++++++++++
 config.mak.uname  |  1 +
 git-compat-util.h |  7 +++++++
 4 files changed, 45 insertions(+)
 create mode 100644 compat/gmtime.c

One of the tests in t4212 checks our behavior when we feed gmtime a date so far in the future that it gives up and returns NULL. Some implementations, like AIX, may actually just provide us a bogus result instead.

It's not worth it for us to come up with heuristics that guess whether the return value is sensible or not. On good platforms where gmtime reports the problem to us with NULL, we will print the epoch value. On bad platforms, we will print garbage. But our test should be written for the lowest common denominator so that it passes everywhere.

Reported-by: Charles Bailey <cbailey32@bloomberg.net>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4212-log-corrupt.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

AIX's gmtime will happily overflow the tm_year field. Let's catch this error before handing the value to gmtime.

Signed-off-by: Jeff King <peff@peff.net>
---
This is an alternative to loosening the test in t4212.

It's really not _that_ ugly. The "LL" here may not be portable, though. 32-bit systems can't represent this timestamp at all (so they're safe), but I don't know what would be the best way to conditionally compile here.

 compat/gmtime.c  | 10 ++++++++++
 config.mak.uname |  1 +
 2 files changed, 11 insertions(+)

Jeff King <peff@peff.net> writes:

Thanks. 2alt does not look too bad, but on the other hand, we are replacing a value that can produce the right result on correctly implemented gmtime with a completely bogus value only because we know there exists one broken implementation---which does not sound a very good trade-off, given that we would get a result that does not correspond to the input anyway with or without the change on the broken implementation.

On Tue, Apr 01, 2014 at 12:07:22PM -0700, Junio C Hamano wrote:

One other option is to push _all_ callers through a git_gmtime() wrapper, and then use Makefile knobs to turn on specific quirks, like:

    struct tm *git_gmtime_r(const time_t *timep, struct tm *result)
    {
    #ifdef GMTIME_OVERFLOWS_32BIT
              if (*timep > 999999999999999)
                        return NULL;
    #endif

              ret = gmtime_r(timep, result);

    #ifdef GMTIME_ERROR_BLANK
              if (ret && !ret->tm_mday)
                        return NULL;
    #endif

              return ret;
    }

and then each platform can turn the knobs as appropriate.

-Peff

Jeff King <peff@peff.net> writes:

Or "3. Just make sure that 'git log' does not segfault"?

On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote:

That would not test the FreeBSD case, which does not segfault, but returns a bogus sentinel value.

I don't know how important that is. This is such a minor feature that it is not worth a lot of maintenance headache in the test. But I also do not know if this is going to be the last report, or we will have a bunch of other systems that need their own special values put into the test.

-Peff

Jeff King <peff@peff.net> writes:

I didn't quite realize that your objective of the change was to signal the failure of gmtime for all implementations, i.e. both (1) the ones that signal an error by giving NULL back to us and (2) the ones that fail to signal an error but leave bogus result (FreeBSD, but it appears AIX might be also giving us another bogus value), by using a single sane sentinel value. If we can do that consistently on every platform, that would indeed be great.

But if that is the case, we should not have to maintain special case values in the test code, I would think. The test instead should expect the output to have that single sentinel value, and if the workaround code fails to detect a breakage in the platform gmtime(), the special case logic to catch these platform-specific breakages should go that "timestamp that cannot be grokked by gmtime---replace it with a sentinel" logic, no?

On Wed, Mar 26, 2014 at 02:01:21PM -0700, Junio C Hamano wrote:

I didn't realize it at the time I wrote the test, either. It was my goal to recognize such failures, but I didn't now at the time that there were failures that would be signaled by anything besides a NULL return.

Agreed. I am not sure we can do so. For FreeBSD, I think it is not hard. I am not sure what the pattern is for AIX. I am hoping Charles will reveal some pattern, but there may not be one.

Right. All of my suggestions to accommodate special values in the test were assuming that the system gmtime was smarter than we are. That it produced a good value for this crazy time and _didn't_ fail.

But after having done the basic math, I don't think that is what is going on here.

Yes, exactly. That is the preferable solution, if we can come up with such logic. Personally I am not optimistic.

The fallback is to accept that we cannot cover all cases, and just loosen the test (i.e., the second patch I posted today).

-Peff