From patchwork Tue May 4 22:12:17 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Lutomirski X-Patchwork-Id: 51701 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id BFD48B7D4C for ; Thu, 6 May 2010 01:04:11 +1000 (EST) Received: from localhost ([127.0.0.1]:57407 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O9g8t-0007iG-Ri for incoming@patchwork.ozlabs.org; Wed, 05 May 2010 11:04:07 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O9QM6-0007Pg-Hf for qemu-devel@nongnu.org; Tue, 04 May 2010 18:12:42 -0400 Received: from [140.186.70.92] (port=44818 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O9QM4-0007NP-VI for qemu-devel@nongnu.org; Tue, 04 May 2010 18:12:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O9QM3-0002wu-G8 for qemu-devel@nongnu.org; Tue, 04 May 2010 18:12:40 -0400 Received: from mail-pz0-f204.google.com ([209.85.222.204]:45044) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O9QM3-0002wY-Aq for qemu-devel@nongnu.org; Tue, 04 May 2010 18:12:39 -0400 Received: by pzk42 with SMTP id 42so1081133pzk.4 for ; Tue, 04 May 2010 15:12:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:mime-version:sender:received:from:date :x-google-sender-auth:message-id:subject:to:content-type; bh=b+yQPiJUQTodWn8+d4ZDK6E/tlTCPbWJOPi9KFgDn+U=; b=mMW0vtxjtYYGvjbiYJqZQEpnmlC0/e59u5Qiu3c2FF6vw4TUcYt9xEceUVxyLY/SBn 238z/dOfm9CDdxcvQ2KYdACn2ry/Gn0wfeBv+L6edUYG9mR4aOFFqXpWKBOnKf9cRu7R 5s3/NKzA9p3cMDwIzi5Fce1ZLiBoLNtchvaQs= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:from:date:x-google-sender-auth:message-id :subject:to:content-type; b=Qlp/oPS5E2PvHJnOWl2UbDLrChCGV3oVKxJ2qO0K/lMhiJtFu+vgts717BAaPcGRPB E7ZdgnbN90KeZS/040KAZXHJvym8KrPU6Vqc8wkLbteWtd6Ktg+pav4ip312bHjfBUcS M4ZSJF4RydGf0Z7ttdZelzdf/lxNvQi4X5AC8= Received: by 10.141.125.10 with SMTP id c10mr5025019rvn.214.1273011157063; Tue, 04 May 2010 15:12:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.140.203.8 with HTTP; Tue, 4 May 2010 15:12:17 -0700 (PDT) From: Andrew Lutomirski Date: Tue, 4 May 2010 18:12:17 -0400 X-Google-Sender-Auth: 71f7c33d240c0a09 Message-ID: To: qemu-devel@nongnu.org X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) Subject: [Qemu-devel] VNC heap corruption when display width is not a multiple of 16 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Hi all- qemu-kvm quite reliably crashes when running with a VNC viewer connected at 1400x1050. (The crash happens when changing resolution *from* 1400x1050 or disconnecting and reconnecting a client.) The problem is that vnc_refresh_server_surface overruns server->data and corrupts heap metadata, causing glibc to explode when the buffer is freed. The patch below (obviously only for testing) demonstrates the problem and prevents the crash, but it introduces a black line 8 pixels wide on the right when running at 1400x1050. I'm not sure what's going on -- either I messed something up (entirely possible) or there's another bug somewhere else. Note: I've only reproduced this on Avi's qemu-kvm and on Fedora's packages -- upstream qemu crashes somewhere else. The code in question looks the same upstream, though. --Andy } @@ -2320,8 +2329,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd) * Check and copy modified bits from guest to server surface. * Update server dirty map. */ - vnc_set_bits(width_mask, (ds_get_width(vd->ds) / 16), VNC_DIRTY_WORDS); - cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds); + vnc_set_bits(width_mask, ((ds_get_width(vd->ds) + 15) / 16), VNC_DIRTY_WORDS); guest_row = vd->guest.ds->data; server_row = vd->server->data; for (y = 0; y < vd->guest.ds->height; y++) { @@ -2332,12 +2340,20 @@ static int vnc_refresh_server_surface(VncDisplay *vd) guest_ptr = guest_row; server_ptr = server_row; + int bpp = ds_get_bytes_per_pixel(vd->ds); + cmp_bytes = 16 * bpp; for (x = 0; x < vd->guest.ds->width; x += 16, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) { if (!vnc_get_bit(vd->guest.dirty[y], (x / 16))) continue; vnc_clear_bit(vd->guest.dirty[y], (x / 16)); + + // If this happens, we'll overrun the line. If it + // happens on the last row, we'll corrupt the heap. + if (cmp_bytes > bpp * (vd->guest.ds->width - x)) + cmp_bytes = bpp * (vd->guest.ds->width - x); + if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0) continue; memcpy(server_ptr, guest_ptr, cmp_bytes); diff --git a/vnc.c b/vnc.c index e678fcc..f9d0ad3 100644 --- a/vnc.c +++ b/vnc.c @@ -527,6 +527,15 @@ static void vnc_dpy_resize(DisplayState *ds) vd->server->data = qemu_mallocz(vd->server->linesize * vd->server->height); + printf("vnc_dpy_resize: linesize = %d, height = %d, bpp = %d, " + "width = %d, height = %d, width%%16 = %d, data = %p\n", + vd->server->linesize, vd->server->height, + ds_get_bytes_per_pixel(ds), + ds_get_width(ds), ds_get_height(ds), + ds_get_width(ds) % 16, + vd->server->data); + + /* guest surface */ if (!vd->guest.ds) vd->guest.ds = qemu_mallocz(sizeof(*vd->guest.ds)); @@ -1740,7 +1749,7 @@ static void framebuffer_update_request(VncState *vs, int incremental, vs->force_update = 1; for (i = 0; i < h; i++) { vnc_set_bits(vs->dirty[y_position + i], - (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS); + ((ds_get_width(vs->ds) + 15) / 16), VNC_DIRTY_WORDS); } }