Re: [Qemu-devel] [PATCH v1] ps2: check PS2Queue pointers in post_load routine

[Dr. David Alan Gilbert]

* Gerd Hoffmann (address@hidden) wrote:
> > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> > > index f388a23c8e..de171a28dd 100644
> > > --- a/hw/input/ps2.c
> > > +++ b/hw/input/ps2.c
> > > @@ -1225,24 +1225,21 @@ static void ps2_common_reset(PS2State *s)
> > >  static void ps2_common_post_load(PS2State *s)
> > >  {
> > >      PS2Queue *q = &s->queue;
> > > -    int size;
> > > -    int i;
> > > -    int tmp_data[PS2_QUEUE_SIZE];
> > > +    uint8_t i, size;
> > > +    uint8_t tmp_data[PS2_QUEUE_SIZE];
> > 
> > Hi Prasad,
> > 
> > you don't need to change the invalid values to sane ones.  Instead, make
> > ps2_common_post_load return an int (just like the .post_load member of
> > VMStateDescription).  You can then detect out of range count/rptr/wptr
> > and return -1 for bad indices.
> 
> Well, it's not that simple because older qemu versions had larger
> queues.  So post_load accepts migrations with queues which are too big.
> It just clears the queue in that case, so the input events are dropped
> in that (highly unlikely) case.
> 
> Also note that the current post_load is broken.  tmp_data is int whereas
> q->data is uint8_t.  So ...
> 
> > > -        memcpy(q->data, tmp_data, size);
> 
> ... this memcpy doesn't work as intended.  Nobody noticed in years.

Oops!

> v1 fixes this bug, v2 doesn't.
> 
> So, I'm tempted to just rip out the whole backward compatibility buffer
> shuffling logic.  Or take v1 of this patch.

I think v1 is OK.

Dave

> cheers,
>   Gerd
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK