[Spice-devel] [PATCH v3 2/8] replay: Handle errors reading strings from record file

[Frediano Ziglio]

> 
> 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?

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