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

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

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