git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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" >expect

I 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
Previous: Junio C HamanoNext: Junio C Hamano
Message 19 of 29 in “fix: prevent date underflow when using positive timezone offset”
  1. fix: prevent date underflow when using positive timezone offsetdarcy via GitGitGadget, May 27, 2024
  2. Patrick SteinhardtMay 28, 2024
  3. Phillip WoodMay 28, 2024
  4. Junio C HamanoMay 28, 2024
  5. date: detect underflow when parsing dates with positive timezone offsetdarcy via GitGitGadget, Jun 2, 2024
  6. Junio C HamanoJun 3, 2024
  7. darcyJun 3, 2024
  8. Phillip WoodJun 3, 2024
  9. darcyJun 4, 2024
  10. Jeff KingJun 4, 2024
  11. darcyJun 5, 2024
  12. Phillip WoodJun 5, 2024
  13. Junio C HamanoJun 5, 2024
  14. darcyJun 6, 2024
  15. date: detect underflow/overflow when parsing dates with timezone offsetdarcy via GitGitGadget, Jun 7, 2024
  16. Junio C HamanoJun 7, 2024
  17. Junio C HamanoJun 8, 2024
  18. Junio C HamanoJun 14, 2024
  19. Karthik NayakJun 15, 2024
  20. Junio C HamanoJun 11, 2024
  21. rsbecker@nexbridge.comJun 11, 2024
  22. Junio C HamanoJun 11, 2024
  23. Phillip WoodJun 12, 2024
  24. Karthik NayakJun 12, 2024
  25. Phillip WoodJun 13, 2024
  26. Junio C HamanoJun 13, 2024
  27. Karthik NayakJun 14, 2024
  28. Junio C HamanoJun 14, 2024
  29. Karthik NayakJun 15, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.