Thanks, applied.
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
>
>
