[spice-server,v2] red-parse-qxl: Fix QUIC images from QXL

Submitted by Frediano Ziglio on July 23, 2019, 8:21 p.m.

Details

Message ID 20190723202114.3076-1-fziglio@redhat.com
State Superseded
Headers show
Series "red-parse-qxl: Fix QUIC images from QXL" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 23, 2019, 8:21 p.m.
The decoding is wrong, the Red and QXL structures are different so
code is not doing what is expected. red-parse-qxl translate from QXL
to Red structures, red-record-qxl saves Red structure to file,
red-replay-qxl is supposed to read from file into QXL directly.

If a Quic image is stored inside QXL memory the layout of the QXLImage
in memory is:
- QXLImageDescriptor
- QXLQUICData
- QXLDataChunk
- first chunk data
and all remaining data in linked QXLDataChunk.
red_replay_image was reading the image as data was all contained in
QXLImage->quic.data however "data" shoule store the first QXLDataChunk
followed by QXLDataChunk data.
Use proper base_size calling red_replay_data_chunks in order to
initialise the image with the first data chunk correctly.

Not easy to reproduce, the only driver is XDDM which means Windows XP
or similars, I got no recording with such images.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/red-replay-qxl.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Changes v1:
- realloc the buffer inside read_binary with the proper size.
  The proper size if not available in red_replay_image.

Patch hide | download patch | download mbox

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 674feae2f..1f1bd8c1d 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -114,6 +114,9 @@  static inline void replay_free(SpiceReplay *replay, void *mem)
 
 static inline void *replay_realloc(SpiceReplay *replay, void *mem, size_t n_bytes)
 {
+    if (mem == NULL) {
+        return replay_malloc(replay, n_bytes);
+    }
     GList *elem = g_list_find(replay->allocated, mem);
     elem->data = g_realloc(mem, n_bytes);
     return elem->data;
@@ -231,9 +234,7 @@  static replay_t read_binary(SpiceReplay *replay, const char *prefix, size_t *siz
         return REPLAY_ERROR;
     }
 
-    if (*buf == NULL) {
-        *buf = replay_malloc(replay, *size + base_size);
-    }
+    *buf = replay_realloc(replay, *buf, *size + base_size);
 #if 0
     {
         int num_read = fread(*buf + base_size, *size, 1, fd);
@@ -497,9 +498,9 @@  static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)
         if (replay->error) {
             return NULL;
         }
-        qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
-                             qxl->quic.data_size);
-        size = red_replay_data_chunks(replay, "quic.data", (uint8_t**)&qxl->quic.data, 0);
+        size = red_replay_data_chunks(replay, "quic.data", (uint8_t**)&qxl,
+                                      sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
+                                      sizeof(QXLDataChunk));
         spice_assert(size == qxl->quic.data_size);
         break;
     default:
@@ -526,7 +527,9 @@  static void red_replay_image_free(SpiceReplay *replay, QXLPHYSICAL p, uint32_t f
     case SPICE_IMAGE_TYPE_SURFACE:
         break;
     case SPICE_IMAGE_TYPE_QUIC:
-        red_replay_data_chunks_free(replay, qxl, 0);
+        red_replay_data_chunks_free(replay, qxl,
+                                    sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
+                                    sizeof(QXLDataChunk));
         qxl = NULL;
         break;
     default:

Comments

Hi Frediano,

On 7/23/19 11:21 PM, Frediano Ziglio wrote:
> The decoding is wrong, the Red and QXL structures are different so
> code is not doing what is expected. red-parse-qxl translate from QXL
> to Red structures, red-record-qxl saves Red structure to file,
> red-replay-qxl is supposed to read from file into QXL directly.
> 
> If a Quic image is stored inside QXL memory the layout of the QXLImage
> in memory is:
> - QXLImageDescriptor
> - QXLQUICData
> - QXLDataChunk
> - first chunk data
> and all remaining data in linked QXLDataChunk.
> red_replay_image was reading the image as data was all contained in
> QXLImage->quic.data however "data" shoule store the first QXLDataChunk

shoule -> should

> followed by QXLDataChunk data.
> Use proper base_size calling red_replay_data_chunks in order to
> initialise the image with the first data chunk correctly.

I like this patch better then the first one.



> 
> Not easy to reproduce, the only driver is XDDM which means Windows XP
> or similars, I got no recording with such images.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>   server/red-replay-qxl.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> Changes v1:
> - realloc the buffer inside read_binary with the proper size.
>    The proper size if not available in red_replay_image.

Can we not just fix the above without moving the realloc call ?
Before only QUIC memory (which was malloc'ed) was realloc'ed.
With this other memory may be realloced.

Allocate more memory (add sizeof(QXLDataChunk))
    qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor)
                         + sizeof(QXLQUICData)
                         + sizeof(QXLDataChunk)
                         + qxl->quic.data_size);

