Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jun 15, 2024, 11:47 UTC
- Message-ID
- <CAOLa=ZT_ZSV7gCexZhcKBvqJCva5UEY+66TgqDUoBo=q=pOqzQ@mail.gmail.com>
- In-Reply-To
- <xmqqwmmsiakq.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 61 quoted lines
> Junio C Hamano <gitster@pobox.com> writes: > >> The CI build on Windows tells me that my worry was warranted. >> >> https://github.com/git/git/actions/runs/9424299208/job/25964281907#step:4:643 >> >> (GitHub seems to show the breakage details only to those who are >> logged in, so you'd need to be logged in to visit that link) > > So here is what I accumulated in SQUASH??? patches on top of your > topic while waiting for an updated version to unbreak the CI. > > * The "end of git time" timestamp does not fit in time_t on 32-bit > systems, so I updated it to use timestamp_t at least for now. > > * t0006 has two tests that use TIME_IS_64BIT,TIME_T_IS_64BIT > prerequisites; I introduced HAVE_64BIT_TIME to simplify them. > > * nobody passes $4 to check_parse to tell it to expect a failure, > so I removed it. It always expects success. > > * check_parse now honors a global variable REQUIRE_64BIT_TIME that > is used as the prerequisite to run its test_expect_success; the > "near the end of git time" tests you added use the mechanism to > pass HAVE_64BIT_TIME prerequisite. > > The last one is a bit questionable, as it only "punts" on 32-bit > systems, instead of making sure we get the expected error messages. > I think it is OK to punt here and have a separate test that checks > timestamp around year 2040 for that condition. > > date.c | 2 +- > t/t0006-date.sh | 20 ++++++++++++++------ > 2 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/date.c b/date.c > index 95776c8a92..bee9fe8f10 100644 > --- a/date.c > +++ b/date.c > @@ -870,7 +870,7 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in > > > /* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */ > -static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1; > +static const timestamp_t timestamp_max = (((timestamp_t)2100 - 1970) * 365 + 32) * 24 * 60 * 60 - 1; > > /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822 > (i.e. English) day/month names, and it doesn't work correctly with %z. */ > diff --git a/t/t0006-date.sh b/t/t0006-date.sh > index e8fdf361ad..fd373e1b39 100755 > --- a/t/t0006-date.sh > +++ b/t/t0006-date.sh > @@ -8,6 +8,11 @@ TEST_PASSES_SANITIZE_LEAK=true > # arbitrary reference time: 2009-08-30 19:20:00 > GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW > > +if test_have_prereq TIME_IS_64BIT,TIME_T_IS_64BIT > +then > + test_set_prereq HAVE_64BIT_TIME > +fi > +
This does make sense, I did check and noticed that the two are always used together everywhere, so outside of these patches, it perhaps would also make sense to combine them altogether.
Show 41 quoted lines
> check_relative() {
> t=$(($GIT_TEST_DATE_NOW - $1))
> echo "$t -> $2" >expect
> @@ -80,14 +85,15 @@ check_show raw "$TIME" '1466000000 -0200'
>
> # arbitrary time absurdly far in the future
> FUTURE="5758122296 -0400"
> -check_show iso "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT,TIME_T_IS_64BIT
> -check_show iso-local "$FUTURE" "2152-06-19 22:24:56 +0000" TIME_IS_64BIT,TIME_T_IS_64BIT
> +check_show iso "$FUTURE" "2152-06-19 18:24:56 -0400" HAVE_64BIT_TIME
> +check_show iso-local "$FUTURE" "2152-06-19 22:24:56 +0000" HAVE_64BIT_TIME
>
> -check_parse() {
> +REQUIRE_64BIT_TIME=
> +check_parse () {
> echo "$1 -> $2" >expect
> - test_expect_${4:-success} "parse date ($1${3:+ TZ=$3})" "
> - TZ=${3:-$TZ} test-tool date parse '$1' >actual &&
> - test_cmp expect actual
> + test_expect_success $REQUIRE_64BIT_TIME "parse date ($1${3:+ TZ=$3}) -> $2" "
> + TZ=${3:-$TZ} test-tool date parse '$1' >actual &&
> + test_cmp expect actual
> "
> }
>
> @@ -133,6 +139,7 @@ check_parse '1969-12-31 23:59:59 Z' bad
> check_parse '1969-12-31 23:59:59 +11' bad
> check_parse '1969-12-31 23:59:59 -11' bad
>
> +REQUIRE_64BIT_TIME=HAVE_64BIT_TIME
> check_parse '2099-12-31 23:59:59' '2099-12-31 23:59:59 +0000'
> check_parse '2099-12-31 23:59:59 +00' '2099-12-31 23:59:59 +0000'
> check_parse '2099-12-31 23:59:59 Z' '2099-12-31 23:59:59 +0000'
> @@ -147,6 +154,7 @@ check_parse '2100-00-00 00:00:00 +00' bad
> check_parse '2100-00-00 00:00:00 Z' bad
> check_parse '2100-00-00 00:00:00 -11' bad
> check_parse '2100-00-00 00:00:00 +11' bad
> +REQUIRE_64BIT_TIME=
>
> check_approxidate() {
> echo "$1 -> $2 +0000" >expectI think this patch looks good. I also think we should probably look into fixing the 2099 limit overall. I think Jeff already posted a patch to do the same already:
https://lore.kernel.org/r/20240604093345.GA1279521@coredump.intra.peff.net