[Qemu-devel] [PATCH] vnc: segmentation fault caused by incorrect 'bytes' count calculated in tight_compress_data()

[Qemu-devel] [PATCH] vnc: segmentation fault caused by incorrect 'bytes' count calculated in tight_compress_data()

[Ulrich Obergfell]

tight_compress_data() calculates an incorrect 'bytes' count
if 'zstream->total_out' is greater than 0x7fffffff, because
the type of the variable 'previous_out' is 'int'.

    852     int previous_out;
     :
    872     previous_out = zstream->total_out;
     :
    881     bytes = zstream->total_out - previous_out;
    882 
    883     tight_send_compact_size(vs, bytes);
    884     vnc_write(vs, vs->tight.zlib.buffer, bytes);


The incorrect 'bytes' count causes segmentation faults in
functions called by tight_compress_data(). For example:

(gdb) bt
#0  0x000000396ab3d2b6 in __memcpy_ssse3_back () from /lib64/libc.so.6
#1  0x00000000004b3b91 in buffer_append (buffer=0x322d660, 
    data=<value optimized out>, len=<value optimized out>)
    at /usr/include/bits/string3.h:52
#2  0x00000000004bbf02 in tight_compress_data (vs=0x32215b0, 
    stream_id=<value optimized out>, bytes=0x100024881, 
    level=<value optimized out>, strategy=<value optimized out>)
    at ui/vnc-enc-tight.c:884
    ...
(gdb) x/i $rip
=> 0x396ab3d2b6 <__memcpy_ssse3_back+6710>:	movdqu -0x10(%rsi),%xmm0
(gdb) print $rsi+0x10
$1 = 0x103f2ab21
(gdb) x/xb 0x103f2ab21
0x103f2ab21:	Cannot access memory at address 0x103f2ab21


The following program illustrates the problem.

    $ cat t.c
    #include <stdio.h>

    main()
    {
        int previous_out;
        unsigned long total_out = 0x80000000;
        size_t bytes;

        previous_out = total_out;
        total_out += 0x10000;
        bytes = total_out - previous_out;
        printf("%lx\n", bytes);
    }
    $ cc t.c -o t
    $ ./t
    100010000


The patch changes the type of 'previous_out' to 'uLong' which
is the same as the type of 'zstream->total_out'.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>


diff -up ./ui/vnc-enc-tight.c.orig0 ./ui/vnc-enc-tight.c
--- ./ui/vnc-enc-tight.c.orig0	2011-03-15 03:53:22.000000000 +0100
+++ ./ui/vnc-enc-tight.c	2011-03-20 12:14:48.013560009 +0100
@@ -849,7 +849,7 @@ static int tight_compress_data(VncState 
                                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);

Re: [Qemu-devel] [PATCH] vnc: segmentation fault caused by incorrect 'bytes' count calculated in tight_compress_data()

[Corentin Chary]

On Mon, Mar 21, 2011 at 8:48 AM, Ulrich Obergfell <uobergfe@redhat.com> wrote:
>
> tight_compress_data() calculates an incorrect 'bytes' count
> if 'zstream->total_out' is greater than 0x7fffffff, because
> the type of the variable 'previous_out' is 'int'.
>
>    852     int previous_out;
>     :
>    872     previous_out = zstream->total_out;
>     :
>    881     bytes = zstream->total_out - previous_out;
>    882
>    883     tight_send_compact_size(vs, bytes);
>    884     vnc_write(vs, vs->tight.zlib.buffer, bytes);
>
>
> The incorrect 'bytes' count causes segmentation faults in
> functions called by tight_compress_data(). For example:
>
> (gdb) bt
> #0  0x000000396ab3d2b6 in __memcpy_ssse3_back () from /lib64/libc.so.6
> #1  0x00000000004b3b91 in buffer_append (buffer=0x322d660,
>    data=<value optimized out>, len=<value optimized out>)
>    at /usr/include/bits/string3.h:52
> #2  0x00000000004bbf02 in tight_compress_data (vs=0x32215b0,
>    stream_id=<value optimized out>, bytes=0x100024881,
>    level=<value optimized out>, strategy=<value optimized out>)
>    at ui/vnc-enc-tight.c:884
>    ...
> (gdb) x/i $rip
> => 0x396ab3d2b6 <__memcpy_ssse3_back+6710>:     movdqu -0x10(%rsi),%xmm0
> (gdb) print $rsi+0x10
> $1 = 0x103f2ab21
> (gdb) x/xb 0x103f2ab21
> 0x103f2ab21:    Cannot access memory at address 0x103f2ab21
>
>
> The following program illustrates the problem.
>
>    $ cat t.c
>    #include <stdio.h>
>
>    main()
>    {
>        int previous_out;
>        unsigned long total_out = 0x80000000;
>        size_t bytes;
>
>        previous_out = total_out;
>        total_out += 0x10000;
>        bytes = total_out - previous_out;
>        printf("%lx\n", bytes);
>    }
>    $ cc t.c -o t
>    $ ./t
>    100010000
>
>
> The patch changes the type of 'previous_out' to 'uLong' which
> is the same as the type of 'zstream->total_out'.
>
> Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
>
>
> diff -up ./ui/vnc-enc-tight.c.orig0 ./ui/vnc-enc-tight.c
> --- ./ui/vnc-enc-tight.c.orig0  2011-03-15 03:53:22.000000000 +0100
> +++ ./ui/vnc-enc-tight.c        2011-03-20 12:14:48.013560009 +0100
> @@ -849,7 +849,7 @@ static int tight_compress_data(VncState
>                                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);
>
>

Hi Ulrich,
Looks a lot like "vnc: tight: Fix crash after 2GB of output", right ?

-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] [PATCH] vnc: segmentation fault caused by incorrect 'bytes' count calculated in tight_compress_data()

[Ulrich Obergfell]

 
> Hi Ulrich,
> Looks a lot like "vnc: tight: Fix crash after 2GB of output", right ?
> 
> --
> Corentin Chary
> http://xf.iksaif.net

Hi Corentin,

yes, this appears to be the same issue as:
http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg02044.html

You posted your patch only a few minutes before I posted mine.
Hence, I wasn't aware that a fix was already available.

Regards,

Uli