{"thread":{"id":"35943","subject":"[PATCH 0/5] handle bogus commit dates","startedAt":"2014-02-24T07:33:48Z","lastAt":"2014-04-01T21:28:42Z","messageCount":48,"participants":["Jeff King","Junio C Hamano","René Scharfe","Charles Bailey"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"235220","messageId":"20140224073348.GA20221@sigill.intra.peff.net","threadId":"35943","inReplyTo":null,"subject":"[PATCH 0/5] handle bogus commit dates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T07:33:48Z","receivedAt":"2014-02-24T07:33:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series improves our handling of commit dates that are numbers, but\nare ridiculously large. The most critical one is the final commit, which\nfixes a segfault, but the others clean up various corner cases.\n\n [1/5]: t4212: test bogus timestamps with git-log\n [2/5]: fsck: report integer overflow in author timestamps\n [3/5]: date: check date overflow against time_t\n [4/5]: pretty: handle integer overflow in timestamps\n [5/5]: log: do not segfault on gmtime errors\n\n-Peff\n"},{"id":"235221","messageId":"20140224073622.GA9969@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140224073348.GA20221@sigill.intra.peff.net","subject":"[PATCH 1/5] t4212: test bogus timestamps with git-log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T07:36:22Z","receivedAt":"2014-02-24T07:36:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When t4212 was originally added by 9dbe7c3d (pretty: handle\nbroken commit headers gracefully, 2013-04-17), it tested our\nhandling of commits with broken ident lines in which the\ntimestamps could not be parsed. It does so using a bogus line\nlike \"Name -<> 1234 -0000\", because that simulates an\nerror that was seen in the wild.\n\nLater, 03818a4 (split_ident: parse timestamp from end of\nline, 2013-10-14) made our parser smart enough to actually\nfind the timestamp on such a line, and t4212 was adjusted to\nmatch. While it's nice that we handle this real-world case,\nthis meant that we were not actually testing the\nbogus-timestamp case anymore.\n\nThis patch adds a test with a totally incomprehensible\ntimestamp to make sure we are testing the code path.\n\nNote that the behavior is slightly different between regular log\noutput and \"--format=%ad\". In the former case, we produce a\nsentinel value and in the latter, we produce an empty\nstring. While at first this seems unnecessarily\ninconsistent, it matches the original behavior given by\n9dbe7c3d.\n\nSigned-off-by: Jeff King \n---\n t/t4212-log-corrupt.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex 93c7c36..83de981 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -44,4 +44,25 @@ test_expect_success 'git log --format with broken author email' '\n \ttest_cmp expect.err actual.err\n '\n \n+munge_author_date () {\n+\tgit cat-file commit \"$1\" >commit.orig &&\n+\tsed \"s/^\\(author .*>\\) [0-9]*/\\1 $2/\" commit.munge &&\n+\tgit hash-object -w -t commit commit.munge\n+}\n+\n+test_expect_success 'unparsable dates produce sentinel value' '\n+\tcommit=$(munge_author_date HEAD totally_bogus) &&\n+\techo \"Date: Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n+\tgit log -1 $commit >actual.full &&\n+\tgrep Date actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'unparsable dates produce sentinel value (%ad)' '\n+\tcommit=$(munge_author_date HEAD totally_bogus) &&\n+\techo >expect &&\n+\tgit log -1 --format=%ad $commit >actual\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235222","messageId":"20140224073904.GB9969@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140224073348.GA20221@sigill.intra.peff.net","subject":"[PATCH 2/5] fsck: report integer overflow in author timestamps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T07:39:04Z","receivedAt":"2014-02-24T07:39:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we check commit objects, we complain if commit->date is\nULONG_MAX, which is an indication that we saw integer\noverflow when parsing it. However, we do not do any check at\nall for author lines, which also contain a timestamp.\n\nLet's actually check the timestamps on each ident line\nwith strtoul. This catches both author and committer lines,\nand we can get rid of the now-redundant commit->date check.\n\nNote that like the existing check, we compare only against\nULONG_MAX. Now that we are calling strtoul at the site of\nthe check, we could be slightly more careful and also check\nthat errno is set to ERANGE. However, this will make further\nrefactoring in future patches a little harder, and it\ndoesn't really matter in practice.\n\nFor 32-bit systems, one would have to create a commit at the\nexact wrong second in 2038. But by the time we get close to\nthat, all systems will hopefully have moved to 64-bit (and\nif they haven't, they have a real problem one second later).\n\nFor 64-bit systems, by the time we get close to ULONG_MAX,\nall systems will hopefully have been consumed in the fiery\nwrath of our expanding Sun.\n\nSigned-off-by: Jeff King \n---\nNote that tags don't get checked here, because we do not feed their\nident lines to fsck_ident at all. This is still a step forward, though,\nas if we ever teach them to check ident lines, they'll get this new\ncheck automatically.\n\n fsck.c | 12 ++++++------\n t/t1450-fsck.sh | 14 ++++++++++++++\n 2 files changed, 20 insertions(+), 6 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 99c0497..760e072 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -245,6 +245,8 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \n static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n {\n+\tchar *end;\n+\n \tif (**ident == '<')\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - missing space before email\");\n \t*ident += strcspn(*ident, \"<>\\n\");\n@@ -264,10 +266,11 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n \t(*ident)++;\n \tif (**ident == '0' && (*ident)[1] != ' ')\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - zero-padded date\");\n-\t*ident += strspn(*ident, \"0123456789\");\n-\tif (**ident != ' ')\n+\tif (strtoul(*ident, &end, 10) == ULONG_MAX)\n+\t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - date causes integer overflow\");\n+\tif (end == *ident || *end != ' ')\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - bad date\");\n-\t(*ident)++;\n+\t*ident = end + 1;\n \tif ((**ident != '+' && **ident != '-') ||\n \t !isdigit((*ident)[1]) ||\n \t !isdigit((*ident)[2]) ||\n@@ -287,9 +290,6 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \tint parents = 0;\n \tint err;\n \n-\tif (commit->date == ULONG_MAX)\n-\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid author/committer line\");\n-\n \tif (memcmp(buffer, \"tree \", 5))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex d730734..8c739c9 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -142,6 +142,20 @@ test_expect_success '> in name is reported' '\n \tgrep \"error in commit $new\" out\n '\n \n+# date is 2^64 + 1\n+test_expect_success 'integer overflow in timestamps is reported' '\n+\tgit cat-file commit HEAD >basis &&\n+\tsed \"s/^\\\\(author .*>\\\\) [0-9]*/\\\\1 18446744073709551617/\" \\\n+\t\tbad-timestamp &&\n+\tnew=$(git hash-object -t commit -w --stdin out &&\n+\tcat out &&\n+\tgrep \"error in commit $new.*integer overflow\" out\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tcat >invalid-tag <<-\\EOF &&\n \tobject ffffffffffffffffffffffffffffffffffffffff\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235223","messageId":"20140224073945.GC9969@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140224073348.GA20221@sigill.intra.peff.net","subject":"[PATCH 3/5] date: check date overflow against time_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T07:39:45Z","receivedAt":"2014-02-24T07:39:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we check whether a timestamp has overflowed, we check\nonly against ULONG_MAX, meaning that strtoul has overflowed.\nHowever, we also feed these timestamps to system functions\nlike gmtime, which expect a time_t. On many systems, time_t\nis actually smaller than \"unsigned long\" (e.g., because it\nis signed), and we would overflow when using these\nfunctions. We don't know the actual size or signedness of\ntime_t, but we can easily check for truncation with a simple\nassignment.\n\nSigned-off-by: Jeff King \n---\n cache.h | 1 +\n date.c | 17 +++++++++++++++++\n fsck.c | 2 +-\n 3 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex dc040fb..e405598 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -959,6 +959,7 @@ void datestamp(char *buf, int bufsize);\n unsigned long approxidate_careful(const char *, int *);\n unsigned long approxidate_relative(const char *date, const struct timeval *now);\n enum date_mode parse_date_format(const char *format);\n+int date_overflows(unsigned long date);\n \n #define IDENT_STRICT\t 1\n #define IDENT_NO_DATE\t 2\ndiff --git a/date.c b/date.c\nindex 83b4166..90b28f7 100644\n--- a/date.c\n+++ b/date.c\n@@ -1113,3 +1113,20 @@ unsigned long approxidate_careful(const char *date, int *error_ret)\n \tgettimeofday(&tv, NULL);\n \treturn approxidate_str(date, &tv, error_ret);\n }\n+\n+int date_overflows(unsigned long t)\n+{\n+\ttime_t sys;\n+\n+\t/* If we overflowed our unsigned long, that's bad... */\n+\tif (t == ULONG_MAX)\n+\t\treturn 1;\n+\n+\t/*\n+\t * ...but we also are going to feed the result to system\n+\t * functions that expect time_t, which is often \"signed long\".\n+\t * Make sure that we fit into time_t, as well.\n+\t */\n+\tsys = t;\n+\treturn t != sys || (t < 1) != (sys < 1);\n+}\ndiff --git a/fsck.c b/fsck.c\nindex 760e072..64bf279 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -266,7 +266,7 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n \t(*ident)++;\n \tif (**ident == '0' && (*ident)[1] != ' ')\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - zero-padded date\");\n-\tif (strtoul(*ident, &end, 10) == ULONG_MAX)\n+\tif (date_overflows(strtoul(*ident, &end, 10)))\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - date causes integer overflow\");\n \tif (end == *ident || *end != ' ')\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - bad date\");\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235224","messageId":"20140224074637.GD9969@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140224073348.GA20221@sigill.intra.peff.net","subject":"[PATCH 4/5] log: handle integer overflow in timestamps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T07:46:37Z","receivedAt":"2014-02-24T07:46:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If an ident line has a ridiculous date value like (2^64)+1,\nwe currently just pass ULONG_MAX along to the date code,\nwhich can produce nonsensical dates.\n\nOn systems with a signed long time_t (e.g., 64-bit glibc\nsystems), this actually doesn't end up too bad. The\nULONG_MAX is converted to -1, we apply the timezone field to\nthat, and the result ends up somewhere between Dec 31, 1969\nand Jan 1, 1970.\n\nHowever, there is still a few good reasons to detect the\noverflow explicitly:\n\n 1. On systems where \"unsigned long\" is smaller than\n time_t, we get a nonsensical date in the future.\n\n 2. Even where it would produce \"Dec 31, 1969\", it's easier\n to recognize \"midnight Jan 1\" as a consistent sentinel\n value for \"we could not parse this\".\n\n 3. Values which do not overflow strtoul but do overflow a\n signed time_t produce nonsensical values in the past.\n For example, on a 64-bit system with a signed long\n time_t, a timestamp of 18446744073000000000 produces a\n date in 1947.\n\nWe also recognize overflow in the timezone field, which\ncould produce nonsensical results. In this case we show the\nparsed date, but in UTC.\n\nSigned-off-by: Jeff King \n---\nA note on these tests. They are designed for 64-bit systems, but should\nrun fine on 32-bit systems (they are both just overflow, and the second\ntest is not doing anything novel that the first is not).\n\nHowever, the second test relies on finding a value that fits into an\n\"unsigned long\" but does not fit into a time_t. For systems with a\n64-bit signed time_t, that's fine. But if a system has an unsigned\n64-bit time_t, the test will fail (it will actually produce some value\n300 billion years from now).\n\nI'm inclined to include it as-is, as I do not know of any such system\n(and it would be kind of lame, since it could not represent dates before\n1970). If somebody comes up with such a system, we can allow either\noutput (we could do it preemptively, but somebody would have to\ncalculate the exact date/time billions of years in the future; we would\nbe just as likely to disagree with the system's gmtime about something\nsilly like leapseconds).\n\n pretty.c | 10 ++++++++--\n t/t4212-log-corrupt.sh | 16 ++++++++++++++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 87db08b..3b811ed 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -401,8 +401,14 @@ static const char *show_ident_date(const struct ident_split *ident,\n \n \tif (ident->date_begin && ident->date_end)\n \t\tdate = strtoul(ident->date_begin, NULL, 10);\n-\tif (ident->tz_begin && ident->tz_end)\n-\t\ttz = strtol(ident->tz_begin, NULL, 10);\n+\tif (date_overflows(date))\n+\t\tdate = 0;\n+\telse {\n+\t\tif (ident->tz_begin && ident->tz_end)\n+\t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n+\t\tif (tz == LONG_MAX || tz == LONG_MIN)\n+\t\t\ttz = 0;\n+\t}\n \treturn show_date(date, tz, mode);\n }\n \ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex 83de981..ba25a2e 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -65,4 +65,20 @@ test_expect_success 'unparsable dates produce sentinel value (%ad)' '\n \ttest_cmp expect actual\n '\n \n+# date is 2^64 + 1\n+test_expect_success 'date parser recognizes integer overflow' '\n+\tcommit=$(munge_author_date HEAD 18446744073709551617) &&\n+\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n+\tgit log -1 --format=%ad $commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# date is 2^64 - 2\n+test_expect_success 'date parser recognizes time_t overflow' '\n+\tcommit=$(munge_author_date HEAD 18446744073709551614) &&\n+\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n+\tgit log -1 --format=%ad $commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235225","messageId":"20140224074905.GE9969@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140224073348.GA20221@sigill.intra.peff.net","subject":"[PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T07:49:05Z","receivedAt":"2014-02-24T07:49:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many code paths assume that show_date and show_ident_date\ncannot return NULL. For the most part, we handle missing or\ncorrupt timestamps by showing the epoch time t=0.\n\nHowever, we might still return NULL if gmtime rejects the\ntime_t we feed it, resulting in a segfault. Let's catch this\ncase and just format t=0.\n\nSigned-off-by: Jeff King \n---\nThis test is of questionable portability, since we are depending on\ngmtime's arbitrary point to decide that our input is crazy and return\nNULL. The value is sufficiently large that I'd expect most to do so,\nthough, so it may be safe.\n\nOn 32-bit systems, of course, the test does nothing (it is just hitting\nthe integer overflow code path). But that's OK, since the output is the\nsame for both cases.\n\n date.c | 6 ++++--\n t/t4212-log-corrupt.sh | 8 ++++++++\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 90b28f7..e1a2cee 100644\n--- a/date.c\n+++ b/date.c\n@@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)\n \t\ttz = local_tzoffset(time);\n \n \ttm = time_to_tm(time, tz);\n-\tif (!tm)\n-\t\treturn NULL;\n+\tif (!tm) {\n+\t\ttm = time_to_tm(0, 0);\n+\t\ttz = 0;\n+\t}\n \n \tstrbuf_reset(&timebuf);\n \tif (mode == DATE_SHORT)\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex ba25a2e..3fa1715 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -81,4 +81,12 @@ test_expect_success 'date parser recognizes time_t overflow' '\n \ttest_cmp expect actual\n '\n \n+# date is within 2^63-1, but enough to choke glibc's gmtime\n+test_expect_success 'absurdly far-in-future dates produce sentinel' '\n+\tcommit=$(munge_author_date HEAD 999999999999999999) &&\n+\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n+\tgit log -1 --format=%ad $commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235285","messageId":"xmqqsir88f4n.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140224074637.GD9969@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] log: handle integer overflow in timestamps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T19:50:00Z","receivedAt":"2014-02-24T19:50:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If an ident line has a ridiculous date value like (2^64)+1,\n> we currently just pass ULONG_MAX along to the date code,\n> which can produce nonsensical dates.\n>\n> On systems with a signed long time_t (e.g., 64-bit glibc\n> systems), this actually doesn't end up too bad. The\n> ULONG_MAX is converted to -1, we apply the timezone field to\n> that, and the result ends up somewhere between Dec 31, 1969\n> and Jan 1, 1970.\n> ...\n> We also recognize overflow in the timezone field, which\n> could produce nonsensical results. In this case we show the\n> parsed date, but in UTC.\n\nBoth are good measures to fallback to sanity, but why is that\nif/else? In other words...\n\n> +\tif (date_overflows(date))\n> +\t\tdate = 0;\n> +\telse {\n> +\t\tif (ident->tz_begin && ident->tz_end)\n> +\t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n> +\t\tif (tz == LONG_MAX || tz == LONG_MIN)\n> +\t\t\ttz = 0;\n> +\t}\n\n... don't we want to fix an input having a bogus timestamp and also\na bogus tz recorded in it?\n\n> \treturn show_date(date, tz, mode);\n> }\n> \n> diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\n> index 83de981..ba25a2e 100755\n> --- a/t/t4212-log-corrupt.sh\n> +++ b/t/t4212-log-corrupt.sh\n> @@ -65,4 +65,20 @@ test_expect_success 'unparsable dates produce sentinel value (%ad)' '\n> \ttest_cmp expect actual\n> '\n> \n> +# date is 2^64 + 1\n> +test_expect_success 'date parser recognizes integer overflow' '\n> +\tcommit=$(munge_author_date HEAD 18446744073709551617) &&\n> +\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n> +\tgit log -1 --format=%ad $commit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +# date is 2^64 - 2\n> +test_expect_success 'date parser recognizes time_t overflow' '\n> +\tcommit=$(munge_author_date HEAD 18446744073709551614) &&\n> +\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n> +\tgit log -1 --format=%ad $commit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"235287","messageId":"20140224195829.GA11940@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqqsir88f4n.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/5] log: handle integer overflow in timestamps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T19:58:29Z","receivedAt":"2014-02-24T19:58:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 24, 2014 at 11:50:00AM -0800, Junio C Hamano wrote:\n\n> > We also recognize overflow in the timezone field, which\n> > could produce nonsensical results. In this case we show the\n> > parsed date, but in UTC.\n> \n> Both are good measures to fallback to sanity, but why is that\n> if/else? In other words...\n> \n> > +\tif (date_overflows(date))\n> > +\t\tdate = 0;\n> > +\telse {\n> > +\t\tif (ident->tz_begin && ident->tz_end)\n> > +\t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n> > +\t\tif (tz == LONG_MAX || tz == LONG_MIN)\n> > +\t\t\ttz = 0;\n> > +\t}\n> \n> ... don't we want to fix an input having a bogus timestamp and also\n> a bogus tz recorded in it?\n\nIf there is a bogus timestamp, then we do not want to look at tz at all.\nWe leave it at \"0\", so that we get a true sentinel:\n\n Thu Jan 1 00:00:00 1970 +0000\n\nand not:\n\n Wed Dec 31 19:00:00 1969 -0500\n\nIf the timestamp is valid, then we bother to parse the zone, and handle\noverflow there.\n\n-Peff\n"},{"id":"235292","messageId":"xmqqeh2s8do2.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140224195829.GA11940@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] log: handle integer overflow in timestamps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T20:21:33Z","receivedAt":"2014-02-24T20:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 24, 2014 at 11:50:00AM -0800, Junio C Hamano wrote:\n>\n>> > We also recognize overflow in the timezone field, which\n>> > could produce nonsensical results. In this case we show the\n>> > parsed date, but in UTC.\n>> \n>> Both are good measures to fallback to sanity, but why is that\n>> if/else? In other words...\n>> \n>> > +\tif (date_overflows(date))\n>> > +\t\tdate = 0;\n>> > +\telse {\n>> > +\t\tif (ident->tz_begin && ident->tz_end)\n>> > +\t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n>> > +\t\tif (tz == LONG_MAX || tz == LONG_MIN)\n>> > +\t\t\ttz = 0;\n>> > +\t}\n>> \n>> ... don't we want to fix an input having a bogus timestamp and also\n>> a bogus tz recorded in it?\n>\n> If there is a bogus timestamp, then we do not want to look at tz at all.\n> We leave it at \"0\", so that we get a true sentinel:\n\nAh, OK, I missed the initialization to 0 at the beginning.\n\nIt might have been more clear if \"int tz\" declaration were left\nuninitialized, and the variable were explicitly cleared to 0 in the\n\"date-overflows\" error codepath, but it is not a big deal.\n\nThanks for clarification.\n"},{"id":"235295","messageId":"20140224203709.GA25506@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqqeh2s8do2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/5] log: handle integer overflow in timestamps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T20:37:09Z","receivedAt":"2014-02-24T20:37:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 24, 2014 at 12:21:33PM -0800, Junio C Hamano wrote:\n\n> >> > +\tif (date_overflows(date))\n> >> > +\t\tdate = 0;\n> >> > +\telse {\n> >> > +\t\tif (ident->tz_begin && ident->tz_end)\n> >> > +\t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n> >> > +\t\tif (tz == LONG_MAX || tz == LONG_MIN)\n> >> > +\t\t\ttz = 0;\n> >> > +\t}\n> >> \n> >> ... don't we want to fix an input having a bogus timestamp and also\n> >> a bogus tz recorded in it?\n> >\n> > If there is a bogus timestamp, then we do not want to look at tz at all.\n> > We leave it at \"0\", so that we get a true sentinel:\n> \n> Ah, OK, I missed the initialization to 0 at the beginning.\n> \n> It might have been more clear if \"int tz\" declaration were left\n> uninitialized, and the variable were explicitly cleared to 0 in the\n> \"date-overflows\" error codepath, but it is not a big deal.\n\nIt might be, but I think it would end up cumbersome. The initialization\nwas already there from the previous version, which was hitting the else\nfor \"ident->tz_begin\". Without fallback initializations, you end up with:\n\n if (ident->date_begin && ident->date_end) {\n date = strtoul(ident->date_begin, NULL, 10);\n if (date_overflows(date)) {\n date = 0;\n tz = 0;\n } else {\n if (ident->tz_begin && ident->tz_end) {\n tz = strtol(ident->tz_begin, NULL, 10);\n if (tz == LONG_MAX || tz == LONG_MIN)\n tz = 0;\n } else\n tz = 0;\n }\n } else {\n date = 0;\n tz = 0;\n }\n\nwhich I think is much more confusing (and hard to verify that the\nvariables are always set). Checking !date as an error condition would\nmake it a little more readable:\n\n if (ident->date_begin && ident->date_end) {\n date = strtoul(ident->date_begin, NULL, 10);\n if (date_overflows(date))\n date = 0;\n } else\n date = 0;\n\n if (date) {\n if (ident->tz_begin && ident->tz_end) {\n tz = strtol(ident->tz_begin, NULL, 10);\n if (tz == LONG_MAX || tz == LONG_MIN)\n tz = 0;\n } else\n tz = 0;\n } else\n tz = 0;\n\nbut then we treat date==0 as a sentinel, and can never correctly parse\ndates on Jan 1, 1970.\n\nSo I'd be in favor of keeping it as-is, but feel free to mark it up if\nyou feel strongly.\n\n-Peff\n"},{"id":"235299","messageId":"xmqqy5106x8a.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140224203709.GA25506@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] log: handle integer overflow in timestamps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T21:01:57Z","receivedAt":"2014-02-24T21:01:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 24, 2014 at 12:21:33PM -0800, Junio C Hamano wrote:\n>\n>> >> > +\tif (date_overflows(date))\n>> >> > +\t\tdate = 0;\n>> >> > +\telse {\n>> >> > +\t\tif (ident->tz_begin && ident->tz_end)\n>> >> > +\t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n>> >> > +\t\tif (tz == LONG_MAX || tz == LONG_MIN)\n>> >> > +\t\t\ttz = 0;\n>> >> > +\t}\n>> >> \n>> >> ... don't we want to fix an input having a bogus timestamp and also\n>> >> a bogus tz recorded in it?\n>> >\n>> > If there is a bogus timestamp, then we do not want to look at tz at all.\n>> > We leave it at \"0\", so that we get a true sentinel:\n>> \n>> Ah, OK, I missed the initialization to 0 at the beginning.\n>> \n>> It might have been more clear if \"int tz\" declaration were left\n>> uninitialized, and the variable were explicitly cleared to 0 in the\n>> \"date-overflows\" error codepath, but it is not a big deal.\n>\n> It might be, but I think it would end up cumbersome.\n> ...\n> So I'd be in favor of keeping it as-is, but feel free to mark it up if\n> you feel strongly.\n\nI'd be in favor of keeping it as-is.\n"},{"id":"237387","messageId":"532D58B5.1080309@web.de","threadId":"35943","inReplyTo":"20140224074905.GE9969@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-03-22T09:32:37Z","receivedAt":"2014-03-22T09:32:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.02.2014 08:49, schrieb Jeff King:\n> Many code paths assume that show_date and show_ident_date\n> cannot return NULL. For the most part, we handle missing or\n> corrupt timestamps by showing the epoch time t=0.\n>\n> However, we might still return NULL if gmtime rejects the\n> time_t we feed it, resulting in a segfault. Let's catch this\n> case and just format t=0.\n>\n> Signed-off-by: Jeff King \n> ---\n> This test is of questionable portability, since we are depending on\n> gmtime's arbitrary point to decide that our input is crazy and return\n> NULL. The value is sufficiently large that I'd expect most to do so,\n> though, so it may be safe.\n\nJust came around to testing on FreeBSD 10 amd64; the new test in t4212 \nfails there:\n\n--- expect 2014-03-22 08:29:44.000000000 +0000\n+++ actual 2014-03-22 08:29:44.000000000 +0000\n@@ -1 +1 @@\n-Thu Jan 1 00:00:00 1970 +0000\n+Sun Jan 0 00:00:00 1900 -0700\nnot ok 9 - absurdly far-in-future dates produce sentinel\n#\n# commit=$(munge_author_date HEAD 999999999999999999) &&\n# echo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n# git log -1 --format=%ad $commit >actual &&\n# test_cmp expect actual\n#\n\n# failed 1 among 9 test(s)\n\nLooks like we get a cleared struct tm instead of a NULL pointer. It \nseems to be a long-standing bug; \nhttp://www.freebsd.org/cgi/query-pr.cgi?pr=kern/145341 was filed in \nApril 2010.\n\n> On 32-bit systems, of course, the test does nothing (it is just hitting\n> the integer overflow code path). But that's OK, since the output is the\n> same for both cases.\n>\n> date.c | 6 ++++--\n> t/t4212-log-corrupt.sh | 8 ++++++++\n> 2 files changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/date.c b/date.c\n> index 90b28f7..e1a2cee 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)\n> \t\ttz = local_tzoffset(time);\n>\n> \ttm = time_to_tm(time, tz);\n> -\tif (!tm)\n> -\t\treturn NULL;\n> +\tif (!tm) {\n\nWould it make sense to work around the FreeBSD issue by adding a check \nlike this?\n\n\tif (!tm || tm->tm_year < 70) {\n\n> +\t\ttm = time_to_tm(0, 0);\n> +\t\ttz = 0;\n> +\t}\n>\n> \tstrbuf_reset(&timebuf);\n> \tif (mode == DATE_SHORT)\n> diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\n> index ba25a2e..3fa1715 100755\n> --- a/t/t4212-log-corrupt.sh\n> +++ b/t/t4212-log-corrupt.sh\n> @@ -81,4 +81,12 @@ test_expect_success 'date parser recognizes time_t overflow' '\n> \ttest_cmp expect actual\n> '\n>\n> +# date is within 2^63-1, but enough to choke glibc's gmtime\n> +test_expect_success 'absurdly far-in-future dates produce sentinel' '\n> +\tcommit=$(munge_author_date HEAD 999999999999999999) &&\n> +\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n> +\tgit log -1 --format=%ad $commit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n>\n"},{"id":"237498","messageId":"20140324213342.GE13728@sigill.intra.peff.net","threadId":"35943","inReplyTo":"532D58B5.1080309@web.de","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-24T21:33:43Z","receivedAt":"2014-03-24T21:33:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 22, 2014 at 10:32:37AM +0100, René Scharfe wrote:\n\n> >This test is of questionable portability, since we are depending on\n> >gmtime's arbitrary point to decide that our input is crazy and return\n> >NULL. The value is sufficiently large that I'd expect most to do so,\n> >though, so it may be safe.\n> \n> Just came around to testing on FreeBSD 10 amd64; the new test in t4212 fails\n> there:\n\nThanks for the report. I don't think the problem is in the test here,\nbut rather that we should do a more thorough job of detecting gmtime's\n\"I don't know what to do with this\" response.\n\n> >@@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)\n> > \t\ttz = local_tzoffset(time);\n> >\n> > \ttm = time_to_tm(time, tz);\n> >-\tif (!tm)\n> >-\t\treturn NULL;\n> >+\tif (!tm) {\n> \n> Would it make sense to work around the FreeBSD issue by adding a check like\n> this?\n> \n> \tif (!tm || tm->tm_year < 70) {\n\nThat feels like a bit of a maintenance headache. Right now we do not\ninternally represent times prior to the epoch, since we mostly use\n\"unsigned long\" through the code. So anything < 70 has to be an error.\nBut it would be nice one day to consistently use a 64-bit signed time_t\nthroughout git, and represent even ancient timestamps (e.g., for people\ndoing historical projects, like importing laws into git). This would set\nquite a trap for people working later on the code.\n\nIf the result is all-zeroes, can we check for that case instead? I\nsuppose that will eventually create a \"trap\" at midnight on January 1st\nof the year 0 (though I am not sure such a date is even meaningful,\ngiven the history of our calendars).\n\n-Peff\n"},{"id":"237510","messageId":"5330ABBE.3080106@web.de","threadId":"35943","inReplyTo":"20140324213342.GE13728@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-03-24T22:03:42Z","receivedAt":"2014-03-24T22:03:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.03.2014 22:33, schrieb Jeff King:\n> On Sat, Mar 22, 2014 at 10:32:37AM +0100, René Scharfe wrote:\n>>> @@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)\n>>> \t\ttz = local_tzoffset(time);\n>>>\n>>> \ttm = time_to_tm(time, tz);\n>>> -\tif (!tm)\n>>> -\t\treturn NULL;\n>>> +\tif (!tm) {\n>>\n>> Would it make sense to work around the FreeBSD issue by adding a check like\n>> this?\n>>\n>> \tif (!tm || tm->tm_year < 70) {\n>\n> That feels like a bit of a maintenance headache. Right now we do not\n> internally represent times prior to the epoch, since we mostly use\n> \"unsigned long\" through the code. So anything < 70 has to be an error.\n> But it would be nice one day to consistently use a 64-bit signed time_t\n> throughout git, and represent even ancient timestamps (e.g., for people\n> doing historical projects, like importing laws into git). This would set\n> quite a trap for people working later on the code.\n>\n> If the result is all-zeroes, can we check for that case instead? I\n> suppose that will eventually create a \"trap\" at midnight on January 1st\n> of the year 0 (though I am not sure such a date is even meaningful,\n> given the history of our calendars).\n\nMakes sense. That would be \"Sun Jan 0 00:00:00 1900\", however -- days \nare 1-based and years 1900-based in struct tm. Since a zeroth day is \ninvalid, would this suffice:\n\n\tif (!tm || !tm->tm_mday) {\n\n(Yes, I'm lazy. :)\n\nRené\n"},{"id":"237514","messageId":"20140324221137.GJ13728@sigill.intra.peff.net","threadId":"35943","inReplyTo":"5330ABBE.3080106@web.de","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-24T22:11:37Z","receivedAt":"2014-03-24T22:11:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 24, 2014 at 11:03:42PM +0100, René Scharfe wrote:\n\n> >If the result is all-zeroes, can we check for that case instead? I\n> >suppose that will eventually create a \"trap\" at midnight on January 1st\n> >of the year 0 (though I am not sure such a date is even meaningful,\n> >given the history of our calendars).\n> \n> Makes sense. That would be \"Sun Jan 0 00:00:00 1900\", however -- days are\n> 1-based and years 1900-based in struct tm.\n\nOh right, I was stupidly forgetting about being 1900-based.\n\n> Since a zeroth day is invalid, would this suffice:\n>\n> \tif (!tm || !tm->tm_mday) {\n> \n> (Yes, I'm lazy. :)\n\nThat looks perfect (and I like that it is quick to check, too, since\nthis case should be extremely rare).\n\n-Peff\n"},{"id":"237795","messageId":"20140326110559.GA32625@hashpling.org","threadId":"35943","inReplyTo":"20140224074905.GE9969@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Charles Bailey","fromEmail":"cbailey32@bloomberg.net","sentAt":"2014-03-26T11:05:59Z","receivedAt":"2014-03-26T11:05:59Z","isPatch":true,"sender":{"key":"cbailey32@bloomberg.net","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote:\n> +# date is within 2^63-1, but enough to choke glibc's gmtime\n> +test_expect_success 'absurdly far-in-future dates produce sentinel' '\n> +\tcommit=$(munge_author_date HEAD 999999999999999999) &&\n> +\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n> +\tgit log -1 --format=%ad $commit >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nGit on AIX seems happy to convert this to Thu Oct 24 18:46:39\n162396404 -0700. I don't know if this is correct for the given input\nbut the test fails.\n\nCharles.\n"},{"id":"237823","messageId":"20140326182103.GB7087@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140326110559.GA32625@hashpling.org","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T18:21:03Z","receivedAt":"2014-03-26T18:21:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 11:05:59AM +0000, Charles Bailey wrote:\n\n> On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote:\n> > +# date is within 2^63-1, but enough to choke glibc's gmtime\n> > +test_expect_success 'absurdly far-in-future dates produce sentinel' '\n> > +\tcommit=$(munge_author_date HEAD 999999999999999999) &&\n> > +\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n> > +\tgit log -1 --format=%ad $commit >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> Git on AIX seems happy to convert this to Thu Oct 24 18:46:39\n> 162396404 -0700. I don't know if this is correct for the given input\n> but the test fails.\n\nIck. I am not sure that dates at that range are even meaningful (will\nOctober really exist in the year 162396404? Did they account for all the\nleap-seconds between then and now?). But at the same time, I cannot\nfault their gmtime for just extrapolating the current rules out as far\nas we ask them to.\n\nUnlike the FreeBSD thing that René brought up, this is not a problem in\nthe code, but just in the test. So I think our options are basically:\n\n 1. Scrap the test as unportable.\n\n 2. Hard-code a few expected values. I'd be unsurprised if some other\n system comes up with a slightly different date in 162396404, so we\n may end up needing several of these.\n\nI think I'd lean towards (2), just because it is testing an actual\nfeature of the code, and I'd like to continue doing so. And while we may\nend up with a handful of values, there's probably not _that_ many\nindependent implementations of gmtime in the wild.\n\n-Peff\n"},{"id":"237829","messageId":"20140326185153.GA12912@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140326182103.GB7087@sigill.intra.peff.net","subject":"[PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T18:51:53Z","receivedAt":"2014-03-26T18:51:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of the tests in t4212 checks our behavior when we feed\ngmtime a date so far in the future that it gives up and\nreturns NULL.\n\nBut some gmtime implementations just refuse to quit. They\nsoldier on, giving us a glimpse of a chilly October evening\nsome 160 million years in the future (and presumably filled\nwith our leather-clad horseback-riding ape descendants).\n\nLet's allow the test to match either the sentinel value\n(i.e., what we want when gmtime gives up) or any reasonable\nvalue returned by known implementations.\n\nReported-by: Charles Bailey \nSigned-off-by: Jeff King \n---\nOn top of jk/commit-dates-parsing-fix (though the test is already in\nv1.9.1).\n\n t/t4212-log-corrupt.sh | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex 85c6df4..08f982c 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -76,12 +76,27 @@ test_expect_success 'date parser recognizes time_t overflow' '\n \ttest_cmp expect actual\n '\n \n-# date is within 2^63-1, but enough to choke glibc's gmtime\n-test_expect_success 'absurdly far-in-future dates produce sentinel' '\n+cmp_one_of () {\n+\tfor candidate in \"$@\"; do\n+\t\techo \"$candidate\" >expect &&\n+\t\ttest_cmp expect actual &&\n+\t\treturn 0\n+\tdone\n+\treturn 1\n+}\n+\n+# date is within 2^63-1, but enough to choke glibc's gmtime.\n+# We check that either the date broke gmtime (and we return the\n+# usual epoch value), or gmtime gave us some sensible value.\n+#\n+# The sensible values are determined experimentally. The first\n+# is from AIX.\n+test_expect_success 'absurdly far-in-future dates' '\n \tcommit=$(munge_author_date HEAD 999999999999999999) &&\n-\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n \tgit log -1 --format=%ad $commit >actual &&\n-\ttest_cmp expect actual\n+\tcmp_one_of \\\n+\t\t\"Thu Jan 1 00:00:00 1970 +0000\" \\\n+\t\t\"Thu Oct 24 18:46:39 162396404 -0700\"\n '\n \n test_done\n-- \n1.9.1.656.ge8a0637\n"},{"id":"237830","messageId":"xmqqvbv0iy6u.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140326182103.GB7087@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-26T18:58:49Z","receivedAt":"2014-03-26T18:58:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 26, 2014 at 11:05:59AM +0000, Charles Bailey wrote:\n>\n>> On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote:\n>> > +# date is within 2^63-1, but enough to choke glibc's gmtime\n>> > +test_expect_success 'absurdly far-in-future dates produce sentinel' '\n>> > +\tcommit=$(munge_author_date HEAD 999999999999999999) &&\n>> > +\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n>> > +\tgit log -1 --format=%ad $commit >actual &&\n>> > +\ttest_cmp expect actual\n>> > +'\n>> \n>> Git on AIX seems happy to convert this to Thu Oct 24 18:46:39\n>> 162396404 -0700. I don't know if this is correct for the given input\n>> but the test fails.\n>\n> Ick. I am not sure that dates at that range are even meaningful (will\n> October really exist in the year 162396404? Did they account for all the\n> leap-seconds between then and now?). But at the same time, I cannot\n> fault their gmtime for just extrapolating the current rules out as far\n> as we ask them to.\n>\n> Unlike the FreeBSD thing that René brought up, this is not a problem in\n> the code, but just in the test. So I think our options are basically:\n>\n> 1. Scrap the test as unportable.\n>\n> 2. Hard-code a few expected values. I'd be unsurprised if some other\n> system comes up with a slightly different date in 162396404, so we\n> may end up needing several of these.\n>\n> I think I'd lean towards (2), just because it is testing an actual\n> feature of the code, and I'd like to continue doing so. And while we may\n> end up with a handful of values, there's probably not _that_ many\n> independent implementations of gmtime in the wild.\n\nOr \"3. Just make sure that 'git log' does not segfault\"?\n"},{"id":"237831","messageId":"20140326190157.GB12912@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqqvbv0iy6u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T19:01:58Z","receivedAt":"2014-03-26T19:01:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote:\n\n> > Unlike the FreeBSD thing that René brought up, this is not a problem in\n> > the code, but just in the test. So I think our options are basically:\n> >\n> > 1. Scrap the test as unportable.\n> >\n> > 2. Hard-code a few expected values. I'd be unsurprised if some other\n> > system comes up with a slightly different date in 162396404, so we\n> > may end up needing several of these.\n> >\n> > I think I'd lean towards (2), just because it is testing an actual\n> > feature of the code, and I'd like to continue doing so. And while we may\n> > end up with a handful of values, there's probably not _that_ many\n> > independent implementations of gmtime in the wild.\n> \n> Or \"3. Just make sure that 'git log' does not segfault\"?\n\nThat would not test the FreeBSD case, which does not segfault, but\nreturns a bogus sentinel value.\n\nI don't know how important that is. This is such a minor feature that it\nis not worth a lot of maintenance headache in the test. But I also do\nnot know if this is going to be the last report, or we will have a bunch\nof other systems that need their own special values put into the test.\n\n-Peff\n"},{"id":"237834","messageId":"xmqqr45oixa6.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140326185153.GA12912@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-26T19:18:25Z","receivedAt":"2014-03-26T19:18:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +cmp_one_of () {\n> +\tfor candidate in \"$@\"; do\n\nStyle ;-)\n\n> +\t\techo \"$candidate\" >expect &&\n> +\t\ttest_cmp expect actual &&\n> +\t\treturn 0\n> +\tdone\n> +\treturn 1\n> +}\n\nIt actually may be easier to understand if you write a trivial case\nstatement at the sole calling site of this helper function, though.\n\nIn any case, would it really be essential to make sure that the\noutput shows a phony (or a seemingly real) datestamp for this test?\n\nThe primary thing you wanted to achieve by the \"gmtime gave us NULL,\nlet's substitute it with an arbitrary value to avoid dereferencing\nthe NULL\" change was *not* that we see that same arbitrary value\ncomes out of the system, but that we do not die by attempting to\nreference the NULL, I think. Not dying is the primary thing we want\nto (and we already do) test, no?\n\n> +# date is within 2^63-1, but enough to choke glibc's gmtime.\n> +# We check that either the date broke gmtime (and we return the\n> +# usual epoch value), or gmtime gave us some sensible value.\n> +#\n> +# The sensible values are determined experimentally. The first\n> +# is from AIX.\n> +test_expect_success 'absurdly far-in-future dates' '\n> \tcommit=$(munge_author_date HEAD 999999999999999999) &&\n> -\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n> \tgit log -1 --format=%ad $commit >actual &&\n> -\ttest_cmp expect actual\n> +\tcmp_one_of \\\n> +\t\t\"Thu Jan 1 00:00:00 1970 +0000\" \\\n> +\t\t\"Thu Oct 24 18:46:39 162396404 -0700\"\n> '\n> \n> test_done\n"},{"id":"237837","messageId":"20140326192536.GA13989@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqqr45oixa6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T19:25:36Z","receivedAt":"2014-03-26T19:25:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 12:18:25PM -0700, Junio C Hamano wrote:\n\n> > +\t\techo \"$candidate\" >expect &&\n> > +\t\ttest_cmp expect actual &&\n> > +\t\treturn 0\n> > +\tdone\n> > +\treturn 1\n> > +}\n> \n> It actually may be easier to understand if you write a trivial case\n> statement at the sole calling site of this helper function, though.\n\nYeah, perhaps. I wanted to build on test_cmp because it has nice output\nfor the failure case, but it is probably simple enough to do:\n\n output=$(cat actual)\n case \"$output\" in\n ...\n *) echo >&2 \"unrecognized date: $output\"\n\n> In any case, would it really be essential to make sure that the\n> output shows a phony (or a seemingly real) datestamp for this test?\n> \n> The primary thing you wanted to achieve by the \"gmtime gave us NULL,\n> let's substitute it with an arbitrary value to avoid dereferencing\n> the NULL\" change was *not* that we see that same arbitrary value\n> comes out of the system, but that we do not die by attempting to\n> reference the NULL, I think. Not dying is the primary thing we want\n> to (and we already do) test, no?\n\nI think there are really two separate behaviors we are testing here (and\nin the surrounding tests):\n\n 1. Don't segfault if gmtime returns NULL.\n\n 2. Whenever we cannot process a date (either because gmtime fails, or\n because we fail before even getting the value to gmtime),\n consistently return the sentinel date (so the reader can easily\n know it's bogus).\n\nHaving the test be particular about its output helped us find a case\nwhere FreeBSD did not trigger (1), but did trigger (2), by returning a\nblanked \"struct tm\".\n\nI'm open to the argument that (2) is not worth worrying about that much\nif it is a hassle to test. But I don't think it is that much hassle\n(yet, anyway).\n\n-Peff\n"},{"id":"237838","messageId":"20140326193359.GA14105@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140326192536.GA13989@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T19:33:59Z","receivedAt":"2014-03-26T19:33:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 03:25:36PM -0400, Jeff King wrote:\n\n> > The primary thing you wanted to achieve by the \"gmtime gave us NULL,\n> > let's substitute it with an arbitrary value to avoid dereferencing\n> > the NULL\" change was *not* that we see that same arbitrary value\n> > comes out of the system, but that we do not die by attempting to\n> > reference the NULL, I think. Not dying is the primary thing we want\n> > to (and we already do) test, no?\n> \n> I think there are really two separate behaviors we are testing here (and\n> in the surrounding tests):\n> \n> 1. Don't segfault if gmtime returns NULL.\n> \n> 2. Whenever we cannot process a date (either because gmtime fails, or\n> because we fail before even getting the value to gmtime),\n> consistently return the sentinel date (so the reader can easily\n> know it's bogus).\n> \n> Having the test be particular about its output helped us find a case\n> where FreeBSD did not trigger (1), but did trigger (2), by returning a\n> blanked \"struct tm\".\n> \n> I'm open to the argument that (2) is not worth worrying about that much\n> if it is a hassle to test. But I don't think it is that much hassle\n> (yet, anyway).\n\nThat being said, is the AIX value actually right? I did not look closely\nat first, but just assumed that it was vaguely right. But:\n\n 999999999999999999 / (86400 * 365)\n\nis something like 31 billion years in the future, not 160 million.\nA real date calculation will have a few tweaks (leap years, etc), but\nthat is orders of magnitude off.\n\nSo I am not sure that AIX is not actually just giving us utter crap. In\nthat case, the test is not wrong; it's pickiness is actually finding a\nreal problem. But I am not sure it is a problem worth solving. I do not\nwant to get into heuristics deciding whether a particular platform's\ngmtime output is crap or not. That pushes this into the realm of \"it's\nnot worth testing\", and we should stick to just testing that we did not\nsegfault.\n\n-Peff\n"},{"id":"237839","messageId":"20140326194043.GB14105@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140326193359.GA14105@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T19:40:43Z","receivedAt":"2014-03-26T19:40:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote:\n\n> That being said, is the AIX value actually right? I did not look closely\n> at first, but just assumed that it was vaguely right. But:\n> \n> 999999999999999999 / (86400 * 365)\n> \n> is something like 31 billion years in the future, not 160 million.\n> A real date calculation will have a few tweaks (leap years, etc), but\n> that is orders of magnitude off.\n\nAssuming my math is right, then here is the most sensible patch, IMHO.\n\n-- >8 --\nSubject: t4212: loosen far-in-future test for AIX\n\nOne of the tests in t4212 checks our behavior when we feed\ngmtime a date so far in the future that it gives up and\nreturns NULL. Some implementations, like AIX, may actually\njust provide us a bogus result instead.\n\nIt's not worth it for us to come up with heuristics that\nguess whether the return value is sensible or not. On good\nplatforms where gmtime reports the problem to us with NULL,\nwe will print the epoch value. On bad platforms, we will\nprint garbage. But our test should be written for the\nlowest common denominator so that it passes everywhere.\n\nReported-by: Charles Bailey \nSigned-off-by: Jeff King \n---\n t/t4212-log-corrupt.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex 85c6df4..bb843ab 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -77,11 +77,14 @@ test_expect_success 'date parser recognizes time_t overflow' '\n '\n \n # date is within 2^63-1, but enough to choke glibc's gmtime\n-test_expect_success 'absurdly far-in-future dates produce sentinel' '\n+#\n+# Ideally we would check the output to make sure we replaced it with\n+# a useful sentinel value, but some platforms will actually hand us back\n+# a nonsensical date. It is not worth our time to try to evaluate these\n+# dates, so just make sure we didn't segfault or otherwise abort.\n+test_expect_success 'absurdly far-in-future dates' '\n \tcommit=$(munge_author_date HEAD 999999999999999999) &&\n-\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n-\tgit log -1 --format=%ad $commit >actual &&\n-\ttest_cmp expect actual\n+\tgit log -1 --format=%ad $commit\n '\n \n test_done\n-- \n1.9.1.656.ge8a0637\n"},{"id":"237844","messageId":"20140326203618.GA6991@hashpling.org","threadId":"35943","inReplyTo":"20140326194043.GB14105@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Charles Bailey","fromEmail":"cbailey32@bloomberg.net","sentAt":"2014-03-26T20:36:18Z","receivedAt":"2014-03-26T20:36:18Z","isPatch":true,"sender":{"key":"cbailey32@bloomberg.net","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Mar 26, 2014 at 03:40:43PM -0400, Jeff King wrote:\n> On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote:\n> \n> > That being said, is the AIX value actually right? I did not look closely\n> > at first, but just assumed that it was vaguely right. But:\n> > \n> > 999999999999999999 / (86400 * 365)\n> > \n> > is something like 31 billion years in the future, not 160 million.\n> > A real date calculation will have a few tweaks (leap years, etc), but\n> > that is orders of magnitude off.\n> \n> Assuming my math is right, then here is the most sensible patch, IMHO.\n> \n\nPerhaps hold onto this one for a little while longer. Splitting things\nout from the test is giving me some inconsistent results, there may be\nsomething else going wrong in our environment here.\n\nCharles.\n"},{"id":"237845","messageId":"20140326203830.GA16332@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140326203618.GA6991@hashpling.org","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T20:38:30Z","receivedAt":"2014-03-26T20:38:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 08:36:18PM +0000, Charles Bailey wrote:\n\n> On Wed, Mar 26, 2014 at 03:40:43PM -0400, Jeff King wrote:\n> > On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote:\n> > \n> > > That being said, is the AIX value actually right? I did not look closely\n> > > at first, but just assumed that it was vaguely right. But:\n> > > \n> > > 999999999999999999 / (86400 * 365)\n> > > \n> > > is something like 31 billion years in the future, not 160 million.\n> > > A real date calculation will have a few tweaks (leap years, etc), but\n> > > that is orders of magnitude off.\n> > \n> > Assuming my math is right, then here is the most sensible patch, IMHO.\n> > \n> \n> Perhaps hold onto this one for a little while longer. Splitting things\n> out from the test is giving me some inconsistent results, there may be\n> something else going wrong in our environment here.\n\nBy the way, can you confirm that this is a 64-bit system? On a 32-bit\nsystem, we should be triggering different code paths (we fail at the\nstrtoul level). Those should be checked by the previous tests, but I'd\nlike to make sure.\n\n-Peff\n"},{"id":"237846","messageId":"20140326204110.GB6991@hashpling.org","threadId":"35943","inReplyTo":"20140326203830.GA16332@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Charles Bailey","fromEmail":"cbailey32@bloomberg.net","sentAt":"2014-03-26T20:41:10Z","receivedAt":"2014-03-26T20:41:10Z","isPatch":true,"sender":{"key":"cbailey32@bloomberg.net","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Mar 26, 2014 at 04:38:30PM -0400, Jeff King wrote:\n> \n> By the way, can you confirm that this is a 64-bit system? On a 32-bit\n> system, we should be triggering different code paths (we fail at the\n> strtoul level). Those should be checked by the previous tests, but I'd\n> like to make sure.\n\nYes, we're only building 64-bit at the moment.\n"},{"id":"237847","messageId":"xmqqeh1oisim.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140326190157.GB12912@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-26T21:01:21Z","receivedAt":"2014-03-26T21:01:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote:\n>\n>> > Unlike the FreeBSD thing that René brought up, this is not a problem in\n>> > the code, but just in the test. So I think our options are basically:\n>> >\n>> > 1. Scrap the test as unportable.\n>> >\n>> > 2. Hard-code a few expected values. I'd be unsurprised if some other\n>> > system comes up with a slightly different date in 162396404, so we\n>> > may end up needing several of these.\n>> >\n>> > I think I'd lean towards (2), just because it is testing an actual\n>> > feature of the code, and I'd like to continue doing so. And while we may\n>> > end up with a handful of values, there's probably not _that_ many\n>> > independent implementations of gmtime in the wild.\n>> \n>> Or \"3. Just make sure that 'git log' does not segfault\"?\n>\n> That would not test the FreeBSD case, which does not segfault, but\n> returns a bogus sentinel value.\n>\n> I don't know how important that is. This is such a minor feature that it\n> is not worth a lot of maintenance headache in the test. But I also do\n> not know if this is going to be the last report, or we will have a bunch\n> of other systems that need their own special values put into the test.\n\nI didn't quite realize that your objective of the change was to\nsignal the failure of gmtime for all implementations, i.e. both (1)\nthe ones that signal an error by giving NULL back to us and (2) the\nones that fail to signal an error but leave bogus result (FreeBSD,\nbut it appears AIX might be also giving us another bogus value), by\nusing a single sane sentinel value. If we can do that consistently\non every platform, that would indeed be great.\n\nBut if that is the case, we should not have to maintain special case\nvalues in the test code, I would think. The test instead should\nexpect the output to have that single sentinel value, and if the\nworkaround code fails to detect a breakage in the platform gmtime(),\nthe special case logic to catch these platform-specific breakages\nshould go that \"timestamp that cannot be grokked by gmtime---replace\nit with a sentinel\" logic, no?\n"},{"id":"237849","messageId":"20140326210929.GA16581@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqqeh1oisim.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/5] log: do not segfault on gmtime errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T21:09:29Z","receivedAt":"2014-03-26T21:09:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 02:01:21PM -0700, Junio C Hamano wrote:\n\n> > I don't know how important that is. This is such a minor feature that it\n> > is not worth a lot of maintenance headache in the test. But I also do\n> > not know if this is going to be the last report, or we will have a bunch\n> > of other systems that need their own special values put into the test.\n> \n> I didn't quite realize that your objective of the change was to\n> signal the failure of gmtime for all implementations,\n\nI didn't realize it at the time I wrote the test, either. It was my goal\nto recognize such failures, but I didn't now at the time that there were\nfailures that would be signaled by anything besides a NULL return.\n\n> the ones that signal an error by giving NULL back to us and (2) the\n> ones that fail to signal an error but leave bogus result (FreeBSD,\n> but it appears AIX might be also giving us another bogus value), by\n> using a single sane sentinel value. If we can do that consistently\n> on every platform, that would indeed be great.\n\nAgreed. I am not sure we can do so. For FreeBSD, I think it is not hard.\nI am not sure what the pattern is for AIX. I am hoping Charles will\nreveal some pattern, but there may not be one.\n\n> But if that is the case, we should not have to maintain special case\n> values in the test code, I would think.\n\nRight. All of my suggestions to accommodate special values in the test\nwere assuming that the system gmtime was smarter than we are. That it\nproduced a good value for this crazy time and _didn't_ fail.\n\nBut after having done the basic math, I don't think that is what is\ngoing on here.\n\n> The test instead should expect the output to have that single sentinel\n> value, and if the workaround code fails to detect a breakage in the\n> platform gmtime(), the special case logic to catch these\n> platform-specific breakages should go that \"timestamp that cannot be\n> grokked by gmtime---replace it with a sentinel\" logic, no?\n\nYes, exactly. That is the preferable solution, if we can come up with\nsuch logic. Personally I am not optimistic.\n\nThe fallback is to accept that we cannot cover all cases, and just\nloosen the test (i.e., the second patch I posted today).\n\n-Peff\n"},{"id":"237851","messageId":"20140326212227.GC6991@hashpling.org","threadId":"35943","inReplyTo":"20140326193359.GA14105@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Charles Bailey","fromEmail":"cbailey32@bloomberg.net","sentAt":"2014-03-26T21:22:27Z","receivedAt":"2014-03-26T21:22:27Z","isPatch":true,"sender":{"key":"cbailey32@bloomberg.net","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote:\n> \n> That being said, is the AIX value actually right? I did not look closely\n> at first, but just assumed that it was vaguely right. But:\n> \n> 999999999999999999 / (86400 * 365)\n> \n> is something like 31 billion years in the future, not 160 million.\n> A real date calculation will have a few tweaks (leap years, etc), but\n> that is orders of magnitude off.\n\nWell, this is embarrassing, while moving this through the corporate\nfirewall (aka typing on one machine while looking at another), I\nmunged the date. It still doesn't seem right but at least you can now\nsee the actual data.\n\nI stopped the test with --immediate and found the dangling commit that\nthe test created and dumped it with the previous version of Git (well\na 1.8.5.5 build)\n\n ibm: trash directory.t4212-log-corrupt $ git log -1 --pretty=raw 1fc17e734e4487c31bdfe05bb3d15618b69c4dca\n commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca\n tree 64fd3796c57084e7b8cbae358ce37970b8e954f6\n author A U Thor 999999999999999999 -0700\n committer C O Mitter 1112911993 -0700\n\n foo\n\n ibm: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca\n commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca\n Author: A U Thor \n Date: Thu Oct 24 18:46:39 1623969404 -0700\n\n foo\n\nSame commit but dumped from a linux machine:\n\n linux: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca\n commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca\n Author: A U Thor \n Date: (null)\n\n foo\n\nCharles.\n"},{"id":"237855","messageId":"20140326215741.GA17716@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140326212227.GC6991@hashpling.org","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-26T21:57:41Z","receivedAt":"2014-03-26T21:57:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 09:22:27PM +0000, Charles Bailey wrote:\n\n> On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote:\n> > \n> > That being said, is the AIX value actually right? I did not look closely\n> > at first, but just assumed that it was vaguely right. But:\n> > \n> > 999999999999999999 / (86400 * 365)\n> > \n> > is something like 31 billion years in the future, not 160 million.\n> > A real date calculation will have a few tweaks (leap years, etc), but\n> > that is orders of magnitude off.\n> \n> Well, this is embarrassing, while moving this through the corporate\n> firewall (aka typing on one machine while looking at another), I\n> munged the date. It still doesn't seem right but at least you can now\n> see the actual data.\n\nHmm, so the year you got is actually: 1623969404. That still seems off\nto me by a factor 20. I don't know if this is really worth digging into\nthat much further, but I wonder what you would get for timestamps of:\n\n 99999999999999999\n 9999999999999999\n 999999999999999\n etc.\n\nDo we start generating weird values at some particular size? Or is AIX\ngmtime really more clever than I am, and is accounting for wobble of the\nEarth or something over the next billion years?\n\n-Peff\n"},{"id":"237873","messageId":"20140326224616.GA9454@hashpling.org","threadId":"35943","inReplyTo":"20140326215741.GA17716@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Charles Bailey","fromEmail":"cbailey32@bloomberg.net","sentAt":"2014-03-26T22:46:16Z","receivedAt":"2014-03-26T22:46:16Z","isPatch":true,"sender":{"key":"cbailey32@bloomberg.net","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Mar 26, 2014 at 05:57:41PM -0400, Jeff King wrote:\n> Hmm, so the year you got is actually: 1623969404. That still seems off\n> to me by a factor 20. I don't know if this is really worth digging into\n> that much further, but I wonder what you would get for timestamps of:\n> \n> 99999999999999999\n> 9999999999999999\n> 999999999999999\n> etc.\n> \n\nAIX goes negative at about the same time Linux and Solaris segfault:\n\n9999999 Sun Apr 26 10:46:39 1970 -0700\n99999999 Sat Mar 3 02:46:39 1973 -0700\n999999999 Sat Sep 8 18:46:39 2001 -0700\n9999999999 Sat Nov 20 10:46:39 2286 -0700\n99999999999 Wed Nov 16 02:46:39 5138 -0700\n999999999999 Thu Sep 26 18:46:39 33658 -0700\n9999999999999 Sun May 20 10:46:39 318857 -0700\n99999999999999 Sat Nov 7 02:46:39 3170843 -0700\n999999999999999 Sat Jul 4 18:46:39 31690708 -0700\n9999999999999999 Sat Jan 25 10:46:39 316889355 -0700\n99999999999999999 Wed Sep 6 02:46:39 -1126091476 -0700\n999999999999999999 Thu Oct 24 18:46:39 1623969404 -0700\n\nSo, very bogus values.\n\nCharles.\n"},{"id":"237951","messageId":"20140327224837.GB32434@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140326224616.GA9454@hashpling.org","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-27T22:48:38Z","receivedAt":"2014-03-27T22:48:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2014 at 10:46:16PM +0000, Charles Bailey wrote:\n\n> On Wed, Mar 26, 2014 at 05:57:41PM -0400, Jeff King wrote:\n> > Hmm, so the year you got is actually: 1623969404. That still seems off\n> > to me by a factor 20. I don't know if this is really worth digging into\n> > that much further, but I wonder what you would get for timestamps of:\n> > \n> > 99999999999999999\n> > 9999999999999999\n> > 999999999999999\n> > etc.\n> > \n> \n> AIX goes negative at about the same time Linux and Solaris segfault:\n> \n> 9999999 Sun Apr 26 10:46:39 1970 -0700\n> 99999999 Sat Mar 3 02:46:39 1973 -0700\n> 999999999 Sat Sep 8 18:46:39 2001 -0700\n> 9999999999 Sat Nov 20 10:46:39 2286 -0700\n> 99999999999 Wed Nov 16 02:46:39 5138 -0700\n> 999999999999 Thu Sep 26 18:46:39 33658 -0700\n> 9999999999999 Sun May 20 10:46:39 318857 -0700\n> 99999999999999 Sat Nov 7 02:46:39 3170843 -0700\n> 999999999999999 Sat Jul 4 18:46:39 31690708 -0700\n> 9999999999999999 Sat Jan 25 10:46:39 316889355 -0700\n> 99999999999999999 Wed Sep 6 02:46:39 -1126091476 -0700\n> 999999999999999999 Thu Oct 24 18:46:39 1623969404 -0700\n\nThanks. Given the value where it fails, it kind of looks like there is\nsome signed 32-bit value at work (~300 million years is OK, but 10 times\nthat, rather than yielding ~3 billion, gets us -1 billion). Perhaps\ntm.tm_year is 32-bit.\n\nSo what do we want to do? I think the options are:\n\n 1. Try to guess when we have a bogus timestamp value with an arbitrary\n cutoff like \"greater than 1 million years from now\" (and enforce it\n via time_t seconds, and avoid gmtime entirely). That is made-up and\n arbitrary, but it also is sufficiently far that it won't ever\n matter, and sufficiently close that any gmtime should behave\n sensibly with it.\n\n 2. Accept that we can't guess at every broken gmtime's output, and\n just loosen the test to make sure we don't segfault.\n\n-Peff\n"},{"id":"238010","messageId":"xmqqd2h6cm26.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140327224837.GB32434@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-28T16:41:53Z","receivedAt":"2014-03-28T16:41:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> 9999999999999999 Sat Jan 25 10:46:39 316889355 -0700\n>> 99999999999999999 Wed Sep 6 02:46:39 -1126091476 -0700\n>> 999999999999999999 Thu Oct 24 18:46:39 1623969404 -0700\n>\n> Thanks. Given the value where it fails, it kind of looks like there is\n> some signed 32-bit value at work (~300 million years is OK, but 10 times\n> that, rather than yielding ~3 billion, gets us -1 billion). Perhaps\n> tm.tm_year is 32-bit.\n\nThat is what it looks like to me, too. It makes me wonder if some\nother platforms may have similar breakage using 16-bit signed\ntm_year and how such a breakage would look like, though ;-)\n\n> So what do we want to do? I think the options are:\n>\n> 1. Try to guess when we have a bogus timestamp value with an arbitrary\n> cutoff like \"greater than 1 million years from now\" (and enforce it\n> via time_t seconds, and avoid gmtime entirely). That is made-up and\n> arbitrary, but it also is sufficiently far that it won't ever\n> matter, and sufficiently close that any gmtime should behave\n> sensibly with it.\n\nI think that two assumptions here are that\n\n (1) we would be able to find a single insane value (like 3 billion\n years from now expressed in time_t seconds) the test script\n would be able to feed and expect it to fail on all platforms we\n care about, even though how exactly that value causes various\n platforms fail may be different (glibc may give us a NULL from\n gmtime, FreeBSD may leave their own sentinel in gmtime we can\n recognize, and some others may simply wrap around the years);\n and that\n\n (2) the only other class of failure mode we haven't considered\n bevore Charles's report is tm_year wrapping around 32-bit\n signed int.\n\nOffhand, the three possible failure modes this thread identified\nsounds to me like the only plausible ones, and I think the best way\nforward might be to\n\n - teach the \"is the result sane, even though we may have got a\n non-NULL from gmtime? otherwise let's signal a failure by\n replacing it with a known sentinel value\" codepath the new\n failure mode Charles's report suggests---if we feed a positive\n timestamp and gmtime gave us back a tm_year+1900 < 0, that is\n certainly an overflow; and\n\n - Use that 3-billion years timestamp from Charles's report in the\n test and make sure we convert it to the known sentinel value.\n\nperhaps?\n\n> 2. Accept that we can't guess at every broken gmtime's output, and\n> just loosen the test to make sure we don't segfault.\n\nOf course that is a simpler option, but we may have identified all\nplausible failure modes, in which case we can afford to go with your\noriginal plan to validate that we not just avoid segfaulting on one\nof the three failure modes from gmtime, but also cover the other two\nfailure modes and signal any of them with a sentinel. That way may\nallow us to identify the fourth failure mode we haven't anticipated.\n"},{"id":"238029","messageId":"20140328184710.GA29987@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqqd2h6cm26.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-28T18:47:10Z","receivedAt":"2014-03-28T18:47:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 28, 2014 at 09:41:53AM -0700, Junio C Hamano wrote:\n\n> Offhand, the three possible failure modes this thread identified\n> sounds to me like the only plausible ones, and I think the best way\n> forward might be to\n> \n> - teach the \"is the result sane, even though we may have got a\n> non-NULL from gmtime? otherwise let's signal a failure by\n> replacing it with a known sentinel value\" codepath the new\n> failure mode Charles's report suggests---if we feed a positive\n> timestamp and gmtime gave us back a tm_year+1900 < 0, that is\n> certainly an overflow; and\n\nI don't think we can analyze the output from gmtime. If it wraps the\nyear at N, then won't N+2014 look like a valid value?\n\nIf we are going to do something trustworthy I think it has to be before\nwe hand off to gmtime. Like:\n\ndiff --git a/date.c b/date.c\nindex e1a2cee..e0c43c4 100644\n--- a/date.c\n+++ b/date.c\n@@ -57,6 +57,8 @@ static time_t gm_time_t(unsigned long time, int tz)\n static struct tm *time_to_tm(unsigned long time, int tz)\n {\n \ttime_t t = gm_time_t(time, tz);\n+\tif (t > 9999999999999999)\n+\t\treturn NULL;\n \treturn gmtime(&t);\n }\n\nI suspect that would handle the FreeBSD case, as well.\n\nBy the way, I have a suspicion that the gm_time_t above can overflow if\nyou specially craft a value at the edge of what time_t can handle (we\ncheck that our value will not overflow time_t earlier, but now we might\nbe adding up to 86400 seconds to it). \n\n-Peff\n"},{"id":"238033","messageId":"xmqqppl69meh.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140328184710.GA29987@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-28T19:02:46Z","receivedAt":"2014-03-28T19:02:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 28, 2014 at 09:41:53AM -0700, Junio C Hamano wrote:\n>\n>> Offhand, the three possible failure modes this thread identified\n>> sounds to me like the only plausible ones, and I think the best way\n>> forward might be to\n>> \n>> - teach the \"is the result sane, even though we may have got a\n>> non-NULL from gmtime? otherwise let's signal a failure by\n>> replacing it with a known sentinel value\" codepath the new\n>> failure mode Charles's report suggests---if we feed a positive\n>> timestamp and gmtime gave us back a tm_year+1900 < 0, that is\n>> certainly an overflow; and\n>\n> I don't think we can analyze the output from gmtime. If it wraps the\n> year at N, then won't N+2014 look like a valid value?\n\nYes, but I was hoping that there are small number of possible N's\n;-)\n\n> If we are going to do something trustworthy I think it has to be before\n> we hand off to gmtime. Like:\n>\n> diff --git a/date.c b/date.c\n> index e1a2cee..e0c43c4 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -57,6 +57,8 @@ static time_t gm_time_t(unsigned long time, int tz)\n> static struct tm *time_to_tm(unsigned long time, int tz)\n> {\n> \ttime_t t = gm_time_t(time, tz);\n> +\tif (t > 9999999999999999)\n> +\t\treturn NULL;\n> \treturn gmtime(&t);\n> }\n>\n> I suspect that would handle the FreeBSD case, as well.\n>\n> By the way, I have a suspicion that the gm_time_t above can overflow if\n> you specially craft a value at the edge of what time_t can handle (we\n> check that our value will not overflow time_t earlier, but now we might\n> be adding up to 86400 seconds to it). \n\nYuck. Let's not go there.\n\nThanks.\n"},{"id":"238034","messageId":"20140328190548.GB30739@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqqppl69meh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-28T19:05:48Z","receivedAt":"2014-03-28T19:05:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 28, 2014 at 12:02:46PM -0700, Junio C Hamano wrote:\n\n> >> - teach the \"is the result sane, even though we may have got a\n> >> non-NULL from gmtime? otherwise let's signal a failure by\n> >> replacing it with a known sentinel value\" codepath the new\n> >> failure mode Charles's report suggests---if we feed a positive\n> >> timestamp and gmtime gave us back a tm_year+1900 < 0, that is\n> >> certainly an overflow; and\n> >\n> > I don't think we can analyze the output from gmtime. If it wraps the\n> > year at N, then won't N+2014 look like a valid value?\n> \n> Yes, but I was hoping that there are small number of possible N's\n> ;-)\n\nI'm not sure I understand. Even if we know N, we've lost information\nduring the truncation done by time_t (we cannot distingiuish true M from\nN+M).\n\n> > diff --git a/date.c b/date.c\n> > index e1a2cee..e0c43c4 100644\n> > --- a/date.c\n> > +++ b/date.c\n> > @@ -57,6 +57,8 @@ static time_t gm_time_t(unsigned long time, int tz)\n> > static struct tm *time_to_tm(unsigned long time, int tz)\n> > {\n> > \ttime_t t = gm_time_t(time, tz);\n> > +\tif (t > 9999999999999999)\n> > +\t\treturn NULL;\n> > \treturn gmtime(&t);\n> > }\n> >\n> > I suspect that would handle the FreeBSD case, as well.\n> >\n> > By the way, I have a suspicion that the gm_time_t above can overflow if\n> > you specially craft a value at the edge of what time_t can handle (we\n> > check that our value will not overflow time_t earlier, but now we might\n> > be adding up to 86400 seconds to it). \n> \n> Yuck. Let's not go there.\n\nDo you mean \"let's not worry about the absurdly specific overflow case\",\nor \"let's not do this gross time_to_tm hack?\"\n\nThis (non-)issue has consumed a lot more brain power than it is probably\nworth. I'd like to figure out which patch to go with and be done. :)\n\n-Peff\n"},{"id":"238040","messageId":"xmqq8uru9l51.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140328190548.GB30739@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-28T19:30:02Z","receivedAt":"2014-03-28T19:30:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This (non-)issue has consumed a lot more brain power than it is probably\n> worth. I'd like to figure out which patch to go with and be done. :)\n\nLet's just deal with a simple known cases (like FreeBSD) in the real\ncode that everybody exercises at runtime, and have the new test only\ncheck we do not segfault on a value we used to segfault upon seeing.\n"},{"id":"238187","messageId":"20140401073848.GA22023@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqq8uru9l51.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T07:38:48Z","receivedAt":"2014-04-01T07:38:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 28, 2014 at 12:30:02PM -0700, Junio C Hamano wrote:\n\n> Let's just deal with a simple known cases (like FreeBSD) in the real\n> code that everybody exercises at runtime, and have the new test only\n> check we do not segfault on a value we used to segfault upon seeing.\n\nOK. Here it is, with the other option as an \"alt\" patch for reference.\n\n [1/2]: date: recognize bogus FreeBSD gmtime output\n [2/2]: t4212: loosen far-in-future test for AIX\n [2alt/2]: work around unreliable gmtime errors on AIX\n\n-Peff\n"},{"id":"238188","messageId":"20140401074251.GA22550@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140401073848.GA22023@sigill.intra.peff.net","subject":"[PATCH 1/2] date: recognize bogus FreeBSD gmtime output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T07:42:51Z","receivedAt":"2014-04-01T07:42:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most gmtime implementations return a NULL value when they\nencounter an error (and this behavior is specified by ANSI C\nand POSIX). FreeBSD's implementation, however, will\nindicate an error by returning a pointer to a \"struct tm\"\nwith all fields set to zero. Let's also recognize this and\nconvert it to a NULL (with this patch, t4212 should pass on\nFreeBSD).\n\nReported-by: René Scharfe \nSigned-off-by: Jeff King \n---\nThere are actually a few callers to gmtime and gmtime_r, so I pushed\nthis fix up into a compat wrapper rather than in time_to_tm to get them\nall. It's possible that localtime() would want to receive the same\ntreatment, too. It's not strictly necessary to make the wrapper\nconditional, but it was easy to do so. We could also just run this code\nall the time.\n\nI don't have a FreeBSD VM handy to test this, so confirmation that it\npasses the test would be nice.\n\n Makefile | 8 ++++++++\n compat/gmtime.c | 26 ++++++++++++++++++++++++++\n config.mak.uname | 1 +\n git-compat-util.h | 7 +++++++\n 4 files changed, 42 insertions(+)\n create mode 100644 compat/gmtime.c\n\ndiff --git a/Makefile b/Makefile\nindex 3646391..2f3758c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -338,6 +338,9 @@ all::\n # Define TEST_GIT_INDEX_VERSION to 2, 3 or 4 to run the test suite\n # with a different indexfile format version. If it isn't set the index\n # file format used is index-v[23].\n+#\n+# Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not\n+# return NULL when it receives a bogus time_t.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -1489,6 +1492,11 @@ ifneq (,$(XDL_FAST_HASH))\n \tBASIC_CFLAGS += -DXDL_FAST_HASH\n endif\n \n+ifdef GMTIME_UNRELIABLE_ERRORS\n+\tCOMPAT_OBJS += compat/gmtime.o\n+\tBASIC_CFLAGS += -DGMTIME_UNRELIABLE_ERRORS\n+endif\n+\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK = NoThanks\n endif\ndiff --git a/compat/gmtime.c b/compat/gmtime.c\nnew file mode 100644\nindex 0000000..ffcabf4\n--- /dev/null\n+++ b/compat/gmtime.c\n@@ -0,0 +1,26 @@\n+#include \"../git-compat-util.h\"\n+#undef gmtime\n+#undef gmtime_r\n+\n+struct tm *git_gmtime(const time_t *timep)\n+{\n+\tstatic struct tm result;\n+\treturn git_gmtime_r(timep, &result);\n+}\n+\n+struct tm *git_gmtime_r(const time_t *timep, struct tm *result)\n+{\n+\tstruct tm *ret;\n+\n+\tret = gmtime_r(timep, result);\n+\n+\t/*\n+\t * Rather than NULL, FreeBSD gmtime will return a \"struct tm\" with all\n+\t * fields zeroed. Since \"mday\" cannot otherwise be zero, we can test\n+\t * this very quickly.\n+\t */\n+\tif (ret && !ret->tm_mday)\n+\t\tret = NULL;\n+\n+\treturn ret;\n+}\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 6069a44..0e22ac0 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -187,6 +187,7 @@ ifeq ($(uname_S),FreeBSD)\n \tendif\n \tPYTHON_PATH = /usr/local/bin/python\n \tHAVE_PATHS_H = YesPlease\n+\tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 892032b..5191866 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -716,4 +716,11 @@ void warn_on_inaccessible(const char *path);\n /* Get the passwd entry for the UID of the current process. */\n struct passwd *xgetpwuid_self(void);\n \n+#ifdef GMTIME_UNRELIABLE_ERRORS\n+struct tm *git_gmtime(const time_t *);\n+struct tm *git_gmtime_r(const time_t *, struct tm *);\n+#define gmtime git_gmtime\n+#define gmtime_r git_gmtime_r\n+#endif\n+\n #endif\n-- \n1.9.1.656.ge8a0637\n"},{"id":"238189","messageId":"20140401074306.GB22550@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140401073848.GA22023@sigill.intra.peff.net","subject":"[PATCH 2/2] t4212: loosen far-in-future test for AIX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T07:43:06Z","receivedAt":"2014-04-01T07:43:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of the tests in t4212 checks our behavior when we feed\ngmtime a date so far in the future that it gives up and\nreturns NULL. Some implementations, like AIX, may actually\njust provide us a bogus result instead.\n\nIt's not worth it for us to come up with heuristics that\nguess whether the return value is sensible or not. On good\nplatforms where gmtime reports the problem to us with NULL,\nwe will print the epoch value. On bad platforms, we will\nprint garbage. But our test should be written for the\nlowest common denominator so that it passes everywhere.\n\nReported-by: Charles Bailey \nSigned-off-by: Jeff King \n---\n t/t4212-log-corrupt.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex 3fa1715..58b792b 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -82,11 +82,9 @@ test_expect_success 'date parser recognizes time_t overflow' '\n '\n \n # date is within 2^63-1, but enough to choke glibc's gmtime\n-test_expect_success 'absurdly far-in-future dates produce sentinel' '\n+test_expect_success 'absurdly far-in-future date' '\n \tcommit=$(munge_author_date HEAD 999999999999999999) &&\n-\techo \"Thu Jan 1 00:00:00 1970 +0000\" >expect &&\n-\tgit log -1 --format=%ad $commit >actual &&\n-\ttest_cmp expect actual\n+\tgit log -1 --format=%ad $commit\n '\n \n test_done\n-- \n1.9.1.656.ge8a0637\n"},{"id":"238190","messageId":"20140401074559.GA22591@sigill.intra.peff.net","threadId":"35943","inReplyTo":"20140401073848.GA22023@sigill.intra.peff.net","subject":"[PATCH 2alt/2] work around unreliable gmtime errors on AIX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T07:45:59Z","receivedAt":"2014-04-01T07:45:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"AIX's gmtime will happily overflow the tm_year field. Let's\ncatch this error before handing the value to gmtime.\n\nSigned-off-by: Jeff King \n---\nThis is an alternative to loosening the test in t4212.\n\nIt's really not _that_ ugly. The \"LL\" here may not be portable, though.\n32-bit systems can't represent this timestamp at all (so they're safe),\nbut I don't know what would be the best way to conditionally compile\nhere.\n\n compat/gmtime.c | 10 ++++++++++\n config.mak.uname | 1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/compat/gmtime.c b/compat/gmtime.c\nindex 75a5835..f95ba50 100644\n--- a/compat/gmtime.c\n+++ b/compat/gmtime.c\n@@ -12,6 +12,16 @@ struct tm *git_gmtime_r(const time_t *timep, struct tm *result)\n {\n \tstruct tm *ret;\n \n+\t/*\n+\t * Some systems, like AIX, will happily overflow the tm_year field.\n+\t * So let's recognize obviously out-of-bound data before it hits gmtime\n+\t * and just mark it as an error. This date is ~316 million years in the\n+\t * future, which is far enough that nobody should care, but close\n+\t * enough for the year to fit into a 32-bit tm_year.\n+\t */\n+\tif (*timep > 9999999999999999LL)\n+\t\treturn NULL;\n+\n \tret = gmtime_r(timep, result);\n \n \t/*\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 0e22ac0..c1110ad 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -236,6 +236,7 @@ ifeq ($(uname_S),AIX)\n \t\tINLINE = ''\n \tendif\n \tGIT_TEST_CMP = cmp\n+\tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n endif\n ifeq ($(uname_S),GNU)\n \t# GNU/Hurd\n-- \n1.9.1.656.ge8a0637\n"},{"id":"238232","messageId":"533AFA86.2090905@web.de","threadId":"35943","inReplyTo":"20140401074251.GA22550@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] date: recognize bogus FreeBSD gmtime output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-04-01T17:42:30Z","receivedAt":"2014-04-01T17:42:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.04.2014 09:42, schrieb Jeff King:\n> diff --git a/compat/gmtime.c b/compat/gmtime.c\n> new file mode 100644\n> index 0000000..ffcabf4\n> --- /dev/null\n> +++ b/compat/gmtime.c\n> @@ -0,0 +1,26 @@\n> +#include \"../git-compat-util.h\"\n> +#undef gmtime\n> +#undef gmtime_r\n> +\n> +struct tm *git_gmtime(const time_t *timep)\n> +{\n> +\tstatic struct tm result;\n> +\treturn git_gmtime_r(timep, &result);\n> +}\n> +\n> +struct tm *git_gmtime_r(const time_t *timep, struct tm *result)\n> +{\n> +\tstruct tm *ret;\n> +\n> +\tret = gmtime_r(timep, result);\n> +\n> +\t/*\n> +\t * Rather than NULL, FreeBSD gmtime will return a \"struct tm\" with all\n> +\t * fields zeroed. Since \"mday\" cannot otherwise be zero, we can test\n> +\t * this very quickly.\n> +\t */\n> +\tif (ret && !ret->tm_mday)\n> +\t\tret = NULL;\n> +\n> +\treturn ret;\n> +}\n\nhttp://pubs.opengroup.org/onlinepubs/009695399/functions/gmtime.html \nsays that errno shall be set on error and only mentions EOVERFLOW as a \npossible error code.\n\nRené\n"},{"id":"238235","messageId":"xmqqbnwk27it.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"20140401073848.GA22023@sigill.intra.peff.net","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T19:07:22Z","receivedAt":"2014-04-01T19:07:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 28, 2014 at 12:30:02PM -0700, Junio C Hamano wrote:\n>\n>> Let's just deal with a simple known cases (like FreeBSD) in the real\n>> code that everybody exercises at runtime, and have the new test only\n>> check we do not segfault on a value we used to segfault upon seeing.\n>\n> OK. Here it is, with the other option as an \"alt\" patch for reference.\n>\n> [1/2]: date: recognize bogus FreeBSD gmtime output\n> [2/2]: t4212: loosen far-in-future test for AIX\n> [2alt/2]: work around unreliable gmtime errors on AIX\n>\n> -Peff\n\nThanks. 2alt does not look too bad, but on the other hand, we are\nreplacing a value that can produce the right result on correctly\nimplemented gmtime with a completely bogus value only because we\nknow there exists one broken implementation---which does not sound a\nvery good trade-off, given that we would get a result that does not\ncorrespond to the input anyway with or without the change on the\nbroken implementation.\n"},{"id":"238236","messageId":"xmqq7g7827h8.fsf@gitster.dls.corp.google.com","threadId":"35943","inReplyTo":"533AFA86.2090905@web.de","subject":"Re: [PATCH 1/2] date: recognize bogus FreeBSD gmtime output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T19:08:19Z","receivedAt":"2014-04-01T19:08:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 01.04.2014 09:42, schrieb Jeff King:\n>> diff --git a/compat/gmtime.c b/compat/gmtime.c\n>> new file mode 100644\n>> index 0000000..ffcabf4\n>> --- /dev/null\n>> +++ b/compat/gmtime.c\n>> @@ -0,0 +1,26 @@\n>> +#include \"../git-compat-util.h\"\n>> +#undef gmtime\n>> +#undef gmtime_r\n>> +\n>> +struct tm *git_gmtime(const time_t *timep)\n>> +{\n>> +\tstatic struct tm result;\n>> +\treturn git_gmtime_r(timep, &result);\n>> +}\n>> +\n>> +struct tm *git_gmtime_r(const time_t *timep, struct tm *result)\n>> +{\n>> +\tstruct tm *ret;\n>> +\n>> +\tret = gmtime_r(timep, result);\n>> +\n>> +\t/*\n>> +\t * Rather than NULL, FreeBSD gmtime will return a \"struct tm\" with all\n>> +\t * fields zeroed. Since \"mday\" cannot otherwise be zero, we can test\n>> +\t * this very quickly.\n>> +\t */\n>> +\tif (ret && !ret->tm_mday)\n>> +\t\tret = NULL;\n>> +\n>> +\treturn ret;\n>> +}\n>\n> http://pubs.opengroup.org/onlinepubs/009695399/functions/gmtime.html\n> says that errno shall be set on error and only mentions EOVERFLOW as a\n> possible error code.\n\nSo are you saying we should set EOVERFLOW ourselves, or does FreeBSD\nset EOVERFLOW for us in this case and we do not have to worry?\n"},{"id":"238253","messageId":"20140401194606.GA20676@sigill.intra.peff.net","threadId":"35943","inReplyTo":"xmqqbnwk27it.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T19:46:06Z","receivedAt":"2014-04-01T19:46:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 12:07:22PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Mar 28, 2014 at 12:30:02PM -0700, Junio C Hamano wrote:\n> >\n> >> Let's just deal with a simple known cases (like FreeBSD) in the real\n> >> code that everybody exercises at runtime, and have the new test only\n> >> check we do not segfault on a value we used to segfault upon seeing.\n> >\n> > OK. Here it is, with the other option as an \"alt\" patch for reference.\n> >\n> > [1/2]: date: recognize bogus FreeBSD gmtime output\n> > [2/2]: t4212: loosen far-in-future test for AIX\n> > [2alt/2]: work around unreliable gmtime errors on AIX\n> >\n> > -Peff\n> \n> Thanks. 2alt does not look too bad, but on the other hand, we are\n> replacing a value that can produce the right result on correctly\n> implemented gmtime with a completely bogus value only because we\n> know there exists one broken implementation---which does not sound a\n> very good trade-off, given that we would get a result that does not\n> correspond to the input anyway with or without the change on the\n> broken implementation.\n\nOne other option is to push _all_ callers through a git_gmtime()\nwrapper, and then use Makefile knobs to turn on specific quirks, like:\n\n struct tm *git_gmtime_r(const time_t *timep, struct tm *result)\n {\n #ifdef GMTIME_OVERFLOWS_32BIT\n if (*timep > 999999999999999)\n return NULL;\n #endif\n\n ret = gmtime_r(timep, result);\n\n #ifdef GMTIME_ERROR_BLANK\n if (ret && !ret->tm_mday)\n return NULL;\n #endif\n\n return ret;\n }\n\nand then each platform can turn the knobs as appropriate.\n\n-Peff\n"},{"id":"238273","messageId":"533B2CDA.30307@web.de","threadId":"35943","inReplyTo":"xmqq7g7827h8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] date: recognize bogus FreeBSD gmtime output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-04-01T21:17:14Z","receivedAt":"2014-04-01T21:17:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.04.2014 21:08, schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Am 01.04.2014 09:42, schrieb Jeff King:\n>>> diff --git a/compat/gmtime.c b/compat/gmtime.c\n>>> new file mode 100644\n>>> index 0000000..ffcabf4\n>>> --- /dev/null\n>>> +++ b/compat/gmtime.c\n>>> @@ -0,0 +1,26 @@\n>>> +#include \"../git-compat-util.h\"\n>>> +#undef gmtime\n>>> +#undef gmtime_r\n>>> +\n>>> +struct tm *git_gmtime(const time_t *timep)\n>>> +{\n>>> +\tstatic struct tm result;\n>>> +\treturn git_gmtime_r(timep, &result);\n>>> +}\n>>> +\n>>> +struct tm *git_gmtime_r(const time_t *timep, struct tm *result)\n>>> +{\n>>> +\tstruct tm *ret;\n>>> +\n>>> +\tret = gmtime_r(timep, result);\n>>> +\n>>> +\t/*\n>>> +\t * Rather than NULL, FreeBSD gmtime will return a \"struct tm\" with all\n>>> +\t * fields zeroed. Since \"mday\" cannot otherwise be zero, we can test\n>>> +\t * this very quickly.\n>>> +\t */\n>>> +\tif (ret && !ret->tm_mday)\n>>> +\t\tret = NULL;\n>>> +\n>>> +\treturn ret;\n>>> +}\n>>\n>> http://pubs.opengroup.org/onlinepubs/009695399/functions/gmtime.html\n>> says that errno shall be set on error and only mentions EOVERFLOW as a\n>> possible error code.\n>\n> So are you saying we should set EOVERFLOW ourselves, or does FreeBSD\n> set EOVERFLOW for us in this case and we do not have to worry?\n\nIf we correct the return value then we should correct errno as well. \ngmtime() on FreeBSD 10 leaves errno untouched when it encounters an \noverflow.\n\nWhile testing this again I just noticed that FreeBSD doesn't strictly \nreturn a pointer to a cleared struct tm. It simply leaves its static \nbuffer untouched. We should probably clear it (memset in git_gmtime_r).\n\nDemo program:\n\n-- >8 --\n#include \n#include \n\nconst static time_t epoch;\n\nint main(int argc, char **argv)\n{\n time_t t = 99999999999999999;\n\n printf(\"%s\", ctime(&t));\n printf(\"%s\", asctime(gmtime(&t)));\n\n printf(\"%s\", ctime(&t));\n gmtime(&epoch);\n printf(\"%s\", asctime(gmtime(&t)));\n\n return 0;\n}\n-- 8< --\n\nResults on FreeBSD 10:\n\n\tWed Sep 6 11:46:39 -1126091476\n\tWed Sep 6 11:46:39 -1126091476\n\tWed Sep 6 11:46:39 -1126091476\n\tThu Jan 1 00:00:00 1970\n\nRené\n"},{"id":"238274","messageId":"20140401212842.GB23578@sigill.intra.peff.net","threadId":"35943","inReplyTo":"533B2CDA.30307@web.de","subject":"Re: [PATCH 1/2] date: recognize bogus FreeBSD gmtime output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-01T21:28:42Z","receivedAt":"2014-04-01T21:28:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 01, 2014 at 11:17:14PM +0200, René Scharfe wrote:\n\n> >So are you saying we should set EOVERFLOW ourselves, or does FreeBSD\n> >set EOVERFLOW for us in this case and we do not have to worry?\n> \n> If we correct the return value then we should correct errno as well.\n> gmtime() on FreeBSD 10 leaves errno untouched when it encounters an\n> overflow.\n> \n> While testing this again I just noticed that FreeBSD doesn't strictly return\n> a pointer to a cleared struct tm. It simply leaves its static buffer\n> untouched. We should probably clear it (memset in git_gmtime_r).\n\nThanks, that all makes sense (we do not ever care about gmtime's errno\nin our code, but it does not hurt to be thorough to avoid surprising any\nnew callers). Here's a replacement for patch 1.\n\n-- >8 --\nSubject: date: recognize bogus FreeBSD gmtime output\n\nMost gmtime implementations return a NULL value when they\nencounter an error (and this behavior is specified by ANSI C\nand POSIX). FreeBSD's implementation, however, will simply\nleave the \"struct tm\" untouched. Let's also recognize this\nand convert it to a NULL (with this patch, t4212 should pass\non FreeBSD).\n\nReported-by: René Scharfe \nSigned-off-by: Jeff King \n---\n Makefile | 8 ++++++++\n compat/gmtime.c | 29 +++++++++++++++++++++++++++++\n config.mak.uname | 1 +\n git-compat-util.h | 7 +++++++\n 4 files changed, 45 insertions(+)\n create mode 100644 compat/gmtime.c\n\ndiff --git a/Makefile b/Makefile\nindex 3646391..2f3758c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -338,6 +338,9 @@ all::\n # Define TEST_GIT_INDEX_VERSION to 2, 3 or 4 to run the test suite\n # with a different indexfile format version. If it isn't set the index\n # file format used is index-v[23].\n+#\n+# Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not\n+# return NULL when it receives a bogus time_t.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -1489,6 +1492,11 @@ ifneq (,$(XDL_FAST_HASH))\n \tBASIC_CFLAGS += -DXDL_FAST_HASH\n endif\n \n+ifdef GMTIME_UNRELIABLE_ERRORS\n+\tCOMPAT_OBJS += compat/gmtime.o\n+\tBASIC_CFLAGS += -DGMTIME_UNRELIABLE_ERRORS\n+endif\n+\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK = NoThanks\n endif\ndiff --git a/compat/gmtime.c b/compat/gmtime.c\nnew file mode 100644\nindex 0000000..e8362dd\n--- /dev/null\n+++ b/compat/gmtime.c\n@@ -0,0 +1,29 @@\n+#include \"../git-compat-util.h\"\n+#undef gmtime\n+#undef gmtime_r\n+\n+struct tm *git_gmtime(const time_t *timep)\n+{\n+\tstatic struct tm result;\n+\treturn git_gmtime_r(timep, &result);\n+}\n+\n+struct tm *git_gmtime_r(const time_t *timep, struct tm *result)\n+{\n+\tstruct tm *ret;\n+\n+\tmemset(result, 0, sizeof(*result));\n+\tret = gmtime_r(timep, result);\n+\n+\t/*\n+\t * Rather than NULL, FreeBSD gmtime simply leaves the \"struct tm\"\n+\t * untouched when it encounters overflow. Since \"mday\" cannot otherwise\n+\t * be zero, we can test this very quickly.\n+\t */\n+\tif (ret && !ret->tm_mday) {\n+\t\tret = NULL;\n+\t\terrno = EOVERFLOW;\n+\t}\n+\n+\treturn ret;\n+}\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 6069a44..0e22ac0 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -187,6 +187,7 @@ ifeq ($(uname_S),FreeBSD)\n \tendif\n \tPYTHON_PATH = /usr/local/bin/python\n \tHAVE_PATHS_H = YesPlease\n+\tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 892032b..5191866 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -716,4 +716,11 @@ void warn_on_inaccessible(const char *path);\n /* Get the passwd entry for the UID of the current process. */\n struct passwd *xgetpwuid_self(void);\n \n+#ifdef GMTIME_UNRELIABLE_ERRORS\n+struct tm *git_gmtime(const time_t *);\n+struct tm *git_gmtime_r(const time_t *, struct tm *);\n+#define gmtime git_gmtime\n+#define gmtime_r git_gmtime_r\n+#endif\n+\n #endif\n-- \n1.9.1.656.ge8a0637\n"}]}