Re: [PATCH] name-rev: avoid cutoff timestamp underflow

From: Phillip Wood <phillip.wood123@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] name-rev: avoid cutoff timestamp underflow
Date: Sun, 22 Sep 2019 19:57:36 +0100	[thread overview]
Message-ID: <5692e751-ae5b-e1c8-e5f7-79f0b43e20c0@gmail.com> (raw)
In-Reply-To: <20190922180143.25026-1-szeder.dev@gmail.com>

Hi Gábor

On 22/09/2019 19:01, SZEDER Gábor wrote:
> When 'git name-rev' is invoked with commit-ish parameters, it tries to
> save some work, and doesn't visit commits older than the committer
> date of the oldest given commit minus a one day worth of slop.  Since
> our 'timestamp_t' is an unsigned type, this leads to a timestamp
> underflow when the committer date of the oldest given commit is within
> a day of the UNIX epoch.  As a result the cutoff timestamp ends up
> far-far in the future, and 'git name-rev' doesn't visit any commits,
> and names each given commit as 'undefined'.
> 
> Check whether substacting the slop from the oldest committer date
> would lead to an underflow, and use a 0 as cutoff in that case.  This
> way it will handle commits shortly after the epoch even if we were to
> switch to a signed 'timestamp_t' (but then we'll have to worry about
> signed underflow for very old commits).
> 
> Note that the type of the cutoff timestamp variable used to be signed
> before 5589e87fd8 (name-rev: change a "long" variable to timestamp_t,
> 2017-05-20).  The behavior was still the same even back then, but the
> underflow didn't happen when substracting the slop from the oldest
> committer date, but when comparing the signed cutoff timestamp with
> unsigned committer dates in name_rev().  IOW, this underflow bug is as
> old as 'git name-rev' itself.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> 
> This patch adds a test at the end of 't6120-describe.sh', so it will
> conflict with my non-recursive name-rev patch series, which adds a
> test there as well, but the conflict should be wasy to resolve.
> 
>    https://public-inbox.org/git/20190919214712.7348-7-szeder.dev@gmail.com/
> 
>   builtin/name-rev.c  | 15 ++++++++++++---
>   t/t6120-describe.sh | 15 +++++++++++++++
>   2 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index c785fe16ba..a4d8d312ab 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -9,7 +9,11 @@
>   #include "sha1-lookup.h"
>   #include "commit-slab.h"
>   
> -#define CUTOFF_DATE_SLOP 86400 /* one day */
> +/*
> + * One day.  See the 'name a rev close to epoch' test in t6120 when
> + * changing this value
> + */
> +#define CUTOFF_DATE_SLOP 86400
>   
>   typedef struct rev_name {
>   	const char *tip_name;
> @@ -481,8 +485,13 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
>   		add_object_array(object, *argv, &revs);
>   	}
>   
> -	if (cutoff)
> -		cutoff = cutoff - CUTOFF_DATE_SLOP;
> +	if (cutoff) {
> +		/* check for undeflow */
> +		if (cutoff - CUTOFF_DATE_SLOP < cutoff)

Nice catch but wouldn't this be clearer as
   if (cutoff > CUTOFF_DATE_SLOP) ?

Best Wishes

Phillip
> +			cutoff = cutoff - CUTOFF_DATE_SLOP;
> +		else
> +			cutoff = 0;
> +	}
>   	for_each_ref(name_ref, &data);
>   
>   	if (transform_stdin) {
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 2b883d8174..965e633c32 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -424,4 +424,19 @@ test_expect_success 'describe complains about missing object' '
>   	test_must_fail git describe $ZERO_OID
>   '
>   
> +test_expect_success 'name-rev a rev shortly after epoch' '
> +	test_when_finished "git checkout master" &&
> +
> +	git checkout --orphan no-timestamp-underflow &&
> +	# Any date closer to epoch than the CUTOFF_DATE_SLOP constant
> +	# in builtin/name-rev.c.
> +	GIT_COMMITTER_DATE="@1234 +0000" \
> +	git commit -m "committer date shortly after epoch" &&
> +	near_commit_oid=$(git rev-parse HEAD) &&
> +
> +	echo "$near_commit_oid no-timestamp-underflow" >expect &&
> +	git name-rev $near_commit_oid >actual &&
> +	test_cmp expect actual
> +'
> +
>   test_done
> 