On Mon, May 27, 2024 at 09:17:06AM +0000, darcy via GitGitGadget wrote:
> From: darcy <[email protected]>

The commit message should start with the subsystem that you're touching,
which in this case would be "date", e.g.:

    date: detect underflow when parsing dates with positive timezone offset

> Overriding the date of a commit to be `1970-01-01` with a large enough
> timezone for the equivalent GMT time to before 1970 is no longer
> accepted.

Okay.

> Example: `GIT_COMMITTER_DATE='1970-01-01T00:00:00+10' git commit` would
> previously be accepted, only to unexpectedly fail in other parts of the
> code, such as `git push`. The timestamp is now checked against postitive
> timezone values.

How exactly does the failure look like before and after?

> Signed-off-by: darcy <[email protected]>
> ---
>     fix: prevent date underflow when using positive timezone offset
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1726%2Fdxrcy%2Fmaster-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1726/dxrcy/master-v1
> Pull-Request: https://github.com/git/git/pull/1726
> 
>  date.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/date.c b/date.c
> index 7365a4ad24f..8388629f267 100644
> --- a/date.c
> +++ b/date.c
> @@ -908,7 +908,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>  			match = match_alpha(date, &tm, offset);
>  		else if (isdigit(c))
>  			match = match_digit(date, &tm, offset, &tm_gmt);
> -		else if ((c == '-' || c == '+') && isdigit(date[1]))
> +		else if ((c == '-' || c == '+') && isdigit(date[1]) && tm.tm_hour != -1)
>  			match = match_tz(date, offset);

Without having a deep understanding of the code I don't quite see the
connection between this change and the problem description. Is it
necessary? If so, it might help to explain why it's needed in the commit
message or in the code.

>  		if (!match) {
> @@ -937,8 +937,13 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>  		}
>  	}
>  
> -	if (!tm_gmt)
> +	if (!tm_gmt) {
> +		if (*offset > 0 && *offset * 60 > *timestamp) {
> +			return -1;
> +		}

Nit: we don't add curly braces around one-line conditional bodies.

This change here is the meat of it and looks like I'd expect.

>  		*timestamp -= *offset * 60;
> +	}
> +
>  	return 0; /* success */
>  }

You should also add at least one test.

Thanks for your contribution!

Patrick

On 28/05/2024 15:05, Patrick Steinhardt wrote:
> On Mon, May 27, 2024 at 09:17:06AM +0000, darcy via GitGitGadget wrote:
>> From: darcy <[email protected]>
>> diff --git a/date.c b/date.c
>> index 7365a4ad24f..8388629f267 100644
>> --- a/date.c
>> +++ b/date.c
>> @@ -908,7 +908,7 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>>   			match = match_alpha(date, &tm, offset);
>>   		else if (isdigit(c))
>>   			match = match_digit(date, &tm, offset, &tm_gmt);
>> -		else if ((c == '-' || c == '+') && isdigit(date[1]))
>> +		else if ((c == '-' || c == '+') && isdigit(date[1]) && tm.tm_hour != -1)
>>   			match = match_tz(date, offset);
> > Without having a deep understanding of the code I don't quite see the
> connection between this change and the problem description. Is it
> necessary? If so, it might help to explain why it's needed in the commit
> message or in the code.

I was wondering about this change too

