VNC segfault

VNC segfault

[Marcelo Tosatti]

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?

diff --git a/qemu/vnc.c b/qemu/vnc.c
index f6ec5cf..5540677 100644
--- a/qemu/vnc.c
+++ b/qemu/vnc.c
@@ -302,7 +302,7 @@ static void vnc_dpy_resize(DisplayState *ds, int w, int h)
     ds->width = w;
     ds->height = h;
     ds->linesize = w * vs->depth;
-    if (vs->csock != -1 && vs->has_resize && size_changed) {
+    if (vs->csock != -1 && size_changed) {
 	vnc_write_u8(vs, 0);  /* msg id */
 	vnc_write_u8(vs, 0);
 	vnc_write_u16(vs, 1); /* number of rects */




-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft 
Defy all challenges. Microsoft(R) Visual Studio 2008. 
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [kvm-devel] VNC segfault

[Lynn Kerby]

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

Lynn Kerby

Re: [kvm-devel] VNC segfault

[Anthony Liguori]

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
>