Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime
On Tue, Apr 01, 2014 at 12:07:22PM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > On Fri, Mar 28, 2014 at 12:30:02PM -0700, Junio C Hamano wrote:
> >
> >> Let's just deal with a simple known cases (like FreeBSD) in the real
> >> code that everybody exercises at runtime, and have the new test only
> >> check we do not segfault on a value we used to segfault upon seeing.
> >
> > OK. Here it is, with the other option as an "alt" patch for reference.
> >
> > [1/2]: date: recognize bogus FreeBSD gmtime output
> > [2/2]: t4212: loosen far-in-future test for AIX
> > [2alt/2]: work around unreliable gmtime errors on AIX
> >
> > -Peff
>
> Thanks. 2alt does not look too bad, but on the other hand, we are
> replacing a value that can produce the right result on correctly
> implemented gmtime with a completely bogus value only because we
> know there exists one broken implementation---which does not sound a
> very good trade-off, given that we would get a result that does not
> correspond to the input anyway with or without the change on the
> broken implementation.
One other option is to push _all_ callers through a git_gmtime()
wrapper, and then use Makefile knobs to turn on specific quirks, like:
struct tm *git_gmtime_r(const time_t *timep, struct tm *result)
{
#ifdef GMTIME_OVERFLOWS_32BIT
if (*timep > 999)
return NULL;
#endif
ret = gmtime_r(timep, result);
#ifdef GMTIME_ERROR_BLANK
if (ret && !ret->tm_mday)
return NULL;
#endif
return ret;
}
and then each platform can turn the knobs as appropriate.
-Peff
--
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] t4212: handle systems with post-apocalyptic gmtime
Jeff King writes: > On Fri, Mar 28, 2014 at 12:30:02PM -0700, Junio C Hamano wrote: > >> Let's just deal with a simple known cases (like FreeBSD) in the real >> code that everybody exercises at runtime, and have the new test only >> check we do not segfault on a value we used to segfault upon seeing. > > OK. Here it is, with the other option as an "alt" patch for reference. > > [1/2]: date: recognize bogus FreeBSD gmtime output > [2/2]: t4212: loosen far-in-future test for AIX > [2alt/2]: work around unreliable gmtime errors on AIX > > -Peff Thanks. 2alt does not look too bad, but on the other hand, we are replacing a value that can produce the right result on correctly implemented gmtime with a completely bogus value only because we know there exists one broken implementation---which does not sound a very good trade-off, given that we would get a result that does not correspond to the input anyway with or without the change on the broken implementation. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Fri, Mar 28, 2014 at 12:30:02PM -0700, Junio C Hamano wrote: > Let's just deal with a simple known cases (like FreeBSD) in the real > code that everybody exercises at runtime, and have the new test only > check we do not segfault on a value we used to segfault upon seeing. OK. Here it is, with the other option as an "alt" patch for reference. [1/2]: date: recognize bogus FreeBSD gmtime output [2/2]: t4212: loosen far-in-future test for AIX [2alt/2]: work around unreliable gmtime errors on AIX -Peff -- 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] t4212: handle systems with post-apocalyptic gmtime
Jeff King writes: > This (non-)issue has consumed a lot more brain power than it is probably > worth. I'd like to figure out which patch to go with and be done. :) Let's just deal with a simple known cases (like FreeBSD) in the real code that everybody exercises at runtime, and have the new test only check we do not segfault on a value we used to segfault upon seeing. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Fri, Mar 28, 2014 at 12:02:46PM -0700, Junio C Hamano wrote:
> >> - teach the "is the result sane, even though we may have got a
> >>non-NULL from gmtime? otherwise let's signal a failure by
> >>replacing it with a known sentinel value" codepath the new
> >>failure mode Charles's report suggests---if we feed a positive
> >>timestamp and gmtime gave us back a tm_year+1900 < 0, that is
> >>certainly an overflow; and
> >
> > I don't think we can analyze the output from gmtime. If it wraps the
> > year at N, then won't N+2014 look like a valid value?
>
> Yes, but I was hoping that there are small number of possible N's
> ;-)
I'm not sure I understand. Even if we know N, we've lost information
during the truncation done by time_t (we cannot distingiuish true M from
N+M).
> > diff --git a/date.c b/date.c
> > index e1a2cee..e0c43c4 100644
> > --- a/date.c
> > +++ b/date.c
> > @@ -57,6 +57,8 @@ static time_t gm_time_t(unsigned long time, int tz)
> > static struct tm *time_to_tm(unsigned long time, int tz)
> > {
> > time_t t = gm_time_t(time, tz);
> > + if (t > )
> > + return NULL;
> > return gmtime(&t);
> > }
> >
> > I suspect that would handle the FreeBSD case, as well.
> >
> > By the way, I have a suspicion that the gm_time_t above can overflow if
> > you specially craft a value at the edge of what time_t can handle (we
> > check that our value will not overflow time_t earlier, but now we might
> > be adding up to 86400 seconds to it).
>
> Yuck. Let's not go there.
Do you mean "let's not worry about the absurdly specific overflow case",
or "let's not do this gross time_to_tm hack?"
This (non-)issue has consumed a lot more brain power than it is probably
worth. I'd like to figure out which patch to go with and be done. :)
-Peff
--
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] t4212: handle systems with post-apocalyptic gmtime
Jeff King writes:
> On Fri, Mar 28, 2014 at 09:41:53AM -0700, Junio C Hamano wrote:
>
>> Offhand, the three possible failure modes this thread identified
>> sounds to me like the only plausible ones, and I think the best way
>> forward might be to
>>
>> - teach the "is the result sane, even though we may have got a
>>non-NULL from gmtime? otherwise let's signal a failure by
>>replacing it with a known sentinel value" codepath the new
>>failure mode Charles's report suggests---if we feed a positive
>>timestamp and gmtime gave us back a tm_year+1900 < 0, that is
>>certainly an overflow; and
>
> I don't think we can analyze the output from gmtime. If it wraps the
> year at N, then won't N+2014 look like a valid value?
Yes, but I was hoping that there are small number of possible N's
;-)
> If we are going to do something trustworthy I think it has to be before
> we hand off to gmtime. Like:
>
> diff --git a/date.c b/date.c
> index e1a2cee..e0c43c4 100644
> --- a/date.c
> +++ b/date.c
> @@ -57,6 +57,8 @@ static time_t gm_time_t(unsigned long time, int tz)
> static struct tm *time_to_tm(unsigned long time, int tz)
> {
> time_t t = gm_time_t(time, tz);
> + if (t > )
> + return NULL;
> return gmtime(&t);
> }
>
> I suspect that would handle the FreeBSD case, as well.
>
> By the way, I have a suspicion that the gm_time_t above can overflow if
> you specially craft a value at the edge of what time_t can handle (we
> check that our value will not overflow time_t earlier, but now we might
> be adding up to 86400 seconds to it).
Yuck. Let's not go there.
Thanks.
--
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] t4212: handle systems with post-apocalyptic gmtime
On Fri, Mar 28, 2014 at 09:41:53AM -0700, Junio C Hamano wrote:
> Offhand, the three possible failure modes this thread identified
> sounds to me like the only plausible ones, and I think the best way
> forward might be to
>
> - teach the "is the result sane, even though we may have got a
>non-NULL from gmtime? otherwise let's signal a failure by
>replacing it with a known sentinel value" codepath the new
>failure mode Charles's report suggests---if we feed a positive
>timestamp and gmtime gave us back a tm_year+1900 < 0, that is
>certainly an overflow; and
I don't think we can analyze the output from gmtime. If it wraps the
year at N, then won't N+2014 look like a valid value?
If we are going to do something trustworthy I think it has to be before
we hand off to gmtime. Like:
diff --git a/date.c b/date.c
index e1a2cee..e0c43c4 100644
--- a/date.c
+++ b/date.c
@@ -57,6 +57,8 @@ static time_t gm_time_t(unsigned long time, int tz)
static struct tm *time_to_tm(unsigned long time, int tz)
{
time_t t = gm_time_t(time, tz);
+ if (t > )
+ return NULL;
return gmtime(&t);
}
I suspect that would handle the FreeBSD case, as well.
By the way, I have a suspicion that the gm_time_t above can overflow if
you specially craft a value at the edge of what time_t can handle (we
check that our value will not overflow time_t earlier, but now we might
be adding up to 86400 seconds to it).
-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] t4212: handle systems with post-apocalyptic gmtime
Jeff King writes: >> Sat Jan 25 10:46:39 316889355 -0700 >> 9 Wed Sep 6 02:46:39 -1126091476 -0700 >> 99 Thu Oct 24 18:46:39 1623969404 -0700 > > Thanks. Given the value where it fails, it kind of looks like there is > some signed 32-bit value at work (~300 million years is OK, but 10 times > that, rather than yielding ~3 billion, gets us -1 billion). Perhaps > tm.tm_year is 32-bit. That is what it looks like to me, too. It makes me wonder if some other platforms may have similar breakage using 16-bit signed tm_year and how such a breakage would look like, though ;-) > So what do we want to do? I think the options are: > > 1. Try to guess when we have a bogus timestamp value with an arbitrary > cutoff like "greater than 1 million years from now" (and enforce it > via time_t seconds, and avoid gmtime entirely). That is made-up and > arbitrary, but it also is sufficiently far that it won't ever > matter, and sufficiently close that any gmtime should behave > sensibly with it. I think that two assumptions here are that (1) we would be able to find a single insane value (like 3 billion years from now expressed in time_t seconds) the test script would be able to feed and expect it to fail on all platforms we care about, even though how exactly that value causes various platforms fail may be different (glibc may give us a NULL from gmtime, FreeBSD may leave their own sentinel in gmtime we can recognize, and some others may simply wrap around the years); and that (2) the only other class of failure mode we haven't considered bevore Charles's report is tm_year wrapping around 32-bit signed int. Offhand, the three possible failure modes this thread identified sounds to me like the only plausible ones, and I think the best way forward might be to - teach the "is the result sane, even though we may have got a non-NULL from gmtime? otherwise let's signal a failure by replacing it with a known sentinel value" codepath the new failure mode Charles's report suggests---if we feed a positive timestamp and gmtime gave us back a tm_year+1900 < 0, that is certainly an overflow; and - Use that 3-billion years timestamp from Charles's report in the test and make sure we convert it to the known sentinel value. perhaps? > 2. Accept that we can't guess at every broken gmtime's output, and > just loosen the test to make sure we don't segfault. Of course that is a simpler option, but we may have identified all plausible failure modes, in which case we can afford to go with your original plan to validate that we not just avoid segfaulting on one of the three failure modes from gmtime, but also cover the other two failure modes and signal any of them with a sentinel. That way may allow us to identify the fourth failure mode we haven't anticipated. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 10:46:16PM +, Charles Bailey wrote: > On Wed, Mar 26, 2014 at 05:57:41PM -0400, Jeff King wrote: > > Hmm, so the year you got is actually: 1623969404. That still seems off > > to me by a factor 20. I don't know if this is really worth digging into > > that much further, but I wonder what you would get for timestamps of: > > > > 9 > > > > 999 > > etc. > > > > AIX goes negative at about the same time Linux and Solaris segfault: > > 999 Sun Apr 26 10:46:39 1970 -0700 > Sat Mar 3 02:46:39 1973 -0700 > 9 Sat Sep 8 18:46:39 2001 -0700 > 99 Sat Nov 20 10:46:39 2286 -0700 > 999 Wed Nov 16 02:46:39 5138 -0700 > Thu Sep 26 18:46:39 33658 -0700 > 9 Sun May 20 10:46:39 318857 -0700 > 99 Sat Nov 7 02:46:39 3170843 -0700 > 999 Sat Jul 4 18:46:39 31690708 -0700 > Sat Jan 25 10:46:39 316889355 -0700 > 9 Wed Sep 6 02:46:39 -1126091476 -0700 > 99 Thu Oct 24 18:46:39 1623969404 -0700 Thanks. Given the value where it fails, it kind of looks like there is some signed 32-bit value at work (~300 million years is OK, but 10 times that, rather than yielding ~3 billion, gets us -1 billion). Perhaps tm.tm_year is 32-bit. So what do we want to do? I think the options are: 1. Try to guess when we have a bogus timestamp value with an arbitrary cutoff like "greater than 1 million years from now" (and enforce it via time_t seconds, and avoid gmtime entirely). That is made-up and arbitrary, but it also is sufficiently far that it won't ever matter, and sufficiently close that any gmtime should behave sensibly with it. 2. Accept that we can't guess at every broken gmtime's output, and just loosen the test to make sure we don't segfault. -Peff -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 05:57:41PM -0400, Jeff King wrote: > Hmm, so the year you got is actually: 1623969404. That still seems off > to me by a factor 20. I don't know if this is really worth digging into > that much further, but I wonder what you would get for timestamps of: > > 9 > > 999 > etc. > AIX goes negative at about the same time Linux and Solaris segfault: 999 Sun Apr 26 10:46:39 1970 -0700 Sat Mar 3 02:46:39 1973 -0700 9 Sat Sep 8 18:46:39 2001 -0700 99 Sat Nov 20 10:46:39 2286 -0700 999 Wed Nov 16 02:46:39 5138 -0700 Thu Sep 26 18:46:39 33658 -0700 9 Sun May 20 10:46:39 318857 -0700 99 Sat Nov 7 02:46:39 3170843 -0700 999 Sat Jul 4 18:46:39 31690708 -0700 Sat Jan 25 10:46:39 316889355 -0700 9 Wed Sep 6 02:46:39 -1126091476 -0700 99 Thu Oct 24 18:46:39 1623969404 -0700 So, very bogus values. 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 09:22:27PM +, Charles Bailey wrote: > On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > > > That being said, is the AIX value actually right? I did not look closely > > at first, but just assumed that it was vaguely right. But: > > > > 99 / (86400 * 365) > > > > is something like 31 billion years in the future, not 160 million. > > A real date calculation will have a few tweaks (leap years, etc), but > > that is orders of magnitude off. > > Well, this is embarrassing, while moving this through the corporate > firewall (aka typing on one machine while looking at another), I > munged the date. It still doesn't seem right but at least you can now > see the actual data. Hmm, so the year you got is actually: 1623969404. That still seems off to me by a factor 20. I don't know if this is really worth digging into that much further, but I wonder what you would get for timestamps of: 9 999 etc. Do we start generating weird values at some particular size? Or is AIX gmtime really more clever than I am, and is accounting for wobble of the Earth or something over the next billion years? -Peff -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > That being said, is the AIX value actually right? I did not look closely > at first, but just assumed that it was vaguely right. But: > > 99 / (86400 * 365) > > is something like 31 billion years in the future, not 160 million. > A real date calculation will have a few tweaks (leap years, etc), but > that is orders of magnitude off. Well, this is embarrassing, while moving this through the corporate firewall (aka typing on one machine while looking at another), I munged the date. It still doesn't seem right but at least you can now see the actual data. I stopped the test with --immediate and found the dangling commit that the test created and dumped it with the previous version of Git (well a 1.8.5.5 build) ibm: trash directory.t4212-log-corrupt $ git log -1 --pretty=raw 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca tree 64fd3796c57084e7b8cbae358ce37970b8e954f6 author A U Thor 99 -0700 committer C O Mitter 1112911993 -0700 foo ibm: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca Author: A U Thor Date: Thu Oct 24 18:46:39 1623969404 -0700 foo Same commit but dumped from a linux machine: linux: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca Author: A U Thor Date: (null) foo 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 04:38:30PM -0400, Jeff King wrote: > > By the way, can you confirm that this is a 64-bit system? On a 32-bit > system, we should be triggering different code paths (we fail at the > strtoul level). Those should be checked by the previous tests, but I'd > like to make sure. Yes, we're only building 64-bit at the moment. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 08:36:18PM +, Charles Bailey wrote: > On Wed, Mar 26, 2014 at 03:40:43PM -0400, Jeff King wrote: > > On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > > > > That being said, is the AIX value actually right? I did not look closely > > > at first, but just assumed that it was vaguely right. But: > > > > > > 99 / (86400 * 365) > > > > > > is something like 31 billion years in the future, not 160 million. > > > A real date calculation will have a few tweaks (leap years, etc), but > > > that is orders of magnitude off. > > > > Assuming my math is right, then here is the most sensible patch, IMHO. > > > > Perhaps hold onto this one for a little while longer. Splitting things > out from the test is giving me some inconsistent results, there may be > something else going wrong in our environment here. By the way, can you confirm that this is a 64-bit system? On a 32-bit system, we should be triggering different code paths (we fail at the strtoul level). Those should be checked by the previous tests, but I'd like to make sure. -Peff -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:40:43PM -0400, Jeff King wrote: > On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > > That being said, is the AIX value actually right? I did not look closely > > at first, but just assumed that it was vaguely right. But: > > > > 99 / (86400 * 365) > > > > is something like 31 billion years in the future, not 160 million. > > A real date calculation will have a few tweaks (leap years, etc), but > > that is orders of magnitude off. > > Assuming my math is right, then here is the most sensible patch, IMHO. > Perhaps hold onto this one for a little while longer. Splitting things out from the test is giving me some inconsistent results, there may be something else going wrong in our environment here. Charles. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > That being said, is the AIX value actually right? I did not look closely > at first, but just assumed that it was vaguely right. But: > > 99 / (86400 * 365) > > is something like 31 billion years in the future, not 160 million. > A real date calculation will have a few tweaks (leap years, etc), but > that is orders of magnitude off. Assuming my math is right, then here is the most sensible patch, IMHO. -- >8 -- Subject: t4212: loosen far-in-future test for AIX One of the tests in t4212 checks our behavior when we feed gmtime a date so far in the future that it gives up and returns NULL. Some implementations, like AIX, may actually just provide us a bogus result instead. It's not worth it for us to come up with heuristics that guess whether the return value is sensible or not. On good platforms where gmtime reports the problem to us with NULL, we will print the epoch value. On bad platforms, we will print garbage. But our test should be written for the lowest common denominator so that it passes everywhere. Reported-by: Charles Bailey Signed-off-by: Jeff King --- t/t4212-log-corrupt.sh | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh index 85c6df4..bb843ab 100755 --- a/t/t4212-log-corrupt.sh +++ b/t/t4212-log-corrupt.sh @@ -77,11 +77,14 @@ test_expect_success 'date parser recognizes time_t overflow' ' ' # date is within 2^63-1, but enough to choke glibc's gmtime -test_expect_success 'absurdly far-in-future dates produce sentinel' ' +# +# Ideally we would check the output to make sure we replaced it with +# a useful sentinel value, but some platforms will actually hand us back +# a nonsensical date. It is not worth our time to try to evaluate these +# dates, so just make sure we didn't segfault or otherwise abort. +test_expect_success 'absurdly far-in-future dates' ' 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 log -1 --format=%ad $commit ' test_done -- 1.9.1.656.ge8a0637 -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:25:36PM -0400, Jeff King wrote: > > The primary thing you wanted to achieve by the "gmtime gave us NULL, > > let's substitute it with an arbitrary value to avoid dereferencing > > the NULL" change was *not* that we see that same arbitrary value > > comes out of the system, but that we do not die by attempting to > > reference the NULL, I think. Not dying is the primary thing we want > > to (and we already do) test, no? > > I think there are really two separate behaviors we are testing here (and > in the surrounding tests): > > 1. Don't segfault if gmtime returns NULL. > > 2. Whenever we cannot process a date (either because gmtime fails, or > because we fail before even getting the value to gmtime), > consistently return the sentinel date (so the reader can easily > know it's bogus). > > Having the test be particular about its output helped us find a case > where FreeBSD did not trigger (1), but did trigger (2), by returning a > blanked "struct tm". > > I'm open to the argument that (2) is not worth worrying about that much > if it is a hassle to test. But I don't think it is that much hassle > (yet, anyway). That being said, is the AIX value actually right? I did not look closely at first, but just assumed that it was vaguely right. But: 99 / (86400 * 365) is something like 31 billion years in the future, not 160 million. A real date calculation will have a few tweaks (leap years, etc), but that is orders of magnitude off. So I am not sure that AIX is not actually just giving us utter crap. In that case, the test is not wrong; it's pickiness is actually finding a real problem. But I am not sure it is a problem worth solving. I do not want to get into heuristics deciding whether a particular platform's gmtime output is crap or not. That pushes this into the realm of "it's not worth testing", and we should stick to just testing that we did not segfault. -Peff -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 12:18:25PM -0700, Junio C Hamano wrote: > > + echo "$candidate" >expect && > > + test_cmp expect actual && > > + return 0 > > + done > > + return 1 > > +} > > It actually may be easier to understand if you write a trivial case > statement at the sole calling site of this helper function, though. Yeah, perhaps. I wanted to build on test_cmp because it has nice output for the failure case, but it is probably simple enough to do: output=$(cat actual) case "$output" in ... *) echo >&2 "unrecognized date: $output" > In any case, would it really be essential to make sure that the > output shows a phony (or a seemingly real) datestamp for this test? > > The primary thing you wanted to achieve by the "gmtime gave us NULL, > let's substitute it with an arbitrary value to avoid dereferencing > the NULL" change was *not* that we see that same arbitrary value > comes out of the system, but that we do not die by attempting to > reference the NULL, I think. Not dying is the primary thing we want > to (and we already do) test, no? I think there are really two separate behaviors we are testing here (and in the surrounding tests): 1. Don't segfault if gmtime returns NULL. 2. Whenever we cannot process a date (either because gmtime fails, or because we fail before even getting the value to gmtime), consistently return the sentinel date (so the reader can easily know it's bogus). Having the test be particular about its output helped us find a case where FreeBSD did not trigger (1), but did trigger (2), by returning a blanked "struct tm". I'm open to the argument that (2) is not worth worrying about that much if it is a hassle to test. But I don't think it is that much hassle (yet, anyway). -Peff -- 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] t4212: handle systems with post-apocalyptic gmtime
Jeff King writes:
> +cmp_one_of () {
> + for candidate in "$@"; do
Style ;-)
> + echo "$candidate" >expect &&
> + test_cmp expect actual &&
> + return 0
> + done
> + return 1
> +}
It actually may be easier to understand if you write a trivial case
statement at the sole calling site of this helper function, though.
In any case, would it really be essential to make sure that the
output shows a phony (or a seemingly real) datestamp for this test?
The primary thing you wanted to achieve by the "gmtime gave us NULL,
let's substitute it with an arbitrary value to avoid dereferencing
the NULL" change was *not* that we see that same arbitrary value
comes out of the system, but that we do not die by attempting to
reference the NULL, I think. Not dying is the primary thing we want
to (and we already do) test, no?
> +# date is within 2^63-1, but enough to choke glibc's gmtime.
> +# We check that either the date broke gmtime (and we return the
> +# usual epoch value), or gmtime gave us some sensible value.
> +#
> +# The sensible values are determined experimentally. The first
> +# is from AIX.
> +test_expect_success 'absurdly far-in-future dates' '
> 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
> + cmp_one_of \
> + "Thu Jan 1 00:00:00 1970 +" \
> + "Thu Oct 24 18:46:39 162396404 -0700"
> '
>
> 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
[PATCH] t4212: handle systems with post-apocalyptic gmtime
One of the tests in t4212 checks our behavior when we feed
gmtime a date so far in the future that it gives up and
returns NULL.
But some gmtime implementations just refuse to quit. They
soldier on, giving us a glimpse of a chilly October evening
some 160 million years in the future (and presumably filled
with our leather-clad horseback-riding ape descendants).
Let's allow the test to match either the sentinel value
(i.e., what we want when gmtime gives up) or any reasonable
value returned by known implementations.
Reported-by: Charles Bailey
Signed-off-by: Jeff King
---
On top of jk/commit-dates-parsing-fix (though the test is already in
v1.9.1).
t/t4212-log-corrupt.sh | 23 +++
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh
index 85c6df4..08f982c 100755
--- a/t/t4212-log-corrupt.sh
+++ b/t/t4212-log-corrupt.sh
@@ -76,12 +76,27 @@ 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' '
+cmp_one_of () {
+ for candidate in "$@"; do
+ echo "$candidate" >expect &&
+ test_cmp expect actual &&
+ return 0
+ done
+ return 1
+}
+
+# date is within 2^63-1, but enough to choke glibc's gmtime.
+# We check that either the date broke gmtime (and we return the
+# usual epoch value), or gmtime gave us some sensible value.
+#
+# The sensible values are determined experimentally. The first
+# is from AIX.
+test_expect_success 'absurdly far-in-future dates' '
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
+ cmp_one_of \
+ "Thu Jan 1 00:00:00 1970 +" \
+ "Thu Oct 24 18:46:39 162396404 -0700"
'
test_done
--
1.9.1.656.ge8a0637
--
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
