[Spice-devel] [PATCH v3 3/8] replay: Detect errors from red_replay_data_chunks

[Jonathon Jongsma]

On Fri, 2016-09-16 at 12:32 +0100, Frediano Ziglio wrote:
> Change the return to ssize_t to be able to distinguish from
> empty buffer to error.
> Check result returned and avoid continuing potentially
> deferencing NULL pointers.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-replay-qxl.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 968a605..a5b9553 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -279,8 +279,8 @@ static replay_t read_binary(SpiceReplay *replay,
> const char *prefix, size_t *siz
>      return replay_fscanf(replay, "\n");
>  }
>  
> -static size_t red_replay_data_chunks(SpiceReplay *replay, const char
> *prefix,
> -                                     uint8_t **mem, size_t
> base_size)
> +static ssize_t red_replay_data_chunks(SpiceReplay *replay, const
> char *prefix,
> +                                      uint8_t **mem, size_t
> base_size)
>  {
>      size_t data_size;
>      int count_chunks;
> @@ -288,12 +288,15 @@ static size_t
> red_replay_data_chunks(SpiceReplay *replay, const char *prefix,
>      QXLDataChunk *cur, *next;
>  
>      replay_fscanf(replay, "data_chunks %d %zu\n", &count_chunks,
> &data_size);
> +    if (replay->error) {
> +        return -1;
> +    }

Same comment as last patch. I'd personally prefer to check the return
of replay_fscanf() here instead of replay->error.

Otherwise fine.

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

>      if (base_size == 0) {
>          base_size = sizeof(QXLDataChunk);
>      }
>  
>      if (read_binary(replay, prefix, &next_data_size, mem, base_size)
> == REPLAY_ERROR) {
> -        return 0;
> +        return -1;
>      }
>      cur = (QXLDataChunk*)(*mem + base_size - sizeof(QXLDataChunk));
>      cur->data_size = next_data_size;
> @@ -302,7 +305,7 @@ static size_t red_replay_data_chunks(SpiceReplay
> *replay, const char *prefix,
>      while (count_chunks-- > 0) {
>          if (read_binary(replay, prefix, &next_data_size,
> (uint8_t**)&cur->next_chunk,
>              sizeof(QXLDataChunk)) == REPLAY_ERROR) {
> -            return 0;
> +            return -1;
>          }
>          data_size += next_data_size;
>          next = QXLPHYSICAL_TO_PTR(cur->next_chunk);
> @@ -355,9 +358,12 @@ static void red_replay_rect_ptr(SpiceReplay
> *replay, const char *prefix, QXLRect
>  static QXLPath *red_replay_path(SpiceReplay *replay)
>  {
>      QXLPath *qxl = NULL;
> -    size_t data_size;
> +    ssize_t data_size;
>  
>      data_size = red_replay_data_chunks(replay, "path",
> (uint8_t**)&qxl, sizeof(QXLPath));
> +    if (data_size < 0) {
> +        return NULL;
> +    }
>      qxl->data_size = data_size;
>      return qxl;
>  }
> @@ -378,7 +384,9 @@ static QXLClipRects
> *red_replay_clip_rects(SpiceReplay *replay)
>      if (replay->error) {
>          return NULL;
>      }
> -    red_replay_data_chunks(replay, "clip_rects", (uint8_t**)&qxl,
> sizeof(QXLClipRects));
> +    if (red_replay_data_chunks(replay, "clip_rects",
> (uint8_t**)&qxl, sizeof(QXLClipRects)) < 0) {
> +        return NULL;
> +    }
>      qxl->num_rects = num_rects;
>      return qxl;
>  }
> @@ -399,7 +407,8 @@ 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;
> -    size_t bitmap_size, size;
> +    size_t bitmap_size;
> +    ssize_t size;
>      uint8_t qxl_flags;
>      int temp;
>      int has_palette;
> @@ -714,13 +723,16 @@ static QXLString *red_replay_string(SpiceReplay
> *replay)
>      uint32_t data_size;
>      uint16_t length;
>      uint16_t flags;
> -    size_t chunk_size;
> +    ssize_t chunk_size;
>      QXLString *qxl = NULL;
>  
>      replay_fscanf(replay, "data_size %d\n", &data_size);
>      replay_fscanf(replay, "length %d\n", &temp); length = temp;
>      replay_fscanf(replay, "flags %d\n", &temp); flags = temp;
>      chunk_size = red_replay_data_chunks(replay, "string",
> (uint8_t**)&qxl, sizeof(QXLString));
> +    if (chunk_size <  0) {
> +        return NULL;
> +    }
>      qxl->data_size = data_size;
>      qxl->length = length;
>      qxl->flags = flags;
> @@ -1143,6 +1155,7 @@ static QXLCursor *red_replay_cursor(SpiceReplay
> *replay)
>  {
>      int temp;
>      QXLCursor cursor, *qxl = NULL;
> +    ssize_t data_size;
>  
>      replay_fscanf(replay, "header.unique %"SCNu64"\n",
> &cursor.header.unique);
>      replay_fscanf(replay, "header.type %d\n", &temp);
> @@ -1160,10 +1173,12 @@ static QXLCursor
> *red_replay_cursor(SpiceReplay *replay)
>      if (replay->error) {
>          return NULL;
>      }
> -    cursor.data_size = temp;
> -    cursor.data_size = red_replay_data_chunks(replay, "cursor",
> (uint8_t**)&qxl, sizeof(QXLCursor));
> +    data_size = red_replay_data_chunks(replay, "cursor",
> (uint8_t**)&qxl, sizeof(QXLCursor));
> +    if (data_size < 0) {
> +        return NULL;
> +    }
>      qxl->header = cursor.header;
> -    qxl->data_size = cursor.data_size;
> +    qxl->data_size = data_size;
>      return qxl;
>  }
>