[Qemu-devel] [PATCH] vnc: Fix heap corruption

[Qemu-devel] [PATCH] vnc: Fix heap corruption

[Stefan Weil]

Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
a severe bug (heap corruption).

bitmap_clear was called with a wrong argument
which caused out-of-bound writes to 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!

Cc: Corentin Chary <corentincj@iksaif.net>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 ui/vnc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index af55156..89f71da 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2401,7 +2401,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
      */
     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);
+                 (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
     cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
     guest_row  = vd->guest.ds->data;
     server_row = vd->server->data;
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] vnc: Fix heap corruption

[Wen Congyang]

At 03/01/2011 05:34 AM, Stefan Weil Write:
> Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
> a severe bug (heap corruption).
> 
> bitmap_clear was called with a wrong argument
> which caused out-of-bound writes to 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!
> 
> Cc: Corentin Chary <corentincj@iksaif.net>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  ui/vnc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index af55156..89f71da 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2401,7 +2401,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>       */
>      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);
> +                 (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);

The third argument of bitmap_clear() is number of bits to be cleared, but we pass
the end bits to be cleared to bitmap_clear().

I think we can fix this bug like this(I can not reproduce this bug, so I do not
know whether it can fix this bug):

diff --git a/ui/vnc.c b/ui/vnc.c
index fff34af..6d54661 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2404,7 +2404,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
      */
     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);
+                 VNC_DIRTY_WORDS * BITS_PER_LONG - (ds_get_width(vd->ds) / 16));
     cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
     guest_row  = vd->guest.ds->data;
     server_row = vd->server->data;

Thanks
Wen Congyang

>      cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
>      guest_row  = vd->guest.ds->data;
>      server_row = vd->server->data;

[Qemu-devel] [PATCH] vnc: Fix stack corruption and other bitmap related bugs

[Stefan Weil]

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 <corentincj@iksaif.net>
Cc: Wen Congyang <wency@cn.fujitsu.com>
Cc: Gerhard Wiesinger <lists@wiesinger.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Stefan Weil <weil@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

[Qemu-devel] Re: [PATCH] vnc: Fix stack corruption and other bitmap related bugs

[Stefan Weil]

Am 03.03.2011 21:37, schrieb Stefan Weil:
> 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<corentincj@iksaif.net>
> Cc: Wen Congyang<wency@cn.fujitsu.com>
> Cc: Gerhard Wiesinger<lists@wiesinger.com>
> Cc: Anthony Liguori<aliguori@us.ibm.com>
> Signed-off-by: Stefan Weil<weil@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);
>    

Here I forgot a small possible improvement: use VNC_DIRTY_BITS.
This is purely cosmetics, so I don't send a new patch.


>       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 */
>
>    

[Qemu-devel] Re: [PATCH] vnc: Fix stack corruption and other bitmap related bugs

[Corentin Chary]

On Thu, Mar 3, 2011 at 9:37 PM, Stefan Weil <weil@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 <corentincj@iksaif.net>
> Cc: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Gerhard Wiesinger <lists@wiesinger.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Stefan Weil <weil@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 ?

-- 
Corentin Chary
http://xf.iksaif.net

[Qemu-devel] Re: [PATCH] vnc: Fix stack corruption and other bitmap related bugs

[Stefan Weil]

Am 04.03.2011 10:02, schrieb Corentin Chary:
> On Thu, Mar 3, 2011 at 9:37 PM, Stefan Weil <weil@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 <corentincj@iksaif.net>
>> Cc: Wen Congyang <wency@cn.fujitsu.com>
>> Cc: Gerhard Wiesinger <lists@wiesinger.com>
>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Stefan Weil <weil@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

[Qemu-devel] Re: [PATCH] vnc: Fix stack corruption and other bitmap related bugs

[Gerhard Wiesinger]

On Fri, 4 Mar 2011, Stefan Weil wrote:

> Am 04.03.2011 10:02, schrieb Corentin Chary:
>> On Thu, Mar 3, 2011 at 9:37 PM, Stefan Weil <weil@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 <corentincj@iksaif.net>
>>> Cc: Wen Congyang <wency@cn.fujitsu.com>
>>> Cc: Gerhard Wiesinger <lists@wiesinger.com>
>>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>>> Signed-off-by: Stefan Weil <weil@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.

Can confirm screen update problems (wrong colors, e.g. red blocks).

Ciao,
Gerhard

--
http://www.wiesinger.com/

[Qemu-devel] Re: [PATCH] vnc: Fix stack corruption and other bitmap related bugs

[Corentin Chary]

> Can confirm screen update problems (wrong colors, e.g. red blocks).

With wich client, encoding, pixel format ? Does using another client
fix the issue ? Could you provide more informations (wireshark dumps,
screenshots, etc...) ?
Thanks,

-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] [PATCH] vnc: Fix stack corruption and other bitmap related bugs

[Anthony Liguori]

On 03/03/2011 02:37 PM, Stefan Weil 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<corentincj@iksaif.net>
> Cc: Wen Congyang<wency@cn.fujitsu.com>
> Cc: Gerhard Wiesinger<lists@wiesinger.com>
> Cc: Anthony Liguori<aliguori@us.ibm.com>
> Signed-off-by: Stefan Weil<weil@mail.berlios.de>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   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 */
>