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

Submitted by Frediano Ziglio on July 31, 2019, 3:13 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 31, 2019, 3:13 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" 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@redhat.com>
---
 server/red-replay-qxl.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Changes from v2:
- back to v1 with reproduction and explanation.

Patch hide | download patch | download mbox

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:

Comments

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