Re: [Qemu-devel] [PATCH for-4.1? 2/2] vmstate.h: Type check VMSTATE_STRUCT_VARRAY macros

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 25 Jul 2019 at 18:27, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > The VMSTATE_STRUCT_VARRAY_UINT32 macro is intended to handle
> > > migrating a field which is an array of structs, but where instead of
> > > migrating the entire array we only migrate a variable number of
> > > elements of it.
> > >
> > > The VMSTATE_STRUCT_VARRAY_POINTER_UINT32 macro is intended to handle
> > > migrating a field which is of pointer type, and points to a
> > > dynamically allocated array of structs of variable size.
> > >
> > > We weren't actually checking that the field passed to
> > > VMSTATE_STRUCT_VARRAY_UINT32 really is an array, with the result that
> > > accidentally using it where the _POINTER_ macro was intended would
> > > compile but silently corrupt memory on migration.
> > >
> > > Add type-checking that enforces that the field passed in is
> > > really of the right array type. This applies to all the VMSTATE
> > > macros which use flags including VMS_VARRAY_* but not VMS_POINTER.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > > ---
> > >  include/migration/vmstate.h | 27 +++++++++++++++++++++------
> > >  1 file changed, 21 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > > index ca68584eba4..2df333c3612 100644
> > > --- a/include/migration/vmstate.h
> > > +++ b/include/migration/vmstate.h
> > > @@ -227,8 +227,19 @@ extern const VMStateInfo vmstate_info_bitmap;
> > >  extern const VMStateInfo vmstate_info_qtailq;
> > >
> > >  #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
> > > +/* Check that t2 is an array of t1 of size n */
> > >  #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
> >
> > I'd have to admit I don't understand why that does what you say;
> > I'd expected something to index a t2 pointer with [n].
> 
> Note that this is just a comment describing what the existing
> macro does, as a way to distinguish its job from that of the
> new macro I'm adding.
> 
> What happens here is that t2 is a type like "foo [32]", ie
> it is an array type already. t1 is the base 'foo' type; so the macro
> is checking that t1[n] matches t2, where n is passed in to us
> and must match the declared array size of the field (32 in
> my example). (In C the size of the array is carried around as
> part of its type, and must match on both sides of the expression;
> so if you pass in the name of an array field that's the wrong size the
> type check will fail, which is what we want.)

Ah, OK that makes sense; what it really needs is that example to make
me realise that t2 was already the array.

Dave

> > However, for the rest of it, from migration I'm happy:
> >
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> > given it's just fixing an ARM bug, and given it'll blow up straight away
> > I think it's OK for 4.1; the only risk is if we find a compiler we don't
> > like.
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK