[Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerhard Wiesinger]

Fix crash with VNC under NT 4.0 and VMWare VGA and window which is outside of the visible area.

Backtrace:
#0  set_bit (addr=<optimized out>, nr=-3) at ./bitops.h:122
#1  vnc_dpy_update (ds=<optimized out>, x=-48, y=145, w=57, h=161) at ui/vnc.c:452
#2  0x00007f1ce057e2ec in dpy_update (s=0x7f1ce1c8c880, h=16, w=66, y=145, x=-57) at ./console.h:242
#3  vmsvga_update_rect (h=16, w=66, y=145, x=-57, s=0x7f1ce1cb3dd0) at hw/vmware_vga.c:324
#4  vmsvga_update_rect_flush (s=0x7f1ce1cb3dd0) at hw/vmware_vga.c:357
#5  vmsvga_update_display (opaque=0x7f1ce1cb3dd0) at hw/vmware_vga.c:960
#6  0x00007f1ce05f0b37 in vnc_refresh (opaque=0x7f1cd8526010) at ui/vnc.c:2590
#7  0x00007f1ce05c002b in qemu_run_timers (clock=0x7f1ce1c4f910) at qemu-timer.c:392
#8  qemu_run_timers (clock=0x7f1ce1c4f910) at qemu-timer.c:373
#9  0x00007f1ce05c028d in qemu_run_all_timers () at qemu-timer.c:449
#10 0x00007f1ce058f2ee in main_loop_wait (nonblocking=<optimized out>) at main-loop.c:502
#11 0x00007f1ce047acb3 in main_loop () at vl.c:1655
#12 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:3826

Signed-off-by: Gerhard Wiesinger <lists@wiesinger.com>
---
  ui/vnc.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index 7c120e6..ae6d819 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -453,6 +453,11 @@ static void vnc_dpy_update(DisplayState *ds, int x, int y, int w, int h)
      w = MIN(x + w, width) - x;
      h = MIN(h, height);

+    x = MAX(x, 0);
+    y = MAX(y, 0);
+    w = MAX(w, 0);
+    h = MAX(h, 0);
+
      for (; y < h; y++)
          for (i = 0; i < w; i += 16)
              set_bit((x + i) / 16, s->dirty[y]);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerhard Wiesinger]

Ping?

On 01.11.2012 21:06, Gerhard Wiesinger wrote:
> Fix crash with VNC under NT 4.0 and VMWare VGA and window which is 
> outside of the visible area.
>
> Backtrace:
> #0  set_bit (addr=<optimized out>, nr=-3) at ./bitops.h:122
> #1  vnc_dpy_update (ds=<optimized out>, x=-48, y=145, w=57, h=161) at 
> ui/vnc.c:452
> #2  0x00007f1ce057e2ec in dpy_update (s=0x7f1ce1c8c880, h=16, w=66, 
> y=145, x=-57) at ./console.h:242
> #3  vmsvga_update_rect (h=16, w=66, y=145, x=-57, s=0x7f1ce1cb3dd0) at 
> hw/vmware_vga.c:324
> #4  vmsvga_update_rect_flush (s=0x7f1ce1cb3dd0) at hw/vmware_vga.c:357
> #5  vmsvga_update_display (opaque=0x7f1ce1cb3dd0) at hw/vmware_vga.c:960
> #6  0x00007f1ce05f0b37 in vnc_refresh (opaque=0x7f1cd8526010) at 
> ui/vnc.c:2590
> #7  0x00007f1ce05c002b in qemu_run_timers (clock=0x7f1ce1c4f910) at 
> qemu-timer.c:392
> #8  qemu_run_timers (clock=0x7f1ce1c4f910) at qemu-timer.c:373
> #9  0x00007f1ce05c028d in qemu_run_all_timers () at qemu-timer.c:449
> #10 0x00007f1ce058f2ee in main_loop_wait (nonblocking=<optimized out>) 
> at main-loop.c:502
> #11 0x00007f1ce047acb3 in main_loop () at vl.c:1655
> #12 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized 
> out>) at vl.c:3826
>
> Signed-off-by: Gerhard Wiesinger <lists@wiesinger.com>
> ---
>  ui/vnc.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 7c120e6..ae6d819 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -453,6 +453,11 @@ static void vnc_dpy_update(DisplayState *ds, int 
> x, int y, int w, int h)
>      w = MIN(x + w, width) - x;
>      h = MIN(h, height);
>
> +    x = MAX(x, 0);
> +    y = MAX(y, 0);
> +    w = MAX(w, 0);
> +    h = MAX(h, 0);
> +
>      for (; y < h; y++)
>          for (i = 0; i < w; i += 16)
>              set_bit((x + i) / 16, s->dirty[y]);

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerhard Wiesinger]

