[Qemu-devel] [PATCH for-4.1? 0/2] Typecheck VMSTATE VARRAY macros and fix bug found

[Qemu-devel] [PATCH for-4.1? 0/2] Typecheck VMSTATE VARRAY macros and fix bug found

[Peter Maydell]

Damien's patch to fix a pl330 vmstate mixup between
VMSTATE_STRUCT_VARRAY_UINT32 and VMSTATE_STRUCT_VARRAY_POINTER_UINT32
led me to think about whether we could catch that particular
mixup. It turns out that we can, by adding a type check that the
field given to the macro is really an array of the correct type.

This only found one other instance of the same bug, in the
stellaris_input device; patch 1 fixes that and then patch 2
is the improved type checking.

We should probably also go through and look at the other
VMSTATE macros that use a raw 'offsetof' rather than one
of the vmstate_offset_foo type-checking macros, and see if
we can add type checks there. (Documentation of the macros
would be nice too...)

I've marked this as for-4.1? because the stellaris bugfix
definitely seems worth including in the release but I'm less
certain about whether to put in the typecheck -- David/Juan can
decide that.

thanks
-- PMM

Based-on: <20190724143553.21557-1-damien.hedde@greensocs.com>
("pl330: fix vmstate description" -- otherwise the new typecheck
will cause a compile failure due to presence of the bug that
patch fixes)

Peter Maydell (2):
  stellaris_input: Fix vmstate description of buttons field
  vmstate.h: Type check VMSTATE_STRUCT_VARRAY macros

 include/migration/vmstate.h | 27 +++++++++++++++++++++------
 hw/input/stellaris_input.c  | 10 ++++++----
 2 files changed, 27 insertions(+), 10 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Peter Maydell]

gamepad_state::buttons is a pointer to an array of structs,
not an array of structs, so should be declared in the vmstate
with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
corrupt memory on incoming migration.

We bump the vmstate version field as the easiest way to
deal with the migration break, since migration wouldn't have
worked reliably before anyway.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/input/stellaris_input.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
index 20c87d86f40..3a666d61d47 100644
--- a/hw/input/stellaris_input.c
+++ b/hw/input/stellaris_input.c
@@ -60,12 +60,14 @@ static const VMStateDescription vmstate_stellaris_button = {
 
 static const VMStateDescription vmstate_stellaris_gamepad = {
     .name = "stellaris_gamepad",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_INT32(extension, gamepad_state),
-        VMSTATE_STRUCT_VARRAY_INT32(buttons, gamepad_state, num_buttons, 0,
-                              vmstate_stellaris_button, gamepad_button),
+        VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state,
+                                            num_buttons,
+                                            vmstate_stellaris_button,
+                                            gamepad_button),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.20.1

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

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> gamepad_state::buttons is a pointer to an array of structs,
> not an array of structs, so should be declared in the vmstate
> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
> corrupt memory on incoming migration.
> 
> We bump the vmstate version field as the easiest way to
> deal with the migration break, since migration wouldn't have
> worked reliably before anyway.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

OK, it would be great to change num_buttons to uint32_t and make that a
UINT32 at some point;  it's hard to press negative buttons.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/input/stellaris_input.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
> index 20c87d86f40..3a666d61d47 100644
> --- a/hw/input/stellaris_input.c
> +++ b/hw/input/stellaris_input.c
> @@ -60,12 +60,14 @@ static const VMStateDescription vmstate_stellaris_button = {
>  
>  static const VMStateDescription vmstate_stellaris_gamepad = {
>      .name = "stellaris_gamepad",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_INT32(extension, gamepad_state),
> -        VMSTATE_STRUCT_VARRAY_INT32(buttons, gamepad_state, num_buttons, 0,
> -                              vmstate_stellaris_button, gamepad_button),
> +        VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state,
> +                                            num_buttons,
> +                                            vmstate_stellaris_button,
> +                                            gamepad_button),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

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

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.


>  #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 / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Philippe Mathieu-Daudé]

On 7/25/19 7:02 PM, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> gamepad_state::buttons is a pointer to an array of structs,
>> not an array of structs, so should be declared in the vmstate
>> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
>> corrupt memory on incoming migration.
>>
>> We bump the vmstate version field as the easiest way to
>> deal with the migration break, since migration wouldn't have
>> worked reliably before anyway.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> OK, it would be great to change num_buttons to uint32_t and make that a
> UINT32 at some point;  it's hard to press negative buttons.

Since the version is incremented, now seems to be a good time.

> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>> ---
>>  hw/input/stellaris_input.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
>> index 20c87d86f40..3a666d61d47 100644
>> --- a/hw/input/stellaris_input.c
>> +++ b/hw/input/stellaris_input.c
>> @@ -60,12 +60,14 @@ static const VMStateDescription vmstate_stellaris_button = {
>>  
>>  static const VMStateDescription vmstate_stellaris_gamepad = {
>>      .name = "stellaris_gamepad",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_INT32(extension, gamepad_state),
>> -        VMSTATE_STRUCT_VARRAY_INT32(buttons, gamepad_state, num_buttons, 0,
>> -                              vmstate_stellaris_button, gamepad_button),
>> +        VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state,
>> +                                            num_buttons,
>> +                                            vmstate_stellaris_button,
>> +                                            gamepad_button),
>>          VMSTATE_END_OF_LIST()
>>      }
>>  };
>> -- 
>> 2.20.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

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

> 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

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Peter Maydell]

