diff mbox series

fix: prevent date underflow when using positive timezone offset

Message ID pull.1726.git.git.1716801427015.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series fix: prevent date underflow when using positive timezone offset | expand

Commit Message

darcy May 27, 2024, 9:17 a.m. UTC
From: darcy <acednes@gmail.com>

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.

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.

Signed-off-by: darcy <acednes@gmail.com>
---
    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(-)


base-commit: b9cfe4845cb2562584837bc0101c0ab76490a239

Comments

Patrick Steinhardt May 28, 2024, 2:05 p.m. UTC | #1
On Mon, May 27, 2024 at 09:17:06AM +0000, darcy via GitGitGadget wrote:
> From: darcy <acednes@gmail.com>

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 <acednes@gmail.com>
> ---
>     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
Phillip Wood May 28, 2024, 2:49 p.m. UTC | #2
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 <acednes@gmail.com>
>> 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
Junio C Hamano May 28, 2024, 5:06 p.m. UTC | #3
Patrick Steinhardt <ps@pks.im> writes:

> On Mon, May 27, 2024 at 09:17:06AM +0000, darcy via GitGitGadget wrote:
>> From: darcy <acednes@gmail.com>
>
> 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 <acednes@gmail.com>
>> ---

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.
diff mbox series

Patch

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);
 
 		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;
+		}
 		*timestamp -= *offset * 60;
+	}
+
 	return 0; /* success */
 }