| Message ID | alpine.LFD.2.02.1211012105080.690@bbs.intern |
|---|---|
| State | New |
| Headers | show |
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]);
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]); > >
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
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
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
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
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
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
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
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
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
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
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 > >
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
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
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
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
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]);
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(+)