On Fri, 2016-09-16 at 12:32 +0100, Frediano Ziglio wrote:
> To check fscanf read all needed information a dummy "%n" is appended
> to any string and the value stored there is tested. This as scanf
> family
> could return a valid value but not entirely process the string so
> adding a "%n" and checking this was processed make sure all expected
> string is found.
> The code to check for a specific string is now a bit more complicated
> as replay_fscanf use a macro which append a constant string.
> The "error" field is used to mark any error happened, so in most
> cases
> there is no explicit check beside when this could cause a problem
> (for instance results of replay_fscanf used which would result in
> uninitialised variable usage).
> 
> Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> ---
>  server/red-replay-qxl.c | 91
> +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 80 insertions(+), 11 deletions(-)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 7eddaf4..968a605 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -49,6 +49,7 @@ struct SpiceReplay {
>      GArray *id_map_inv; // replay id -> record id
>      GArray *id_free; // free list
>      int nsurfaces;
> +    int end_pos;
>  
>      GList *allocated;
>  
> @@ -69,11 +70,13 @@ static int replay_fread(SpiceReplay *replay,
> uint8_t *buf, size_t size)
>  }
>  
>  __attribute__((format(scanf, 2, 3)))
> -static replay_t replay_fscanf(SpiceReplay *replay, const char *fmt,
> ...)
> +static replay_t replay_fscanf_check(SpiceReplay *replay, const char
> *fmt, ...)
>  {
>      va_list ap;
>      int ret;
>  
> +    replay->end_pos = -1;
> +
>      if (replay->error) {
>          return REPLAY_ERROR;
>      }
> @@ -84,11 +87,13 @@ static replay_t replay_fscanf(SpiceReplay
> *replay, const char *fmt, ...)
>      va_start(ap, fmt);
>      ret = vfscanf(replay->fd, fmt, ap);
>      va_end(ap);
> -    if (ret == EOF) {
> +    if (ret == EOF || replay->end_pos < 0) {
>          replay->error = TRUE;
>      }
>      return replay->error ? REPLAY_ERROR : REPLAY_OK;
>  }

I like the idea, but I'm not a big fan of the implementation here. This
function makes two hard assumptions. It assumes that it is called with
a format string that ends with %n and it assumes that the last argument
is &r->end_pos. But it doesn't verify either of these assumptions, so
it's really easy to misuse.

> +#define replay_fscanf(r, fmt, ...) \
> +    replay_fscanf_check(r, fmt "%n", ## __VA_ARGS__, &r->end_pos)

Here we're enforcing the two assumptions mentioned above. So when code
below uses this macro, things are OK. But we can't use this macro in
all situations. 

>  
>  static inline void *replay_malloc(SpiceReplay *replay, size_t size)
>  {
> @@ -210,9 +215,11 @@ static replay_t read_binary(SpiceReplay *replay,
> const char *prefix, size_t *siz
>      uint8_t *zlib_buffer;
>      z_stream strm;
>  
> -    snprintf(template, sizeof(template), "binary %%d %s %%ld:",
> prefix);
> -    if (replay_fscanf(replay, template, &with_zlib, size) ==
> REPLAY_ERROR)
> +    snprintf(template, sizeof(template), "binary %%d %s %%ld:%%n",
> prefix);
> +    replay_fscanf_check(replay, template, &with_zlib, size, &replay-
> >end_pos);
> +    if (replay->error) {
>          return REPLAY_ERROR;
> +    }

Here for example. 

>  
>      if (*buf == NULL) {
>          *buf = replay_malloc(replay, *size + base_size);
> @@ -224,15 +231,19 @@ static replay_t read_binary(SpiceReplay
> *replay, const char *prefix, size_t *siz
>          hexdump(*buf + base_size, *size);
>      }
>  #else
> -    spice_return_val_if_fail(with_zlib != -1, REPLAY_ERROR);
>      if (with_zlib) {
>          int ret;
>          GList *elem;
>  
>          replay_fscanf(replay, "%d:", &zlib_size);
> +        if (replay->error) {
> +            return REPLAY_ERROR;
> +        }
>          zlib_buffer = replay_malloc(replay, zlib_size);
>          elem = replay->allocated;
> -        replay_fread(replay, zlib_buffer, zlib_size);
> +        if (replay_fread(replay, zlib_buffer, zlib_size) !=
> zlib_size) {
> +            return REPLAY_ERROR;
> +        }
>          strm.zalloc = Z_NULL;
>          strm.zfree = Z_NULL;
>          strm.opaque = Z_NULL;
> @@ -265,8 +276,7 @@ static replay_t read_binary(SpiceReplay *replay,
> const char *prefix, size_t *siz
>          replay_fread(replay, *buf + base_size, *size);
>      }
>  #endif
> -    replay_fscanf(replay, "\n");
> -    return REPLAY_OK;
> +    return replay_fscanf(replay, "\n");
>  }
>  
>  static size_t red_replay_data_chunks(SpiceReplay *replay, const char
> *prefix,
> @@ -337,8 +347,9 @@ static void red_replay_rect_ptr(SpiceReplay
> *replay, const char *prefix, QXLRect
>  {
>      char template[1024];
>  
> -    snprintf(template, sizeof(template), "rect %s %%d %%d %%d
> %%d\n", prefix);
> -    replay_fscanf(replay, template, &qxl->top, &qxl->left, &qxl-
> >bottom, &qxl->right);
> +    snprintf(template, sizeof(template), "rect %s %%d %%d %%d
> %%d\n%%n", prefix);
> +    replay_fscanf_check(replay, template, &qxl->top, &qxl->left,
> &qxl->bottom, &qxl->right,
> +                        &replay->end_pos);
>  }
>  
>  static QXLPath *red_replay_path(SpiceReplay *replay)
> @@ -364,6 +375,9 @@ static QXLClipRects
> *red_replay_clip_rects(SpiceReplay *replay)
>      int num_rects;
>  
>      replay_fscanf(replay, "num_rects %d\n", &num_rects);
> +    if (replay->error) {
> +        return NULL;
> +    }


I personally think it would be cleaner in these cases to do something
like

if (replay_fscanf(...) == REPLAY_ERROR) {
    return NULL;
}

instead of relying on the side-effect of setting the replay->error
variable.


>      red_replay_data_chunks(replay, "clip_rects", (uint8_t**)&qxl,
> sizeof(QXLClipRects));
>      qxl->num_rects = num_rects;
>      return qxl;
> @@ -392,6 +406,9 @@ static QXLImage *red_replay_image(SpiceReplay
> *replay, uint32_t flags)
>      int has_image;
>  
>      replay_fscanf(replay, "image %d\n", &has_image);
> +    if (replay->error) {
> +        return NULL;
> +    }
>      if (!has_image) {
>          return NULL;
>      }
> @@ -402,6 +419,9 @@ static QXLImage *red_replay_image(SpiceReplay
> *replay, uint32_t flags)
>      replay_fscanf(replay, "descriptor.flags %d\n", &temp); qxl-
> >descriptor.flags = temp;
>      replay_fscanf(replay, "descriptor.width %d\n", &qxl-
> >descriptor.width);
>      replay_fscanf(replay, "descriptor.height %d\n", &qxl-
> >descriptor.height);
> +    if (replay->error) {
> +        return NULL;
> +    }
>  
>      switch (qxl->descriptor.type) {
>      case SPICE_IMAGE_TYPE_BITMAP:
> @@ -417,6 +437,9 @@ static QXLImage *red_replay_image(SpiceReplay
> *replay, uint32_t flags)
>              int i, num_ents;
>  
>              replay_fscanf(replay, "qp.num_ents %d\n", &num_ents);
> +            if (replay->error) {
> +                return NULL;
> +            }
>              qp = replay_malloc(replay, sizeof(QXLPalette) + num_ents
> * sizeof(qp->ents[0]));
>              qp->num_ents = num_ents;
>              qxl->bitmap.palette = QXLPHYSICAL_FROM_PTR(qp);
> @@ -441,12 +464,18 @@ static QXLImage *red_replay_image(SpiceReplay
> *replay, uint32_t flags)
>          break;
>      case SPICE_IMAGE_TYPE_SURFACE:
>          replay_fscanf(replay, "surface_image.surface_id %d\n", &qxl-
> >surface_image.surface_id);
> +        if (replay->error) {
> +            return NULL;
> +        }
>          qxl->surface_image.surface_id = replay_id_get(replay, qxl-
> >surface_image.surface_id);
>          break;
>      case SPICE_IMAGE_TYPE_QUIC:
>          // TODO - make this much nicer (precompute size and allocs,
> store them during
>          // record, then reread into them. and use MPEG-4).
>          replay_fscanf(replay, "quic.data_size %d\n", &qxl-
> >quic.data_size);
> +        if (replay->error) {
> +            return NULL;
> +        }
>          qxl = realloc(qxl, sizeof(QXLImageDescriptor) +
> sizeof(QXLQUICData) +
>                        qxl->quic.data_size);
>          size = red_replay_data_chunks(replay, "quic.data",
> (uint8_t**)&qxl->quic.data, 0);
> @@ -488,6 +517,10 @@ static void red_replay_image_free(SpiceReplay
> *replay, QXLPHYSICAL p, uint32_t f
>  static void red_replay_brush_ptr(SpiceReplay *replay, QXLBrush *qxl,
> uint32_t flags)
>  {
>      replay_fscanf(replay, "type %d\n", &qxl->type);
> +    if (replay->error) {
> +        return;
> +    }
> +
>      switch (qxl->type) {
>      case SPICE_BRUSH_TYPE_SOLID:
>          replay_fscanf(replay, "u.color %d\n", &qxl->u.color);
> @@ -652,6 +685,9 @@ static void red_replay_stroke_ptr(SpiceReplay
> *replay, QXLStroke *qxl, uint32_t
>  
>      qxl->path = QXLPHYSICAL_FROM_PTR(red_replay_path(replay));
>      replay_fscanf(replay, "attr.flags %d\n", &temp); qxl->attr.flags 
> = temp;
> +    if (replay->error) {
> +        return;
> +    }
>      if (qxl->attr.flags & SPICE_LINE_FLAGS_STYLED) {
>          size_t size;
>  
> @@ -749,6 +785,9 @@ static void red_replay_invers_free(SpiceReplay
> *replay, QXLInvers *qxl, uint32_t
>  static void red_replay_clip_ptr(SpiceReplay *replay, QXLClip *qxl)
>  {
>      replay_fscanf(replay, "type %d\n", &qxl->type);
> +    if (replay->error) {
> +        return;
> +    }
>      switch (qxl->type) {
>      case SPICE_CLIP_TYPE_RECTS:
>          qxl->data =
> QXLPHYSICAL_FROM_PTR(red_replay_clip_rects(replay));
> @@ -778,7 +817,7 @@ static uint8_t *red_replay_transform(SpiceReplay
> *replay)
>  
>  static void red_replay_composite_ptr(SpiceReplay *replay,
> QXLComposite *qxl, uint32_t flags)
>  {
> -    int enabled;
> +    int enabled = 0;
>  
>      replay_fscanf(replay, "flags %d\n", &qxl->flags);
>      qxl->src = QXLPHYSICAL_FROM_PTR(red_replay_image(replay,
> flags));
> @@ -818,15 +857,24 @@ static QXLDrawable
> *red_replay_native_drawable(SpiceReplay *replay, uint32_t fla
>      replay_fscanf(replay, "self_bitmap %d\n", &temp); qxl-
> >self_bitmap = temp;
>      red_replay_rect_ptr(replay, "self_bitmap_area", &qxl-
> >self_bitmap_area);
>      replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
> +    if (replay->error) {
> +        return NULL;
> +    }
>      qxl->surface_id = replay_id_get(replay, qxl->surface_id);
>  
>      for (i = 0; i < 3; i++) {
>          replay_fscanf(replay, "surfaces_dest %d\n", &qxl-
> >surfaces_dest[i]);
> +        if (replay->error) {
> +            return NULL;
> +        }
>          qxl->surfaces_dest[i] = replay_id_get(replay, qxl-
> >surfaces_dest[i]);
>          red_replay_rect_ptr(replay, "surfaces_rects", &qxl-
> >surfaces_rects[i]);
>      }
>  
>      replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> +    if (replay->error) {
> +        return NULL;
> +    }
>      switch (qxl->type) {
>      case QXL_DRAW_ALPHA_BLEND:
>          red_replay_alpha_blend_ptr(replay, &qxl->u.alpha_blend,
> flags);
> @@ -949,6 +997,9 @@ static QXLCompatDrawable
> *red_replay_compat_drawable(SpiceReplay *replay, uint32
>      red_replay_rect_ptr(replay, "bitmap_area", &qxl->bitmap_area);
>  
>      replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> +    if (replay->error) {
> +        return NULL;
> +    }
>      switch (qxl->type) {
>      case QXL_DRAW_ALPHA_BLEND:
>          red_replay_alpha_blend_ptr_compat(replay, &qxl-
> >u.alpha_blend, flags);
> @@ -1019,6 +1070,9 @@ static QXLUpdateCmd
> *red_replay_update_cmd(SpiceReplay *replay)
>      red_replay_rect_ptr(replay, "area", &qxl->area);
>      replay_fscanf(replay, "update_id %d\n", &qxl->update_id);
>      replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
> +    if (replay->error) {
> +        return NULL;
> +    }
>      qxl->surface_id = replay_id_get(replay, qxl->surface_id);
>  
>      return qxl;
> @@ -1044,6 +1098,9 @@ static QXLSurfaceCmd
> *red_replay_surface_cmd(SpiceReplay *replay)
>      replay_fscanf(replay, "surface_id %d\n", &qxl->surface_id);
>      replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
>      replay_fscanf(replay, "flags %d\n", &qxl->flags);
> +    if (replay->error) {
> +        return NULL;
> +    }
>  
>      switch (qxl->type) {
>      case QXL_SURFACE_CMD_CREATE:
> @@ -1051,6 +1108,9 @@ static QXLSurfaceCmd
> *red_replay_surface_cmd(SpiceReplay *replay)
>          replay_fscanf(replay, "u.surface_create.width %d\n", &qxl-
> >u.surface_create.width);
>          replay_fscanf(replay, "u.surface_create.height %d\n", &qxl-
> >u.surface_create.height);
>          replay_fscanf(replay, "u.surface_create.stride %d\n", &qxl-
> >u.surface_create.stride);
> +        if (replay->error) {
> +            return NULL;
> +        }
>          size = qxl->u.surface_create.height * abs(qxl-
> >u.surface_create.stride);
>          if ((qxl->flags & QXL_SURF_FLAG_KEEP_DATA) != 0) {
>              read_binary(replay, "data", &read_size, (uint8_t**)&qxl-
> >u.surface_create.data, 0);
> @@ -1097,6 +1157,9 @@ static QXLCursor *red_replay_cursor(SpiceReplay
> *replay)
>      cursor.header.hot_spot_y = temp;
>  
>      replay_fscanf(replay, "data_size %d\n", &temp);
> +    if (replay->error) {
> +        return NULL;
> +    }
>      cursor.data_size = temp;
>      cursor.data_size = red_replay_data_chunks(replay, "cursor",
> (uint8_t**)&qxl, sizeof(QXLCursor));
>      qxl->header = cursor.header;
> @@ -1111,6 +1174,9 @@ static QXLCursorCmd
> *red_replay_cursor_cmd(SpiceReplay *replay)
>  
>      replay_fscanf(replay, "cursor_cmd\n");
>      replay_fscanf(replay, "type %d\n", &temp);
> +    if (replay->error) {
> +        return NULL;
> +    }
>      qxl->type = temp;
>      switch (qxl->type) {
>      case QXL_CURSOR_SET:
> @@ -1160,6 +1226,9 @@ static void
> replay_handle_create_primary(QXLWorker *worker, SpiceReplay *replay)
>          &surface.stride, &surface.format);
>      replay_fscanf(replay, "%d %d %d %d\n", &surface.position,
> &surface.mouse_mode,
>          &surface.flags, &surface.type);
> +    if (replay->error) {
> +        return;
> +    }
>      read_binary(replay, "data", &size, &mem, 0);
>      surface.group_id = 0;
>      surface.mem = QXLPHYSICAL_FROM_PTR(mem);
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to