Re: [Qemu-devel] [PATCH v2] hw/display/qxl: fix signed to unsigned comparison

[Peter Maydell]

On 20 January 2014 13:29, Alon Levy <address@hidden> wrote:
> Fix signed to unsigned comparison in qxl_create_guest_primary and add
> the size of the framebuffer to the error message used when setting the
> guest bug state (which causes a complete guess blackout until reset, so
> it helps if it is verbose).
>
> Signed-off-by: Alon Levy <address@hidden>
> ---
>  hw/display/qxl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index e4f172e..b799b51 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1360,14 +1360,15 @@ static void qxl_create_guest_primary(PCIQXLDevice 
> *qxl, int loadvm,
>  {
>      QXLDevSurfaceCreate surface;
>      QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
> -    int size;
> +    uint32_t size;
>      int requested_height = le32_to_cpu(sc->height);
>      int requested_stride = le32_to_cpu(sc->stride);
>
>      size = abs(requested_stride) * requested_height;

This looks a bit dubious -- the multiply is still going
to be done with signed arithmetic, so if it's possible
we might overflow so as to require a uint32_t size
then we've already hit undefined behaviour. Also, if
the multiply overflows 32 bits the check will not
catch this. Probably better to do this as a 64 bit
multiply.

Is requested_height really supposed to be signed?
Why is requested_stride an int that needs to go
through abs()? What is the behaviour supposed to be
if it is the minimum integer (in which case abs(x)
is undefined)?

>      if (size > qxl->vgamem_size) {
> -        qxl_set_guest_bug(qxl, "%s: requested primary larger then 
> framebuffer"
> -                               " size", __func__);
> +        qxl_set_guest_bug(qxl, "%s: requested primary larger than 
> framebuffer"
> +                               " size %u > %u", __func__, size,
> +                               qxl->vgamem_size);
>          return;
>      }

PRIu32 is more portable for printing uint32_t types.

thanks
-- PMM