VNC segfault

Quoted 36 lines · 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

Quoted 8 lines

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:

vnc.c +5 -0

index 842124b..4b57838 100644
@@ -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

vnc.c renamed from vnc.c.orig +0 -0

@@ -265,6 +265,11 @@ static void vnc_dpy_update(DisplayState
     w += (x % 16);

Quoted 75 lines · Anthony Liguori

Quoted 36 lines · 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

Quoted 8 lines

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:

vnc.c +5 -0

index 842124b..4b57838 100644
@@ -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);

Quoted 3 lines

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.

Quoted 6 lines

vnc.c 2008-05-19 15:55:47.000000000 -0700 renamed from vnc.c.orig 2008-05-12 04:30:43.000000000 -0700 +0 -0

@@ -265,6 +265,11 @@ static void vnc_dpy_update(DisplayState
     w += (x % 16);
     x -= (x % 16);
 

vnc.c 2008-05-19 15:55:47.000000000 -0700 static void vnc_dpy_update(DisplayState · line 275

Quoted 8 lines

vnc.c 2008-05-19 15:55:47.000000000 -0700 renamed from vnc.c.orig 2008-05-12 04:30:43.000000000 -0700 +5 -0

+    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