[Spice-devel] [PATCH spice-server v2] red-parse-qxl: Fix QUIC images from QXL

[Uri Lublin]

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 at 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:
>>>