On Thu, 25 Jul 2019 at 18:02, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Peter Maydell (peter.maydell@linaro.org) wrote:
> > gamepad_state::buttons is a pointer to an array of structs,
> > not an array of structs, so should be declared in the vmstate
> > with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
> > corrupt memory on incoming migration.
> >
> > We bump the vmstate version field as the easiest way to
> > deal with the migration break, since migration wouldn't have
> > worked reliably before anyway.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> OK, it would be great to change num_buttons to uint32_t and make that a
> UINT32 at some point;  it's hard to press negative buttons.

Is there much benefit though?

As an aside, I'm surprised also the macro doesn't complain
that we said the num_buttons field is int32 but it's really "int"...
arguably a different kind of missing type check.

thanks
-- PMM

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

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 25 Jul 2019 at 18:02, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > gamepad_state::buttons is a pointer to an array of structs,
> > > not an array of structs, so should be declared in the vmstate
> > > with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
> > > corrupt memory on incoming migration.
> > >
> > > We bump the vmstate version field as the easiest way to
> > > deal with the migration break, since migration wouldn't have
> > > worked reliably before anyway.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > OK, it would be great to change num_buttons to uint32_t and make that a
> > UINT32 at some point;  it's hard to press negative buttons.
> 
> Is there much benefit though?

Not much and it's not urgent

> As an aside, I'm surprised also the macro doesn't complain
> that we said the num_buttons field is int32 but it's really "int"...
> arguably a different kind of missing type check.

I was just concerned what would happen if your migration stream
had a negative value in.

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Damien Hedde]

On 7/25/19 7:59 PM, Peter Maydell wrote:
> On Thu, 25 Jul 2019 at 18:02, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
>>
>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>> gamepad_state::buttons is a pointer to an array of structs,
>>> not an array of structs, so should be declared in the vmstate
>>> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
>>> corrupt memory on incoming migration.
>>>
>>> We bump the vmstate version field as the easiest way to
>>> deal with the migration break, since migration wouldn't have
>>> worked reliably before anyway.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>

> As an aside, I'm surprised also the macro doesn't complain
> that we said the num_buttons field is int32 but it's really "int"...
> arguably a different kind of missing type check.

We would need to compile on a host with int size not being 32bit to
catch this kind of problem I think.

--
Damien

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Peter Maydell]

On Fri, 26 Jul 2019 at 09:25, Damien Hedde <damien.hedde@greensocs.com> wrote:
> On 7/25/19 7:59 PM, Peter Maydell wrote:
> > As an aside, I'm surprised also the macro doesn't complain
> > that we said the num_buttons field is int32 but it's really "int"...
> > arguably a different kind of missing type check.
>
> We would need to compile on a host with int size not being 32bit to
> catch this kind of problem I think.

I was under the impression we did catch attempts to use "int" with the
VMSTATE_INT32() macro, but we do apply the type-checking magic to
the 'value' fields in the VARRAY macros, so I guess I'm misremembering.

thanks
-- PMM

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

[Damien Hedde]

On 7/25/19 7:27 PM, Dr. David Alan Gilbert 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>
> 
> However, for the rest of it, from migration I'm happy:
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 

Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Tested-by: Damien Hedde <damien.hedde@greensocs.com>

Damien

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 19:00, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * 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:
> > > >  #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.

Would

/*
 * Check that type t2 is an array of type t1 of size n,
 * eg if t1 is 'foo' and n is 32 then t2 must be 'foo[32]'
 */

be clearer ?

thanks
-- PMM

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

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 25 Jul 2019 at 19:00, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * 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:
> > > > >  #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.
> 
> Would
> 
> /*
>  * Check that type t2 is an array of type t1 of size n,
>  * eg if t1 is 'foo' and n is 32 then t2 must be 'foo[32]'
>  */
> 
> be clearer ?

Yep.

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Peter Maydell]

On Fri, 26 Jul 2019 at 10:32, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Peter Maydell (peter.maydell@linaro.org) wrote:
> > Would
> >
> > /*
> >  * Check that type t2 is an array of type t1 of size n,
> >  * eg if t1 is 'foo' and n is 32 then t2 must be 'foo[32]'
> >  */
> >
> > be clearer ?
>
> Yep.

OK, I'll fold that in. Are you happy for me to take this
via the target-arm tree for 4.1, given the two dependent
patches are both for arm devices?

thanks
-- PMM

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

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Fri, 26 Jul 2019 at 10:32, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > Would
> > >
> > > /*
> > >  * Check that type t2 is an array of type t1 of size n,
> > >  * eg if t1 is 'foo' and n is 32 then t2 must be 'foo[32]'
> > >  */
> > >
> > > be clearer ?
> >
> > Yep.
> 
> OK, I'll fold that in. Are you happy for me to take this
> via the target-arm tree for 4.1, given the two dependent
> patches are both for arm devices?

Yep

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Peter Maydell]

On Thu, 25 Jul 2019 at 18:40, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 7/25/19 7:02 PM, Dr. David Alan Gilbert wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> gamepad_state::buttons is a pointer to an array of structs,
> >> not an array of structs, so should be declared in the vmstate
> >> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
> >> corrupt memory on incoming migration.
> >>
> >> We bump the vmstate version field as the easiest way to
> >> deal with the migration break, since migration wouldn't have
> >> worked reliably before anyway.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > OK, it would be great to change num_buttons to uint32_t and make that a
> > UINT32 at some point;  it's hard to press negative buttons.
>
> Since the version is incremented, now seems to be a good time.

...except this patch is for 4.1 and we've already done rc2,
so it's not really an ideal time to put in code cleanups...

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 25 Jul 2019 at 18:40, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > On 7/25/19 7:02 PM, Dr. David Alan Gilbert wrote:
> > > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > >> gamepad_state::buttons is a pointer to an array of structs,
> > >> not an array of structs, so should be declared in the vmstate
> > >> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
> > >> corrupt memory on incoming migration.
> > >>
> > >> We bump the vmstate version field as the easiest way to
> > >> deal with the migration break, since migration wouldn't have
> > >> worked reliably before anyway.
> > >>
> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > >
> > > OK, it would be great to change num_buttons to uint32_t and make that a
> > > UINT32 at some point;  it's hard to press negative buttons.
> >
> > Since the version is incremented, now seems to be a good time.
> 
> ...except this patch is for 4.1 and we've already done rc2,
> so it's not really an ideal time to put in code cleanups...

Don't worry; you can always change the int to a uint later without
bumping the version again.  Unless someone somewhere has a device with
-ve buttons it'll be fine.

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-4.1? 1/2] stellaris_input: Fix vmstate description of buttons field

[Peter Maydell]

On Fri, 26 Jul 2019 at 10:59, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> Don't worry; you can always change the int to a uint later without
> bumping the version again.  Unless someone somewhere has a device with
> -ve buttons it'll be fine.

The number of buttons is a constant set up when the device is
created (it would be a QOM property if this device had been
converted to QOM; as it is it's an argument to the
stellaris_gamepad_init() function) and the num_buttons field itself
is not migrated. As it happens the one caller of this function
passes the constant value "5"...

thanks
-- PMM