Yet another ping.

Ciao,
Gerhard

On 04.11.2012 11:28, Gerhard Wiesinger wrote:
> Ping?
>
> On 01.11.2012 21:06, Gerhard Wiesinger wrote:
>> Fix crash with VNC under NT 4.0 and VMWare VGA and window which is 
>> outside of the visible area.
>>
>> Backtrace:
>> #0  set_bit (addr=<optimized out>, nr=-3) at ./bitops.h:122
>> #1  vnc_dpy_update (ds=<optimized out>, x=-48, y=145, w=57, h=161) at 
>> ui/vnc.c:452
>> #2  0x00007f1ce057e2ec in dpy_update (s=0x7f1ce1c8c880, h=16, w=66, 
>> y=145, x=-57) at ./console.h:242
>> #3  vmsvga_update_rect (h=16, w=66, y=145, x=-57, s=0x7f1ce1cb3dd0) 
>> at hw/vmware_vga.c:324
>> #4  vmsvga_update_rect_flush (s=0x7f1ce1cb3dd0) at hw/vmware_vga.c:357
>> #5  vmsvga_update_display (opaque=0x7f1ce1cb3dd0) at hw/vmware_vga.c:960
>> #6  0x00007f1ce05f0b37 in vnc_refresh (opaque=0x7f1cd8526010) at 
>> ui/vnc.c:2590
>> #7  0x00007f1ce05c002b in qemu_run_timers (clock=0x7f1ce1c4f910) at 
>> qemu-timer.c:392
>> #8  qemu_run_timers (clock=0x7f1ce1c4f910) at qemu-timer.c:373
>> #9  0x00007f1ce05c028d in qemu_run_all_timers () at qemu-timer.c:449
>> #10 0x00007f1ce058f2ee in main_loop_wait (nonblocking=<optimized 
>> out>) at main-loop.c:502
>> #11 0x00007f1ce047acb3 in main_loop () at vl.c:1655
>> #12 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized 
>> out>) at vl.c:3826
>>
>> Signed-off-by: Gerhard Wiesinger <lists@wiesinger.com>
>> ---
>>  ui/vnc.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 7c120e6..ae6d819 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -453,6 +453,11 @@ static void vnc_dpy_update(DisplayState *ds, int 
>> x, int y, int w, int h)
>>      w = MIN(x + w, width) - x;
>>      h = MIN(h, height);
>>
>> +    x = MAX(x, 0);
>> +    y = MAX(y, 0);
>> +    w = MAX(w, 0);
>> +    h = MAX(h, 0);
>> +
>>      for (; y < h; y++)
>>          for (i = 0; i < w; i += 16)
>>              set_bit((x + i) / 16, s->dirty[y]);
>
>

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Peter Maydell]

On 1 November 2012 21:06, Gerhard Wiesinger <lists@wiesinger.com> wrote:
> Fix crash with VNC under NT 4.0 and VMWare VGA and window which is outside
> of the visible area.
>
> Backtrace:
> #0  set_bit (addr=<optimized out>, nr=-3) at ./bitops.h:122
> #1  vnc_dpy_update (ds=<optimized out>, x=-48, y=145, w=57, h=161) at
> ui/vnc.c:452
> #2  0x00007f1ce057e2ec in dpy_update (s=0x7f1ce1c8c880, h=16, w=66, y=145,
> x=-57) at ./console.h:242
> #3  vmsvga_update_rect (h=16, w=66, y=145, x=-57, s=0x7f1ce1cb3dd0) at
> hw/vmware_vga.c:324
> #4  vmsvga_update_rect_flush (s=0x7f1ce1cb3dd0) at hw/vmware_vga.c:357
> #5  vmsvga_update_display (opaque=0x7f1ce1cb3dd0) at hw/vmware_vga.c:960
> #6  0x00007f1ce05f0b37 in vnc_refresh (opaque=0x7f1cd8526010) at
> ui/vnc.c:2590
> #7  0x00007f1ce05c002b in qemu_run_timers (clock=0x7f1ce1c4f910) at
> qemu-timer.c:392
> #8  qemu_run_timers (clock=0x7f1ce1c4f910) at qemu-timer.c:373
> #9  0x00007f1ce05c028d in qemu_run_all_timers () at qemu-timer.c:449
> #10 0x00007f1ce058f2ee in main_loop_wait (nonblocking=<optimized out>) at
> main-loop.c:502
> #11 0x00007f1ce047acb3 in main_loop () at vl.c:1655
> #12 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
> at vl.c:3826
>
> Signed-off-by: Gerhard Wiesinger <lists@wiesinger.com>
> ---
>  ui/vnc.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 7c120e6..ae6d819 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -453,6 +453,11 @@ static void vnc_dpy_update(DisplayState *ds, int x, int
> y, int w, int h)
>      w = MIN(x + w, width) - x;
>      h = MIN(h, height);
>
> +    x = MAX(x, 0);
> +    y = MAX(y, 0);
> +    w = MAX(w, 0);
> +    h = MAX(h, 0);
> +
>      for (; y < h; y++)
>          for (i = 0; i < w; i += 16)
>              set_bit((x + i) / 16, s->dirty[y]);
> --
> 1.7.11.7

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?

