| Message ID | 20190725163710.11703-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Typecheck VMSTATE VARRAY macros and fix bug found | expand |
* 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
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
* 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
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
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
* 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
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
* 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
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) {\
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(-) -- 2.20.1