Re: [kvm-devel] VNC segfault

[Anthony Liguori <anthony@codemonkey.ws>]

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
>