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

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Johannes Sixt <j6t@kdbg.org>
Cc: phillip.wood@dunelm.org.uk, Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org
Subject: Re: [PATCH] name-rev: avoid cutoff timestamp underflow
Date: Mon, 23 Sep 2019 10:37:23 +0200	[thread overview]
Message-ID: <20190923083723.GD10866@szeder.dev> (raw)
In-Reply-To: <8e7617ef-85d0-df3f-4418-5a2502b8e726@kdbg.org>

On Sun, Sep 22, 2019 at 11:01:26PM +0200, Johannes Sixt wrote:
> Am 22.09.19 um 21:53 schrieb SZEDER Gábor:
> > On Sun, Sep 22, 2019 at 07:57:36PM +0100, Phillip Wood wrote:
> >> On 22/09/2019 19:01, SZEDER Gábor wrote:
> >>> +/*
> >>> + * 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) ?
> > 
> > It would only be clearer now, with an unsigned 'timestamp_t'.  I
> > tried to future-proof for a signed 'timestamp_t' and a cutoff date
> > before the UNIX epoch.
> 
> Huh? For signed cutoff and positive CUTOFF_DATE_SLOP,
> cutoff - CUTOFF_DATE_SLOP < cutoff is ALWAYS true. Signed interger
> underflow is undefined behavior and signed integer arithmetic does not
> wrap around!
> 
> IOW, the new condition makes only sense today, because cutoff is an
> unsigned type, but breaks down should we switch to a signed type.

Yeah, that's what I meant with worrying about signed underflow in the
commit message.  As long as the cutoff is at least a day later than
the minimum value of our future signed 'timestamp_t', the condition
does the right thing.  And considering that oldest time a signed 64
bit timestamp can represent far exceeds the age of the universe, and
the oldest value of even a signed 32 bit timestamp is almost half the
age of the Earth, I wasn't too worried.

> You need this on top:
> 
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index a4d8d312ab..2d83c2b172 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -487,10 +487,10 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
>  
>  	if (cutoff) {
>  		/* check for undeflow */
> -		if (cutoff - CUTOFF_DATE_SLOP < cutoff)
> +		if (cutoff > TIME_MIN + CUTOFF_DATE_SLOP)
>  			cutoff = cutoff - CUTOFF_DATE_SLOP;
>  		else
> -			cutoff = 0;
> +			cutoff = TIME_MIN;
>  	}
>  	for_each_ref(name_ref, &data);
>  
> diff --git a/git-compat-util.h b/git-compat-util.h
> index c68c61d07c..1bdc21a069 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -344,6 +344,7 @@ typedef uintmax_t timestamp_t;
>  #define PRItime PRIuMAX
>  #define parse_timestamp strtoumax
>  #define TIME_MAX UINTMAX_MAX
> +#define TIME_MIN 0
>  
>  #ifndef PATH_SEP
>  #define PATH_SEP ':'