From patchwork Fri Mar 4 11:46:39 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Tokarev X-Patchwork-Id: 85417 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 39F06B708B for ; Fri, 4 Mar 2011 22:48:16 +1100 (EST) Received: from localhost ([127.0.0.1]:49371 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PvTUT-000068-Bu for incoming@patchwork.ozlabs.org; Fri, 04 Mar 2011 06:48:13 -0500 Received: from [140.186.70.92] (port=47942 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PvTT1-000827-Tj for qemu-devel@nongnu.org; Fri, 04 Mar 2011 06:46:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PvTT0-0004Lb-TF for qemu-devel@nongnu.org; Fri, 04 Mar 2011 06:46:43 -0500 Received: from isrv.corpit.ru ([86.62.121.231]:38510) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PvTT0-0004HP-Fi for qemu-devel@nongnu.org; Fri, 04 Mar 2011 06:46:42 -0500 Received: from [192.168.88.2] (mjt.vpn.tls.msk.ru [192.168.177.99]) by isrv.corpit.ru (Postfix) with ESMTP id 65601A0996; Fri, 4 Mar 2011 14:46:39 +0300 (MSK) Message-ID: <4D70D11F.6070303@msgid.tls.msk.ru> Date: Fri, 04 Mar 2011 14:46:39 +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: Corentin Chary Subject: Re: [Qemu-devel] [PATCH] vnc: tight: Fix crash after 2GB of output References: <1299200265-32685-1-git-send-email-roland@kernel.org> <4D709619.5020103@msgid.tls.msk.ru> In-Reply-To: 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 , Roland Dreier , 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 04.03.2011 11:56, Corentin Chary wrote: >>> >>> bytes = zstream->total_out - previous_out; > > Good catch > >> 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. > > If you're certain that total_out is not used by zlib, could you also > send a patch for zlib encoding please ? (vnc-enc-zlib.c) > Thanks, Yes, I noticed this too (the same code is in enc-zlib), and mentioned this in my previous email. The attached slightly different patch fixes both places and fixes them for good (hopefully anyway). Runtime-tested for the tight case, but honestly, I didn't wait for 2G of output ;) Thanks! /mjt fix 2Gb integer overflow in in VNC tight and zlib encodings As found by Roland Dreier (excellent catch!), when amount of VNC compressed data produced by zlib and sent to client exceeds 2Gb, integer overflow occurs because currently, we calculate amount of data produced at each step by comparing saved total_out with new total_out, and total_out is something which grows without bounds. Compare it with previous avail_out instead of total_out, and leave total_out alone. The same code is used in vnc-enc-tight.c and vnc-enc-zlib.c, so fix both cases. There, there's no actual need to save previous_out value, since capacity-offset (which is how that value is calculated) stays the same so it can be recalculated again after call to deflate(), but whole thing becomes less readable this way. Reported-by: Roland Dreier Signed-off-by: Michael Tokarev diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c index 2522936..87fdf35 100644 --- a/ui/vnc-enc-tight.c +++ b/ui/vnc-enc-tight.c @@ -868,8 +868,8 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes, zstream->avail_in = vs->tight.tight.offset; zstream->next_out = vs->tight.zlib.buffer + vs->tight.zlib.offset; zstream->avail_out = vs->tight.zlib.capacity - vs->tight.zlib.offset; + previous_out = zstream->avail_out; zstream->data_type = Z_BINARY; - previous_out = zstream->total_out; /* start encoding */ if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) { @@ -878,7 +878,8 @@ 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; + /* ...how much data has actually been produced by deflate() */ + bytes = previous_out - zstream->avail_out; tight_send_compact_size(vs, bytes); vnc_write(vs, vs->tight.zlib.buffer, bytes); diff --git a/ui/vnc-enc-zlib.c b/ui/vnc-enc-zlib.c index 3c6e6ab..e32e4cd 100644 --- a/ui/vnc-enc-zlib.c +++ b/ui/vnc-enc-zlib.c @@ -103,8 +103,8 @@ static int vnc_zlib_stop(VncState *vs) zstream->avail_in = vs->zlib.zlib.offset; zstream->next_out = vs->output.buffer + vs->output.offset; zstream->avail_out = vs->output.capacity - vs->output.offset; + previous_out = zstream->avail_out; zstream->data_type = Z_BINARY; - previous_out = zstream->total_out; // start encoding if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) { @@ -113,7 +113,7 @@ static int vnc_zlib_stop(VncState *vs) } vs->output.offset = vs->output.capacity - zstream->avail_out; - return zstream->total_out - previous_out; + return previous_out - zstream->avail_out; } int vnc_zlib_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)