From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [kvm-devel] VNC segfault Date: Mon, 19 May 2008 18:42:19 -0500 Message-ID: <4832105B.3020707@codemonkey.ws> References: <20080516213958.GA3256@dmt> <482F543C.1070901@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Marcelo Tosatti To: Lynn Kerby Return-path: Received: from py-out-1112.google.com ([64.233.166.178]:9132 "EHLO py-out-1112.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756631AbYESXm2 (ORCPT ); Mon, 19 May 2008 19:42:28 -0400 Received: by py-out-1112.google.com with SMTP id u52so2147893pyb.10 for ; Mon, 19 May 2008 16:42:26 -0700 (PDT) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: Lynn Kerby wrote: > > On May 17, 2008, at 2:55 PM, Anthony Liguori wrote: > >> Marcelo Tosatti wrote: >>> Hi Anthony, >>> >>> We're experiencing qemu segfaults when using VNC over high latency >>> links. >>> >>> (gdb) bt >>> #0 0x0000003a8ec838d3 in memcpy () from /lib64/libc.so.6 >>> #1 0x00000000004b9aff in vnc_update_client (opaque=0x3514140) at >>> vnc.c:223 >>> #2 0x000000000040822d in qemu_run_timers (ptimer_head=0x8e9500, >>> current_time=5942450) >>> at /root/marcelo/kvm-userspace/qemu/vl.c:1112 >>> #3 0x0000000000413208 in main_loop_wait (timeout=1000) >>> at /root/marcelo/kvm-userspace/qemu/vl.c:7482 >>> #4 0x000000000060de86 in kvm_main_loop () >>> at /root/marcelo/kvm-userspace/qemu/qemu-kvm.c:524 >>> #5 0x0000000000413259 in main_loop () at >>> /root/marcelo/kvm-userspace/qemu/vl.c:7506 >>> #6 0x0000000000415d3a in main (argc=21, argv=0x7fff00907dd8) >>> at /root/marcelo/kvm-userspace/qemu/vl.c:9369 >>> >>> Problem is that sometimes vs->width and vs->weight are not updated to >>> reflect the size allocated for the display memory. If they are larger >>> than whats allocated it segfaults: >>> >>> (gdb) p vs->old_data_h >>> $22 = 400 >>> (gdb) p vs->old_data_w >>> $23 = 720 >>> (gdb) p vs->old_data_depth >>> $24 = 4 >>> >>> (gdb) p vs->height >>> $20 = 480 >>> (gdb) p vs->width >>> $21 = 640 >>> (gdb) p vs->depth >>> $25 = 4 >>> >>> old_data_h, old_data_w and old_data_depth have been saved from the last >>> vnc_dpy_resize run. The code relies on the client's "set_encondings" >>> processing to happen _before_ the vnc_update_client() timer triggers, >>> which might not always be the case. >>> >>> I have no clue about correctness of the following though. What do you >>> say? >>> >> >> The patch isn't right because it will unconditionally send DesktopResize >> updates even with clients that don't support it. It happens to work for >> you because your client ignores it and there's no data associated with >> DesktopResize. A client really should just stop though. >> >> vs->{width,height} are supposed to correspond to the client's buffer. >> In the absence of DesktopResize, the client's buffer stays fixed. >> >> I think what's probably needed is some checks vnc_dpy_update() to not >> exceed the dimensions of vs->{width,height}. Something like the >> following: >> >> diff --git a/vnc.c b/vnc.c >> index 842124b..4b57838 100644 >> --- a/vnc.c >> +++ b/vnc.c >> @@ -265,6 +265,11 @@ static void vnc_dpy_update(DisplayState *ds, int x, >> int y, >> w += (x % 16); >> x -= (x % 16); >> >> + x = MIN(x, vs->width); >> + y = MIN(y, vs->height); >> + w = MIN(x + w, vs->width) - x; >> + h = MIN(y + h, vs->height) - y; >> + >> for (; y < h; y++) >> for (i = 0; i < w; i += 16) >> vnc_set_bit(vs->dirty_row[y], (x + i) / 16); >> >> Regards, >> >> Anthony Liguori > > This doesn't work right for me. > > Based on my observations the setting of w and h are causing the > display updates to not occur in the lower portion of the VNC screen. > I discovered this because my mouse pointer stopped before hitting the > bottom of the screen which made installation of RedHat EL 5 nearly > impossible to complete. The following patch just limits w and h to > the max width and it works *much* better but still seems like a hack > instead of a real fix. The incorrect x and y values could still be > used by this function to modify w and h. > >> --- qemu/vnc.c.orig 2008-05-12 04:30:43.000000000 -0700 >> +++ qemu/vnc.c 2008-05-19 15:55:47.000000000 -0700 >> @@ -265,6 +265,11 @@ static void vnc_dpy_update(DisplayState >> w += (x % 16); >> x -= (x % 16); >> >> + x = MIN(x, vs->width); >> + y = MIN(y, vs->height); >> + w = MIN(w, vs->width); >> + h = MIN(h, vs->height); >> + >> for (; y < h; y++) >> for (i = 0; i < w; i += 16) >> vnc_set_bit(vs->dirty_row[y], (x + i) / 16); Strange, your patch can still result in a region that is too large (if x == vs->width for instance). Can you add some debugging to see if your patch ever results in a smaller region than my patch? Regards, Anthony Liguori > Lynn Kerby >