-- PMM

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerd Hoffmann]

  Hi,

> 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 ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[BALATON Zoltan]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 552 bytes --]

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

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1738 bytes --]

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(+)

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);
-- 
1.7.10

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Michael Tokarev]

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....

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Michael Tokarev]

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

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerhard Wiesinger]

On 08.11.2012 22:07, Gerd Hoffmann wrote:
>    Hi,
>
>> 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 ...
>

Regarding fail safe programming I think it should be fixed/handled in 
both modules:
vmware_vga.c should not trigger wrong values but also other modules 
should verify or even correct there input parameters.
(think of situations where bits might not be accurate due to CPU bugs or 
even QEMU/KVM in aerospace where
bits fall to other states due to high energy cosmic ray).

Best solution is IHMO for vnc.c:
1.) Log the problem (that other modules can be fixed, too).
2.) Fix parameters (so that program doesn't crash)

In mission critical software application like aerospace, airplanes, 
cars, etc. (e.g. where people might get unhealthy) handling such 
situations where input parameters aren't as expected is a must.

See:
https://en.wikipedia.org/wiki/Fail-safe
https://en.wikipedia.org/wiki/Cosmic_ray#Effect_on_electronics
https://en.wikipedia.org/wiki/Radiation_hardening

Precondition:
https://en.wikipedia.org/wiki/Eiffel_%28programming_language%29#Design_by_Contract

Ciao,
Gerhard

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Peter Maydell]

On 9 November 2012 08:13, Gerhard Wiesinger <lists@wiesinger.com> wrote:
> (think of situations where bits might not be accurate due to CPU bugs or
> even QEMU/KVM in aerospace where
> bits fall to other states due to high energy cosmic ray).

If any aeroplane manufacturer is using QEMU for some safety critical
purpose it would be nice if they'd let us know. I could then avoid
flying with them in future :-)

-- PMM

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 November 2012 08:13, Gerhard Wiesinger <lists@wiesinger.com> wrote:
>> (think of situations where bits might not be accurate due to CPU bugs or
>> even QEMU/KVM in aerospace where
>> bits fall to other states due to high energy cosmic ray).
>
> If any aeroplane manufacturer is using QEMU for some safety critical
> purpose it would be nice if they'd let us know. I could then avoid
> flying with them in future :-)

While the abstract discussion is fun, it never hurts to be defensive.  I
agree the root cause is vmware-vga but checking in vnc doesn't hurt.

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Peter Maydell]

On 9 November 2012 10:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
> While the abstract discussion is fun, it never hurts to be defensive.  I
> agree the root cause is vmware-vga but checking in vnc doesn't hurt.

Defensive programming would suggest doing the clipping in the
console.c layer. That sounds a reasonable plan to me (especially
if we've hit similar problems multiple times in the past).

-- PMM

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerd Hoffmann]

On 11/09/12 10:50, Peter Maydell wrote:
> On 9 November 2012 10:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> While the abstract discussion is fun, it never hurts to be defensive.  I
>> agree the root cause is vmware-vga but checking in vnc doesn't hurt.
> 
> Defensive programming would suggest doing the clipping in the
> console.c layer. That sounds a reasonable plan to me (especially
> if we've hit similar problems multiple times in the past).

Fully agree, I'll cook up a patch as I'm touching that anyway.

Question is just whenever we'll go silently fixup stuff in console.c or
use assert()s to enforce callers getting this correct.  I'd tend to use
assert() as vmware-vga passing bogous stuff there IMHO indicates there
is a bug in vmware-vga.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Marek Vasut]

Dear Gerd Hoffmann,

