Re: [PATCH] fix: prevent date underflow when using positive timezone offset

From: Patrick Steinhardt <ps@pks.im>
Date: May 28, 2024, 14:05 UTC
Message-ID: <ZlXkvEeR-PgZMitx@tanuki>
In-Reply-To: <pull.1726.git.git.1716801427015.gitgitgadget@gmail.com>

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?

Show 22 quoted lines

> 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.

Show 10 quoted lines

>  		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.

Show 5 quoted lines

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

You should also add at least one test.

Thanks for your contribution!

Patrick