Am 04.03.2011 10:02, schrieb Corentin Chary:
On Thu, Mar 3, 2011 at 9:37 PM, Stefan Weil <w...@mail.berlios.de> wrote:
Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
a severe bug (stack corruption).

bitmap_clear was called with a wrong argument
which caused out-of-bound writes to the local variable width_mask.

This bug was detected with QEMU running on windows.
It also occurs with wine:

*** stack smashing detected ***:  terminated
wine: Unhandled illegal instruction at address 0x6115c7 (thread 0009), starting debugger...

The bug is not windows specific!

Instead of fixing the wrong parameter value, bitmap_clear(), bitmap_set
and width_mask were removed, and bitmap_intersect() was replaced by
!bitmap_empty(). The new operation is much shorter and equivalent to
the old operations.

The declarations of the dirty bitmaps in vnc.h were also wrong for 64 bit
hosts because of a rounding effect: for these hosts, VNC_MAX_WIDTH is no
longer a multiple of (16 * BITS_PER_LONG), so the rounded value of
VNC_DIRTY_WORDS was too small.

Fix both declarations by using the macro which is designed for this
purpose.

Cc: Corentin Chary <corenti...@iksaif.net>
Cc: Wen Congyang <we...@cn.fujitsu.com>
Cc: Gerhard Wiesinger <li...@wiesinger.com>
Cc: Anthony Liguori <aligu...@us.ibm.com>
Signed-off-by: Stefan Weil <w...@mail.berlios.de>
---
 ui/vnc.c |    6 +-----
 ui/vnc.h |    9 ++++++---
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 610f884..34dc0cd 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2383,7 +2383,6 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
    uint8_t *guest_row;
    uint8_t *server_row;
    int cmp_bytes;
-    unsigned long width_mask[VNC_DIRTY_WORDS];
    VncState *vs;
    int has_dirty = 0;

@@ -2399,14 +2398,11 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
     * Check and copy modified bits from guest to server surface.
     * Update server dirty map.
     */
-    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
-    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
-                 VNC_DIRTY_WORDS * BITS_PER_LONG);
    cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
    guest_row  = vd->guest.ds->data;
    server_row = vd->server->data;
    for (y = 0; y < vd->guest.ds->height; y++) {
- if (bitmap_intersects(vd->guest.dirty[y], width_mask, VNC_DIRTY_WORDS)) {
+        if (!bitmap_empty(vd->guest.dirty[y], VNC_DIRTY_BITS)) {
            int x;
            uint8_t *guest_ptr;
            uint8_t *server_ptr;
diff --git a/ui/vnc.h b/ui/vnc.h
index 8a1e7b9..f10c5dc 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -79,9 +79,12 @@ typedef void VncSendHextileTile(VncState *vs,
                                void *last_fg,
                                int *has_bg, int *has_fg);

+/* VNC_MAX_WIDTH must be a multiple of 16. */
 #define VNC_MAX_WIDTH 2560
 #define VNC_MAX_HEIGHT 2048
-#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
+
+/* VNC_DIRTY_BITS is the number of bits in the dirty bitmap. */
+#define VNC_DIRTY_BITS (VNC_MAX_WIDTH / 16)

 #define VNC_STAT_RECT  64
 #define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
@@ -114,7 +117,7 @@ typedef struct VncRectStat VncRectStat;
 struct VncSurface
 {
    struct timeval last_freq_check;
-    unsigned long dirty[VNC_MAX_HEIGHT][VNC_DIRTY_WORDS];
+    DECLARE_BITMAP(dirty[VNC_MAX_HEIGHT], VNC_MAX_WIDTH / 16);
    VncRectStat stats[VNC_STAT_ROWS][VNC_STAT_COLS];
    DisplaySurface *ds;
 };
@@ -234,7 +237,7 @@ struct VncState
    int csock;

    DisplayState *ds;
-    unsigned long dirty[VNC_MAX_HEIGHT][VNC_DIRTY_WORDS];
+    DECLARE_BITMAP(dirty[VNC_MAX_HEIGHT], VNC_DIRTY_BITS);
    uint8_t **lossy_rect; /* Not an Array to avoid costly memcpy in
                           * vnc-jobs-async.c */

--
1.7.2.3



Hi,
Thanks, this patch is a lot cleaner that my early port to new
bitmap/bitops operations.
This patch fix all previous bugs, but not the
framebuffer_update_request + !incremental bug right ?

Hi,

I think so, but I'm not sure about the framebuffer_update_request bug.

At least my patch fixes the most urgent failures (stack corruption),
so I hope it will be applied to git master asap.

There remain more vnc related bugs. Just a short summary of those
which I am aware of:

* Screen update in tight mode has problems (wrong colors, parts missing).

* The threaded vnc server obviously has reentrancy problems (corruption
  of malloc'ed memory. I also noticed memory leaks but maybe these were
  fixed by a patch whcih I noticed on qemu-devel recently.

* The vnc server should not abort connections when it gets unknown commands.

* In reverse mode, the gui is no longer accessible if the vnc listener terminates.

Regards,
Stefan


Reply via email to