| Message ID | 20161108095603.72301-8-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward > for trying to migrate an array with some null pointers in it was an > illegal memory access, that is a swift and painless death of the process. > Let's make vmstate cope with this scenario at least for pointers to > structs. The general approach is when we encounter a null pointer > (element) instead of following the pointer to save/load the data behind > it we save/load a placeholder. This way we can detect if we expected a > null pointer at the load side but not null data was saved instead. Sadly > all other error scenarios are not detected by this scheme (and would > require the usage of the JSON meta data). > > Limitations: Does not work for pointers to primitives. Please document that limitation in a comment in the vmstate.h near the macros that use it. > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by: > Guenther Hutzl <hutzl@linux.vnet.ibm.com> --- > > We will need this to load/save some on demand created state from within > the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an > example). > --- > include/migration/vmstate.h | 5 +++++ > migration/vmstate.c | 32 ++++++++++++++++++++++++++++++-- > 2 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 5940db0..86d4aca 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16; > extern const VMStateInfo vmstate_info_uint32; > extern const VMStateInfo vmstate_info_uint64; > > +/** Put this in the stream when migrating a null pointer.*/ > +#define VMS_NULLPTR_MARKER ((int8_t) -1) You seem to be making things harder by using a signed int everywhere when you just want a byte; I suggest using '0' > +extern const VMStateInfo vmstate_info_nullptr; > + > extern const VMStateInfo vmstate_info_float64; > extern const VMStateInfo vmstate_info_cpudouble; > > @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap; > .size = sizeof(_type *), \ > .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \ > .offset = vmstate_offset_array(_s, _f, _type*, _n), \ > + .info = &vmstate_info_nullptr, \ What's this change for? > } > > #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \ > diff --git a/migration/vmstate.c b/migration/vmstate.c > index 10a7645..ce3490a 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > if (field->flags & VMS_ARRAY_OF_POINTER) { > curr_elem = *(void **)curr_elem; > } > - if (field->flags & VMS_STRUCT) { > + if (!curr_elem) { > + /* if null pointer check placeholder and do not follow */ > + assert(field->flags & VMS_ARRAY_OF_POINTER); > + vmstate_info_nullptr.get(f, curr_elem, size); > + } else if (field->flags & VMS_STRUCT) { > ret = vmstate_load_state(f, field->vmsd, curr_elem, > field->vmsd->version_id); > } else { > @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, > assert(curr_elem); > curr_elem = *(void **)curr_elem; > } > - if (field->flags & VMS_STRUCT) { > + if (!curr_elem) { > + /* if null pointer write placeholder and do not follow */ > + assert(field->flags & VMS_ARRAY_OF_POINTER); > + vmstate_info_nullptr.put(f, curr_elem, size); > + } else if (field->flags & VMS_STRUCT) { > vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop); > } else { > field->info->put(f, curr_elem, size); > @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = { > .put = put_uint64, > }; > > +static int get_nullptr(QEMUFile *f, void *pv, size_t size) > +{ > + int8_t tmp; > + qemu_get_s8s(f, &tmp); Now that I've suggested using just a character you can turn that into: return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL; > + return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL; > +} > + > +static void put_nullptr(QEMUFile *f, void *pv, size_t size) > +{ > + int8_t tmp = VMS_NULLPTR_MARKER; > + assert(pv == NULL); > + qemu_put_s8s(f, &tmp); and similarly put_byte. Dave > +} > + > +const VMStateInfo vmstate_info_nullptr = { > + .name = "uint64", > + .get = get_nullptr, > + .put = put_nullptr, > +}; > + > /* 64 bit unsigned int. See that the received value is the same than the one > in the field */ > > -- > 2.8.4 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 12/15/2016 01:33 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward >> for trying to migrate an array with some null pointers in it was an >> illegal memory access, that is a swift and painless death of the process. >> Let's make vmstate cope with this scenario at least for pointers to >> structs. The general approach is when we encounter a null pointer >> (element) instead of following the pointer to save/load the data behind >> it we save/load a placeholder. This way we can detect if we expected a >> null pointer at the load side but not null data was saved instead. Sadly >> all other error scenarios are not detected by this scheme (and would >> require the usage of the JSON meta data). >> >> Limitations: Does not work for pointers to primitives. > > Please document that limitation in a comment in the vmstate.h near the > macros that use it. > After looking at this again I do not know what was my intention whit the limitations statement. It was probably about arrays of pointers to primitives, but then the statement is wrong. The same scheme does work for primitives too. It just that I had a previous version which had this limitation. It could also be about just pointer (not array of pointers), but the commit message does not mention this case at all. I will just drop the limitations statement. >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by: >> Guenther Hutzl <hutzl@linux.vnet.ibm.com> --- >> >> We will need this to load/save some on demand created state from within >> the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an >> example). >> --- >> include/migration/vmstate.h | 5 +++++ >> migration/vmstate.c | 32 ++++++++++++++++++++++++++++++-- >> 2 files changed, 35 insertions(+), 2 deletions(-) >> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >> index 5940db0..86d4aca 100644 >> --- a/include/migration/vmstate.h >> +++ b/include/migration/vmstate.h >> @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16; >> extern const VMStateInfo vmstate_info_uint32; >> extern const VMStateInfo vmstate_info_uint64; >> >> +/** Put this in the stream when migrating a null pointer.*/ >> +#define VMS_NULLPTR_MARKER ((int8_t) -1) > > You seem to be making things harder by using a signed int everywhere > when you just want a byte; I suggest using '0' > I'm fine with byte 0x30, but I have an irrational bad feeling about using a character literal here. I know, in practice should not matter, but the C11 standard says nothing about how members of the basic execution character set are represented, and it's also not among the 'usual QEMU platform assumptions' (AFAIK). Are you OK defining the constant/macro as Ox30U instead of '0'? >> +extern const VMStateInfo vmstate_info_nullptr; >> + >> extern const VMStateInfo vmstate_info_float64; >> extern const VMStateInfo vmstate_info_cpudouble; >> >> @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap; >> .size = sizeof(_type *), \ >> .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \ >> .offset = vmstate_offset_array(_s, _f, _type*, _n), \ >> + .info = &vmstate_info_nullptr, \ > > What's this change for? It's garbage. A remainder of a previous version I mentioned above. Will remove. Good catch! > >> } >> >> #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \ >> diff --git a/migration/vmstate.c b/migration/vmstate.c >> index 10a7645..ce3490a 100644 >> --- a/migration/vmstate.c >> +++ b/migration/vmstate.c >> @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, >> if (field->flags & VMS_ARRAY_OF_POINTER) { >> curr_elem = *(void **)curr_elem; >> } >> - if (field->flags & VMS_STRUCT) { >> + if (!curr_elem) { >> + /* if null pointer check placeholder and do not follow */ >> + assert(field->flags & VMS_ARRAY_OF_POINTER); >> + vmstate_info_nullptr.get(f, curr_elem, size); >> + } else if (field->flags & VMS_STRUCT) { >> ret = vmstate_load_state(f, field->vmsd, curr_elem, >> field->vmsd->version_id); >> } else { >> @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, >> assert(curr_elem); >> curr_elem = *(void **)curr_elem; >> } >> - if (field->flags & VMS_STRUCT) { >> + if (!curr_elem) { >> + /* if null pointer write placeholder and do not follow */ >> + assert(field->flags & VMS_ARRAY_OF_POINTER); >> + vmstate_info_nullptr.put(f, curr_elem, size); >> + } else if (field->flags & VMS_STRUCT) { >> vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop); >> } else { >> field->info->put(f, curr_elem, size); >> @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = { >> .put = put_uint64, >> }; >> >> +static int get_nullptr(QEMUFile *f, void *pv, size_t size) >> +{ >> + int8_t tmp; >> + qemu_get_s8s(f, &tmp); > > Now that I've suggested using just a character you can turn that > into: > > return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL; > You are right! I was over-complicating. Will do. >> + return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL; >> +} >> + >> +static void put_nullptr(QEMUFile *f, void *pv, size_t size) >> +{ >> + int8_t tmp = VMS_NULLPTR_MARKER; >> + assert(pv == NULL); >> + qemu_put_s8s(f, &tmp); > > and similarly put_byte. > Will do. > Dave > Thanks for the review. I have already adopted mostly your proposals, but I will wait a bit in the hope that the fate of this patch borrowed form my other patch set (subject:"[PATCH v2 2/2] migration: drop unused VMStateField.start" gets resolved. By the way you could answer my question regarding that yourself, since you became co-maintainer for migration starting 2017-01-24. Congrats :). Regards, Halil >> +} >> + >> +const VMStateInfo vmstate_info_nullptr = { >> + .name = "uint64", >> + .get = get_nullptr, >> + .put = put_nullptr, >> +}; >> + >> /* 64 bit unsigned int. See that the received value is the same than the one >> in the field */ >> >> -- >> 2.8.4 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 12/15/2016 01:33 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward > >> for trying to migrate an array with some null pointers in it was an > >> illegal memory access, that is a swift and painless death of the process. > >> Let's make vmstate cope with this scenario at least for pointers to > >> structs. The general approach is when we encounter a null pointer > >> (element) instead of following the pointer to save/load the data behind > >> it we save/load a placeholder. This way we can detect if we expected a > >> null pointer at the load side but not null data was saved instead. Sadly > >> all other error scenarios are not detected by this scheme (and would > >> require the usage of the JSON meta data). > >> > >> Limitations: Does not work for pointers to primitives. > > > > Please document that limitation in a comment in the vmstate.h near the > > macros that use it. > > > > After looking at this again I do not know what was my intention whit the > limitations statement. It was probably about arrays of pointers to > primitives, but then the statement is wrong. The same scheme does work > for primitives too. It just that I had a previous version which had this > limitation. > > It could also be about just pointer (not array of pointers), but the > commit message does not mention this case at all. > > I will just drop the limitations statement. > > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by: > >> Guenther Hutzl <hutzl@linux.vnet.ibm.com> --- > >> > >> We will need this to load/save some on demand created state from within > >> the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an > >> example). > >> --- > >> include/migration/vmstate.h | 5 +++++ > >> migration/vmstate.c | 32 ++++++++++++++++++++++++++++++-- > >> 2 files changed, 35 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > >> index 5940db0..86d4aca 100644 > >> --- a/include/migration/vmstate.h > >> +++ b/include/migration/vmstate.h > >> @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16; > >> extern const VMStateInfo vmstate_info_uint32; > >> extern const VMStateInfo vmstate_info_uint64; > >> > >> +/** Put this in the stream when migrating a null pointer.*/ > >> +#define VMS_NULLPTR_MARKER ((int8_t) -1) > > > > You seem to be making things harder by using a signed int everywhere > > when you just want a byte; I suggest using '0' > > > > I'm fine with byte 0x30, but I have an irrational bad feeling about > using a character literal here. I know, in practice should > not matter, but the C11 standard says nothing about how members of the > basic execution character set are represented, and it's also not among > the 'usual QEMU platform assumptions' (AFAIK). Are you OK defining > the constant/macro as Ox30U instead of '0'? 0x30 is fine if you prefer (although I'd be pretty surprised if we worked on a non-ASCII system!); My only requirements are it's just a plain normal unsigned char and it's not the type of value that's common (like 0x00). > >> +extern const VMStateInfo vmstate_info_nullptr; > >> + > >> extern const VMStateInfo vmstate_info_float64; > >> extern const VMStateInfo vmstate_info_cpudouble; > >> > >> @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap; > >> .size = sizeof(_type *), \ > >> .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \ > >> .offset = vmstate_offset_array(_s, _f, _type*, _n), \ > >> + .info = &vmstate_info_nullptr, \ > > > > What's this change for? > > It's garbage. A remainder of a previous version I mentioned above. Will > remove. Good catch! > > > > >> } > >> > >> #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \ > >> diff --git a/migration/vmstate.c b/migration/vmstate.c > >> index 10a7645..ce3490a 100644 > >> --- a/migration/vmstate.c > >> +++ b/migration/vmstate.c > >> @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > >> if (field->flags & VMS_ARRAY_OF_POINTER) { > >> curr_elem = *(void **)curr_elem; > >> } > >> - if (field->flags & VMS_STRUCT) { > >> + if (!curr_elem) { > >> + /* if null pointer check placeholder and do not follow */ > >> + assert(field->flags & VMS_ARRAY_OF_POINTER); > >> + vmstate_info_nullptr.get(f, curr_elem, size); > >> + } else if (field->flags & VMS_STRUCT) { > >> ret = vmstate_load_state(f, field->vmsd, curr_elem, > >> field->vmsd->version_id); > >> } else { > >> @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, > >> assert(curr_elem); > >> curr_elem = *(void **)curr_elem; > >> } > >> - if (field->flags & VMS_STRUCT) { > >> + if (!curr_elem) { > >> + /* if null pointer write placeholder and do not follow */ > >> + assert(field->flags & VMS_ARRAY_OF_POINTER); > >> + vmstate_info_nullptr.put(f, curr_elem, size); > >> + } else if (field->flags & VMS_STRUCT) { > >> vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop); > >> } else { > >> field->info->put(f, curr_elem, size); > >> @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = { > >> .put = put_uint64, > >> }; > >> > >> +static int get_nullptr(QEMUFile *f, void *pv, size_t size) > >> +{ > >> + int8_t tmp; > >> + qemu_get_s8s(f, &tmp); > > > > Now that I've suggested using just a character you can turn that > > into: > > > > return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL; > > > > You are right! I was over-complicating. Will do. > > >> + return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL; > >> +} > >> + > >> +static void put_nullptr(QEMUFile *f, void *pv, size_t size) > >> +{ > >> + int8_t tmp = VMS_NULLPTR_MARKER; > >> + assert(pv == NULL); > >> + qemu_put_s8s(f, &tmp); > > > > and similarly put_byte. > > > > Will do. > > > Dave > > > > Thanks for the review. I have already adopted mostly your proposals, > but I will wait a bit in the hope that the fate of this patch borrowed > form my other patch set (subject:"[PATCH v2 2/2] migration: drop unused > VMStateField.start" gets resolved. Yep, I'll look at that in a minute. Dave > By the way you could answer my question regarding that yourself, since > you became co-maintainer for migration starting 2017-01-24. Congrats :). > > > Regards, > Halil > > >> +} > >> + > >> +const VMStateInfo vmstate_info_nullptr = { > >> + .name = "uint64", > >> + .get = get_nullptr, > >> + .put = put_nullptr, > >> +}; > >> + > >> /* 64 bit unsigned int. See that the received value is the same than the one > >> in the field */ > >> > >> -- > >> 2.8.4 > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 5940db0..86d4aca 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16; extern const VMStateInfo vmstate_info_uint32; extern const VMStateInfo vmstate_info_uint64; +/** Put this in the stream when migrating a null pointer.*/ +#define VMS_NULLPTR_MARKER ((int8_t) -1) +extern const VMStateInfo vmstate_info_nullptr; + extern const VMStateInfo vmstate_info_float64; extern const VMStateInfo vmstate_info_cpudouble; @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap; .size = sizeof(_type *), \ .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \ .offset = vmstate_offset_array(_s, _f, _type*, _n), \ + .info = &vmstate_info_nullptr, \ } #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \ diff --git a/migration/vmstate.c b/migration/vmstate.c index 10a7645..ce3490a 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, if (field->flags & VMS_ARRAY_OF_POINTER) { curr_elem = *(void **)curr_elem; } - if (field->flags & VMS_STRUCT) { + if (!curr_elem) { + /* if null pointer check placeholder and do not follow */ + assert(field->flags & VMS_ARRAY_OF_POINTER); + vmstate_info_nullptr.get(f, curr_elem, size); + } else if (field->flags & VMS_STRUCT) { ret = vmstate_load_state(f, field->vmsd, curr_elem, field->vmsd->version_id); } else { @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, assert(curr_elem); curr_elem = *(void **)curr_elem; } - if (field->flags & VMS_STRUCT) { + if (!curr_elem) { + /* if null pointer write placeholder and do not follow */ + assert(field->flags & VMS_ARRAY_OF_POINTER); + vmstate_info_nullptr.put(f, curr_elem, size); + } else if (field->flags & VMS_STRUCT) { vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop); } else { field->info->put(f, curr_elem, size); @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = { .put = put_uint64, }; +static int get_nullptr(QEMUFile *f, void *pv, size_t size) +{ + int8_t tmp; + qemu_get_s8s(f, &tmp); + return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL; +} + +static void put_nullptr(QEMUFile *f, void *pv, size_t size) +{ + int8_t tmp = VMS_NULLPTR_MARKER; + assert(pv == NULL); + qemu_put_s8s(f, &tmp); +} + +const VMStateInfo vmstate_info_nullptr = { + .name = "uint64", + .get = get_nullptr, + .put = put_nullptr, +}; + /* 64 bit unsigned int. See that the received value is the same than the one in the field */
Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward for trying to migrate an array with some null pointers in it was an illegal memory access, that is a swift and painless death of the process. Let's make vmstate cope with this scenario at least for pointers to structs. The general approach is when we encounter a null pointer (element) instead of following the pointer to save/load the data behind it we save/load a placeholder. This way we can detect if we expected a null pointer at the load side but not null data was saved instead. Sadly all other error scenarios are not detected by this scheme (and would require the usage of the JSON meta data). Limitations: Does not work for pointers to primitives. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com> --- We will need this to load/save some on demand created state from within the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an example). --- include/migration/vmstate.h | 5 +++++ migration/vmstate.c | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-)