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

[Uri Lublin]

On 7/31/19 6:13 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" should 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. To enable QUIC encoding I added "image-compression=quic"
> and "streaming-video=off" to "-spice" Qemu option in order to force
> QUIC encoding in the guest driver (thanks to Uri Lublin for the help
> reproducing it).
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Ack.

Uri.

> ---
>   server/red-replay-qxl.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> Changes from v2:
> - back to v1 with reproduction and explanation.
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 674feae2f..7104c81cd 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -416,7 +416,7 @@ static uint8_t *red_replay_image_data_flat(SpiceReplay *replay, size_t *size)
>   
>   static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)
>   {
> -    QXLImage* qxl = NULL;
> +    QXLImage* qxl = NULL, *data;
>       size_t bitmap_size;
>       ssize_t size;
>       uint8_t qxl_flags;
> @@ -497,10 +497,15 @@ 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);
> +        data = NULL;
> +        size = red_replay_data_chunks(replay, "quic.data", (uint8_t**)&data,
> +                                      sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
> +                                      sizeof(QXLDataChunk));
>           spice_assert(size == qxl->quic.data_size);
> +        data->descriptor = qxl->descriptor;
> +        data->quic.data_size = qxl->quic.data_size;
> +        replay_free(replay, qxl);
> +        qxl = data;
>           break;
>       default:
>           spice_warn_if_reached();
> @@ -526,7 +531,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:
>