{"thread":{"id":"61551","subject":"[PATCH] fix: prevent date underflow when using positive timezone offset","startedAt":"2024-05-27T09:17:10Z","lastAt":"2024-06-15T11:49:28Z","messageCount":29,"participants":["darcy via GitGitGadget","Patrick Steinhardt","Phillip Wood","Junio C Hamano","darcy","Jeff King","rsbecker@nexbridge.com","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"495669","messageId":"pull.1726.git.git.1716801427015.gitgitgadget@gmail.com","threadId":"61551","inReplyTo":null,"subject":"[PATCH] fix: prevent date underflow when using positive timezone offset","fromName":"darcy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-27T09:17:06Z","receivedAt":"2024-05-27T09:17:10Z","isPatch":true,"sender":{"key":"acednes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44690813?v=4"},"body":"From: darcy <acednes@gmail.com>\n\nOverriding the date of a commit to be `1970-01-01` with a large enough\ntimezone for the equivalent GMT time to before 1970 is no longer\naccepted.\n\nExample: `GIT_COMMITTER_DATE='1970-01-01T00:00:00+10' git commit` would\npreviously be accepted, only to unexpectedly fail in other parts of the\ncode, such as `git push`. The timestamp is now checked against postitive\ntimezone values.\n\nSigned-off-by: darcy <acednes@gmail.com>\n---\n    fix: prevent date underflow when using positive timezone offset\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1726%2Fdxrcy%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1726/dxrcy/master-v1\nPull-Request: https://github.com/git/git/pull/1726\n\n date.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 7365a4ad24f..8388629f267 100644\n--- a/date.c\n+++ b/date.c\n@@ -908,7 +908,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n \t\t\tmatch = match_alpha(date, &tm, offset);\n \t\telse if (isdigit(c))\n \t\t\tmatch = match_digit(date, &tm, offset, &tm_gmt);\n-\t\telse if ((c == '-' || c == '+') && isdigit(date[1]))\n+\t\telse if ((c == '-' || c == '+') && isdigit(date[1]) && tm.tm_hour != -1)\n \t\t\tmatch = match_tz(date, offset);\n \n \t\tif (!match) {\n@@ -937,8 +937,13 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n \t\t}\n \t}\n \n-\tif (!tm_gmt)\n+\tif (!tm_gmt) {\n+\t\tif (*offset > 0 && *offset * 60 > *timestamp) {\n+\t\t\treturn -1;\n+\t\t}\n \t\t*timestamp -= *offset * 60;\n+\t}\n+\n \treturn 0; /* success */\n }\n \n\nbase-commit: b9cfe4845cb2562584837bc0101c0ab76490a239\n-- \ngitgitgadget\n"},{"id":"495752","messageId":"ZlXkvEeR-PgZMitx@tanuki","threadId":"61551","inReplyTo":"pull.1726.git.git.1716801427015.gitgitgadget@gmail.com","subject":"Re: [PATCH] fix: prevent date underflow when using positive timezone offset","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-28T14:05:48Z","receivedAt":"2024-05-28T14:05:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 27, 2024 at 09:17:06AM +0000, darcy via GitGitGadget wrote:\n> From: darcy <acednes@gmail.com>\n\nThe commit message should start with the subsystem that you're touching,\nwhich in this case would be \"date\", e.g.:\n\n    date: detect underflow when parsing dates with positive timezone offset\n\n> Overriding the date of a commit to be `1970-01-01` with a large enough\n> timezone for the equivalent GMT time to before 1970 is no longer\n> accepted.\n\nOkay.\n\n> Example: `GIT_COMMITTER_DATE='1970-01-01T00:00:00+10' git commit` would\n> previously be accepted, only to unexpectedly fail in other parts of the\n> code, such as `git push`. The timestamp is now checked against postitive\n> timezone values.\n\nHow exactly does the failure look like before and after?\n\n> Signed-off-by: darcy <acednes@gmail.com>\n> ---\n>     fix: prevent date underflow when using positive timezone offset\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1726%2Fdxrcy%2Fmaster-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1726/dxrcy/master-v1\n> Pull-Request: https://github.com/git/git/pull/1726\n> \n>  date.c | 9 +++++++--\n>  1 file changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/date.c b/date.c\n> index 7365a4ad24f..8388629f267 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -908,7 +908,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>  \t\t\tmatch = match_alpha(date, &tm, offset);\n>  \t\telse if (isdigit(c))\n>  \t\t\tmatch = match_digit(date, &tm, offset, &tm_gmt);\n> -\t\telse if ((c == '-' || c == '+') && isdigit(date[1]))\n> +\t\telse if ((c == '-' || c == '+') && isdigit(date[1]) && tm.tm_hour != -1)\n>  \t\t\tmatch = match_tz(date, offset);\n\nWithout having a deep understanding of the code I don't quite see the\nconnection between this change and the problem description. Is it\nnecessary? If so, it might help to explain why it's needed in the commit\nmessage or in the code.\n\n>  \t\tif (!match) {\n> @@ -937,8 +937,13 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>  \t\t}\n>  \t}\n>  \n> -\tif (!tm_gmt)\n> +\tif (!tm_gmt) {\n> +\t\tif (*offset > 0 && *offset * 60 > *timestamp) {\n> +\t\t\treturn -1;\n> +\t\t}\n\nNit: we don't add curly braces around one-line conditional bodies.\n\nThis change here is the meat of it and looks like I'd expect.\n\n>  \t\t*timestamp -= *offset * 60;\n> +\t}\n> +\n>  \treturn 0; /* success */\n>  }\n\nYou should also add at least one test.\n\nThanks for your contribution!\n\nPatrick\n"},{"id":"495754","messageId":"0c2addd0-104d-4142-969a-500aab39c90e@gmail.com","threadId":"61551","inReplyTo":"ZlXkvEeR-PgZMitx@tanuki","subject":"Re: [PATCH] fix: prevent date underflow when using positive timezone offset","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-05-28T14:49:26Z","receivedAt":"2024-05-28T14:49:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 28/05/2024 15:05, Patrick Steinhardt wrote:\n> On Mon, May 27, 2024 at 09:17:06AM +0000, darcy via GitGitGadget wrote:\n>> From: darcy <acednes@gmail.com>\n>> diff --git a/date.c b/date.c\n>> index 7365a4ad24f..8388629f267 100644\n>> --- a/date.c\n>> +++ b/date.c\n>> @@ -908,7 +908,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>>   \t\t\tmatch = match_alpha(date, &tm, offset);\n>>   \t\telse if (isdigit(c))\n>>   \t\t\tmatch = match_digit(date, &tm, offset, &tm_gmt);\n>> -\t\telse if ((c == '-' || c == '+') && isdigit(date[1]))\n>> +\t\telse if ((c == '-' || c == '+') && isdigit(date[1]) && tm.tm_hour != -1)\n>>   \t\t\tmatch = match_tz(date, offset);\n> \n> Without having a deep understanding of the code I don't quite see the\n> connection between this change and the problem description. Is it\n> necessary? If so, it might help to explain why it's needed in the commit\n> message or in the code.\n\nI was wondering about this change too\n\n>>   \t\tif (!match) {\n>> @@ -937,8 +937,13 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>>   \t\t}\n>>   \t}\n>>   \n>> -\tif (!tm_gmt)\n>> +\tif (!tm_gmt) {\n>> +\t\tif (*offset > 0 && *offset * 60 > *timestamp) {\n>> +\t\t\treturn -1;\n>> +\t\t}\n> \n> Nit: we don't add curly braces around one-line conditional bodies.\n> \n> This change here is the meat of it and looks like I'd expect.\n> \n>>   \t\t*timestamp -= *offset * 60;\n\nDo we also want to check for overflows in the other direction (a large \ntimestamp with a negative timezone offset)?\n\nBest Wishes\n\nPhillip\n"},{"id":"495762","messageId":"xmqqmso96f3j.fsf@gitster.g","threadId":"61551","inReplyTo":"ZlXkvEeR-PgZMitx@tanuki","subject":"Re: [PATCH] fix: prevent date underflow when using positive timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-28T17:06:24Z","receivedAt":"2024-05-28T17:06:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Mon, May 27, 2024 at 09:17:06AM +0000, darcy via GitGitGadget wrote:\n>> From: darcy <acednes@gmail.com>\n>\n> The commit message should start with the subsystem that you're touching,\n> which in this case would be \"date\", e.g.:\n>\n>     date: detect underflow when parsing dates with positive timezone offset\n>\n>> Overriding the date of a commit to be `1970-01-01` with a large enough\n>> timezone for the equivalent GMT time to before 1970 is no longer\n>> accepted.\n>\n> Okay.\n\n\"is no longer accepted\" made me read the sentence three times to get\nwhat the author wants to say.  Initially I thought the author wanted\nto report a regression where we used to accept but with a recent\nchange we stopped accepting.\n\nIn our convention, we present the status quo, point out why it is\nawkard/incorrect/bad, and then propose a new behaviour.\n\n    Overriding ... before 1970 BEHAVES THIS WAY.\n\n    This leads to BAD BEHAVIOUR FOR SUCH AND SUCH REASONS.\n\n    Instead check the timezone offset and fail if the resulting time\n    becomes before the epoch, \"1970-01-01T00:00:00Z\", when parsing.\n\nwith the blanks filled in appropriately would have been much easier\nto see.\n\n>> Example: `GIT_COMMITTER_DATE='1970-01-01T00:00:00+10' git commit` would\n>> previously be accepted, only to unexpectedly fail in other parts of the\n>> code, such as `git push`. The timestamp is now checked against postitive\n>> timezone values.\n>\n> How exactly does the failure look like before and after?\n\nYes, good question.\n\n>> Signed-off-by: darcy <acednes@gmail.com>\n>> ---\n\nI cannot offhand tell if Documentation/SubmittingPatches:real-name\nis followed here or ignored, so just to double check...\n\nEverything else in your review made sense to me.  I guess that\nchecking for tm_hour is assuming that TZ offset should always come\nbefore the values necessary to compute the timestamp comes, but it\nsmells like an unwarranted assumption and not explaining the change\nin the proposed log message is a bad sign.\n\nThanks.\n"},{"id":"496059","messageId":"pull.1726.v2.git.git.1717369608923.gitgitgadget@gmail.com","threadId":"61551","inReplyTo":"pull.1726.git.git.1716801427015.gitgitgadget@gmail.com","subject":"[PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"darcy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-02T23:06:48Z","receivedAt":"2024-06-02T23:06:53Z","isPatch":true,"sender":{"key":"acednes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44690813?v=4"},"body":"From: darcy <acednes@gmail.com>\n\nOverriding the date of a commit to be close to \"1970-01-01 00:00:00\"\nwith a large enough timezone for the equivelant GMT time to be before\nthe epoch is considered valid by `parse_date_basic`.\n\nThis leads to an integer underflow in the commit timestamp, which is not\ncaught by `git-commit`, but will cause other services to fail, such as\n`git-fsck`, which reports \"badDateOverflow: invalid author/committer\nline - date causes integer overflow\".\n\nInstead check the timezone offset and fail if the resulting time comes\nbefore the epoch, \"1970-01-01T00:00:00Z\", when parsing.\n\nSigned-off-by: Darcy Burke <acednes@gmail.com>\n---\n    fix: prevent date underflow when using positive timezone offset\n    \n    cc: Patrick Steinhardt ps@pks.im cc: Phillip Wood\n    phillip.wood123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1726%2Fdxrcy%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1726/dxrcy/master-v2\nPull-Request: https://github.com/git/git/pull/1726\n\nRange-diff vs v1:\n\n 1:  4542d984aab ! 1:  db508b2f533 fix: prevent date underflow when using positive timezone offset\n     @@ Metadata\n      Author: darcy <acednes@gmail.com>\n      \n       ## Commit message ##\n     -    fix: prevent date underflow when using positive timezone offset\n     +    date: detect underflow when parsing dates with positive timezone offset\n      \n     -    Overriding the date of a commit to be `1970-01-01` with a large enough\n     -    timezone for the equivalent GMT time to before 1970 is no longer\n     -    accepted.\n     +    Overriding the date of a commit to be close to \"1970-01-01 00:00:00\"\n     +    with a large enough timezone for the equivelant GMT time to be before\n     +    the epoch is considered valid by `parse_date_basic`.\n      \n     -    Example: `GIT_COMMITTER_DATE='1970-01-01T00:00:00+10' git commit` would\n     -    previously be accepted, only to unexpectedly fail in other parts of the\n     -    code, such as `git push`. The timestamp is now checked against postitive\n     -    timezone values.\n     +    This leads to an integer underflow in the commit timestamp, which is not\n     +    caught by `git-commit`, but will cause other services to fail, such as\n     +    `git-fsck`, which reports \"badDateOverflow: invalid author/committer\n     +    line - date causes integer overflow\".\n      \n     -    Signed-off-by: darcy <acednes@gmail.com>\n     +    Instead check the timezone offset and fail if the resulting time comes\n     +    before the epoch, \"1970-01-01T00:00:00Z\", when parsing.\n     +\n     +    Signed-off-by: Darcy Burke <acednes@gmail.com>\n      \n       ## date.c ##\n     -@@ date.c: int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n     - \t\t\tmatch = match_alpha(date, &tm, offset);\n     - \t\telse if (isdigit(c))\n     - \t\t\tmatch = match_digit(date, &tm, offset, &tm_gmt);\n     --\t\telse if ((c == '-' || c == '+') && isdigit(date[1]))\n     -+\t\telse if ((c == '-' || c == '+') && isdigit(date[1]) && tm.tm_hour != -1)\n     - \t\t\tmatch = match_tz(date, offset);\n     - \n     - \t\tif (!match) {\n      @@ date.c: int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n       \t\t}\n       \t}\n       \n      -\tif (!tm_gmt)\n      +\tif (!tm_gmt) {\n     -+\t\tif (*offset > 0 && *offset * 60 > *timestamp) {\n     ++\t\tif (*offset > 0 && *offset * 60 > *timestamp)\n      +\t\t\treturn -1;\n     -+\t\t}\n       \t\t*timestamp -= *offset * 60;\n      +\t}\n      +\n       \treturn 0; /* success */\n       }\n       \n     +\n     + ## t/t0006-date.sh ##\n     +@@ t/t0006-date.sh: check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n     + check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n     + check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n     + check_parse 'Thu, 7 Apr 2005 15:14:13 -0700' '2005-04-07 15:14:13 -0700'\n     ++check_parse '1970-01-01 00:00:00' '1970-01-01 00:00:00 +0000'\n     ++check_parse '1970-01-01 00:00:00 +00' '1970-01-01 00:00:00 +0000'\n     ++check_parse '1970-01-01 00:00:00 Z' '1970-01-01 00:00:00 +0000'\n     ++check_parse '1970-01-01 00:00:00 -01' '1970-01-01 00:00:00 -0100'\n     ++check_parse '1970-01-01 00:00:00 +01' bad\n     ++check_parse '1970-01-01 00:00:00 +11' bad\n     ++check_parse '1970-01-01 00:59:59 +01' bad\n     ++check_parse '1970-01-01 01:00:00 +01' '1970-01-01 01:00:00 +0100'\n     ++check_parse '1970-01-01 01:00:00 +11' bad\n     ++check_parse '1970-01-02 00:00:00 +11' '1970-01-02 00:00:00 +1100'\n     ++check_parse '1969-12-31 23:59:59' bad\n     ++check_parse '1969-12-31 23:59:59 +00' bad\n     ++check_parse '1969-12-31 23:59:59 Z' bad\n     ++check_parse '1969-12-31 23:59:59 +11' bad\n     ++check_parse '1969-12-31 23:59:59 -11' bad\n     + \n     + check_approxidate() {\n     + \techo \"$1 -> $2 +0000\" >expect\n\n\n date.c          |  6 +++++-\n t/t0006-date.sh | 15 +++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/date.c b/date.c\nindex 7365a4ad24f..8e3ec1bcb00 100644\n--- a/date.c\n+++ b/date.c\n@@ -937,8 +937,12 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n \t\t}\n \t}\n \n-\tif (!tm_gmt)\n+\tif (!tm_gmt) {\n+\t\tif (*offset > 0 && *offset * 60 > *timestamp)\n+\t\t\treturn -1;\n \t\t*timestamp -= *offset * 60;\n+\t}\n+\n \treturn 0; /* success */\n }\n \ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex 3031256d143..cdbb40bec01 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -116,6 +116,21 @@ check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n check_parse 'Thu, 7 Apr 2005 15:14:13 -0700' '2005-04-07 15:14:13 -0700'\n+check_parse '1970-01-01 00:00:00' '1970-01-01 00:00:00 +0000'\n+check_parse '1970-01-01 00:00:00 +00' '1970-01-01 00:00:00 +0000'\n+check_parse '1970-01-01 00:00:00 Z' '1970-01-01 00:00:00 +0000'\n+check_parse '1970-01-01 00:00:00 -01' '1970-01-01 00:00:00 -0100'\n+check_parse '1970-01-01 00:00:00 +01' bad\n+check_parse '1970-01-01 00:00:00 +11' bad\n+check_parse '1970-01-01 00:59:59 +01' bad\n+check_parse '1970-01-01 01:00:00 +01' '1970-01-01 01:00:00 +0100'\n+check_parse '1970-01-01 01:00:00 +11' bad\n+check_parse '1970-01-02 00:00:00 +11' '1970-01-02 00:00:00 +1100'\n+check_parse '1969-12-31 23:59:59' bad\n+check_parse '1969-12-31 23:59:59 +00' bad\n+check_parse '1969-12-31 23:59:59 Z' bad\n+check_parse '1969-12-31 23:59:59 +11' bad\n+check_parse '1969-12-31 23:59:59 -11' bad\n \n check_approxidate() {\n \techo \"$1 -> $2 +0000\" >expect\n\nbase-commit: 9eaef5822cd76bbeb53b6479ce0ddaad34ee2b14\n-- \ngitgitgadget\n"},{"id":"496148","messageId":"xmqqwmn66zzc.fsf@gitster.g","threadId":"61551","inReplyTo":"pull.1726.v2.git.git.1717369608923.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-03T11:13:27Z","receivedAt":"2024-06-03T11:13:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"darcy via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     fix: prevent date underflow when using positive timezone offset\n>     \n>     cc: Patrick Steinhardt ps@pks.im cc: Phillip Wood\n>     phillip.wood123@gmail.com\n\nYou're expected to respond to review comments before you send in\nupdated patches.  I didn't see the review comments responded to in\nthe thread:\n\n  https://lore.kernel.org/git/pull.1726.git.git.1716801427015.gitgitgadget@gmail.com/\n\nPlease see \"A typical life cycle of a patch series\" section of the\nSubmittingPatches document.\n\n  https://git.github.io/htmldocs/SubmittingPatches.html#patch-flow\n\nStep #3 and Step #4 are distinct.\n"},{"id":"496149","messageId":"2d771a72-3021-46db-ac32-e008a7ace34c@gmail.com","threadId":"61551","inReplyTo":"xmqqwmn66zzc.fsf@gitster.g","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"darcy","fromEmail":"acednes@gmail.com","sentAt":"2024-06-03T11:44:53Z","receivedAt":"2024-06-03T11:44:57Z","isPatch":true,"sender":{"key":"acednes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44690813?v=4"},"body":"> Without having a deep understanding of the code I don't quite see the\n> connection between this change and the problem description. Is it\n> necessary? If so, it might help to explain why it's needed in the commit\n> message or in the code.\n\n> ...I guess that\n> checking for tm_hour is assuming that TZ offset should always come\n> before the values necessary to compute the timestamp comes, but it\n> smells like an unwarranted assumption and not explaining the change\n> in the proposed log message is a bad sign.\n\nThis line has been reverted. The point was it would only parse the\ntimezone offset if it occurs after the time part of the date, but I\nhave realized that this is unrelated to the purpose of this change.\n\n> You should also add at least one test.\n\nYep, thanks, added now to `t0006-date.sh`.\n\n> Do we also want to check for overflows in the other direction (a large timestamp with a negative timezone offset)?\n\nIs this something people want added? I am happy to implement this if\nso, though it wasn't my original intention.\n\nIssues with the commit message should also be resolved. Thank you\neveryone for your patience :)\n\nOn 3/6/24 21:13, Junio C Hamano wrote:\n\n> \"darcy via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>>      fix: prevent date underflow when using positive timezone offset\n>>      \n>>      cc: Patrick Steinhardt ps@pks.im cc: Phillip Wood\n>>      phillip.wood123@gmail.com\n> You're expected to respond to review comments before you send in\n> updated patches.  I didn't see the review comments responded to in\n> the thread:\n>\n>    https://lore.kernel.org/git/pull.1726.git.git.1716801427015.gitgitgadget@gmail.com/\n>\n> Please see \"A typical life cycle of a patch series\" section of the\n> SubmittingPatches document.\n>\n>    https://git.github.io/htmldocs/SubmittingPatches.html#patch-flow\n>\n> Step #3 and Step #4 are distinct.\n"},{"id":"496152","messageId":"67c23773-21a4-4ad5-9b98-5e44578166dd@gmail.com","threadId":"61551","inReplyTo":"2d771a72-3021-46db-ac32-e008a7ace34c@gmail.com","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-03T14:13:03Z","receivedAt":"2024-06-03T14:13:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi darcy\n\nOn 03/06/2024 12:44, darcy wrote:\n>> Do we also want to check for overflows in the other direction (a large \n>> timestamp with a negative timezone offset)?\n> \n> Is this something people want added? I am happy to implement this if\n> so, though it wasn't my original intention.\n\nI think if we're worried about the date overflowing in one direction it \nmakes sense to fix overflows in the other direction at the same time \nespecially as I think that the other case involves a signed integer \noverflow which is undefined behavior in C.\n\nBest Wishes\n\nPhillip\n"},{"id":"496213","messageId":"4574e410-ca1a-495f-9835-14ee3fa454a1@gmail.com","threadId":"61551","inReplyTo":"67c23773-21a4-4ad5-9b98-5e44578166dd@gmail.com","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"darcy","fromEmail":"acednes@gmail.com","sentAt":"2024-06-04T08:48:59Z","receivedAt":"2024-06-04T08:49:06Z","isPatch":true,"sender":{"key":"acednes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44690813?v=4"},"body":"On 4/6/24 00:13, Phillip Wood wrote:\n\n> Hi darcy\n> On 03/06/2024 12:44, darcy wrote:\n>>> Do we also want to check for overflows in the other direction (a large\n>>> timestamp with a negative timezone offset)?\n>> Is this something people want added? I am happy to implement this if\n>> so, though it wasn't my original intention.\n> I think if we're worried about the date overflowing in one direction it\n> makes sense to fix overflows in the other direction at the same time\n> especially as I think that the other case involves a signed integer\n> overflow which is undefined behavior in C.\n\nThat makes sense.\n\nThough I am reading the `tm_to_time_t` code now and it only allows years\nup to 2099.\n\n> \tif (year < 0 || year > 129) /* algo only works for 1970-2099 */\n>\t\treturn -1;\n\nI can of course add a check here for dates close to the end of 2099, but\nit seems that the bigger issue is that some day people will want to use\nGit after 2099... Should I see if I can extend this range? I'm not sure\nwhere that specific year comes from, it doesn't seem to be based on a\nlimit of the size of `time_t`, and the comment or git logs don't seem to\nprovide a reason.\n\n\n"},{"id":"496217","messageId":"20240604093345.GA1279521@coredump.intra.peff.net","threadId":"61551","inReplyTo":"4574e410-ca1a-495f-9835-14ee3fa454a1@gmail.com","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T09:33:45Z","receivedAt":"2024-06-04T09:33:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 04, 2024 at 06:48:59PM +1000, darcy wrote:\n\n> Though I am reading the `tm_to_time_t` code now and it only allows years\n> up to 2099.\n> \n> > \tif (year < 0 || year > 129) /* algo only works for 1970-2099 */\n> > \t\treturn -1;\n> \n> I can of course add a check here for dates close to the end of 2099, but\n> it seems that the bigger issue is that some day people will want to use\n> Git after 2099... Should I see if I can extend this range? I'm not sure\n> where that specific year comes from, it doesn't seem to be based on a\n> limit of the size of `time_t`, and the comment or git logs don't seem to\n> provide a reason.\n\nI think the 2099 limitation is there because 2100 is not a leap year,\nand the code has over-simplified computation there (it counts every 4th\nyear as a leap year, which is not generally true for century boundaries,\nexcept for 2000 because it is divisible by 400).\n\nFrom the log message of ecee9d9e79 ([PATCH] Do date parsing by hand...,\n2005-04-30), I think the 1970 limitation may be about avoiding\nnon-60-minute DST (though it is not clear to me how the function cares\nabout dst either way, as it operates on UTC). You can't represent times\nbefore 1970 in our code base anyway, because our epoch timestamp is\nunsigned.\n\nSo yes, we could do better than that function. I worked on a series to\nhandle negative timestamps a while ago, and as part of that, I had to\nreplace this function. You can see the full series at the\n\"jk/time-negative-wip\" branch of https://github.com/peff/git, but I'll\nreproduce the commit in question below for the list archive.\n\nThe resulting series worked fine for me, but IIRC there was some issue\non Windows. I think its gmtime() did not understand negative timestamps,\nmaybe? And we'd need the reverse equivalent of the patch below (which we\ncould take from the same source).\n\nThere are some overflow checks already in date.c (grep for \"overflow\")\nthat might cover us, but I won't be surprised if there's a case that's\nmissing (although we store the timestamps as uintmax_t, so you're\nunlikely to overflow that in practice). Underflow is more likely, though\nif we finally support negative timestamps, then it becomes a non-issue\n(and we'd actually want to revert the patch you're working on).\n\nIf you're interested in picking up the negative timestamp work, I'd be\nhappy to discuss it. It's been on my todo list for a long time, but I\nnever quite get around to it.\n\nAnyway, here's the tm_to_time_t() cleanup.\n\n-- >8 --\ndate: make tm_to_time_t() less restricted\n\nWe've been using a hand-rolled tm_to_time_t() conversion since\necee9d9e79 ([PATCH] Do date parsing by hand..., 2005-04-30). It works\nfine for recent dates, but it doesn't work before 1970 nor after 2099.\n\nLet's lift those restrictions by using a more robust algorithm. The\ndays_from_civil() code here is lifted from Howard Hinnant's date\nlibrary, which can be found at:\n\n  https://github.com/HowardHinnant/date\n\nThe code is in the year_month_day::to_days() function, but I also relied\non his excellent explanation in:\n\n  http://howardhinnant.github.io/date_algorithms.html\n\nThe library is under the MIT license, which is GPL compatible. I've also\nleft a comment in the source file marking the licensing (the code is\nsubstantially similar to the original; I only expanded the variable\nnames to make it easier to follow).\n\nIdeally we could just rely on a system function to do this, but there\nisn't one. POSIX specifies mktime(), but it uses the local timezone.\nThere's a timegm() function available in glibc and some BSD systems, as\nwell as a similar function on Windows (with a different name). However,\nit's likely that there are still going to be some platforms without it,\nso we'd need a fallback anyway. We may as well just use that fallback\neverywhere to make sure that Git behaves consistently (and get more\ncomplete test coverage).\n\nNote that some callers, like parse_date_basic(), rely on tm_to_time_t()\ncomplaining when we have negative values in any field. So we'll continue\nto detect and complain about this case (the alternative would be to turn\n\"month -1\" into \"month 11\" of the previous year, and so on).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n date.c | 58 ++++++++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 42 insertions(+), 16 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 4337e18abd..df6c414c78 100644\n--- a/date.c\n+++ b/date.c\n@@ -10,29 +10,55 @@\n #include \"pager.h\"\n #include \"strbuf.h\"\n \n+/*\n+ * Convert a year-month-day time into a number of days since 1970 (possibly\n+ * negative). Note that \"year\" is the full year (not offset from 1900), and\n+ * \"month\" is in the range 1-12 (unlike a \"struct tm\" 0-11).\n+ *\n+ * Working in time_t is overkill (since it usually represents seconds), but\n+ * this makes sure we don't hit any integer range issues.\n+ *\n+ * This function is taken from https://github.com/HowardHinnant/date,\n+ * which is released under an MIT license.\n+ */\n+static time_t days_from_civil(int year, int month, int day)\n+{\n+\ttime_t era;\n+\tunsigned year_of_era, day_of_year, day_of_era;\n+\n+\tyear -= month <= 2;\n+\tera = (year >= 0 ? year : year - 399) / 400;\n+\tyear_of_era = year - era * 400;\n+\tday_of_year = (153 * (month + (month > 2 ? -3 : 9)) + 2) / 5 + day - 1;\n+\tday_of_era = year_of_era * 365 + year_of_era / 4 - year_of_era / 100\n+\t\t\t+ day_of_year;\n+\treturn era * 146097 + (time_t)day_of_era - 719468;\n+}\n+\n /*\n  * This is like mktime, but without normalization of tm_wday and tm_yday.\n  * It also always operates in UTC, not the local timezone.\n  */\n time_t tm_to_time_t(const struct tm *tm)\n {\n-\tstatic const int mdays[] = {\n-\t    0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334\n-\t};\n-\tint year = tm->tm_year - 70;\n-\tint month = tm->tm_mon;\n-\tint day = tm->tm_mday;\n-\n-\tif (year < 0 || year > 129) /* algo only works for 1970-2099 */\n-\t\treturn -1;\n-\tif (month < 0 || month > 11) /* array bounds */\n-\t\treturn -1;\n-\tif (month < 2 || (year + 2) % 4)\n-\t\tday--;\n-\tif (tm->tm_hour < 0 || tm->tm_min < 0 || tm->tm_sec < 0)\n+\ttime_t days, hours, minutes, seconds;\n+\n+\tif (tm->tm_year < 0 ||\n+\t    tm->tm_mon < 0 ||\n+\t    tm->tm_mday < 0 ||\n+\t    tm->tm_hour < 0 ||\n+\t    tm->tm_min < 0 ||\n+\t    tm->tm_sec < 0)\n \t\treturn -1;\n-\treturn (year * 365 + (year + 1) / 4 + mdays[month] + day) * 24*60*60UL +\n-\t\ttm->tm_hour * 60*60 + tm->tm_min * 60 + tm->tm_sec;\n+\n+\tdays = days_from_civil(tm->tm_year + 1900,\n+\t\t\t       tm->tm_mon + 1,\n+\t\t\t       tm->tm_mday);\n+\thours = 24 * days + tm->tm_hour;\n+\tminutes = 60 * hours + tm->tm_min;\n+\tseconds = 60 * minutes + tm->tm_sec;\n+\n+\treturn seconds;\n }\n \n static const char *month_names[] = {\n\n"},{"id":"496336","messageId":"c525e112-b4da-4871-88fd-6e62b3ff1ba1@gmail.com","threadId":"61551","inReplyTo":"20240604093345.GA1279521@coredump.intra.peff.net","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"darcy","fromEmail":"acednes@gmail.com","sentAt":"2024-06-05T06:52:21Z","receivedAt":"2024-06-05T06:52:26Z","isPatch":true,"sender":{"key":"acednes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44690813?v=4"},"body":"On 4/6/24 19:33, Jeff King wrote:\n\n> I think the 2099 limitation is there because 2100 is not a leap year,\n> and the code has over-simplified computation there (it counts every 4th\n> year as a leap year, which is not generally true for century boundaries,\n> except for 2000 because it is divisible by 400).\n\nAh, that makes sense, thanks.\n\n> If you're interested in picking up the negative timestamp work, I'd be\n> happy to discuss it. It's been on my todo list for a long time, but I\n> never quite get around to it.\n\nI may look at this in the future, but this looks a little out of my depth\nat the moment, as I am still unfamiliar with the codebase.\n\nFor the moment, I will just add the check at year 2099, and possibly will\nrevisit this when I have more time on my hands.\n\n"},{"id":"496367","messageId":"7ae05b6a-fb63-4ec7-8006-e968862cc5c7@gmail.com","threadId":"61551","inReplyTo":"4574e410-ca1a-495f-9835-14ee3fa454a1@gmail.com","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-05T13:10:23Z","receivedAt":"2024-06-05T13:10:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/06/2024 09:48, darcy wrote:\n> On 4/6/24 00:13, Phillip Wood wrote:\n> \n>> Hi darcy\n>> On 03/06/2024 12:44, darcy wrote:\n>>>> Do we also want to check for overflows in the other direction (a large\n>>>> timestamp with a negative timezone offset)?\n>>> Is this something people want added? I am happy to implement this if\n>>> so, though it wasn't my original intention.\n>> I think if we're worried about the date overflowing in one direction it\n>> makes sense to fix overflows in the other direction at the same time\n>> especially as I think that the other case involves a signed integer\n>> overflow which is undefined behavior in C.\n> \n> That makes sense.\n> \n> Though I am reading the `tm_to_time_t` code now and it only allows years\n> up to 2099.\n> \n>>     if (year < 0 || year > 129) /* algo only works for 1970-2099 */\n>>         return -1;\n\nThanks for taking a closer look, I wasn't aware of that. Reading Peff's \nreply it seems like timestamp is actually unsigned and as we limit the \nmaximum year to 2099 it seems unlikely we'll overflow from too larger \ndate after all.\n\nBest Wishes\n\nPhillip\n\n> I can of course add a check here for dates close to the end of 2099, but\n> it seems that the bigger issue is that some day people will want to use\n> Git after 2099... Should I see if I can extend this range? I'm not sure\n> where that specific year comes from, it doesn't seem to be based on a\n> limit of the size of `time_t`, and the comment or git logs don't seem to\n> provide a reason.\n> \n> \n\n"},{"id":"496386","messageId":"xmqq1q5bti4g.fsf@gitster.g","threadId":"61551","inReplyTo":"7ae05b6a-fb63-4ec7-8006-e968862cc5c7@gmail.com","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T17:27:27Z","receivedAt":"2024-06-05T17:27:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Thanks for taking a closer look, I wasn't aware of that. Reading\n> Peff's reply it seems like timestamp is actually unsigned and as we\n> limit the maximum year to 2099 it seems unlikely we'll overflow from\n> too larger date after all.\n\nThanks all.\n\nI haven't seen one question I posed answered, though.  Has\nhttps://git.github.io/htmldocs/SubmittingPatches.html#real-name\nbeen followed in this patch when signing off the patch?\n\n"},{"id":"496414","messageId":"a3b8a245-fd88-4798-990c-a06c911ce067@gmail.com","threadId":"61551","inReplyTo":"xmqq1q5bti4g.fsf@gitster.g","subject":"Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset","fromName":"darcy","fromEmail":"acednes@gmail.com","sentAt":"2024-06-06T04:56:08Z","receivedAt":"2024-06-06T04:56:15Z","isPatch":true,"sender":{"key":"acednes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44690813?v=4"},"body":"On 6/6/24 03:27, Junio C Hamano wrote:\n\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n>\n>> Thanks for taking a closer look, I wasn't aware of that. Reading\n>> Peff's reply it seems like timestamp is actually unsigned and as we\n>> limit the maximum year to 2099 it seems unlikely we'll overflow from\n>> too larger date after all.\n> Thanks all.\n>\n> I haven't seen one question I posed answered, though.  Has\n> https://git.github.io/htmldocs/SubmittingPatches.html#real-name\n> been followed in this patch when signing off the patch?\n\nI have changed the line in the commit to properly follow that rule.\n\n"},{"id":"496594","messageId":"pull.1726.v3.git.git.1717719428510.gitgitgadget@gmail.com","threadId":"61551","inReplyTo":"pull.1726.v2.git.git.1717369608923.gitgitgadget@gmail.com","subject":"[PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"darcy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-07T00:17:08Z","receivedAt":"2024-06-07T00:17:12Z","isPatch":true,"sender":{"key":"acednes@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44690813?v=4"},"body":"From: darcy <acednes@gmail.com>\n\nOverriding the date of a commit to be close to \"1970-01-01 00:00:00\"\nwith a large enough positive timezone for the equivelant GMT time to be\nbefore the epoch is considered valid by `parse_date_basic`. Similar\nbehaviour occurs when using a date close to \"2099-12-31 23:59:59\" (the\nmaximum date allowed by `tm_to_time_t`) with a large enough negative\ntimezone offset.\n\nThis leads to an integer underflow or underflow respectively in the\ncommit timestamp, which is not caught by `git-commit`, but will cause\nother services to fail, such as `git-fsck`, which, for the first case,\nreports \"badDateOverflow: invalid author/committer line - date causes\ninteger overflow\".\n\nInstead check the timezone offset and fail if the resulting time comes\nbefore the epoch \"1970-01-01T00:00:00Z\" or after the maximum date\n\"2099-12-31T23:59:59Z\".\n\nSigned-off-by: Darcy Burke <acednes@gmail.com>\n---\n    fix: prevent date underflow when using positive timezone offset\n    \n    cc: Patrick Steinhardt ps@pks.im cc: Phillip Wood\n    phillip.wood123@gmail.com cc: darcy acednes@gmail.com cc: Jeff King\n    peff@peff.net\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1726%2Fdxrcy%2Fmaster-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1726/dxrcy/master-v3\nPull-Request: https://github.com/git/git/pull/1726\n\nRange-diff vs v2:\n\n 1:  db508b2f533 ! 1:  f0de3a2c543 date: detect underflow when parsing dates with positive timezone offset\n     @@ Metadata\n      Author: darcy <acednes@gmail.com>\n      \n       ## Commit message ##\n     -    date: detect underflow when parsing dates with positive timezone offset\n     +    date: detect underflow/overflow when parsing dates with timezone offset\n      \n          Overriding the date of a commit to be close to \"1970-01-01 00:00:00\"\n     -    with a large enough timezone for the equivelant GMT time to be before\n     -    the epoch is considered valid by `parse_date_basic`.\n     +    with a large enough positive timezone for the equivelant GMT time to be\n     +    before the epoch is considered valid by `parse_date_basic`. Similar\n     +    behaviour occurs when using a date close to \"2099-12-31 23:59:59\" (the\n     +    maximum date allowed by `tm_to_time_t`) with a large enough negative\n     +    timezone offset.\n      \n     -    This leads to an integer underflow in the commit timestamp, which is not\n     -    caught by `git-commit`, but will cause other services to fail, such as\n     -    `git-fsck`, which reports \"badDateOverflow: invalid author/committer\n     -    line - date causes integer overflow\".\n     +    This leads to an integer underflow or underflow respectively in the\n     +    commit timestamp, which is not caught by `git-commit`, but will cause\n     +    other services to fail, such as `git-fsck`, which, for the first case,\n     +    reports \"badDateOverflow: invalid author/committer line - date causes\n     +    integer overflow\".\n      \n          Instead check the timezone offset and fail if the resulting time comes\n     -    before the epoch, \"1970-01-01T00:00:00Z\", when parsing.\n     +    before the epoch \"1970-01-01T00:00:00Z\" or after the maximum date\n     +    \"2099-12-31T23:59:59Z\".\n      \n          Signed-off-by: Darcy Burke <acednes@gmail.com>\n      \n       ## date.c ##\n     +@@ date.c: static int match_object_header_date(const char *date, timestamp_t *timestamp, in\n     + \treturn 0;\n     + }\n     + \n     ++\n     ++/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n     ++static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n     ++\n     + /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822\n     +    (i.e. English) day/month names, and it doesn't work correctly with %z. */\n     + int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n      @@ date.c: int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n       \t\t}\n       \t}\n     @@ date.c: int parse_date_basic(const char *date, timestamp_t *timestamp, int *offs\n      -\tif (!tm_gmt)\n      +\tif (!tm_gmt) {\n      +\t\tif (*offset > 0 && *offset * 60 > *timestamp)\n     ++\t\t\treturn -1;\n     ++\t\tif (*offset < 0 && -*offset * 60 > timestamp_max - *timestamp)\n      +\t\t\treturn -1;\n       \t\t*timestamp -= *offset * 60;\n      +\t}\n     @@ date.c: int parse_date_basic(const char *date, timestamp_t *timestamp, int *offs\n       \n      \n       ## t/t0006-date.sh ##\n     -@@ t/t0006-date.sh: check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n     - check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n     +@@ t/t0006-date.sh: check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n       check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n       check_parse 'Thu, 7 Apr 2005 15:14:13 -0700' '2005-04-07 15:14:13 -0700'\n     + \n      +check_parse '1970-01-01 00:00:00' '1970-01-01 00:00:00 +0000'\n      +check_parse '1970-01-01 00:00:00 +00' '1970-01-01 00:00:00 +0000'\n      +check_parse '1970-01-01 00:00:00 Z' '1970-01-01 00:00:00 +0000'\n     @@ t/t0006-date.sh: check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +00\n      +check_parse '1969-12-31 23:59:59 Z' bad\n      +check_parse '1969-12-31 23:59:59 +11' bad\n      +check_parse '1969-12-31 23:59:59 -11' bad\n     - \n     ++\n     ++check_parse '2099-12-31 23:59:59' '2099-12-31 23:59:59 +0000'\n     ++check_parse '2099-12-31 23:59:59 +00' '2099-12-31 23:59:59 +0000'\n     ++check_parse '2099-12-31 23:59:59 Z' '2099-12-31 23:59:59 +0000'\n     ++check_parse '2099-12-31 23:59:59 +01' '2099-12-31 23:59:59 +0100'\n     ++check_parse '2099-12-31 23:59:59 -01' bad\n     ++check_parse '2099-12-31 23:59:59 -11' bad\n     ++check_parse '2099-12-31 23:00:00 -01' bad\n     ++check_parse '2099-12-31 22:59:59 -01' '2099-12-31 22:59:59 -0100'\n     ++check_parse '2100-00-00 00:00:00' bad\n     ++check_parse '2099-12-30 00:00:00 -11' '2099-12-30 00:00:00 -1100'\n     ++check_parse '2100-00-00 00:00:00 +00' bad\n     ++check_parse '2100-00-00 00:00:00 Z' bad\n     ++check_parse '2100-00-00 00:00:00 -11' bad\n     ++check_parse '2100-00-00 00:00:00 +11' bad\n     ++\n       check_approxidate() {\n       \techo \"$1 -> $2 +0000\" >expect\n     + \ttest_expect_${3:-success} \"parse approxidate ($1)\" \"\n\n\n date.c          | 12 +++++++++++-\n t/t0006-date.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 1 deletion(-)\n\ndiff --git a/date.c b/date.c\nindex 7365a4ad24f..95776c8a92f 100644\n--- a/date.c\n+++ b/date.c\n@@ -868,6 +868,10 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in\n \treturn 0;\n }\n \n+\n+/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n+static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n+\n /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822\n    (i.e. English) day/month names, and it doesn't work correctly with %z. */\n int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n@@ -937,8 +941,14 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n \t\t}\n \t}\n \n-\tif (!tm_gmt)\n+\tif (!tm_gmt) {\n+\t\tif (*offset > 0 && *offset * 60 > *timestamp)\n+\t\t\treturn -1;\n+\t\tif (*offset < 0 && -*offset * 60 > timestamp_max - *timestamp)\n+\t\t\treturn -1;\n \t\t*timestamp -= *offset * 60;\n+\t}\n+\n \treturn 0; /* success */\n }\n \ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex 3031256d143..e8fdf361ad4 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -117,6 +117,37 @@ check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n check_parse 'Thu, 7 Apr 2005 15:14:13 -0700' '2005-04-07 15:14:13 -0700'\n \n+check_parse '1970-01-01 00:00:00' '1970-01-01 00:00:00 +0000'\n+check_parse '1970-01-01 00:00:00 +00' '1970-01-01 00:00:00 +0000'\n+check_parse '1970-01-01 00:00:00 Z' '1970-01-01 00:00:00 +0000'\n+check_parse '1970-01-01 00:00:00 -01' '1970-01-01 00:00:00 -0100'\n+check_parse '1970-01-01 00:00:00 +01' bad\n+check_parse '1970-01-01 00:00:00 +11' bad\n+check_parse '1970-01-01 00:59:59 +01' bad\n+check_parse '1970-01-01 01:00:00 +01' '1970-01-01 01:00:00 +0100'\n+check_parse '1970-01-01 01:00:00 +11' bad\n+check_parse '1970-01-02 00:00:00 +11' '1970-01-02 00:00:00 +1100'\n+check_parse '1969-12-31 23:59:59' bad\n+check_parse '1969-12-31 23:59:59 +00' bad\n+check_parse '1969-12-31 23:59:59 Z' bad\n+check_parse '1969-12-31 23:59:59 +11' bad\n+check_parse '1969-12-31 23:59:59 -11' bad\n+\n+check_parse '2099-12-31 23:59:59' '2099-12-31 23:59:59 +0000'\n+check_parse '2099-12-31 23:59:59 +00' '2099-12-31 23:59:59 +0000'\n+check_parse '2099-12-31 23:59:59 Z' '2099-12-31 23:59:59 +0000'\n+check_parse '2099-12-31 23:59:59 +01' '2099-12-31 23:59:59 +0100'\n+check_parse '2099-12-31 23:59:59 -01' bad\n+check_parse '2099-12-31 23:59:59 -11' bad\n+check_parse '2099-12-31 23:00:00 -01' bad\n+check_parse '2099-12-31 22:59:59 -01' '2099-12-31 22:59:59 -0100'\n+check_parse '2100-00-00 00:00:00' bad\n+check_parse '2099-12-30 00:00:00 -11' '2099-12-30 00:00:00 -1100'\n+check_parse '2100-00-00 00:00:00 +00' bad\n+check_parse '2100-00-00 00:00:00 Z' bad\n+check_parse '2100-00-00 00:00:00 -11' bad\n+check_parse '2100-00-00 00:00:00 +11' bad\n+\n check_approxidate() {\n \techo \"$1 -> $2 +0000\" >expect\n \ttest_expect_${3:-success} \"parse approxidate ($1)\" \"\n\nbase-commit: cd77e87115eab5e80e6b161e7b84a79ba1a12cdc\n-- \ngitgitgadget\n"},{"id":"496661","messageId":"xmqq1q58ejnw.fsf@gitster.g","threadId":"61551","inReplyTo":"pull.1726.v3.git.git.1717719428510.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T17:40:03Z","receivedAt":"2024-06-07T17:40:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"darcy via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: darcy <acednes@gmail.com>\n\nThis ident should match what is used on \"Signed-off-by:\" line.\n\n> Overriding the date of a commit to be close to \"1970-01-01 00:00:00\"\n> with a large enough positive timezone for the equivelant GMT time to be\n> before the epoch is considered valid by `parse_date_basic`. Similar\n> behaviour occurs when using a date close to \"2099-12-31 23:59:59\" (the\n> maximum date allowed by `tm_to_time_t`) with a large enough negative\n> timezone offset.\n>\n> This leads to an integer underflow or underflow respectively in the\n\n\"underflow or underflow respectively\"?\n\n> commit timestamp, which is not caught by `git-commit`, but will cause\n> other services to fail, such as `git-fsck`, which, for the first case,\n> reports \"badDateOverflow: invalid author/committer line - date causes\n> integer overflow\".\n>\n> Instead check the timezone offset and fail if the resulting time comes\n> before the epoch \"1970-01-01T00:00:00Z\" or after the maximum date\n> \"2099-12-31T23:59:59Z\".\n\nNicely described otherwise.\n\n> +\n> +/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n> +static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n\nI wonder if this should be of timestamp_t type instead, as the check\nis done against *timestamp in parse_date_basic() where *timestamp is\nof type timestamp_t to match?\n\n>  int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n> @@ -937,8 +941,14 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n>  \t\t}\n>  \t}\n>  \n> -\tif (!tm_gmt)\n> +\tif (!tm_gmt) {\n> +\t\tif (*offset > 0 && *offset * 60 > *timestamp)\n> +\t\t\treturn -1;\n> +\t\tif (*offset < 0 && -*offset * 60 > timestamp_max - *timestamp)\n> +\t\t\treturn -1;\n>  \t\t*timestamp -= *offset * 60;\n> +\t}\n> +\n>  \treturn 0; /* success */\n>  }\n\nThanks.\n"},{"id":"496721","messageId":"xmqqcyorcldv.fsf@gitster.g","threadId":"61551","inReplyTo":"xmqq1q58ejnw.fsf@gitster.g","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-08T18:58:04Z","receivedAt":"2024-06-08T18:58:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> +/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n>> +static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n>\n> I wonder if this should be of timestamp_t type instead, as the check\n> is done against *timestamp in parse_date_basic() where *timestamp is\n> of type timestamp_t to match?\n\nThe CI build on Windows tells me that my worry was warranted.\n\n  https://github.com/git/git/actions/runs/9424299208/job/25964281907#step:4:643\n\n(GitHub seems to show the breakage details only to those who are\nlogged in, so you'd need to be logged in to visit that link)\n\n\n  Error: date.c:873:75: integer overflow in expression of type 'long int' results in '-192522496' [-Werror=overflow]\n    873 | static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n        |                                                                           ^\n  Error: date.c:873:1: overflow in constant expression [-Werror=overflow]\n\n"},{"id":"496967","messageId":"xmqqtthzrraw.fsf@gitster.g","threadId":"61551","inReplyTo":"xmqq1q58ejnw.fsf@gitster.g","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-11T23:30:15Z","receivedAt":"2024-06-11T23:30:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I wonder if this should be of timestamp_t type instead, as the check\n> is done against *timestamp in parse_date_basic() where *timestamp is\n> of type timestamp_t to match?\n\nAlso, as you can see at one of the GitHub CI jobs, e.g.,\n\n  https://github.com/git/git/actions/runs/9455916669/job/26046731619#step:6:1915\n\nyou'd need to either exclude some \"these are too large timestamps\nfor the system\" tests from 32-bit systems or expect output on them\nto be different from 64-bit systems.\n\nAs you are actively detecting the condition and giving an error\nmessage \"too large for _this_ system\", I think it is a good idea\nto actually do the latter, i.e. on 64-bit system make sure parsing\nis done correctly, and on 32-bit system make sure you get that \"too\nlarge for this system\" error.\n\nThanks.\n\n"},{"id":"496968","messageId":"114401dabc5a$08a72060$19f56120$@nexbridge.com","threadId":"61551","inReplyTo":"xmqqtthzrraw.fsf@gitster.g","subject":"RE: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-06-11T23:49:37Z","receivedAt":"2024-06-11T23:49:46Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Tuesday, June 11, 2024 7:30 PM, Junio C Hamano wrote:\n>Junio C Hamano <gitster@pobox.com> writes:\n>\n>> I wonder if this should be of timestamp_t type instead, as the check\n>> is done against *timestamp in parse_date_basic() where *timestamp is\n>> of type timestamp_t to match?\n>\n>Also, as you can see at one of the GitHub CI jobs, e.g.,\n>\n>\n>https://github.com/git/git/actions/runs/9455916669/job/26046731619#step:6:\n>1915\n>\n>you'd need to either exclude some \"these are too large timestamps for the\nsystem\"\n>tests from 32-bit systems or expect output on them to be different from\n64-bit\n>systems.\n>\n>As you are actively detecting the condition and giving an error message\n\"too large\n>for _this_ system\", I think it is a good idea to actually do the latter,\ni.e. on 64-bit\n>system make sure parsing is done correctly, and on 32-bit system make sure\nyou get\n>that \"too large for this system\" error.\n\nDoes this imply that timestamp tests will fail on 32-bit systems? I am\nunsure how to interpret this. Can you please clarify?\n\nThanks,\nRandall\n\n"},{"id":"496969","messageId":"xmqqjzivrq93.fsf@gitster.g","threadId":"61551","inReplyTo":"114401dabc5a$08a72060$19f56120$@nexbridge.com","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-11T23:52:56Z","receivedAt":"2024-06-11T23:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"<rsbecker@nexbridge.com> writes:\n\n>>Also, as you can see at one of the GitHub CI jobs, e.g.,\n>>\n>>\n>> https://github.com/git/git/actions/runs/9455916669/job/26046731619#step:6:1915\n>>\n> Does this imply that timestamp tests will fail on 32-bit systems? I am\n> unsure how to interpret this. Can you please clarify?\n\nSorry, but I have nothing more to clarify than what appears in the\nquoted CI log.\n"},{"id":"497006","messageId":"2c2aade1-ee83-4ea2-94ae-3d6714a52db1@gmail.com","threadId":"61551","inReplyTo":"xmqq1q58ejnw.fsf@gitster.g","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-12T09:07:50Z","receivedAt":"2024-06-12T09:07:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/06/2024 18:40, Junio C Hamano wrote:\n> \"darcy via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n >\n>> +/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n>> +static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n> \n> I wonder if this should be of timestamp_t type instead, as the check\n> is done against *timestamp in parse_date_basic() where *timestamp is\n> of type timestamp_t to match?\n\nI think that would make sense. We'd want to cast one of the numbers to \ntimestamp_t to ensure that sum uses a wide enough integer as well. Using \n2100L is probably OK but an explicit cast would be clearer I think.\n\nBest Wishes\n\nPhillip\n"},{"id":"497020","messageId":"CAOLa=ZTgP+OwVrT87=og4tVdhsqjErMUJzCuT9bdDatFS=J8hQ@mail.gmail.com","threadId":"61551","inReplyTo":"pull.1726.v3.git.git.1717719428510.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-12T09:49:23Z","receivedAt":"2024-06-12T09:49:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\"darcy via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: darcy <acednes@gmail.com>\n>\n> Overriding the date of a commit to be close to \"1970-01-01 00:00:00\"\n> with a large enough positive timezone for the equivelant GMT time to be\n\ns/equivelant/equivalent\n\n[snip]\n\n> diff --git a/date.c b/date.c\n> index 7365a4ad24f..95776c8a92f 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -868,6 +868,10 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in\n>  \treturn 0;\n>  }\n>\n> +\n> +/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n> +static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n>\n\nNit: but since we're calculating the number of years here (2100L -\n1970), shouldn't we also be calculating the number of leap days instead\nof hardcoding it?\n"},{"id":"497090","messageId":"a03cd45e-e4e0-42c7-bb98-b25cac12e42f@gmail.com","threadId":"61551","inReplyTo":"CAOLa=ZTgP+OwVrT87=og4tVdhsqjErMUJzCuT9bdDatFS=J8hQ@mail.gmail.com","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-13T13:31:06Z","receivedAt":"2024-06-13T13:31:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Karthik\n\nOn 12/06/2024 10:49, Karthik Nayak wrote:\n> \"darcy via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> +\n>> +/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n>> +static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n>>\n> \n> Nit: but since we're calculating the number of years here (2100L -\n> 1970), shouldn't we also be calculating the number of leap days instead\n> of hardcoding it?\n\nI'm happy with a hard coded constant for the number of leap days - I \nthink it is probably easier to check that (which I have done) than it \nwould be to check the calculation as I'm not sure off the top of my head \nif is it safe to do (2100-1970)/4 or whether we need something more \ncomplicated.\n\nBest Wishes\n\nPhillip\n"},{"id":"497095","messageId":"xmqqle38n7hv.fsf@gitster.g","threadId":"61551","inReplyTo":"a03cd45e-e4e0-42c7-bb98-b25cac12e42f@gmail.com","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-13T16:16:12Z","receivedAt":"2024-06-13T16:16:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>>> +/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n>>> +static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n>>>\n>> Nit: but since we're calculating the number of years here (2100L -\n>> 1970), shouldn't we also be calculating the number of leap days instead\n>> of hardcoding it?\n>\n> I'm happy with a hard coded constant for the number of leap days - I\n> think it is probably easier to check that (which I have done) than it\n> would be to check the calculation as I'm not sure off the top of my\n> head if is it safe to do (2100-1970)/4 or whether we need something\n> more complicated.\n\nIt's even OK to use a hard coded constant for the number of days\nsince the epoch to the git-end-of-time ;-)\n\nThe timestamp of the git-end-of-time would not fit in time_t on\n32-bit systems, I would presume?  If our tests are trying to see if\ntimestamps around the beginning of year 2100 are handled\n\"correctly\", the definition of the correctness needs to be\nconsitional on the platform.\n\nOn systems with TIME_T_IS_64BIT, we'd want to see such a timestamp\nto be represented fine.  On systems without, we'd want to see the\n\"Timestamp too large for this system\" error when we feed such a\ntimestamp to be parsed.\n\nThanks.\n"},{"id":"497120","messageId":"xmqqwmmsiakq.fsf@gitster.g","threadId":"61551","inReplyTo":"xmqqcyorcldv.fsf@gitster.g","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-14T01:20:53Z","receivedAt":"2024-06-14T01:20:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The CI build on Windows tells me that my worry was warranted.\n>\n>   https://github.com/git/git/actions/runs/9424299208/job/25964281907#step:4:643\n>\n> (GitHub seems to show the breakage details only to those who are\n> logged in, so you'd need to be logged in to visit that link)\n\nSo here is what I accumulated in SQUASH??? patches on top of your\ntopic while waiting for an updated version to unbreak the CI.\n\n * The \"end of git time\" timestamp does not fit in time_t on 32-bit\n   systems, so I updated it to use timestamp_t at least for now.\n\n * t0006 has two tests that use TIME_IS_64BIT,TIME_T_IS_64BIT\n   prerequisites; I introduced HAVE_64BIT_TIME to simplify them.\n\n * nobody passes $4 to check_parse to tell it to expect a failure,\n   so I removed it.  It always expects success.\n\n * check_parse now honors a global variable REQUIRE_64BIT_TIME that\n   is used as the prerequisite to run its test_expect_success; the\n   \"near the end of git time\" tests you added use the mechanism to\n   pass HAVE_64BIT_TIME prerequisite.\n\nThe last one is a bit questionable, as it only \"punts\" on 32-bit\nsystems, instead of making sure we get the expected error messages.\nI think it is OK to punt here and have a separate test that checks\ntimestamp around year 2040 for that condition.\n\n date.c          |  2 +-\n t/t0006-date.sh | 20 ++++++++++++++------\n 2 files changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 95776c8a92..bee9fe8f10 100644\n--- a/date.c\n+++ b/date.c\n@@ -870,7 +870,7 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in\n \n \n /* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n-static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n+static const timestamp_t timestamp_max = (((timestamp_t)2100 - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n \n /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822\n    (i.e. English) day/month names, and it doesn't work correctly with %z. */\ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex e8fdf361ad..fd373e1b39 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -8,6 +8,11 @@ TEST_PASSES_SANITIZE_LEAK=true\n # arbitrary reference time: 2009-08-30 19:20:00\n GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n \n+if test_have_prereq TIME_IS_64BIT,TIME_T_IS_64BIT\n+then\n+\ttest_set_prereq HAVE_64BIT_TIME\n+fi\n+\n check_relative() {\n \tt=$(($GIT_TEST_DATE_NOW - $1))\n \techo \"$t -> $2\" >expect\n@@ -80,14 +85,15 @@ check_show raw \"$TIME\" '1466000000 -0200'\n \n # arbitrary time absurdly far in the future\n FUTURE=\"5758122296 -0400\"\n-check_show iso       \"$FUTURE\" \"2152-06-19 18:24:56 -0400\" TIME_IS_64BIT,TIME_T_IS_64BIT\n-check_show iso-local \"$FUTURE\" \"2152-06-19 22:24:56 +0000\" TIME_IS_64BIT,TIME_T_IS_64BIT\n+check_show iso       \"$FUTURE\" \"2152-06-19 18:24:56 -0400\" HAVE_64BIT_TIME\n+check_show iso-local \"$FUTURE\" \"2152-06-19 22:24:56 +0000\" HAVE_64BIT_TIME\n \n-check_parse() {\n+REQUIRE_64BIT_TIME=\n+check_parse () {\n \techo \"$1 -> $2\" >expect\n-\ttest_expect_${4:-success} \"parse date ($1${3:+ TZ=$3})\" \"\n-\tTZ=${3:-$TZ} test-tool date parse '$1' >actual &&\n-\ttest_cmp expect actual\n+\ttest_expect_success $REQUIRE_64BIT_TIME \"parse date ($1${3:+ TZ=$3}) -> $2\" \"\n+\t\tTZ=${3:-$TZ} test-tool date parse '$1' >actual &&\n+\t\ttest_cmp expect actual\n \t\"\n }\n \n@@ -133,6 +139,7 @@ check_parse '1969-12-31 23:59:59 Z' bad\n check_parse '1969-12-31 23:59:59 +11' bad\n check_parse '1969-12-31 23:59:59 -11' bad\n \n+REQUIRE_64BIT_TIME=HAVE_64BIT_TIME\n check_parse '2099-12-31 23:59:59' '2099-12-31 23:59:59 +0000'\n check_parse '2099-12-31 23:59:59 +00' '2099-12-31 23:59:59 +0000'\n check_parse '2099-12-31 23:59:59 Z' '2099-12-31 23:59:59 +0000'\n@@ -147,6 +154,7 @@ check_parse '2100-00-00 00:00:00 +00' bad\n check_parse '2100-00-00 00:00:00 Z' bad\n check_parse '2100-00-00 00:00:00 -11' bad\n check_parse '2100-00-00 00:00:00 +11' bad\n+REQUIRE_64BIT_TIME=\n \n check_approxidate() {\n \techo \"$1 -> $2 +0000\" >expect\n"},{"id":"497196","messageId":"CAOLa=ZT2goTBD6WBtjYOYqa6vEsp=P=UzUBzs-5Om3d9HLZYbw@mail.gmail.com","threadId":"61551","inReplyTo":"xmqqle38n7hv.fsf@gitster.g","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-14T20:09:08Z","receivedAt":"2024-06-14T20:09:12Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n>\n>>>> +/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n>>>> +static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n>>>>\n>>> Nit: but since we're calculating the number of years here (2100L -\n>>> 1970), shouldn't we also be calculating the number of leap days instead\n>>> of hardcoding it?\n>>\n>> I'm happy with a hard coded constant for the number of leap days - I\n>> think it is probably easier to check that (which I have done) than it\n>> would be to check the calculation as I'm not sure off the top of my\n>> head if is it safe to do (2100-1970)/4 or whether we need something\n>> more complicated.\n>\n> It's even OK to use a hard coded constant for the number of days\n> since the epoch to the git-end-of-time ;-)\n\nThat's why I noted it as a _Nit_, mostly because it wasn't anything big.\nBut I found that part of it being dynamic and part of it being static\nwas inconsistent.\n\n> The timestamp of the git-end-of-time would not fit in time_t on\n> 32-bit systems, I would presume?  If our tests are trying to see if\n> timestamps around the beginning of year 2100 are handled\n> \"correctly\", the definition of the correctness needs to be\n> consitional on the platform.\n>\n> On systems with TIME_T_IS_64BIT, we'd want to see such a timestamp\n> to be represented fine.  On systems without, we'd want to see the\n> \"Timestamp too large for this system\" error when we feed such a\n> timestamp to be parsed.\n>\n> Thanks.\n"},{"id":"497197","messageId":"xmqqjzirdyqm.fsf@gitster.g","threadId":"61551","inReplyTo":"CAOLa=ZT2goTBD6WBtjYOYqa6vEsp=P=UzUBzs-5Om3d9HLZYbw@mail.gmail.com","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-14T21:02:25Z","receivedAt":"2024-06-14T21:03:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>> It's even OK to use a hard coded constant for the number of days\n>> since the epoch to the git-end-of-time ;-)\n>\n> That's why I noted it as a _Nit_, mostly because it wasn't anything big.\n> But I found that part of it being dynamic and part of it being static\n> was inconsistent.\n\nSure, but it is so tiny thing, we shouldn't waste more time than we\nspend getting the tests right even on 32-bit systems.  We seem to be\ndoing the opposite by talking about this part even more, which is a\nbit sad.  Any comments on the actual patch I sent as a follow-up?\n\n>> The timestamp of the git-end-of-time would not fit in time_t on\n>> 32-bit systems, I would presume?  If our tests are trying to see if\n>> timestamps around the beginning of year 2100 are handled\n>> \"correctly\", the definition of the correctness needs to be\n>> consitional on the platform.\n>>\n>> On systems with TIME_T_IS_64BIT, we'd want to see such a timestamp\n>> to be represented fine.  On systems without, we'd want to see the\n>> \"Timestamp too large for this system\" error when we feed such a\n>> timestamp to be parsed.\n>>\n>> Thanks.\n"},{"id":"497208","messageId":"CAOLa=ZT_ZSV7gCexZhcKBvqJCva5UEY+66TgqDUoBo=q=pOqzQ@mail.gmail.com","threadId":"61551","inReplyTo":"xmqqwmmsiakq.fsf@gitster.g","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-15T11:47:54Z","receivedAt":"2024-06-15T11:47:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> The CI build on Windows tells me that my worry was warranted.\n>>\n>>   https://github.com/git/git/actions/runs/9424299208/job/25964281907#step:4:643\n>>\n>> (GitHub seems to show the breakage details only to those who are\n>> logged in, so you'd need to be logged in to visit that link)\n>\n> So here is what I accumulated in SQUASH??? patches on top of your\n> topic while waiting for an updated version to unbreak the CI.\n>\n>  * The \"end of git time\" timestamp does not fit in time_t on 32-bit\n>    systems, so I updated it to use timestamp_t at least for now.\n>\n>  * t0006 has two tests that use TIME_IS_64BIT,TIME_T_IS_64BIT\n>    prerequisites; I introduced HAVE_64BIT_TIME to simplify them.\n>\n>  * nobody passes $4 to check_parse to tell it to expect a failure,\n>    so I removed it.  It always expects success.\n>\n>  * check_parse now honors a global variable REQUIRE_64BIT_TIME that\n>    is used as the prerequisite to run its test_expect_success; the\n>    \"near the end of git time\" tests you added use the mechanism to\n>    pass HAVE_64BIT_TIME prerequisite.\n>\n> The last one is a bit questionable, as it only \"punts\" on 32-bit\n> systems, instead of making sure we get the expected error messages.\n> I think it is OK to punt here and have a separate test that checks\n> timestamp around year 2040 for that condition.\n>\n>  date.c          |  2 +-\n>  t/t0006-date.sh | 20 ++++++++++++++------\n>  2 files changed, 15 insertions(+), 7 deletions(-)\n>\n> diff --git a/date.c b/date.c\n> index 95776c8a92..bee9fe8f10 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -870,7 +870,7 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in\n>\n>\n>  /* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */\n> -static const time_t timestamp_max = ((2100L - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n> +static const timestamp_t timestamp_max = (((timestamp_t)2100 - 1970) * 365 + 32) * 24 * 60 * 60 - 1;\n>\n>  /* Gr. strptime is crap for this; it doesn't have a way to require RFC2822\n>     (i.e. English) day/month names, and it doesn't work correctly with %z. */\n> diff --git a/t/t0006-date.sh b/t/t0006-date.sh\n> index e8fdf361ad..fd373e1b39 100755\n> --- a/t/t0006-date.sh\n> +++ b/t/t0006-date.sh\n> @@ -8,6 +8,11 @@ TEST_PASSES_SANITIZE_LEAK=true\n>  # arbitrary reference time: 2009-08-30 19:20:00\n>  GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n>\n> +if test_have_prereq TIME_IS_64BIT,TIME_T_IS_64BIT\n> +then\n> +\ttest_set_prereq HAVE_64BIT_TIME\n> +fi\n> +\n\nThis does make sense, I did check and noticed that the two are always\nused together everywhere, so outside of these patches, it perhaps would\nalso make sense to combine them altogether.\n\n>  check_relative() {\n>  \tt=$(($GIT_TEST_DATE_NOW - $1))\n>  \techo \"$t -> $2\" >expect\n> @@ -80,14 +85,15 @@ check_show raw \"$TIME\" '1466000000 -0200'\n>\n>  # arbitrary time absurdly far in the future\n>  FUTURE=\"5758122296 -0400\"\n> -check_show iso       \"$FUTURE\" \"2152-06-19 18:24:56 -0400\" TIME_IS_64BIT,TIME_T_IS_64BIT\n> -check_show iso-local \"$FUTURE\" \"2152-06-19 22:24:56 +0000\" TIME_IS_64BIT,TIME_T_IS_64BIT\n> +check_show iso       \"$FUTURE\" \"2152-06-19 18:24:56 -0400\" HAVE_64BIT_TIME\n> +check_show iso-local \"$FUTURE\" \"2152-06-19 22:24:56 +0000\" HAVE_64BIT_TIME\n>\n> -check_parse() {\n> +REQUIRE_64BIT_TIME=\n> +check_parse () {\n>  \techo \"$1 -> $2\" >expect\n> -\ttest_expect_${4:-success} \"parse date ($1${3:+ TZ=$3})\" \"\n> -\tTZ=${3:-$TZ} test-tool date parse '$1' >actual &&\n> -\ttest_cmp expect actual\n> +\ttest_expect_success $REQUIRE_64BIT_TIME \"parse date ($1${3:+ TZ=$3}) -> $2\" \"\n> +\t\tTZ=${3:-$TZ} test-tool date parse '$1' >actual &&\n> +\t\ttest_cmp expect actual\n>  \t\"\n>  }\n>\n> @@ -133,6 +139,7 @@ check_parse '1969-12-31 23:59:59 Z' bad\n>  check_parse '1969-12-31 23:59:59 +11' bad\n>  check_parse '1969-12-31 23:59:59 -11' bad\n>\n> +REQUIRE_64BIT_TIME=HAVE_64BIT_TIME\n>  check_parse '2099-12-31 23:59:59' '2099-12-31 23:59:59 +0000'\n>  check_parse '2099-12-31 23:59:59 +00' '2099-12-31 23:59:59 +0000'\n>  check_parse '2099-12-31 23:59:59 Z' '2099-12-31 23:59:59 +0000'\n> @@ -147,6 +154,7 @@ check_parse '2100-00-00 00:00:00 +00' bad\n>  check_parse '2100-00-00 00:00:00 Z' bad\n>  check_parse '2100-00-00 00:00:00 -11' bad\n>  check_parse '2100-00-00 00:00:00 +11' bad\n> +REQUIRE_64BIT_TIME=\n>\n>  check_approxidate() {\n>  \techo \"$1 -> $2 +0000\" >expect\n\nI think this patch looks good. I also think we should probably look into\nfixing the 2099 limit overall. I think Jeff already posted a patch to do\nthe same already:\n\nhttps://lore.kernel.org/r/20240604093345.GA1279521@coredump.intra.peff.net\n"},{"id":"497209","messageId":"CAOLa=ZSJEwha4qc9EOdkLekRAUUUYRxE=aMeu-k3M_7LX2T+SQ@mail.gmail.com","threadId":"61551","inReplyTo":"xmqqjzirdyqm.fsf@gitster.g","subject":"Re: [PATCH v3] date: detect underflow/overflow when parsing dates with timezone offset","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-06-15T11:49:26Z","receivedAt":"2024-06-15T11:49:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>>> It's even OK to use a hard coded constant for the number of days\n>>> since the epoch to the git-end-of-time ;-)\n>>\n>> That's why I noted it as a _Nit_, mostly because it wasn't anything big.\n>> But I found that part of it being dynamic and part of it being static\n>> was inconsistent.\n>\n> Sure, but it is so tiny thing, we shouldn't waste more time than we\n> spend getting the tests right even on 32-bit systems.  We seem to be\n> doing the opposite by talking about this part even more, which is a\n> bit sad.  Any comments on the actual patch I sent as a follow-up?\n>\n\nAgreed! I looked at your patch and it looks good to me, thanks Junio.\n"}]}