Hey,
On Mon, Sep 12, 2016 at 12:56:05PM +0100, Frediano Ziglio wrote:
> Detect errors in record file. This can happen from a wrong version or
> corruption of files.
> Allocations are kept into a GList to be able to free in case some
> errors happened.
It would be nice to have this bit in a separate commit
> 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.
And this one in a separate one too
> The "eof" 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).
Should we rename that field then? If it's no longer 'end-of-file', but
'error-occurred', 'eof' is very misleading.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/red-replay-qxl.c | 256 ++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 185 insertions(+), 71 deletions(-)
>
> Don't remember which version of the patch is.
> If I remember it should fix latests comments.
>
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index b17c38b..f916fa8 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -49,29 +49,33 @@ struct SpiceReplay {
> GArray *id_map_inv; // replay id -> record id
> GArray *id_free; // free list
> int nsurfaces;
> + int end_pos;
> +
> + GList *allocated;
>
> pthread_mutex_t mutex;
> pthread_cond_t cond;
> };
>
> -static int replay_fread(SpiceReplay *replay, uint8_t *buf, size_t size)
> +static ssize_t replay_fread(SpiceReplay *replay, uint8_t *buf, size_t size)
> {
> - if (replay->eof) {
> - return 0;
> - }
> - if (feof(replay->fd)) {
> + if (replay->eof
> + || feof(replay->fd)
> + || fread(buf, 1, size, replay->fd) != size) {
Really not a big fan of chaining calls in an if, what about something
like this?
if (replay->eof)
goto error;
if (feof(replay->fd)
goto error;
if (fread(...) ..)
goto error;
return size;
error:
...
> replay->eof = 1;
> return 0;
> }
> - return fread(buf, size, 1, replay->fd);
> + return 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->eof) {
> return REPLAY_EOF;
> }
> @@ -82,11 +86,27 @@ 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->eof = 1;
> }
> return replay->eof ? REPLAY_EOF : REPLAY_OK;
> }
> +#define replay_fscanf(r, fmt, ...) \
> + replay_fscanf_check(r, fmt "%n", ## __VA_ARGS__, &r->end_pos)
> +
> +static inline void *replay_malloc(SpiceReplay *replay, size_t size)
> +{
> + void *p = spice_malloc(size);
Can we get a more expressive name than 'p' ? :) mem, allocation or
allocated_mem or whatever
> + replay->allocated = g_list_prepend(replay->allocated, p);
> + return p;
> +}
> +
> +static inline void *replay_malloc0(SpiceReplay *replay, size_t size)
> +{
> + void *p = spice_malloc0(size);
> + replay->allocated = g_list_prepend(replay->allocated, p);
> + return p;
> +}
replay_malloc0 could be:
void *mem = replay_malloc(replay, size);
memset(mem, '\0', size);
>
> static uint32_t replay_id_get(SpiceReplay *replay, uint32_t id)
> {
> @@ -188,18 +208,20 @@ static void hexdump(uint8_t *hex, uint8_t bytes)
> static replay_t read_binary(SpiceReplay *replay, const char *prefix, size_t *size, uint8_t
> **buf, size_t base_size)
> {
> - char template[1024];
> - int with_zlib = -1;
> - int zlib_size;
> - uint8_t *zlib_buffer;
> + char file_prefix[1024];
> + int with_zlib;
> z_stream strm;
>
> - snprintf(template, sizeof(template), "binary %%d %s %%ld:", prefix);
> - if (replay_fscanf(replay, template, &with_zlib, size) == REPLAY_EOF)
> + if (replay_fscanf(replay, "binary %d %1000s %ld:", &with_zlib, file_prefix, size) == REPLAY_EOF) {
file_prefix is 1024 bytes, but you only use 1000 here?
> return REPLAY_EOF;
> + }
> + if (strcmp(file_prefix, prefix) != 0) {
> + replay->eof = 1;
> + return REPLAY_EOF;
> + }
>
> if (*buf == NULL) {
> - *buf = spice_malloc(*size + base_size);
> + *buf = replay_malloc(replay, *size + base_size);
> }
> #if 0
> {
> @@ -211,10 +233,18 @@ static replay_t read_binary(SpiceReplay *replay, const char *prefix, size_t *siz
> spice_return_val_if_fail(with_zlib != -1, REPLAY_EOF);
> if (with_zlib) {
> int ret;
> + GList *elem;
> + uint8_t *zlib_buffer;
> + int zlib_size;
>
> - replay_fscanf(replay, "%d:", &zlib_size);
> - zlib_buffer = spice_malloc(zlib_size);
> - replay_fread(replay, zlib_buffer, zlib_size);
> + if (replay_fscanf(replay, "%d:", &zlib_size) == REPLAY_EOF) {
> + return REPLAY_EOF;
> + }
> + zlib_buffer = replay_malloc(replay, zlib_size);
> + elem = replay->allocated;
> + if (replay_fread(replay, zlib_buffer, zlib_size) != zlib_size) {
Test is slightly redundant as replay_fread now is going to return either
0 or zlib_size.
> + return REPLAY_EOF;
> + }
> strm.zalloc = Z_NULL;
> strm.zfree = Z_NULL;
> strm.opaque = Z_NULL;
> @@ -241,16 +271,16 @@ static replay_t read_binary(SpiceReplay *replay, const char *prefix, size_t *siz
> }
> }
> (void)inflateEnd(&strm);
> + replay->allocated = g_list_remove_link(replay->allocated, elem);
> free(zlib_buffer); // TODO - avoid repeat alloc/dealloc by keeping last
> } else {
> 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,
> +static ssize_t red_replay_data_chunks(SpiceReplay *replay, const char *prefix,
> uint8_t **mem, size_t base_size)
> {
> size_t data_size;
> @@ -258,7 +288,9 @@ static size_t red_replay_data_chunks(SpiceReplay *replay, const char *prefix,
> size_t next_data_size;
> QXLDataChunk *cur, *next;
>
> - replay_fscanf(replay, "data_chunks %d %zu\n", &count_chunks, &data_size);
> + if (replay_fscanf(replay, "data_chunks %d %zu\n", &count_chunks, &data_size) == REPLAY_EOF) {
> + return REPLAY_EOF;
> + }
> if (base_size == 0) {
> base_size = sizeof(QXLDataChunk);
> }
> @@ -316,19 +348,26 @@ static void red_replay_point16_ptr(SpiceReplay *replay, QXLPoint16 *qxl)
>
> static void red_replay_rect_ptr(SpiceReplay *replay, const char *prefix, QXLRect *qxl)
> {
> - char template[1024];
> + char file_prefix[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);
> + if (replay_fscanf(replay, "rect %1000s %d %d %d %d\n", file_prefix, &qxl->top, &qxl->left,
> + &qxl->bottom, &qxl->right) == REPLAY_EOF) {
> + return;
> + }
> + if (strcmp(file_prefix, prefix) != 0) {
> + replay->eof = 1;
> + }
Fwiw, I'd say the snprintf version was a bit more obvious than this one.
> }
>
> 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));
> - qxl->data_size = data_size;
> + if (data_size >= 0) {
> + qxl->data_size = data_size;
> + }
> return qxl;
I think this should make sure that NULL is returned if
red_replay_data_chunks() failed (or red_replay_data_chunks() should do
that).
> }
>
> @@ -344,9 +383,11 @@ static QXLClipRects *red_replay_clip_rects(SpiceReplay *replay)
> QXLClipRects *qxl = NULL;
> int num_rects;
>
> - replay_fscanf(replay, "num_rects %d\n", &num_rects);
> - red_replay_data_chunks(replay, "clip_rects", (uint8_t**)&qxl, sizeof(QXLClipRects));
> - qxl->num_rects = num_rects;
> + if (replay_fscanf(replay, "num_rects %d\n", &num_rects) == REPLAY_EOF) {
> + return NULL;
> + }
> + if (red_replay_data_chunks(replay, "clip_rects", (uint8_t**)&qxl, sizeof(QXLClipRects)) >= 0)
> + qxl->num_rects = num_rects;
Same comment here.
> return qxl;
> }
>
> @@ -365,24 +406,29 @@ 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;
> + QXLImage *qxl = NULL;
> + size_t bitmap_size;
> + ssize_t size;
> uint8_t qxl_flags;
> int temp;
> int has_palette;
> int has_image;
>
> - replay_fscanf(replay, "image %d\n", &has_image);
> + if (replay_fscanf(replay, "image %d\n", &has_image) == REPLAY_EOF) {
> + return NULL;
> + }
> if (!has_image) {
> return NULL;
> }
>
> - qxl = spice_new0(QXLImage, 1);
> + qxl = (QXLImage*)replay_malloc0(replay, sizeof(QXLImage));
> replay_fscanf(replay, "descriptor.id %"PRIu64"\n", &qxl->descriptor.id);
> replay_fscanf(replay, "descriptor.type %d\n", &temp); qxl->descriptor.type = temp;
> 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_fscanf(replay, "descriptor.height %d\n", &qxl->descriptor.height) == REPLAY_EOF) {
> + return NULL;
> + }
>
> switch (qxl->descriptor.type) {
> case SPICE_IMAGE_TYPE_BITMAP:
> @@ -397,8 +443,10 @@ static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)
> QXLPalette *qp;
> int i, num_ents;
>
> - replay_fscanf(replay, "qp.num_ents %d\n", &num_ents);
> - qp = spice_malloc(sizeof(QXLPalette) + num_ents * sizeof(qp->ents[0]));
> + if (replay_fscanf(replay, "qp.num_ents %d\n", &num_ents) == REPLAY_EOF) {
> + 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);
> replay_fscanf(replay, "unique %"PRIu64"\n", &qp->unique);
> @@ -413,7 +461,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)
> if (qxl_flags & QXL_BITMAP_DIRECT) {
> qxl->bitmap.data = QXLPHYSICAL_FROM_PTR(red_replay_image_data_flat(replay, &bitmap_size));
> } else {
> - size = red_replay_data_chunks(replay, "bitmap.data", (uint8_t**)&qxl->bitmap.data, 0);
> + ssize_t size = red_replay_data_chunks(replay, "bitmap.data", (uint8_t**)&qxl->bitmap.data, 0);
> if (size != bitmap_size) {
> spice_printerr("bad image, %zu != %zu", size, bitmap_size);
> return NULL;
> @@ -421,13 +469,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_fscanf(replay, "surface_image.surface_id %d\n", &qxl->surface_image.surface_id)
> + == REPLAY_EOF) {
> + 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_fscanf(replay, "quic.data_size %d\n", &qxl->quic.data_size) == REPLAY_EOF) {
> + 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);
> @@ -468,7 +521,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_fscanf(replay, "type %d\n", &qxl->type) == REPLAY_EOF) {
> + return;
> + }
> +
> switch (qxl->type) {
> case SPICE_BRUSH_TYPE_SOLID:
> replay_fscanf(replay, "u.color %d\n", &qxl->u.color);
> @@ -632,7 +688,10 @@ static void red_replay_stroke_ptr(SpiceReplay *replay, QXLStroke *qxl, uint32_t
> int temp;
>
> qxl->path = QXLPHYSICAL_FROM_PTR(red_replay_path(replay));
> - replay_fscanf(replay, "attr.flags %d\n", &temp); qxl->attr.flags = temp;
> + if (replay_fscanf(replay, "attr.flags %d\n", &temp) == REPLAY_EOF) {
> + return;
> + }
> + qxl->attr.flags = temp;
> if (qxl->attr.flags & SPICE_LINE_FLAGS_STYLED) {
> size_t size;
>
> @@ -659,17 +718,19 @@ 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));
> - qxl->data_size = data_size;
> - qxl->length = length;
> - qxl->flags = flags;
> - spice_assert(chunk_size == qxl->data_size);
> + if (chunk_size >= 0) {
> + qxl->data_size = data_size;
> + qxl->length = length;
> + qxl->flags = flags;
> + spice_assert(chunk_size == qxl->data_size);
> + }
else { return NULL; }
> return qxl;
> }
>
> @@ -729,7 +790,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_fscanf(replay, "type %d\n", &qxl->type) == REPLAY_EOF) {
> + return;
> + }
> switch (qxl->type) {
> case SPICE_CLIP_TYPE_RECTS:
> qxl->data = QXLPHYSICAL_FROM_PTR(red_replay_clip_rects(replay));
> @@ -759,7 +822,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));
> @@ -788,7 +851,7 @@ static void red_replay_composite_free(SpiceReplay *replay, QXLComposite *qxl, ui
>
> static QXLDrawable *red_replay_native_drawable(SpiceReplay *replay, uint32_t flags)
> {
> - QXLDrawable *qxl = spice_malloc0(sizeof(QXLDrawable)); // TODO - this is too large usually
> + QXLDrawable *qxl = replay_malloc0(replay, sizeof(QXLDrawable)); // TODO - this is too large usually
> int i;
> int temp;
>
> @@ -798,16 +861,22 @@ static QXLDrawable *red_replay_native_drawable(SpiceReplay *replay, uint32_t fla
> replay_fscanf(replay, "mm_time %d\n", &qxl->mm_time);
> 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_fscanf(replay, "surface_id %d\n", &qxl->surface_id) == REPLAY_EOF) {
> + 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_fscanf(replay, "surfaces_dest %d\n", &qxl->surfaces_dest[i]) == REPLAY_EOF) {
> + 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_fscanf(replay, "type %d\n", &temp) == REPLAY_EOF)
> + return NULL;
> + qxl->type = temp;
> switch (qxl->type) {
> case QXL_DRAW_ALPHA_BLEND:
> red_replay_alpha_blend_ptr(replay, &qxl->u.alpha_blend, flags);
> @@ -857,7 +926,11 @@ static QXLDrawable *red_replay_native_drawable(SpiceReplay *replay, uint32_t fla
> spice_warn_if_reached();
> break;
> };
> - return qxl;
> + if (!replay->eof) {
> + return qxl;
> + }
> +
> + return NULL;
> }
>
> static void red_replay_native_drawable_free(SpiceReplay *replay, QXLDrawable *qxl, uint32_t flags)
> @@ -919,7 +992,7 @@ static void red_replay_native_drawable_free(SpiceReplay *replay, QXLDrawable *qx
> static QXLCompatDrawable *red_replay_compat_drawable(SpiceReplay *replay, uint32_t flags)
> {
> int temp;
> - QXLCompatDrawable *qxl = spice_malloc0(sizeof(QXLCompatDrawable)); // TODO - too large usually
> + QXLCompatDrawable *qxl = replay_malloc0(replay, sizeof(QXLCompatDrawable)); // TODO - too large usually
>
> red_replay_rect_ptr(replay, "bbox", &qxl->bbox);
> red_replay_clip_ptr(replay, &qxl->clip);
> @@ -929,7 +1002,10 @@ static QXLCompatDrawable *red_replay_compat_drawable(SpiceReplay *replay, uint32
> replay_fscanf(replay, "bitmap_offset %d\n", &temp); qxl->bitmap_offset = temp;
> red_replay_rect_ptr(replay, "bitmap_area", &qxl->bitmap_area);
>
> - replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
> + if (replay_fscanf(replay, "type %d\n", &temp) == REPLAY_EOF) {
> + return NULL;
> + }
> + qxl->type = temp;
> switch (qxl->type) {
> case QXL_DRAW_ALPHA_BLEND:
> red_replay_alpha_blend_ptr_compat(replay, &qxl->u.alpha_blend, flags);
> @@ -976,14 +1052,15 @@ static QXLCompatDrawable *red_replay_compat_drawable(SpiceReplay *replay, uint32
> spice_error("%s: unknown type %d", __FUNCTION__, qxl->type);
> break;
> };
> - return qxl;
> + if (!replay->eof) {
> + return qxl;
> + }
> +
> + return NULL;
> }
>
> static QXLPHYSICAL red_replay_drawable(SpiceReplay *replay, uint32_t flags)
> {
> - if (replay->eof) {
> - return 0;
> - }
> replay_fscanf(replay, "drawable\n");
> if (flags & QXL_COMMAND_FLAG_COMPAT) {
> return QXLPHYSICAL_FROM_PTR(red_replay_compat_drawable(replay, flags));
> @@ -994,12 +1071,14 @@ static QXLPHYSICAL red_replay_drawable(SpiceReplay *replay, uint32_t flags)
>
> static QXLUpdateCmd *red_replay_update_cmd(SpiceReplay *replay)
> {
> - QXLUpdateCmd *qxl = spice_malloc0(sizeof(QXLUpdateCmd));
> + QXLUpdateCmd *qxl = replay_malloc0(replay, sizeof(QXLUpdateCmd));
>
> replay_fscanf(replay, "update\n");
> 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_fscanf(replay, "surface_id %d\n", &qxl->surface_id) == REPLAY_EOF) {
> + return NULL;
> + }
> qxl->surface_id = replay_id_get(replay, qxl->surface_id);
>
> return qxl;
> @@ -1024,14 +1103,19 @@ static QXLSurfaceCmd *red_replay_surface_cmd(SpiceReplay *replay)
> replay_fscanf(replay, "surface_cmd\n");
> 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_fscanf(replay, "flags %d\n", &qxl->flags) == REPLAY_EOF) {
> + return NULL;
> + }
Hmm, so if I followed correctly, only the last _fscanf is tested for
errors because replay->eof is going to be set if any failed, and
replay_fscanf() will always return an error when this is set?
If yes, I think I'd never check replay_fscanf() return value, and just
add an explicit check for replay->eof (or a helper doing this check)
after the last fscanf.
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel