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

[Michael S. Tsirkin]

On Mon, Nov 09, 2015 at 02:56:31PM +0000, 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>

Agree, this makes no sense the way it's written.

Reviewed-by: Michael S. Tsirkin <address@hidden>

I'll pick this up in the next pull if Paolo doesn't
beat me to it.

> ---
>  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);
>  }
>  
>  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);
>  }
>  
>  static uint64_t ticks_to_ns(uint64_t value)
> -- 
> 2.6.2