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

2014-01-20 Thread Alon Levy 
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 al...@redhat.com
---
 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;
 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;
 }
 
-- 
1.8.4.2

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

2014-01-20 Thread Markus Armbruster 
Alon Levy al...@redhat.com writes:

 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 al...@redhat.com

Reviewed-by: Markus Armbruster arm...@redhat.com

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

2014-01-20 Thread Peter Maydell 
On 20 January 2014 13:29, Alon Levy al...@redhat.com 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 al...@redhat.com
 ---
  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