> On 11/09/12 10:50, Peter Maydell wrote:
> > On 9 November 2012 10:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
> >> While the abstract discussion is fun, it never hurts to be defensive.  I
> >> agree the root cause is vmware-vga but checking in vnc doesn't hurt.
> > 
> > Defensive programming would suggest doing the clipping in the
> > console.c layer. That sounds a reasonable plan to me (especially
> > if we've hit similar problems multiple times in the past).
> 
> Fully agree, I'll cook up a patch as I'm touching that anyway.
> 
> Question is just whenever we'll go silently fixup stuff in console.c or
> use assert()s to enforce callers getting this correct.  I'd tend to use
> assert() as vmware-vga passing bogous stuff there IMHO indicates there
> is a bug in vmware-vga.

Or rather some revisions of the guest X driver. Though it's worth investigating 
it in the right place indeed.

> cheers,
>   Gerd

Best regards,
Marek Vasut

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Peter Maydell]

On 10 November 2012 00:45, Marek Vasut <marex@denx.de> wrote:
> Gerd Hoffmann wrote:
>> Question is just whenever we'll go silently fixup stuff in console.c or
>> use assert()s to enforce callers getting this correct.  I'd tend to use
>> assert() as vmware-vga passing bogous stuff there IMHO indicates there
>> is a bug in vmware-vga.
>
> Or rather some revisions of the guest X driver.

If qemu's vmware-vga is blithely trusting what the guest driver
hands it then that is itself a bug...

To answer Gerd's question, I think I'd go for clip rather than assert
(especially at this point in the release cycle), though I don't feel
very strongly about it.

-- PMM

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerhard Wiesinger]

On 10.11.2012 00:52, Peter Maydell wrote:
> On 10 November 2012 00:45, Marek Vasut <marex@denx.de> wrote:
>> Gerd Hoffmann wrote:
>>> Question is just whenever we'll go silently fixup stuff in console.c or
>>> use assert()s to enforce callers getting this correct.  I'd tend to use
>>> assert() as vmware-vga passing bogous stuff there IMHO indicates there
>>> is a bug in vmware-vga.
>> Or rather some revisions of the guest X driver.
> If qemu's vmware-vga is blithely trusting what the guest driver
> hands it then that is itself a bug...
>
> To answer Gerd's question, I think I'd go for clip rather than assert
> (especially at this point in the release cycle), though I don't feel
> very strongly about it.

I'd go for clipping rather than asserting too (no crash) in all layers 
as a defensive approach (console.c/vnc.c). Additionally logging that 
condition would be helpful that the arising bug (which occurred several 
times with a lot of unapplied fixes) can be detected by users easily and 
fixed accordingly.

Ciao,
Gerhard

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Marek Vasut]

Dear Gerhard Wiesinger,

> On 10.11.2012 00:52, Peter Maydell wrote:
> > On 10 November 2012 00:45, Marek Vasut <marex@denx.de> wrote:
> >> Gerd Hoffmann wrote:
> >>> Question is just whenever we'll go silently fixup stuff in console.c or
> >>> use assert()s to enforce callers getting this correct.  I'd tend to use
> >>> assert() as vmware-vga passing bogous stuff there IMHO indicates there
> >>> is a bug in vmware-vga.
> >> 
> >> Or rather some revisions of the guest X driver.
> > 
> > If qemu's vmware-vga is blithely trusting what the guest driver
> > hands it then that is itself a bug...
> > 
> > To answer Gerd's question, I think I'd go for clip rather than assert
> > (especially at this point in the release cycle), though I don't feel
> > very strongly about it.
> 
> I'd go for clipping rather than asserting too (no crash) in all layers
> as a defensive approach (console.c/vnc.c).

Won't that be an unnecessary slowdown?

> Additionally logging that
> condition would be helpful that the arising bug (which occurred several
> times with a lot of unapplied fixes) can be detected by users easily and
> fixed accordingly.


Sounds good.

> Ciao,
> Gerhard

Best regards,
Marek Vasut

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerd Hoffmann]

  Hi,

>> I'd go for clipping rather than asserting too (no crash) in all layers
>> as a defensive approach (console.c/vnc.c).
> 
> Won't that be an unnecessary slowdown?

Thats why I tend to prefer assert for additional sanity checks.  They
help finding bugs, but can optionally be compiled out.

But adding new asserts just before a release isn't the smartest move
indeed, maybe clip now and turn the checks into asserts once 1.3 is out
of the door.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Gerd Hoffmann]

