[Qemu-devel] [RFC PATCH 3/4] migration/vmstate: fix array of pointers to struct

[Halil Pasic]

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 <address@hidden>
Reviewed-by: Guenther Hutzl <address@hidden>
---

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

I'm not sure about some asserts I introduced. There may be a better way
to handle these conditions (like returning an error code in load for
example).
---
 include/migration/vmstate.h |  2 +
 migration/vmstate.c         | 91 ++++++++++++++++++++++++++++-----------------
 2 files changed, 59 insertions(+), 34 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1638ee5..1e0c71c 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -236,6 +236,7 @@ extern const VMStateInfo vmstate_info_uint8;
 extern const VMStateInfo vmstate_info_uint16;
 extern const VMStateInfo vmstate_info_uint32;
 extern const VMStateInfo vmstate_info_uint64;
+extern const VMStateInfo vmstate_info_nullptr;
 
 extern const VMStateInfo vmstate_info_float64;
 extern const VMStateInfo vmstate_info_cpudouble;
@@ -454,6 +455,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 0bc9f35..1e65a93 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -46,33 +46,18 @@ static int vmstate_size(void *opaque, VMStateField *field)
             size *= field->size;
         }
     }
-
     return size;
 }
 
-static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
+static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque)
 {
-    void *base_addr = opaque + field->offset;
-
-    if (field->flags & VMS_POINTER) {
-        if (alloc && (field->flags & VMS_ALLOC)) {
-            gsize size = 0;
-            if (field->flags & VMS_VBUFFER) {
-                size = vmstate_size(opaque, field);
-            } else {
-                int n_elems = vmstate_n_elems(opaque, field);
-                if (n_elems) {
-                    size = n_elems * field->size;
-                }
-            }
-            if (size) {
-                *((void **)base_addr + field->start) = g_malloc(size);
-            }
+    if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) {
+        gsize size = vmstate_size(opaque, field);
+        size *= vmstate_n_elems(opaque, field);
+        if (size) {
+            *(void **)ptr = g_malloc(size);
         }
-        base_addr = *(void **)base_addr + field->start;
     }
-
-    return base_addr;
 }
 
 int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
@@ -108,21 +93,30 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
              field->field_exists(opaque, version_id)) ||
             (!field->field_exists &&
              field->version_id <= version_id)) {
-            void *base_addr = vmstate_base_addr(opaque, field, true);
+            void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
 
+            vmstate_handle_alloc(first_elem, field, opaque);
+            if (field->flags & VMS_POINTER) {
+                first_elem = *(void **)first_elem;
+                assert(first_elem);
+            }
             for (i = 0; i < n_elems; i++) {
-                void *addr = base_addr + size * i;
+                void *curr_elem = first_elem + size * i;
 
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
-                    addr = *(void **)addr;
+                    curr_elem = *(void **)curr_elem;
                 }
-                if (field->flags & VMS_STRUCT) {
-                    ret = vmstate_load_state(f, field->vmsd, addr,
+                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 {
-                    ret = field->info->get(f, addr, size);
+                    ret = field->info->get(f, curr_elem, size);
 
                 }
                 if (ret >= 0) {
@@ -312,25 +306,33 @@ void vmstate_save_state(QEMUFile *f, const 
VMStateDescription *vmsd,
     while (field->name) {
         if (!field->field_exists ||
             field->field_exists(opaque, vmsd->version_id)) {
-            void *base_addr = vmstate_base_addr(opaque, field, false);
+            void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
             int64_t old_offset, written_bytes;
             QJSON *vmdesc_loop = vmdesc;
 
+            if (field->flags & VMS_POINTER) {
+                first_elem = *(void **)first_elem;
+                assert(first_elem);
+            }
             for (i = 0; i < n_elems; i++) {
-                void *addr = base_addr + size * i;
+                void *curr_elem = first_elem + size * i;
 
                 vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
                 old_offset = qemu_ftell_fast(f);
-
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
-                    addr = *(void **)addr;
+                    assert(curr_elem);
+                    curr_elem = *(void **)curr_elem;
                 }
-                if (field->flags & VMS_STRUCT) {
-                    vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
+                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, addr, size);
+                    field->info->put(f, curr_elem, size);
                 }
 
                 written_bytes = qemu_ftell_fast(f) - old_offset;
@@ -720,6 +722,27 @@ 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);
+    assert(tmp == 0);
+    return 0;
+}
+
+static void put_nullptr(QEMUFile *f, void *pv, size_t size)
+{
+    int8_t tmp = 0;
+    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 */
 
-- 
2.8.4