Patchwork ui/vnc.c: Fix crash with VNC

login
register
mail settings
Submitter BALATON Zoltan
Date Nov. 8, 2012, 11:55 p.m.
Message ID <alpine.GSO.2.00.1211090054080.11771@mono>
Download mbox | patch
Permalink /patch/197904/
State New
Headers show

Comments

BALATON Zoltan - Nov. 8, 2012, 11:55 p.m.
On Thu, 8 Nov 2012, Gerd Hoffmann wrote:
>> I think this is fixing this at the wrong level. Either we
>> should require that drivers (in this case vmware_vga.c)
>> must not call dpy_gfx_update() with out of range values,
>> or we should do the clipping in the console.c layer, but
>> I don't think requiring every UI backend to clip is the
>> right thing. Anthony?
>
> Agree.  IMHO vmware_vga.c is at fault here and should be fixed.  We can
> add some asserts to console.[ch] to enforce this ...

Would the attached patch help?

Regards,
BALATON Zoltan
From e1ea12f3fa70298f630c0b829d0f304339ca9799 Mon Sep 17 00:00:00 2001
From: BALATON Zoltan <balaton@eik.bme.hu>

Date: Fri, 9 Nov 2012 00:44:29 +0100
Subject: [PATCH 2/2] vmware_vga: Clip updates with negative out of range
 rects to visible area

Added checks and clipping also for negative out of range values in
update rects which have been seen to happen at least with VNC under
Windows NT 4.0 when a window is outside the visible area.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

---
 hw/vmware_vga.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
1.7.10
Michael Tokarev - Nov. 9, 2012, 9 a.m.
On 09.11.2012 03:55, BALATON Zoltan wrote:
> On Thu, 8 Nov 2012, Gerd Hoffmann wrote:
>>> I think this is fixing this at the wrong level. Either we
>>> should require that drivers (in this case vmware_vga.c)
>>> must not call dpy_gfx_update() with out of range values,
>>> or we should do the clipping in the console.c layer, but
>>> I don't think requiring every UI backend to clip is the
>>> right thing. Anthony?
>>
>> Agree.  IMHO vmware_vga.c is at fault here and should be fixed.  We can
>> add some asserts to console.[ch] to enforce this ...
> 
> Would the attached patch help?

I fixed this 2 times, and I remember two other people fixing
the same thing too already.  Lemme find some refs...

thread.gmane.org/gmane.comp.emulators.qemu/166064

---
Is it the same as https://bugs.launchpad.net/bugs/918791 ?
At least it appears to be the same theme...  But there,
the patch (https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff)
also updates width/height.  My comment:
https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791/comments/21
---

Adding some Cc's....
Michael Tokarev - Nov. 9, 2012, 9:06 a.m.
On 09.11.2012 13:00, Michael Tokarev wrote:
> On 09.11.2012 03:55, BALATON Zoltan wrote:
>> On Thu, 8 Nov 2012, Gerd Hoffmann wrote:
>>>> I think this is fixing this at the wrong level. Either we
>>>> should require that drivers (in this case vmware_vga.c)
>>>> must not call dpy_gfx_update() with out of range values,
>>>> or we should do the clipping in the console.c layer, but
>>>> I don't think requiring every UI backend to clip is the
>>>> right thing. Anthony?
>>>
>>> Agree.  IMHO vmware_vga.c is at fault here and should be fixed.  We can
>>> add some asserts to console.[ch] to enforce this ...
>>
>> Would the attached patch help?
> 
> I fixed this 2 times, and I remember two other people fixing
> the same thing too already.  Lemme find some refs...
> 
> http://thread.gmane.org/gmane.comp.emulators.qemu/166064
> 
> ---
> Is it the same as https://bugs.launchpad.net/bugs/918791 ?
> At least it appears to be the same theme...  But there,
> the patch (https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff)
> also updates width/height.  My comment:
> https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791/comments/21
> ---

Another reference: the same problem in qxl (Gerd should know this area):

 http://thread.gmane.org/gmane.comp.emulators.qemu/171093

this patch is a cleanup, -- the problem has been fixed twice in a row in qxl.
We've 3 fixes for it in vmware now too.

So figuring out the proper level where to fix it is important...

/mjt

Patch

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c

index 834588d..e59ab3a 100644

--- a/hw/vmware_vga.c

+++ b/hw/vmware_vga.c

@@ -296,6 +296,14 @@  static inline void vmsvga_update_rect(struct vmsvga_state_s *s,

     uint8_t *src;
     uint8_t *dst;
 
+    if (x < 0 || x + w < 0) {

+        fprintf(stderr, "%s: update negative x position: %d, w: %d\n",

+                __func__, x, w);

+        w -= x;

+        x = MAX(x, 0);

+        y = MAX(w, 0);

+    }

+

     if (x + w > ds_get_width(s->vga.ds)) {
         fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
                 __func__, x, w);
@@ -303,6 +311,14 @@  static inline void vmsvga_update_rect(struct vmsvga_state_s *s,

         w = ds_get_width(s->vga.ds) - x;
     }
 
+    if (y < 0 || y + h < 0) {

+        fprintf(stderr, "%s: update negative y position: %d, h: %d\n",

+                __func__, y, h);

+        h -= y;

+        y = MAX(y, 0);

+        h = MAX(h, 0);

+    }

+

     if (y + h > ds_get_height(s->vga.ds)) {
         fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
                 __func__, y, h);