On 11/10/12 00:45, Marek Vasut wrote:
> Dear Gerd Hoffmann,
> 
>> On 11/09/12 10:50, Peter Maydell wrote:
>>> On 9 November 2012 10:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>>> While the abstract discussion is fun, it never hurts to be defensive.  I
>>>> agree the root cause is vmware-vga but checking in vnc doesn't hurt.
>>>
>>> Defensive programming would suggest doing the clipping in the
>>> console.c layer. That sounds a reasonable plan to me (especially
>>> if we've hit similar problems multiple times in the past).
>>
>> Fully agree, I'll cook up a patch as I'm touching that anyway.
>>
>> Question is just whenever we'll go silently fixup stuff in console.c or
>> use assert()s to enforce callers getting this correct.  I'd tend to use
>> assert() as vmware-vga passing bogous stuff there IMHO indicates there
>> is a bug in vmware-vga.
> 
> Or rather some revisions of the guest X driver. Though it's worth investigating 
> it in the right place indeed.

That too, but we must add a check to qemu nevertheless.  We can't trust
the guest to not pass in bogous data, be it intentionally or by mistake.
 vmware-vga must sanity check the guest input no matter what, but
validating the guests input once should be enougth.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[BALATON Zoltan]

On Mon, 12 Nov 2012, Gerd Hoffmann wrote:
> On 11/10/12 00:45, Marek Vasut wrote:
>> Dear Gerd Hoffmann,
>>
>>> On 11/09/12 10:50, Peter Maydell wrote:
>>>> On 9 November 2012 10:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>>>> While the abstract discussion is fun, it never hurts to be defensive.  I
>>>>> agree the root cause is vmware-vga but checking in vnc doesn't hurt.
>>>>
>>>> Defensive programming would suggest doing the clipping in the
>>>> console.c layer. That sounds a reasonable plan to me (especially
>>>> if we've hit similar problems multiple times in the past).
>>>
>>> Fully agree, I'll cook up a patch as I'm touching that anyway.
>>>
>>> Question is just whenever we'll go silently fixup stuff in console.c or
>>> use assert()s to enforce callers getting this correct.  I'd tend to use
>>> assert() as vmware-vga passing bogous stuff there IMHO indicates there
>>> is a bug in vmware-vga.
>>
>> Or rather some revisions of the guest X driver. Though it's worth investigating
>> it in the right place indeed.
>
> That too, but we must add a check to qemu nevertheless.  We can't trust
> the guest to not pass in bogous data, be it intentionally or by mistake.
> vmware-vga must sanity check the guest input no matter what, but
> validating the guests input once should be enougth.

For vmware_vga you could take this:
http://patchwork.ozlabs.org/patch/197904/
or modify it/come up with a similar patch as needed.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] ui/vnc.c: Fix crash with VNC

[Blue Swirl]

On Fri, Nov 9, 2012 at 7:13 AM, Gerhard Wiesinger <lists@wiesinger.com> wrote:
> On 08.11.2012 22:07, Gerd Hoffmann wrote:
>>
>>    Hi,
>>
>>> 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 ...
>>
>
> Regarding fail safe programming I think it should be fixed/handled in both
> modules:
> vmware_vga.c should not trigger wrong values but also other modules should
> verify or even correct there input parameters.
> (think of situations where bits might not be accurate due to CPU bugs or
> even QEMU/KVM in aerospace where
> bits fall to other states due to high energy cosmic ray).
>
> Best solution is IHMO for vnc.c:
> 1.) Log the problem (that other modules can be fixed, too).
> 2.) Fix parameters (so that program doesn't crash)
>
> In mission critical software application like aerospace, airplanes, cars,
> etc. (e.g. where people might get unhealthy) handling such situations where
> input parameters aren't as expected is a must.

There are plenty of other considerations. For example, QEMU tests
don't have 100% code coverage, without even considering MC/DC. Adding
two overlapping checks could even result in <100% coverage.

Another issue is that the guest may cause QEMU to abort().

The code has not been formally reviewed, it's known to be in violation
to the few specifications we have (HACKING, CODING_STYLE, docs/*).

While QEMU is obviously a fine solution for many real world problems,
I don't recommend using QEMU for any safety critical tasks.

> See:
> https://en.wikipedia.org/wiki/Fail-safe
> https://en.wikipedia.org/wiki/Cosmic_ray#Effect_on_electronics
> https://en.wikipedia.org/wiki/Radiation_hardening
>
> Precondition:
> https://en.wikipedia.org/wiki/Eiffel_%28programming_language%29#Design_by_Contract
>
> Ciao,
> Gerhard
>
>