Clean 'make check' against gcc -fsanitize=address,undefined

Richard Kettlewell rjk at greenend.org.uk
Sat Jun 27 14:44:53 UTC 2015


On 2015-06-26 19:23, Julien ÉLIE wrote:
> Hi Richard,
> 
>>> The current code is:
>>> now = (tv.tv_sec - base.tv_sec) * 1000;
>>> now += (tv.tv_usec - base.tv_usec) / 1000;
>>
>> That doesn't help.  In the current code the multiplication is done in
>> the signed type.  If it overflows (as it will after a few weeks, on
>> 32-bit platforms) then we are already into undefined behavior (C99
>> s6.5#5).
> 
> Then, so as to prevent overflows, maybe we should call TMRsummary() more
> often, or force it when the timer is near to overflowing.  I believe the
> issue only appears when innconf->timer is 0 because calls to
> TMRsummary() are not done in this configuration.
> 
> Oh, I see that the default value for innconf->timer is 0 whereas the
> inn.conf documentation indicates that "enabling this is highly
> recommended".  Maybe we could change the default value for INN 2.6.0!
> Suggestion:  600s (10mn).
> 
> If set to 0, maybe we could force the log anyway when it reaches 42
> days?  It won't do much harm, and it will prevent these overflows.
> 
> Any thought about that?

(See below.)

>>> If I follow your suggestion with tv=2,2s and base=1,4s:
>>>
>>> now = (unsigned long)(tv.tv_sec - base.tv_sec) * 1000u;
>>> usec = tv.tv_usec - base.tv_usec; /* maybe -ve */
>>> msec = usec / 1000; /* still maybe -ve */
>>> now += (unsigned long)msec;
>>> I believe it would result in:
>>> now = 1000 + (-200+UINT_MAX+1) which would overflow because
>>> of the negative msec being converted to unsigned, instead of:
>>
>> Overflow behavior for unsigned types (including conversion of negative
>> values to unsigned types) is defined: the result is the residue modulo
>> 2^n (C99 s6.2.5#9).  The result with the patch is therefore 800.
> 
> OK, thanks.
> Patch applied.
> 
> Do you believe something similar should be done for the computation of
> OVERartcheck and OVERtime in nnrpd code?
> and statistics->busy as well as total in backends/overchan.c?

In principle yes, but I think overflow will take much longer.

> and STATUSgettime() in innd/status.c?

I think so, yes.

In all cases, perhaps the best approach is to use 64-bit types
throughout?  That pushes all overflows nearly 300M years into the future.

ttfn/rjk



More information about the inn-workers mailing list