[Qemu-devel] [Patch] VNC: Fix crash with non-resizing clients

[Qemu-devel] [Patch] VNC: Fix crash with non-resizing clients

[Eduardo Felipe]

[-- Attachment #1.1: Type: text/plain, Size: 510 bytes --]

Hi,

2007/9/25, GUERRAZ Francois <francois.guerraz@alyotech.fr>:
>
>
> About your VNC problems : I have had problems w/ vnc too (see
> http://qemu-forum.ipi.fi/viewtopic.php?p=10468) but had no answer as
> well...


This problem happens when the VNC client doesn't support the DesktopSize
pseudo-encoding. Qemu crashes when the guest resizes down its display and
the VNC client sends a SetPixelFormat afterwards.

Attached patch should fix this. It also forces a full buffer update after
resize.

Regards,
Edu

[-- Attachment #1.2: Type: text/html, Size: 862 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vnc-crash-on-resize.patch --]
[-- Type: text/x-patch; name="vnc-crash-on-resize.patch", Size: 1052 bytes --]

*** vnc.c	30 Sep 2007 13:01:15 -0000	1.25
--- vnc.c	3 Oct 2007 18:44:17 -0000
@@ -289,15 +289,19 @@
     ds->width = w;
     ds->height = h;
     ds->linesize = w * vs->depth;
-    if (vs->csock != -1 && vs->has_resize && size_changed) {
-	vnc_write_u8(vs, 0);  /* msg id */
-	vnc_write_u8(vs, 0);
-	vnc_write_u16(vs, 1); /* number of rects */
-	vnc_framebuffer_update(vs, 0, 0, ds->width, ds->height, -223);
-	vnc_flush(vs);
-	vs->width = ds->width;
-	vs->height = ds->height;
+    if (size_changed) {
+        if (vs->csock != -1 && vs->has_resize) {
+            vnc_write_u8(vs, 0);  /* msg id */
+            vnc_write_u8(vs, 0);
+            vnc_write_u16(vs, 1); /* number of rects */
+            vnc_framebuffer_update(vs, 0, 0, ds->width, ds->height, -223);
+            vnc_flush(vs);
+        }
+        memset(vs->dirty_row, 0xFF, sizeof(vs->dirty_row));
+        memset(vs->old_data, 42, ds->linesize * ds->height);
     }
+    vs->width = ds->width;
+    vs->height = ds->height;
 }
 
 /* fastest code */

Re: [Qemu-devel] [Patch] VNC: Fix crash with non-resizing clients

[Daniel P. Berrange]

On Wed, Oct 03, 2007 at 09:29:59PM +0200, Eduardo Felipe wrote:
> Hi,
> 
> 2007/9/25, GUERRAZ Francois <francois.guerraz@alyotech.fr>:
> >
> >
> > About your VNC problems : I have had problems w/ vnc too (see
> > http://qemu-forum.ipi.fi/viewtopic.php?p=10468) but had no answer as
> > well...
> 
> 
> This problem happens when the VNC client doesn't support the DesktopSize
> pseudo-encoding. Qemu crashes when the guest resizes down its display and
> the VNC client sends a SetPixelFormat afterwards.
> 
> Attached patch should fix this. It also forces a full buffer update after
> resize.

The memset calls in that patch are bogus & not correctly fixing the buffer
update problem. You're merely setting the 'old data' to have pixel value
42 - if the guest OS framebuffer happens to also have aras with pixel value
 of 42 too, the frame buffer will still not correctly update. The root
problem is overly-aggressive update minimization logic in vnc_update_client.
This is in turn flawed beause the dirty_row aray is trying to encode two
separate concepts - areas which are dirty, and areas which need to be 
sent to the client. The latter are a superset of the former, but the code
in vnc_update_client minimizes based on dirtiness, so updates will get
missed out. Setting the old data to 42 merely changes which areas will get
missed updates.

The QEMU code in Xen has added a update_row field, separate from the dirty_row
field. Thus after a resize it can update the entire framebuffer, regardless
of whether QEMU's copy of the framebuffer is dirty wrt to the guest copy.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [Qemu-devel] [Patch] VNC: Fix crash with non-resizing clients

[Eduardo Felipe]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

2007/10/3, Daniel P. Berrange <berrange@redhat.com>:
>
>
> The memset calls in that patch are bogus & not correctly fixing the buffer
> update problem. You're merely setting the 'old data' to have pixel value
> 42 - if the guest OS framebuffer happens to also have aras with pixel
> value
> of 42 too, the frame buffer will still not correctly update. The root
> problem is overly-aggressive update minimization logic in
> vnc_update_client.
> This is in turn flawed beause the dirty_row aray is trying to encode two
> separate concepts - areas which are dirty, and areas which need to be
> sent to the client. The latter are a superset of the former, but the code
> in vnc_update_client minimizes based on dirtiness, so updates will get
> missed out. Setting the old data to 42 merely changes which areas will get
> missed updates.


Agreed. I just used the same hack that is scattered around several other
places to make a patch quickly.


The QEMU code in Xen has added a update_row field, separate from the
> dirty_row
> field. Thus after a resize it can update the entire framebuffer,
> regardless
> of whether QEMU's copy of the framebuffer is dirty wrt to the guest copy.


I think that VNC server needs a deep rework, as several qemu related
projects have done, but meanwhile correcting bugs with small non-intrusive
patches may be the way to go.

Edu.

[-- Attachment #2: Type: text/html, Size: 1818 bytes --]