Call red_replay_data_chunks as you do in this patch
Call red_replay_data_chunks_free as you do in this patch.

Uri.

> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 674feae2f..1f1bd8c1d 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -114,6 +114,9 @@ static inline void replay_free(SpiceReplay *replay, void *mem)
>   
>   static inline void *replay_realloc(SpiceReplay *replay, void *mem, size_t n_bytes)
>   {
> +    if (mem == NULL) {
> +        return replay_malloc(replay, n_bytes);
> +    }
>       GList *elem = g_list_find(replay->allocated, mem);
>       elem->data = g_realloc(mem, n_bytes);
>       return elem->data;
> @@ -231,9 +234,7 @@ static replay_t read_binary(SpiceReplay *replay, const char *prefix, size_t *siz
>           return REPLAY_ERROR;
>       }
>   
> -    if (*buf == NULL) {
> -        *buf = replay_malloc(replay, *size + base_size);
> -    }
> +    *buf = replay_realloc(replay, *buf, *size + base_size);
>   #if 0
>       {
>           int num_read = fread(*buf + base_size, *size, 1, fd);
> @@ -497,9 +498,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)
>           if (replay->error) {
>               return NULL;
>           }
> -        qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
> -                             qxl->quic.data_size);
> -        size = red_replay_data_chunks(replay, "quic.data", (uint8_t**)&qxl->quic.data, 0);
> +        size = red_replay_data_chunks(replay, "quic.data", (uint8_t**)&qxl,
> +                                      sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
> +                                      sizeof(QXLDataChunk));
>           spice_assert(size == qxl->quic.data_size);
>           break;
>       default:
> @@ -526,7 +527,9 @@ static void red_replay_image_free(SpiceReplay *replay, QXLPHYSICAL p, uint32_t f
>       case SPICE_IMAGE_TYPE_SURFACE:
>           break;
>       case SPICE_IMAGE_TYPE_QUIC:
> -        red_replay_data_chunks_free(replay, qxl, 0);
> +        red_replay_data_chunks_free(replay, qxl,
> +                                    sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
> +                                    sizeof(QXLDataChunk));
>           qxl = NULL;
>           break;
>       default:
>
> 
> Hi Frediano,
> 
> On 7/23/19 11:21 PM, Frediano Ziglio wrote:
> > The decoding is wrong, the Red and QXL structures are different so
> > code is not doing what is expected. red-parse-qxl translate from QXL
> > to Red structures, red-record-qxl saves Red structure to file,
> > red-replay-qxl is supposed to read from file into QXL directly.
> > 
> > If a Quic image is stored inside QXL memory the layout of the QXLImage
> > in memory is:
> > - QXLImageDescriptor
> > - QXLQUICData
> > - QXLDataChunk
> > - first chunk data
> > and all remaining data in linked QXLDataChunk.
> > red_replay_image was reading the image as data was all contained in
> > QXLImage->quic.data however "data" shoule store the first QXLDataChunk
> 
> shoule -> should
> 

I'll fix, thanks

> > followed by QXLDataChunk data.
> > Use proper base_size calling red_replay_data_chunks in order to
> > initialise the image with the first data chunk correctly.
> 
> I like this patch better then the first one.
> 
> 
> 
> > 
> > Not easy to reproduce, the only driver is XDDM which means Windows XP
> > or similars, I got no recording with such images.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >   server/red-replay-qxl.c | 17 ++++++++++-------
> >   1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > Changes v1:
> > - realloc the buffer inside read_binary with the proper size.
> >    The proper size if not available in red_replay_image.
> 
> Can we not just fix the above without moving the realloc call ?
> Before only QUIC memory (which was malloc'ed) was realloc'ed.
> With this other memory may be realloced.

I checked all calls, only for QUIC the initial pointer is NULL
so it will get reallocated.

> 
> Allocate more memory (add sizeof(QXLDataChunk))
>     qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor)
>                          + sizeof(QXLQUICData)
>                          + sizeof(QXLDataChunk)
>                          + qxl->quic.data_size);
> 

No, I rather prefer patch version 1 instead. The replay code should
attempt to allocate memory more like drivers. This will also help
using sanitizer tools. In this specific case allocating a larger
buffer will avoid some potential buffer overflow check.

