[1/4] vnc: tight: Fix crash after 2GB of output

From: Michael Tokarev <mjt@tls.msk.ru>

fix 2Gb integer overflow in in VNC tight and zlib encodings

As found by Roland Dreier <roland@purestorage.com> (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 <roland@purestorage.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
---
 ui/vnc-enc-tight.c |    5 +++--
 ui/vnc-enc-zlib.c  |    4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

On Mon, Mar 21, 2011 at 09:34:35AM +0100, Corentin Chary wrote:
> From: Michael Tokarev <mjt@tls.msk.ru>
> 
> fix 2Gb integer overflow in in VNC tight and zlib encodings
> 
> As found by Roland Dreier <roland@purestorage.com> (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 <roland@purestorage.com>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
> ---
>  ui/vnc-enc-tight.c |    5 +++--
>  ui/vnc-enc-zlib.c  |    4 ++--
>  2 files changed, 5 insertions(+), 4 deletions(-)

Thanks, applied.

> 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)
> -- 
> 1.7.3.4
> 
> 
>

Aurelien, could you also take a look at
http://patchwork.ozlabs.org/patch/87717/ ?
Thanks,

On Mon, Apr 11, 2011 at 11:27:23AM +0200, Corentin Chary wrote:
> Aurelien, could you also take a look at
> http://patchwork.ozlabs.org/patch/87717/ ?

I gave a look, but I don't really know this part of QEMU, so I would
prefer if someone else look at it.