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

[Stefan Weil <weil@mail.berlios.de>]

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