Dear Michael Tokarev, > 26.01.2013 19:09, Blue Swirl wrote: > > Thanks, applied. > > Now when I've comments from BALATON Zoltan, I asked Serge to try > out the current qemu in the environment where this buh was easy > to trigger (ubuntu guest with unity desktop) - and he says he > can't reproduce the issue anymore (this does not mean it does not > exist ofcourse). > > So maybe this patch isn't really needed? I dunno.
Or maybe the vmware driver was fixed on the host side. This doesn't mean there aren't buggy installations in the wild. > Thanks, > > /mjt > > > On Fri, Jan 25, 2013 at 5:23 PM, Michael Tokarev <m...@tls.msk.ru> wrote: > >> This is a follow up for several attempts to fix this issue. > >> > >> Previous incarnations: > >> > >> 1. http://thread.gmane.org/gmane.linux.ubuntu.bugs.general/3156089 > >> https://bugs.launchpad.net/bugs/918791 > >> "qemu-kvm dies when using vmvga driver and unity in the guest" bug. > >> > >> Fix by Serge Hallyn: > >> https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff > >> > >> This fix is incomplete, since it does not check width and height > >> for being negative. Serge weren't sure if that's the right place > >> to fix it, maybe the fix should be up the stack somewhere. > >> > >> 2. http://thread.gmane.org/gmane.comp.emulators.qemu/166064 > >> by Marek Vasut: "vmware_vga: Redraw only visible area" > >> > >> This one adds the (incomplete) check to vmsvga_update_rect_delayed(), > >> the routine just queues the rect updating but does no interesting > >> stuff. It is also incomplete in the same way as patch by Serge, > >> but also does not touch width&height at all after adjusting x&y, > >> which is wrong. > >> > >> As far as I can see, when processing guest requests, the device > >> places them into a queue (vmsvga_update_rect_delayed()) and > >> processes this queue in different place/time, namely, in > >> vmsvga_update_rect(). Sometimes, vmsvga_update_rect() is > >> called directly, without placing the request to the gueue. > >> This is the place this patch changes, which is the last > >> (deepest) in the stack. I'm not sure if this is the right > >> place still, since it is possible we have some queue optimization > >> (or may have in the future) which will be upset by negative/wrong > >> values here, so maybe we should check for validity of input > >> right when receiving request from the guest (and maybe even > >> use unsigned types there). But I don't know the protocol > >> and implementation enough to have a definitive answer. > >> > >> But since vmsvga_update_rect() has other sanity checks already, > >> I'm adding the missing ones there as well. > >> > >> Cc'ing BALATON Zoltan and Andrzej Zaborowski who shows in `git blame' > >> output and may know something in this area. > >> > >> If this patch is accepted, it should be applied to all active > >> stable branches (at least since 1.1, maybe even before), with > >> minor context change (ds_get_*(s->vga.ds) => s->*). I'm not > >> Cc'ing -stable yet, will do it explicitly once the patch is > >> accepted. > >> > >> BTW, these checks use fprintf(stderr) -- it should be converted > >> to something more appropriate, since stderr will most likely > >> disappear somewhere. > >> > >> Cc: Marek Vasut <ma...@denx.de> > >> CC: Serge Hallyn <serge.hal...@ubuntu.com> > >> Cc: BALATON Zoltan <bala...@eik.bme.hu> > >> Cc: Andrzej Zaborowski <balr...@gmail.com> > >> Signed-off-by: Michael Tokarev <m...@tls.msk.ru> > >> Reviewed-by: Marek Vasut <ma...@denx.de> > >> Signed-off-by: Serge Hallyn <serge.hal...@ubuntu.com> > >> --- > >> > >> hw/vmware_vga.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c > >> index 62771bb..cd15ee4 100644 > >> --- a/hw/vmware_vga.c > >> +++ b/hw/vmware_vga.c > >> @@ -296,6 +296,15 @@ static inline void vmsvga_update_rect(struct > >> vmsvga_state_s *s, > >> > >> uint8_t *src; > >> uint8_t *dst; > >> > >> + if (x < 0) { > >> + fprintf(stderr, "%s: update x was < 0 (%d)\n", __func__, x); > >> + w += x; > >> + x = 0; > >> + } > >> + if (w < 0) { > >> + fprintf(stderr, "%s: update w was < 0 (%d)\n", __func__, w); > >> + 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 +312,15 @@ static inline void vmsvga_update_rect(struct > >> vmsvga_state_s *s, > >> > >> w = ds_get_width(s->vga.ds) - x; > >> > >> } > >> > >> + if (y < 0) { > >> + fprintf(stderr, "%s: update y was < 0 (%d)\n", __func__, y); > >> + h += y; > >> + y = 0; > >> + } > >> + if (h < 0) { > >> + fprintf(stderr, "%s: update h was < 0 (%d)\n", __func__, h); > >> + 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.4 Best regards, Marek Vasut
