Re: [PATCH 5/5] log: do not segfault on gmtime errors
On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote: +# date is within 2^63-1, but enough to choke glibc's gmtime +test_expect_success 'absurdly far-in-future dates produce sentinel' ' + commit=$(munge_author_date HEAD 99) + echo Thu Jan 1 00:00:00 1970 + expect + git log -1 --format=%ad $commit actual + test_cmp expect actual +' Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 162396404 -0700. I don't know if this is correct for the given input but the test fails. Charles. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
On Wed, Mar 26, 2014 at 11:05:59AM +, Charles Bailey wrote: On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote: +# date is within 2^63-1, but enough to choke glibc's gmtime +test_expect_success 'absurdly far-in-future dates produce sentinel' ' + commit=$(munge_author_date HEAD 99) + echo Thu Jan 1 00:00:00 1970 + expect + git log -1 --format=%ad $commit actual + test_cmp expect actual +' Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 162396404 -0700. I don't know if this is correct for the given input but the test fails. Ick. I am not sure that dates at that range are even meaningful (will October really exist in the year 162396404? Did they account for all the leap-seconds between then and now?). But at the same time, I cannot fault their gmtime for just extrapolating the current rules out as far as we ask them to. Unlike the FreeBSD thing that René brought up, this is not a problem in the code, but just in the test. So I think our options are basically: 1. Scrap the test as unportable. 2. Hard-code a few expected values. I'd be unsurprised if some other system comes up with a slightly different date in 162396404, so we may end up needing several of these. I think I'd lean towards (2), just because it is testing an actual feature of the code, and I'd like to continue doing so. And while we may end up with a handful of values, there's probably not _that_ many independent implementations of gmtime in the wild. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
Jeff King p...@peff.net writes: On Wed, Mar 26, 2014 at 11:05:59AM +, Charles Bailey wrote: On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote: +# date is within 2^63-1, but enough to choke glibc's gmtime +test_expect_success 'absurdly far-in-future dates produce sentinel' ' + commit=$(munge_author_date HEAD 99) + echo Thu Jan 1 00:00:00 1970 + expect + git log -1 --format=%ad $commit actual + test_cmp expect actual +' Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 162396404 -0700. I don't know if this is correct for the given input but the test fails. Ick. I am not sure that dates at that range are even meaningful (will October really exist in the year 162396404? Did they account for all the leap-seconds between then and now?). But at the same time, I cannot fault their gmtime for just extrapolating the current rules out as far as we ask them to. Unlike the FreeBSD thing that René brought up, this is not a problem in the code, but just in the test. So I think our options are basically: 1. Scrap the test as unportable. 2. Hard-code a few expected values. I'd be unsurprised if some other system comes up with a slightly different date in 162396404, so we may end up needing several of these. I think I'd lean towards (2), just because it is testing an actual feature of the code, and I'd like to continue doing so. And while we may end up with a handful of values, there's probably not _that_ many independent implementations of gmtime in the wild. Or 3. Just make sure that 'git log' does not segfault? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote: Unlike the FreeBSD thing that René brought up, this is not a problem in the code, but just in the test. So I think our options are basically: 1. Scrap the test as unportable. 2. Hard-code a few expected values. I'd be unsurprised if some other system comes up with a slightly different date in 162396404, so we may end up needing several of these. I think I'd lean towards (2), just because it is testing an actual feature of the code, and I'd like to continue doing so. And while we may end up with a handful of values, there's probably not _that_ many independent implementations of gmtime in the wild. Or 3. Just make sure that 'git log' does not segfault? That would not test the FreeBSD case, which does not segfault, but returns a bogus sentinel value. I don't know how important that is. This is such a minor feature that it is not worth a lot of maintenance headache in the test. But I also do not know if this is going to be the last report, or we will have a bunch of other systems that need their own special values put into the test. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
Jeff King p...@peff.net writes: On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote: Unlike the FreeBSD thing that René brought up, this is not a problem in the code, but just in the test. So I think our options are basically: 1. Scrap the test as unportable. 2. Hard-code a few expected values. I'd be unsurprised if some other system comes up with a slightly different date in 162396404, so we may end up needing several of these. I think I'd lean towards (2), just because it is testing an actual feature of the code, and I'd like to continue doing so. And while we may end up with a handful of values, there's probably not _that_ many independent implementations of gmtime in the wild. Or 3. Just make sure that 'git log' does not segfault? That would not test the FreeBSD case, which does not segfault, but returns a bogus sentinel value. I don't know how important that is. This is such a minor feature that it is not worth a lot of maintenance headache in the test. But I also do not know if this is going to be the last report, or we will have a bunch of other systems that need their own special values put into the test. I didn't quite realize that your objective of the change was to signal the failure of gmtime for all implementations, i.e. both (1) the ones that signal an error by giving NULL back to us and (2) the ones that fail to signal an error but leave bogus result (FreeBSD, but it appears AIX might be also giving us another bogus value), by using a single sane sentinel value. If we can do that consistently on every platform, that would indeed be great. But if that is the case, we should not have to maintain special case values in the test code, I would think. The test instead should expect the output to have that single sentinel value, and if the workaround code fails to detect a breakage in the platform gmtime(), the special case logic to catch these platform-specific breakages should go that timestamp that cannot be grokked by gmtime---replace it with a sentinel logic, no? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
On Wed, Mar 26, 2014 at 02:01:21PM -0700, Junio C Hamano wrote: I don't know how important that is. This is such a minor feature that it is not worth a lot of maintenance headache in the test. But I also do not know if this is going to be the last report, or we will have a bunch of other systems that need their own special values put into the test. I didn't quite realize that your objective of the change was to signal the failure of gmtime for all implementations, I didn't realize it at the time I wrote the test, either. It was my goal to recognize such failures, but I didn't now at the time that there were failures that would be signaled by anything besides a NULL return. the ones that signal an error by giving NULL back to us and (2) the ones that fail to signal an error but leave bogus result (FreeBSD, but it appears AIX might be also giving us another bogus value), by using a single sane sentinel value. If we can do that consistently on every platform, that would indeed be great. Agreed. I am not sure we can do so. For FreeBSD, I think it is not hard. I am not sure what the pattern is for AIX. I am hoping Charles will reveal some pattern, but there may not be one. But if that is the case, we should not have to maintain special case values in the test code, I would think. Right. All of my suggestions to accommodate special values in the test were assuming that the system gmtime was smarter than we are. That it produced a good value for this crazy time and _didn't_ fail. But after having done the basic math, I don't think that is what is going on here. The test instead should expect the output to have that single sentinel value, and if the workaround code fails to detect a breakage in the platform gmtime(), the special case logic to catch these platform-specific breakages should go that timestamp that cannot be grokked by gmtime---replace it with a sentinel logic, no? Yes, exactly. That is the preferable solution, if we can come up with such logic. Personally I am not optimistic. The fallback is to accept that we cannot cover all cases, and just loosen the test (i.e., the second patch I posted today). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
On Sat, Mar 22, 2014 at 10:32:37AM +0100, René Scharfe wrote:
This test is of questionable portability, since we are depending on
gmtime's arbitrary point to decide that our input is crazy and return
NULL. The value is sufficiently large that I'd expect most to do so,
though, so it may be safe.
Just came around to testing on FreeBSD 10 amd64; the new test in t4212 fails
there:
Thanks for the report. I don't think the problem is in the test here,
but rather that we should do a more thorough job of detecting gmtime's
I don't know what to do with this response.
@@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum
date_mode mode)
tz = local_tzoffset(time);
tm = time_to_tm(time, tz);
-if (!tm)
-return NULL;
+if (!tm) {
Would it make sense to work around the FreeBSD issue by adding a check like
this?
if (!tm || tm-tm_year 70) {
That feels like a bit of a maintenance headache. Right now we do not
internally represent times prior to the epoch, since we mostly use
unsigned long through the code. So anything 70 has to be an error.
But it would be nice one day to consistently use a 64-bit signed time_t
throughout git, and represent even ancient timestamps (e.g., for people
doing historical projects, like importing laws into git). This would set
quite a trap for people working later on the code.
If the result is all-zeroes, can we check for that case instead? I
suppose that will eventually create a trap at midnight on January 1st
of the year 0 (though I am not sure such a date is even meaningful,
given the history of our calendars).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
Am 24.03.2014 22:33, schrieb Jeff King:
On Sat, Mar 22, 2014 at 10:32:37AM +0100, René Scharfe wrote:
@@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum
date_mode mode)
tz = local_tzoffset(time);
tm = time_to_tm(time, tz);
- if (!tm)
- return NULL;
+ if (!tm) {
Would it make sense to work around the FreeBSD issue by adding a check like
this?
if (!tm || tm-tm_year 70) {
That feels like a bit of a maintenance headache. Right now we do not
internally represent times prior to the epoch, since we mostly use
unsigned long through the code. So anything 70 has to be an error.
But it would be nice one day to consistently use a 64-bit signed time_t
throughout git, and represent even ancient timestamps (e.g., for people
doing historical projects, like importing laws into git). This would set
quite a trap for people working later on the code.
If the result is all-zeroes, can we check for that case instead? I
suppose that will eventually create a trap at midnight on January 1st
of the year 0 (though I am not sure such a date is even meaningful,
given the history of our calendars).
Makes sense. That would be Sun Jan 0 00:00:00 1900, however -- days
are 1-based and years 1900-based in struct tm. Since a zeroth day is
invalid, would this suffice:
if (!tm || !tm-tm_mday) {
(Yes, I'm lazy. :)
René
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
On Mon, Mar 24, 2014 at 11:03:42PM +0100, René Scharfe wrote:
If the result is all-zeroes, can we check for that case instead? I
suppose that will eventually create a trap at midnight on January 1st
of the year 0 (though I am not sure such a date is even meaningful,
given the history of our calendars).
Makes sense. That would be Sun Jan 0 00:00:00 1900, however -- days are
1-based and years 1900-based in struct tm.
Oh right, I was stupidly forgetting about being 1900-based.
Since a zeroth day is invalid, would this suffice:
if (!tm || !tm-tm_mday) {
(Yes, I'm lazy. :)
That looks perfect (and I like that it is quick to check, too, since
this case should be extremely rare).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
Am 24.02.2014 08:49, schrieb Jeff King:
Many code paths assume that show_date and show_ident_date
cannot return NULL. For the most part, we handle missing or
corrupt timestamps by showing the epoch time t=0.
However, we might still return NULL if gmtime rejects the
time_t we feed it, resulting in a segfault. Let's catch this
case and just format t=0.
Signed-off-by: Jeff King p...@peff.net
---
This test is of questionable portability, since we are depending on
gmtime's arbitrary point to decide that our input is crazy and return
NULL. The value is sufficiently large that I'd expect most to do so,
though, so it may be safe.
Just came around to testing on FreeBSD 10 amd64; the new test in t4212
fails there:
--- expect 2014-03-22 08:29:44.0 +
+++ actual 2014-03-22 08:29:44.0 +
@@ -1 +1 @@
-Thu Jan 1 00:00:00 1970 +
+Sun Jan 0 00:00:00 1900 -0700
not ok 9 - absurdly far-in-future dates produce sentinel
#
# commit=$(munge_author_date HEAD 99)
# echo Thu Jan 1 00:00:00 1970 + expect
# git log -1 --format=%ad $commit actual
# test_cmp expect actual
#
# failed 1 among 9 test(s)
Looks like we get a cleared struct tm instead of a NULL pointer. It
seems to be a long-standing bug;
http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/145341 was filed in
April 2010.
On 32-bit systems, of course, the test does nothing (it is just hitting
the integer overflow code path). But that's OK, since the output is the
same for both cases.
date.c | 6 --
t/t4212-log-corrupt.sh | 8
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/date.c b/date.c
index 90b28f7..e1a2cee 100644
--- a/date.c
+++ b/date.c
@@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum
date_mode mode)
tz = local_tzoffset(time);
tm = time_to_tm(time, tz);
- if (!tm)
- return NULL;
+ if (!tm) {
Would it make sense to work around the FreeBSD issue by adding a check
like this?
if (!tm || tm-tm_year 70) {
+ tm = time_to_tm(0, 0);
+ tz = 0;
+ }
strbuf_reset(timebuf);
if (mode == DATE_SHORT)
diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh
index ba25a2e..3fa1715 100755
--- a/t/t4212-log-corrupt.sh
+++ b/t/t4212-log-corrupt.sh
@@ -81,4 +81,12 @@ test_expect_success 'date parser recognizes time_t overflow'
'
test_cmp expect actual
'
+# date is within 2^63-1, but enough to choke glibc's gmtime
+test_expect_success 'absurdly far-in-future dates produce sentinel' '
+ commit=$(munge_author_date HEAD 99)
+ echo Thu Jan 1 00:00:00 1970 + expect
+ git log -1 --format=%ad $commit actual
+ test_cmp expect actual
+'
+
test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
