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.
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.
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 in the
(s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
an example).
---
include/migration/vmstate.h | 4 ++++
migration/vmstate.c | 40 ++++++++++++++++++++++++++++++++++++++--
2 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 63e7b02..f2dbf84 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -253,6 +253,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 (0x30U) /* '0' */
+extern const VMStateInfo vmstate_info_nullptr;
+
extern const VMStateInfo vmstate_info_float64;
extern const VMStateInfo vmstate_info_cpudouble;
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 836a7a4..78b3cd4 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -117,7 +117,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);
+ ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
+ } else if (field->flags & VMS_STRUCT) {
ret = vmstate_load_state(f, field->vmsd, curr_elem,
field->vmsd->version_id);
} else {
@@ -332,7 +336,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, NULL, NULL);
+ } else if (field->flags & VMS_STRUCT) {
vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
} else {
field->info->put(f, curr_elem, size, field, vmdesc_loop);
@@ -747,6 +755,34 @@ const VMStateInfo vmstate_info_uint64 = {
.put = put_uint64,
};
+static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
+
+{
+ if (qemu_get_byte(f) == VMS_NULLPTR_MARKER) {
+ return 0;
+ }
+ error_report("vmstate: get_nullptr expected VMS_NULLPTR_MARKER");
+ return -EINVAL;
+}
+
+static int put_nullptr(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
+
+{
+ if (pv == NULL) {
+ qemu_put_byte(f, VMS_NULLPTR_MARKER);
+ return 0;
+ }
+ error_report("vmstate: put_nullptr must be called with pv == NULL");
+ return -EINVAL;
+}
+
+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
* 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.
>
> 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.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> We will need this to load/save some on demand created state in the
> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
> an example).
> ---
> include/migration/vmstate.h | 4 ++++
> migration/vmstate.c | 40 ++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 63e7b02..f2dbf84 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -253,6 +253,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 (0x30U) /* '0' */
> +extern const VMStateInfo vmstate_info_nullptr;
> +
> extern const VMStateInfo vmstate_info_float64;
> extern const VMStateInfo vmstate_info_cpudouble;
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 836a7a4..78b3cd4 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -117,7 +117,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);
> + ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
> + } else if (field->flags & VMS_STRUCT) {
> ret = vmstate_load_state(f, field->vmsd, curr_elem,
> field->vmsd->version_id);
> } else {
> @@ -332,7 +336,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, NULL, NULL);
> + } else if (field->flags & VMS_STRUCT) {
> vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
> } else {
> field->info->put(f, curr_elem, size, field, vmdesc_loop);
> @@ -747,6 +755,34 @@ const VMStateInfo vmstate_info_uint64 = {
> .put = put_uint64,
> };
>
> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> +
> +{
> + if (qemu_get_byte(f) == VMS_NULLPTR_MARKER) {
> + return 0;
> + }
> + error_report("vmstate: get_nullptr expected VMS_NULLPTR_MARKER");
> + return -EINVAL;
> +}
> +
> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
> + VMStateField *field, QJSON *vmdesc)
> +
> +{
> + if (pv == NULL) {
> + qemu_put_byte(f, VMS_NULLPTR_MARKER);
> + return 0;
> + }
> + error_report("vmstate: put_nullptr must be called with pv == NULL");
> + return -EINVAL;
> +}
> +
> +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 02/24/2017 01:29 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. >> >> 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. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Thanks! Seems there are no further objections. Is the series going in via your tree (softfreeze starting 28.02, me worried)? Regards, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 02/24/2017 01:29 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. > >> > >> 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. > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com> > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > Thanks! Seems there are no further objections. Is the series going in > via your tree (softfreeze starting 28.02, me worried)? I'm ok with that; and there will be a migration pull tomorrow. So lets see what we can do. Dave > > Regards, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
© 2016 - 2026 Red Hat, Inc.