[RfC] Fix vnc memory corruption with width = 1400

Submitted by Gerd Hoffmann on June 14, 2010, 3:35 p.m.

Details

Message ID 1276529747-22891-1-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann June 14, 2010, 3:35 p.m.
vnc assumes that the screen width is a multiple of 16 in several places.
If this is not the case vnc will overrun buffers, corrupt memory, make
qemu crash.

This is the minimum fix for this bug. It makes sure we don't overrun the
scanline, thereby fixing the segfault.  The rendering is *not* correct
though, there is a black border at the right side of the screen, 8
pixels wide because 1400 % 16 == 8.


Not sure what the best way to fix that for real is.  qemu allways sends
updates which are 16 (or a multiple of 16) pixels wide.  I'm not sure
whenever this is something specified in the protocol or just in the qemu
implementation.

jpeg encodes 16x16 blocks too.  How does vnc+jpeg-encoding handle odd
screen sizes?


Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vnc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Markus Armbruster June 15, 2010, 8:13 a.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

> vnc assumes that the screen width is a multiple of 16 in several places.
> If this is not the case vnc will overrun buffers, corrupt memory, make
> qemu crash.
>
> This is the minimum fix for this bug. It makes sure we don't overrun the
> scanline, thereby fixing the segfault.  The rendering is *not* correct
> though, there is a black border at the right side of the screen, 8
> pixels wide because 1400 % 16 == 8.
>
>
> Not sure what the best way to fix that for real is.  qemu allways sends
> updates which are 16 (or a multiple of 16) pixels wide.  I'm not sure
> whenever this is something specified in the protocol or just in the qemu
> implementation.
>
> jpeg encodes 16x16 blocks too.  How does vnc+jpeg-encoding handle odd
> screen sizes?
>
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  vnc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/vnc.c b/vnc.c
> index 142e769..a4187a1 100644
> --- a/vnc.c
> +++ b/vnc.c
> @@ -2205,7 +2205,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>              guest_ptr  = guest_row;
>              server_ptr = server_row;
>  
> -            for (x = 0; x < vd->guest.ds->width;
> +            for (x = 0; x + 15 < vd->guest.ds->width;
>                      x += 16, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) {
>                  if (!vnc_get_bit(vd->guest.dirty[y], (x / 16)))
>                      continue;

                   vnc_clear_bit(vd->guest.dirty[y], (x / 16));
read overrun  ---> if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0)
                       continue;
write overrun ---> memcpy(server_ptr, guest_ptr, cmp_bytes);
                   QTAILQ_FOREACH(vs, &vd->clients, next) {
                       vnc_set_bit(vs->dirty[y], (x / 16));
                   }
                   has_dirty++;
               }
           }

Patch hide | download patch | download mbox

diff --git a/vnc.c b/vnc.c
index 142e769..a4187a1 100644
--- a/vnc.c
+++ b/vnc.c
@@ -2205,7 +2205,7 @@  static int vnc_refresh_server_surface(VncDisplay *vd)
             guest_ptr  = guest_row;
             server_ptr = server_row;
 
-            for (x = 0; x < vd->guest.ds->width;
+            for (x = 0; x + 15 < vd->guest.ds->width;
                     x += 16, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) {
                 if (!vnc_get_bit(vd->guest.dirty[y], (x / 16)))
                     continue;