Hi Junio,
On Sun, 23 Apr 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>
> > diff --git a/date.c b/date.c
> > index 92ab31aa441..75f6335cd09 100644
> > --- a/date.c
> > +++ b/date.c
> > @@ -46,7 +46,10 @@ static time_t gm_time_t(timestamp_t time, int tz)
> > minutes = tz < 0 ? -tz : tz;
> > minutes = (minutes / 100)*60 + (minutes % 100);
> > minutes = tz < 0 ? -minutes : minutes;
> > - return time + minutes * 60;
> > +
> > + if (date_overflows(time + minutes * 60))
> > + die("Timestamp too large for this system: %"PRItime, time);
> > + return (time_t)time + minutes * 60;
> > }
>
> All the other calls to date_overflows() take a variable that holds
> timestamp_t and presumably they are checking for integer wraparound
> when the values are computed, but this one is not.
I was debating whether this extra check is necessary and had decided
against it. Apparently I was wrong to do so.
> Perhaps we want to make it a bit more careful here? I wonder if
> something like this is a good approach:
>
> #define date_overflows(time) date_overflows_add(time, 0)
>
> int date_overflows_add(timestamp_t base, timestamp_t minutes)
> {
> timestamp_t t;
> if (unsigned_add_overflows(base, minutes))
> return 1;
> t = base + minutes;
> if ((uintmax_t) t >= TIME_MAX)
> return 1;
> ... what you have in date_overflows() ...
> }
That sounds like uglifying the common case for a single user. Let's not.
The code would also be incorrect, as the `minutes` variable can be
negative, which the `unsigned_add_overflows()` macro cannot handle (it
would report very, very false positives, and it would hurt you more than
me because I live East of Greenwich).
Apart from that, the signature should use seconds, not minutes, or
alternatively multiply by 60.
I'll add a fixed extra check, to the single location that needs it.
Ciao,
Dscho