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

From: Phillip Wood <[email-masked]>
Date: Mon, 23 Sep 2019 10:30:39 +0100

On 23/09/2019 09:37, SZEDER Gábor wrote:
> 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.

It's still going to trip up static analysers though isn't it? Also it 
will confuse readers who have to reason that it does not overflow in 
practice as we (probably?) never use very negative values

Best Wishes

Phillip

>> 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 ':'