[Spice-devel] [PATCH v3 2/8] replay: Handle errors reading strings from record file
Jonathon Jongsma
jjongsma at redhat.com
Tue Sep 20 16:25:31 UTC 2016
On Tue, 2016-09-20 at 12:00 -0400, Frediano Ziglio wrote:
> >
> >
> > 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 <fziglio at 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.
> >
>
> Yes, I always used the macro till Christophe said that template code
> was more readable.
> Any alternative suggestion that make all happy?
>
OK, I missed this earlier review. I guess I prefer the old way. But I
don't care too much.
> >
> > >
> > >
> > > 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.
> >
> >
>
> This was previous code but now it's similar to the way ferror works
> and also make code similar when there are more replay_fscanf one
> after
> each other where test is only on the last replay_fscanf which was
> indeed
> quite confusing.
>
> Frediano
Personal preference, I guess. I don't really like this style, but if
you prefer it, that's fine. Then you should ignore the comments on the
following patches as well.
Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
More information about the Spice-devel
mailing list