>>   		if (!match) {
>> @@ -937,8 +937,13 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
>>   		}
>>   	}
>>   >> -	if (!tm_gmt)
>> +	if (!tm_gmt) {
>> +		if (*offset > 0 && *offset * 60 > *timestamp) {
>> +			return -1;
>> +		}
> > Nit: we don't add curly braces around one-line conditional bodies.
> > This change here is the meat of it and looks like I'd expect.
> >>   		*timestamp -= *offset * 60;

Do we also want to check for overflows in the other direction (a large timestamp with a negative timezone offset)?

Best Wishes

Phillip

Patrick Steinhardt <[email protected]> writes:

> On Mon, May 27, 2024 at 09:17:06AM +0000, darcy via GitGitGadget wrote:
>> From: darcy <[email protected]>
>
> The commit message should start with the subsystem that you're touching,
> which in this case would be "date", e.g.:
>
>     date: detect underflow when parsing dates with positive timezone offset
>
>> Overriding the date of a commit to be `1970-01-01` with a large enough
>> timezone for the equivalent GMT time to before 1970 is no longer
>> accepted.
>
> Okay.

"is no longer accepted" made me read the sentence three times to get
what the author wants to say.  Initially I thought the author wanted
to report a regression where we used to accept but with a recent
change we stopped accepting.

In our convention, we present the status quo, point out why it is
awkard/incorrect/bad, and then propose a new behaviour.

    Overriding ... before 1970 BEHAVES THIS WAY.

    This leads to BAD BEHAVIOUR FOR SUCH AND SUCH REASONS.

    Instead check the timezone offset and fail if the resulting time
    becomes before the epoch, "1970-01-01T00:00:00Z", when parsing.

with the blanks filled in appropriately would have been much easier
to see.

>> Example: `GIT_COMMITTER_DATE='1970-01-01T00:00:00+10' git commit` would
>> previously be accepted, only to unexpectedly fail in other parts of the
>> code, such as `git push`. The timestamp is now checked against postitive
>> timezone values.
>
> How exactly does the failure look like before and after?

Yes, good question.

>> Signed-off-by: darcy <[email protected]>
>> ---

I cannot offhand tell if Documentation/SubmittingPatches:real-name
is followed here or ignored, so just to double check...

Everything else in your review made sense to me.  I guess that
checking for tm_hour is assuming that TZ offset should always come
before the values necessary to compute the timestamp comes, but it
smells like an unwarranted assumption and not explaining the change
in the proposed log message is a bad sign.

Thanks.

"darcy via GitGitGadget" <[email protected]> writes:

>     fix: prevent date underflow when using positive timezone offset
>     
>     cc: Patrick Steinhardt [email protected] cc: Phillip Wood
>     [email protected]

You're expected to respond to review comments before you send in
updated patches.  I didn't see the review comments responded to in
the thread:

  https://lore.kernel.org/git/[email protected]/

Please see "A typical life cycle of a patch series" section of the
SubmittingPatches document.

  https://git.github.io/htmldocs/SubmittingPatches.html#patch-flow

Step #3 and Step #4 are distinct.

> Without having a deep understanding of the code I don't quite see the
> connection between this change and the problem description. Is it
> necessary? If so, it might help to explain why it's needed in the commit
> message or in the code.

> ...I guess that
> checking for tm_hour is assuming that TZ offset should always come
> before the values necessary to compute the timestamp comes, but it
> smells like an unwarranted assumption and not explaining the change
> in the proposed log message is a bad sign.

This line has been reverted. The point was it would only parse the
timezone offset if it occurs after the time part of the date, but I
have realized that this is unrelated to the purpose of this change.

> You should also add at least one test.

Yep, thanks, added now to `t0006-date.sh`.

> Do we also want to check for overflows in the other direction (a large timestamp with a negative timezone offset)?

Is this something people want added? I am happy to implement this if
so, though it wasn't my original intention.

Issues with the commit message should also be resolved. Thank you
everyone for your patience :)

On 3/6/24 21:13, Junio C Hamano wrote:

> "darcy via GitGitGadget" <[email protected]> writes:
>
>>      fix: prevent date underflow when using positive timezone offset
>>      >>      cc: Patrick Steinhardt [email protected] cc: Phillip Wood
>>      [email protected]
> You're expected to respond to review comments before you send in
> updated patches.  I didn't see the review comments responded to in
> the thread:
>
>    https://lore.kernel.org/git/[email protected]/
>
> Please see "A typical life cycle of a patch series" section of the
> SubmittingPatches document.
>
>    https://git.github.io/htmldocs/SubmittingPatches.html#patch-flow
>
> Step #3 and Step #4 are distinct.

Junio C Hamano <[email protected]> writes:

> Karthik Nayak <[email protected]> writes:
>
>>> It's even OK to use a hard coded constant for the number of days
>>> since the epoch to the git-end-of-time ;-)
>>
>> That's why I noted it as a _Nit_, mostly because it wasn't anything big.
>> But I found that part of it being dynamic and part of it being static
>> was inconsistent.
>
> Sure, but it is so tiny thing, we shouldn't waste more time than we
> spend getting the tests right even on 32-bit systems.  We seem to be
> doing the opposite by talking about this part even more, which is a
> bit sad.  Any comments on the actual patch I sent as a follow-up?
>

Agreed! I looked at your patch and it looks good to me, thanks Junio.

The system must support 64-bit time and its time_t must be 64-bit
wide to pass these tests.  Combine these two prerequisites together
to simplify the tests.  In theory, they could be fulfilled
independently and tests could require only one without the other,
but in practice, but in practice these must come hand-in-hand.

Update the "check_parse" test helper to pay attention to the
REQUIRE_64BIT_TIME variable, which can be set to the HAVE_64BIT_TIME
prerequisite so that a parse test can be skipped on 32-bit systems.
This will be used in the next step to skip tests for timestamps near
the end of year 2099, as 32-bit systems will not be able to express
a timestamp beyond 2038 anyway.

Signed-off-by: Junio C Hamano <[email protected]>
---
 t/t0006-date.sh | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 3031256d14..24e8647f26 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
+
 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
 	"
 }
 
-- 
2.45.2-796-g2ef7a3d713

From: Darcy Burke <[email protected]>

Overriding the date of a commit to be close to "1970-01-01 00:00:00"
with a large enough positive timezone for the equivelant GMT time to be
before the epoch is considered valid by `parse_date_basic`. Similar
behaviour occurs when using a date close to "2099-12-31 23:59:59" (the
maximum date allowed by `tm_to_time_t`) with a large enough negative
timezone offset.

This leads to an integer underflow or underflow respectively in the
commit timestamp, which is not caught by `git-commit`, but will cause
other services to fail, such as `git-fsck`, which, for the first case,
reports "badDateOverflow: invalid author/committer line - date causes
integer overflow".

Instead check the timezone offset and fail if the resulting time comes
before the epoch "1970-01-01T00:00:00Z" or after the maximum date
"2099-12-31T23:59:59Z".

Using the REQUIRE_64BIT_TIME prerequisite, make sure that the tests
near the end of Git time (aka end of year 2099) are not attempted on
purely 32-bit systems, as they cannot express timestamp beyond 2038
anyway.

Signed-off-by: Darcy Burke <[email protected]>
[jc: fixups for 32-bit platforms]
Signed-off-by: Junio C Hamano <[email protected]>
---
 date.c          | 12 +++++++++++-
 t/t0006-date.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/date.c b/date.c
index 7365a4ad24..bee9fe8f10 100644
--- a/date.c
+++ b/date.c
@@ -868,6 +868,10 @@ static int match_object_header_date(const char *date, timestamp_t *timestamp, in
 	return 0;
 }
 
+
+/* timestamp of 2099-12-31T23:59:59Z, including 32 leap days */
+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. */
 int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
@@ -937,8 +941,14 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
 		}
 	}
 
-	if (!tm_gmt)
+	if (!tm_gmt) {
+		if (*offset > 0 && *offset * 60 > *timestamp)
+			return -1;
+		if (*offset < 0 && -*offset * 60 > timestamp_max - *timestamp)
+			return -1;
 		*timestamp -= *offset * 60;
+	}
+
 	return 0; /* success */
 }
 
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 24e8647f26..fd373e1b39 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -123,6 +123,39 @@ check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
 check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
 check_parse 'Thu, 7 Apr 2005 15:14:13 -0700' '2005-04-07 15:14:13 -0700'
 
+check_parse '1970-01-01 00:00:00' '1970-01-01 00:00:00 +0000'
+check_parse '1970-01-01 00:00:00 +00' '1970-01-01 00:00:00 +0000'
+check_parse '1970-01-01 00:00:00 Z' '1970-01-01 00:00:00 +0000'
+check_parse '1970-01-01 00:00:00 -01' '1970-01-01 00:00:00 -0100'
+check_parse '1970-01-01 00:00:00 +01' bad
+check_parse '1970-01-01 00:00:00 +11' bad
+check_parse '1970-01-01 00:59:59 +01' bad
+check_parse '1970-01-01 01:00:00 +01' '1970-01-01 01:00:00 +0100'
+check_parse '1970-01-01 01:00:00 +11' bad
+check_parse '1970-01-02 00:00:00 +11' '1970-01-02 00:00:00 +1100'
+check_parse '1969-12-31 23:59:59' bad
+check_parse '1969-12-31 23:59:59 +00' bad
+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'
+check_parse '2099-12-31 23:59:59 +01' '2099-12-31 23:59:59 +0100'
+check_parse '2099-12-31 23:59:59 -01' bad
+check_parse '2099-12-31 23:59:59 -11' bad
+check_parse '2099-12-31 23:00:00 -01' bad
+check_parse '2099-12-31 22:59:59 -01' '2099-12-31 22:59:59 -0100'
+check_parse '2100-00-00 00:00:00' bad
+check_parse '2099-12-30 00:00:00 -11' '2099-12-30 00:00:00 -1100'
+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
 	test_expect_${3:-success} "parse approxidate ($1)" "
-- 
2.45.2-796-g2ef7a3d713

On Tue, Jun 25, 2024 at 7:13 PM Junio C Hamano <[email protected]> wrote:
> The system must support 64-bit time and its time_t must be 64-bit
> wide to pass these tests.  Combine these two prerequisites together
> to simplify the tests.  In theory, they could be fulfilled
> independently and tests could require only one without the other,
> but in practice, but in practice these must come hand-in-hand.

s/but in practice, but in practice/but in practice/

> Update the "check_parse" test helper to pay attention to the
> REQUIRE_64BIT_TIME variable, which can be set to the HAVE_64BIT_TIME
> prerequisite so that a parse test can be skipped on 32-bit systems.
> This will be used in the next step to skip tests for timestamps near
> the end of year 2099, as 32-bit systems will not be able to express
> a timestamp beyond 2038 anyway.
>
> Signed-off-by: Junio C Hamano <[email protected]>

Eric Sunshine <[email protected]> writes:

> On Tue, Jun 25, 2024 at 7:13 PM Junio C Hamano <[email protected]> wrote:
>> The system must support 64-bit time and its time_t must be 64-bit
>> wide to pass these tests.  Combine these two prerequisites together
>> to simplify the tests.  In theory, they could be fulfilled
>> independently and tests could require only one without the other,
>> but in practice, but in practice these must come hand-in-hand.
>
> s/but in practice, but in practice/but in practice/

Thanks, always, for your sharp eyes.