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

[Uri Lublin]

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 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.

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