From patchwork Fri Mar 4 07:36:08 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Tokarev X-Patchwork-Id: 85371 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 43F12B70BA for ; Fri, 4 Mar 2011 18:38:52 +1100 (EST) Received: from localhost ([127.0.0.1]:59144 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PvPb7-0000kP-LL for incoming@patchwork.ozlabs.org; Fri, 04 Mar 2011 02:38:49 -0500 Received: from [140.186.70.92] (port=45248 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PvPYc-00085W-N8 for qemu-devel@nongnu.org; Fri, 04 Mar 2011 02:36:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PvPYa-00016A-DU for qemu-devel@nongnu.org; Fri, 04 Mar 2011 02:36:14 -0500 Received: from isrv.corpit.ru ([86.62.121.231]:48006) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PvPYZ-00015f-VA for qemu-devel@nongnu.org; Fri, 04 Mar 2011 02:36:12 -0500 Received: from [192.168.88.2] (mjt.vpn.tls.msk.ru [192.168.177.99]) by isrv.corpit.ru (Postfix) with ESMTP id ED403A0917; Fri, 4 Mar 2011 10:36:10 +0300 (MSK) Message-ID: <4D709668.7010105@msgid.tls.msk.ru> Date: Fri, 04 Mar 2011 10:36:08 +0300 From: Michael Tokarev Organization: Telecom Service, JSC User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1.16) Gecko/20101227 Icedove/3.0.11 MIME-Version: 1.0 To: Roland Dreier Subject: Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output References: <1299200265-32685-1-git-send-email-roland@kernel.org> In-Reply-To: <1299200265-32685-1-git-send-email-roland@kernel.org> X-Enigmail-Version: 1.0.1 OpenPGP: id=804465C5 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 86.62.121.231 Cc: Roland Dreier , Corentin Chary , qemu-devel@nongnu.org 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 [And sure thing I forgot to Cc Corentin Chary. ENOCOFFEE. Please excuse me for the double post] 04.03.2011 03:57, Roland Dreier wrote: > From: Roland Dreier > > If one leaves a VNC session with tight compression running for long > enough, Qemu crashes. This is because of the computation > > bytes = zstream->total_out - previous_out; > > in tight_compress_data, where zstream->total_out is a uLong but > previous_out is an int. As soon as zstream->total_out gets past > INT_MAX (ie 2GB), previous_out becomes negative and therefore the > result of the subtraction, bytes, becomes a huge positive number that > causes havoc for obvious reasons when passed as a length to > vnc_write(). Excellent work, Roland! No doubt I wasn't able to hit this bug locally - I tried many things, but I never waited long enough to hit this 2Gb border ;) > The fix for this is simple: keep previous_out as a uLong too, which > avoids any problems with sign conversion or truncation. This looks wrong to me. On 32bit x86 uLong is 32bits. Yes it's unsigned there, but it's still 32bits. And sure thing it _will_ overflow again, not at 2Gb but now at 4Gb, and the next total_out will start from zero again, so that now bytes will be a large integer again because "next" (new total_out) will be less than "current" (previous_out). If the whole total_out is actually needed, that counter should be kept separate and added to when necessary. But this total_out should be reset to avoid letting it large values in the first place. total_out isn't used by zlib internally, so if the resulting "total" counter is not needed in qemu, we can just zero-out the total_out in this function before calling zlib, and use the resulting value directly as "bytes", without saving its previous value in previous_out. Something like the attached patch does. By the way, the same thing is used in ui/vnc-enc-zlib.c too, and it looks like that place has the same issue as well. Cc'ng to Corentin Chary since he made lots of work in that area recently. Thanks! > Signed-off-by: Roland Dreier > --- > ui/vnc-enc-tight.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c > index af45edd..59ec0e3 100644 > --- a/ui/vnc-enc-tight.c > +++ b/ui/vnc-enc-tight.c > @@ -829,7 +829,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes, > int level, int strategy) > { > z_streamp zstream = &vs->tight.stream[stream_id]; > - int previous_out; > + uLong previous_out; > > if (bytes < VNC_TIGHT_MIN_TO_COMPRESS) { > vnc_write(vs, vs->tight.tight.buffer, vs->tight.tight.offset); > fix overflow after 2Gb of output in ui/vnc-enc-tight.c When amount of compressed data is more than 2Gb we will hit integer overflow (or when it's unsigned, the border is 4Gb). We don't use zstream->total_out for anything, so instead of remembering its previois value and compare with hext one, we can just reset it on entry and use its resulting value as the amount of bytes to deal with. diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c index 2522936..e1843cb 100644 --- a/ui/vnc-enc-tight.c +++ b/ui/vnc-enc-tight.c @@ -849,7 +849,6 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes, int level, int strategy) { z_streamp zstream = &vs->tight.stream[stream_id]; - int previous_out; if (bytes < VNC_TIGHT_MIN_TO_COMPRESS) { vnc_write(vs, vs->tight.tight.buffer, vs->tight.tight.offset); @@ -869,7 +868,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes, zstream->next_out = vs->tight.zlib.buffer + vs->tight.zlib.offset; zstream->avail_out = vs->tight.zlib.capacity - vs->tight.zlib.offset; zstream->data_type = Z_BINARY; - previous_out = zstream->total_out; + zstream->total_out = 0; /* start encoding */ if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) { @@ -878,7 +877,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes, } vs->tight.zlib.offset = vs->tight.zlib.capacity - zstream->avail_out; - bytes = zstream->total_out - previous_out; + bytes = zstream->total_out; tight_send_compact_size(vs, bytes); vnc_write(vs, vs->tight.zlib.buffer, bytes);