From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40643 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PvYYb-0006B3-K0 for qemu-devel@nongnu.org; Fri, 04 Mar 2011 12:12:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PvYYZ-00038T-Dn for qemu-devel@nongnu.org; Fri, 04 Mar 2011 12:12:49 -0500 Received: from moutng.kundenserver.de ([212.227.17.9]:62457) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PvYYZ-00037o-4m for qemu-devel@nongnu.org; Fri, 04 Mar 2011 12:12:47 -0500 Message-ID: <4D711D80.3080006@mail.berlios.de> Date: Fri, 04 Mar 2011 18:12:32 +0100 From: Stefan Weil MIME-Version: 1.0 References: <1298928892-24039-1-git-send-email-weil@mail.berlios.de> <1299184675-11631-1-git-send-email-weil@mail.berlios.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] vnc: Fix stack corruption and other bitmap related bugs List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corentin Chary Cc: Gerhard Wiesinger , Anthony Liguori , qemu-devel@nongnu.org Am 04.03.2011 10:02, schrieb Corentin Chary: > On Thu, Mar 3, 2011 at 9: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 >> Cc: Wen Congyang >> Cc: Gerhard Wiesinger >> Cc: Anthony Liguori >> Signed-off-by: Stefan Weil >> --- >> 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