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

[Peter Maydell]

On Thu, 25 Jul 2019 at 18:27, Dr. David Alan Gilbert
<address@hidden> wrote:
>
> * Peter Maydell (address@hidden) 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 <address@hidden>
>
> > ---
> >  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.)

> However, for the rest of it, from migration I'm happy:
>
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>
> 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