On 8/17/13, Theo de Raadt <dera...@cvs.openbsd.org> wrote:
>> I see naddy@ already commited a fix for this from NetBSD. Thank
>> you!
>>
>> My patch fwiw was to cast second arg to std::max() to long.
>>
>> $OpenBSD$
>> --- src/Toolbar.cc.orig Sat Aug 17 16:58:14 2013
>> +++ src/Toolbar.cc Sat Aug 17 17:08:12 2013
>> @@ -44,7 +44,7 @@ long nextTimeout(int resolution)
>> {
>> timeval now;
>> gettimeofday(&now, 0);
>> - return (std::max(1000l, ((((resolution - (now.tv_sec % resolution)) *
>> 1000l))
>> + return (std::max(1000l, (long)((((resolution - (now.tv_sec %
>> resolution)) * 1000l))
>> - (now.tv_usec / 1000l))));
>> }
>
> So basically, this takes a long long value, and casts it to a long.
Yes, but that's not the entire story.
the function declaration is: long nextTimeout(int resolution)
So it would truncate anyway. But look at it closer. It
takes the value of timeval.tv_sec (your time_t / long long)
and mod's it with resolution (an int value). The only thing
it then does is multiply it by 1000L.
The only reason this function "broke" is C++'s std::max()
defined as a template function, requiring both args be of
the same typename. Since the original code uses 1000L
literal for the first arg, the compiler was having issues with
the long long as the second arg., but ...
What is long long % int? another int value? Yes, I realize
it would be a long long according to the compiler, but,
I fail to see where/how 0xFFFFFFFFFFFFFFFF % 0xFF would
ever be greater than 0xfe.
As is, the best you can expect from that function is a long
second argument for std::max(). Unless I'm completely missing
something here (and I'll certainly budget for that possibility).
--patrick
> So, on a 32 bit machine, that throws away the high bits.
>
> Meaning it will make it compile today, but it will break in 2038.
>
> So, let me summarize. the transition to 'long long' exposes a real
> problem. The addition of this (long) cast now hides the problem; so
> that sometime further into the future someone is going to have a
> harder time fixing it right.
>
> That is wrong.
>
