Re: [Qemu-devel] [PATCH for-2.5] hw/timer/hpet.c: Avoid signed integer overflow which results in bugs on OSX

[Peter Maydell]

On 9 November 2015 at 15:25, Paolo Bonzini <address@hidden> wrote:
>
>
> On 09/11/2015 15:56, Peter Maydell wrote:
>> Signed integer overflow in C is undefined behaviour, and the compiler
>> is at liberty to assume it can never happen and optimize accordingly.
>> In particular, the subtractions in hpet_time_after() and hpet_time_after64()
>> were causing OSX clang to optimize the code such that it was prone to
>> hangs and complaints about the main loop stalling (presumably because
>> we were spending all our time trying to service very high frequency
>> HPET timer callbacks). The clang sanitizer confirms the UB:
>>
>> hw/timer/hpet.c:119:26: runtime error: signed integer overflow: -2146967296 
>> - 2147003978 cannot be represented in type 'int'
>>
>> Fix this by doing the subtraction as an unsigned operation and then
>> converting to signed for the comparison.
>>
>> Reported-by: Aaron Elkins <address@hidden>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/timer/hpet.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
>> index 3037bef..7f0391c 100644
>> --- a/hw/timer/hpet.c
>> +++ b/hw/timer/hpet.c
>> @@ -116,12 +116,12 @@ static uint32_t timer_enabled(HPETTimer *t)
>>
>>  static uint32_t hpet_time_after(uint64_t a, uint64_t b)
>>  {
>> -    return ((int32_t)(b) - (int32_t)(a) < 0);
>> +    return ((int32_t)(b - a) < 0);
>
> Does checkpatch complain about the outer parentheses?

Nope :-)

>>  }
>>
>>  static uint32_t hpet_time_after64(uint64_t a, uint64_t b)
>>  {
>> -    return ((int64_t)(b) - (int64_t)(a) < 0);
>> +    return ((int64_t)(b - a) < 0);
>>  }
>
> Cc: address@hidden

Whoops, yes, cc'ing stable would be a good idea.

> Looks good, thanks!  Can you apply it yourself?

Sure.

thanks
-- PMM