Looking more at the code I noted that in many places the "header"
(information before chunks) are read into temporary variables
then raw data is allocated with the structure than the structure
header is filled (like red_replay_clip_rects for instance,
another more complicated one is red_replay_cursor).
The exception is red_replay_image where the "qxl" (in this case
a QXLImage type) is allocated at the beginning. One difference
between bitmap and quic is that "data" in QXLBitmap is a
QXLPHYSICAL, not a uint8_t[0].

> Call red_replay_data_chunks as you do in this patch
> Call red_replay_data_chunks_free as you do in this patch.
> 

Surely the offset is wrong, is expected to be the offset of raw
data to write.

> Uri.
> 
> > 
> > diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> > index 674feae2f..1f1bd8c1d 100644
> > --- a/server/red-replay-qxl.c
> > +++ b/server/red-replay-qxl.c
> > @@ -114,6 +114,9 @@ static inline void replay_free(SpiceReplay *replay,
> > void *mem)
> >   
> >   static inline void *replay_realloc(SpiceReplay *replay, void *mem, size_t
> >   n_bytes)
> >   {
> > +    if (mem == NULL) {
> > +        return replay_malloc(replay, n_bytes);
> > +    }
> >       GList *elem = g_list_find(replay->allocated, mem);
> >       elem->data = g_realloc(mem, n_bytes);
> >       return elem->data;
> > @@ -231,9 +234,7 @@ static replay_t read_binary(SpiceReplay *replay, const
> > char *prefix, size_t *siz
> >           return REPLAY_ERROR;
> >       }
> >   
> > -    if (*buf == NULL) {
> > -        *buf = replay_malloc(replay, *size + base_size);
> > -    }
> > +    *buf = replay_realloc(replay, *buf, *size + base_size);
> >   #if 0
> >       {
> >           int num_read = fread(*buf + base_size, *size, 1, fd);
> > @@ -497,9 +498,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> > uint32_t flags)
> >           if (replay->error) {
> >               return NULL;
> >           }
> > -        qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) +
> > sizeof(QXLQUICData) +
> > -                             qxl->quic.data_size);
> > -        size = red_replay_data_chunks(replay, "quic.data",
> > (uint8_t**)&qxl->quic.data, 0);
> > +        size = red_replay_data_chunks(replay, "quic.data",
> > (uint8_t**)&qxl,
> > +                                      sizeof(QXLImageDescriptor) +
> > sizeof(QXLQUICData) +
> > +                                      sizeof(QXLDataChunk));
> >           spice_assert(size == qxl->quic.data_size);
> >           break;
> >       default:
> > @@ -526,7 +527,9 @@ static void red_replay_image_free(SpiceReplay *replay,
> > QXLPHYSICAL p, uint32_t f
> >       case SPICE_IMAGE_TYPE_SURFACE:
> >           break;
> >       case SPICE_IMAGE_TYPE_QUIC:
> > -        red_replay_data_chunks_free(replay, qxl, 0);
> > +        red_replay_data_chunks_free(replay, qxl,
> > +                                    sizeof(QXLImageDescriptor) +
> > sizeof(QXLQUICData) +
> > +                                    sizeof(QXLDataChunk));
> >           qxl = NULL;
> >           break;
> >       default:
> >
On 7/24/19 2:37 PM, Frediano Ziglio wrote:
>>
>> Hi Frediano,
>>
>> On 7/23/19 11:21 PM, Frediano Ziglio wrote:
>>> The decoding is wrong, the Red and QXL structures are different so
>>> code is not doing what is expected. red-parse-qxl translate from QXL
>>> to Red structures, red-record-qxl saves Red structure to file,
>>> red-replay-qxl is supposed to read from file into QXL directly.
>>>
>>> If a Quic image is stored inside QXL memory the layout of the QXLImage
>>> in memory is:
>>> - QXLImageDescriptor
>>> - QXLQUICData
>>> - QXLDataChunk
>>> - first chunk data
>>> and all remaining data in linked QXLDataChunk.
>>> red_replay_image was reading the image as data was all contained in
>>> QXLImage->quic.data however "data" shoule store the first QXLDataChunk
>>
>> shoule -> should
>>
> 
> I'll fix, thanks
> 
>>> followed by QXLDataChunk data.
>>> Use proper base_size calling red_replay_data_chunks in order to
>>> initialise the image with the first data chunk correctly.
>>
>> I like this patch better then the first one.
>>
>>
>>
>>>
>>> Not easy to reproduce, the only driver is XDDM which means Windows XP
>>> or similars, I got no recording with such images.
>>>
>>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>>> ---
>>>    server/red-replay-qxl.c | 17 ++++++++++-------
>>>    1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>> Changes v1:
>>> - realloc the buffer inside read_binary with the proper size.
>>>     The proper size if not available in red_replay_image.
>>
>> Can we not just fix the above without moving the realloc call ?
>> Before only QUIC memory (which was malloc'ed) was realloc'ed.
>> With this other memory may be realloced.
> 
> I checked all calls, only for QUIC the initial pointer is NULL
> so it will get reallocated.
> 
>>
>> Allocate more memory (add sizeof(QXLDataChunk))
>>      qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor)
>>                           + sizeof(QXLQUICData)
>>                           + sizeof(QXLDataChunk)
>>                           + qxl->quic.data_size);
>>
> 
> No, I rather prefer patch version 1 instead. The replay code should
> attempt to allocate memory more like drivers. This will also help
> using sanitizer tools. In this specific case allocating a larger
> buffer will avoid some potential buffer overflow check.
> 
> Looking more at the code I noted that in many places the "header"
> (information before chunks) are read into temporary variables
> then raw data is allocated with the structure than the structure
> header is filled (like red_replay_clip_rects for instance,
> another more complicated one is red_replay_cursor).
> The exception is red_replay_image where the "qxl" (in this case
> a QXLImage type) is allocated at the beginning. One difference
> between bitmap and quic is that "data" in QXLBitmap is a
> QXLPHYSICAL, not a uint8_t[0].

