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

[Dr. David Alan Gilbert]

* 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].

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.


>  #define type_check_pointer(t1,t2) ((t1**)0 - (t2*)0)
> +/*
> + * type of element 0 of the specified (array) field of the type.
> + * Note that if the field is a pointer then this will return the
> + * pointed-to type rather than complaining.
> + */
> +#define typeof_elt_of_field(type, field) typeof(((type *)0)->field[0])
> +/* Check that field f in struct type t2 is an array of t1, of any size */
> +#define type_check_varray(t1, t2, f)                                 \
> +    (type_check(t1, typeof_elt_of_field(t2, f))                      \
> +     + QEMU_BUILD_BUG_ON_ZERO(!QEMU_IS_ARRAY(((t2 *)0)->f)))
>  
>  #define vmstate_offset_value(_state, _field, _type)                  \
>      (offsetof(_state, _field) +                                      \
> @@ -253,6 +264,10 @@ extern const VMStateInfo vmstate_info_qtailq;
>      vmstate_offset_array(_state, _field, uint8_t,                    \
>                           sizeof(typeof_field(_state, _field)))
>  
> +#define vmstate_offset_varray(_state, _field, _type)                 \
> +    (offsetof(_state, _field) +                                      \
> +     type_check_varray(_type, _state, _field))
> +
>  /* In the macros below, if there is a _version, that means the macro's
>   * field will be processed only if the version being received is >=
>   * the _version specified.  In general, if you add a new field, you
> @@ -347,7 +362,7 @@ extern const VMStateInfo vmstate_info_qtailq;
>      .info       = &(_info),                                          \
>      .size       = sizeof(_type),                                     \
>      .flags      = VMS_VARRAY_UINT32|VMS_MULTIPLY_ELEMENTS,           \
> -    .offset     = offsetof(_state, _field),                          \
> +    .offset     = vmstate_offset_varray(_state, _field, _type),      \
>  }
>  
>  #define VMSTATE_ARRAY_TEST(_field, _state, _num, _test, _info, _type) {\
> @@ -376,7 +391,7 @@ extern const VMStateInfo vmstate_info_qtailq;
>      .info       = &(_info),                                          \
>      .size       = sizeof(_type),                                     \
>      .flags      = VMS_VARRAY_INT32,                                  \
> -    .offset     = offsetof(_state, _field),                          \
> +    .offset     = vmstate_offset_varray(_state, _field, _type),      \
>  }
>  
>  #define VMSTATE_VARRAY_INT32(_field, _state, _field_num, _version, _info, 
> _type) {\
> @@ -416,7 +431,7 @@ extern const VMStateInfo vmstate_info_qtailq;
>      .info       = &(_info),                                          \
>      .size       = sizeof(_type),                                     \
>      .flags      = VMS_VARRAY_UINT16,                                 \
> -    .offset     = offsetof(_state, _field),                          \
> +    .offset     = vmstate_offset_varray(_state, _field, _type),      \
>  }
>  
>  #define VMSTATE_VSTRUCT_TEST(_field, _state, _test, _version, _vmsd, _type, 
> _struct_version) { \
> @@ -520,7 +535,7 @@ extern const VMStateInfo vmstate_info_qtailq;
>      .vmsd       = &(_vmsd),                                          \
>      .size       = sizeof(_type),                                     \
>      .flags      = VMS_STRUCT|VMS_VARRAY_UINT8,                       \
> -    .offset     = offsetof(_state, _field),                          \
> +    .offset     = vmstate_offset_varray(_state, _field, _type),      \
>  }
>  
>  /* a variable length array (i.e. _type *_field) but we know the
> @@ -573,7 +588,7 @@ extern const VMStateInfo vmstate_info_qtailq;
>      .vmsd       = &(_vmsd),                                          \
>      .size       = sizeof(_type),                                     \
>      .flags      = VMS_STRUCT|VMS_VARRAY_INT32,                       \
> -    .offset     = offsetof(_state, _field),                          \
> +    .offset     = vmstate_offset_varray(_state, _field, _type),      \
>  }
>  
>  #define VMSTATE_STRUCT_VARRAY_UINT32(_field, _state, _field_num, _version, 
> _vmsd, _type) { \
> @@ -583,7 +598,7 @@ extern const VMStateInfo vmstate_info_qtailq;
>      .vmsd       = &(_vmsd),                                          \
>      .size       = sizeof(_type),                                     \
>      .flags      = VMS_STRUCT|VMS_VARRAY_UINT32,                      \
> -    .offset     = offsetof(_state, _field),                          \
> +    .offset     = vmstate_offset_varray(_state, _field, _type),      \
>  }
>  
>  #define VMSTATE_STRUCT_VARRAY_ALLOC(_field, _state, _field_num, _version, 
> _vmsd, _type) {\
> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK