From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40592 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pvr8c-0000DZ-9t for qemu-devel@nongnu.org; Sat, 05 Mar 2011 08:03:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pvr8X-0004l6-1v for qemu-devel@nongnu.org; Sat, 05 Mar 2011 08:03:14 -0500 Received: from chello084112167138.7.11.vie.surfer.at ([84.112.167.138]:53398 helo=wiesinger.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pvr8W-0004kx-Mk for qemu-devel@nongnu.org; Sat, 05 Mar 2011 08:03:09 -0500 Date: Sat, 5 Mar 2011 14:02:22 +0100 (CET) From: Gerhard Wiesinger In-Reply-To: <4D711D80.3080006@mail.berlios.de> Message-ID: References: <1298928892-24039-1-git-send-email-weil@mail.berlios.de> <1299184675-11631-1-git-send-email-weil@mail.berlios.de> <4D711D80.3080006@mail.berlios.de> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed 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: Stefan Weil Cc: Anthony Liguori , qemu-devel@nongnu.org, Corentin Chary 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 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. Can confirm screen update problems (wrong colors, e.g. red blocks). Ciao, Gerhard -- http://www.wiesinger.com/