Yes, you are right. I think your V1 indeed fits better to
the code around.

I was able to reproduce with running windows XP guest and
setting -spice image-compression=quic on qemu-kvm command line.

It indeed crashes on master and fixed by V1 and V2.

> 
>> Call red_replay_data_chunks as you do in this patch
>> Call red_replay_data_chunks_free as you do in this patch.
>>
> 
> Surely the offset is wrong, is expected to be the offset of raw
> data to write.

Yes, the offset of original code is wrong but you fixed it in both
versions.


Uri.

>>
>>>
>>> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
>>> index 674feae2f..1f1bd8c1d 100644
>>> --- a/server/red-replay-qxl.c
>>> +++ b/server/red-replay-qxl.c
>>> @@ -114,6 +114,9 @@ static inline void replay_free(SpiceReplay *replay,
>>> void *mem)
>>>    
>>>    static inline void *replay_realloc(SpiceReplay *replay, void *mem, size_t
>>>    n_bytes)
>>>    {
>>> +    if (mem == NULL) {
>>> +        return replay_malloc(replay, n_bytes);
>>> +    }
>>>        GList *elem = g_list_find(replay->allocated, mem);
>>>        elem->data = g_realloc(mem, n_bytes);
>>>        return elem->data;
>>> @@ -231,9 +234,7 @@ static replay_t read_binary(SpiceReplay *replay, const
>>> char *prefix, size_t *siz
>>>            return REPLAY_ERROR;
>>>        }
>>>    
>>> -    if (*buf == NULL) {
>>> -        *buf = replay_malloc(replay, *size + base_size);
>>> -    }
>>> +    *buf = replay_realloc(replay, *buf, *size + base_size);
>>>    #if 0
>>>        {
>>>            int num_read = fread(*buf + base_size, *size, 1, fd);
>>> @@ -497,9 +498,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
>>> uint32_t flags)
>>>            if (replay->error) {
>>>                return NULL;
>>>            }
>>> -        qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) +
>>> sizeof(QXLQUICData) +
>>> -                             qxl->quic.data_size);
>>> -        size = red_replay_data_chunks(replay, "quic.data",
>>> (uint8_t**)&qxl->quic.data, 0);
>>> +        size = red_replay_data_chunks(replay, "quic.data",
>>> (uint8_t**)&qxl,
>>> +                                      sizeof(QXLImageDescriptor) +
>>> sizeof(QXLQUICData) +
>>> +                                      sizeof(QXLDataChunk));
>>>            spice_assert(size == qxl->quic.data_size);
>>>            break;
>>>        default:
>>> @@ -526,7 +527,9 @@ static void red_replay_image_free(SpiceReplay *replay,
>>> QXLPHYSICAL p, uint32_t f
>>>        case SPICE_IMAGE_TYPE_SURFACE:
>>>            break;
>>>        case SPICE_IMAGE_TYPE_QUIC:
>>> -        red_replay_data_chunks_free(replay, qxl, 0);
>>> +        red_replay_data_chunks_free(replay, qxl,
>>> +                                    sizeof(QXLImageDescriptor) +
>>> sizeof(QXLQUICData) +
>>> +                                    sizeof(QXLDataChunk));
>>>            qxl = NULL;
>>>            break;
>>>        default:
>>>