[Qemu-devel] [RFC PATCH 0/4] VMS_ARRAY_OF_POINTER with null pointers

[Qemu-devel] [RFC PATCH 0/4] VMS_ARRAY_OF_POINTER with null pointers

[Halil Pasic]

Make VMS_ARRAY_OF_POINTER cope with null pointers. Currently the reward
for trying to migrate an array with some null pointers in it is 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.

We need this functionality for the migration of the channel subsystem
(hw/s390x/css.c).

Halil Pasic (4):
  tests/test-vmstate.c: add save_buffer util func
  tests/test-vmstate.c: add array of pointer to struct
  migration/vmstate: fix array of pointers to struct
  tests/test-vmstate.c: add array of pointers to struct with NULL

 include/migration/vmstate.h |   2 +
 migration/vmstate.c         |  91 ++++++++++++++++++-----------
 tests/test-vmstate.c        | 138 ++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 185 insertions(+), 46 deletions(-)

-- 
2.8.4

[Qemu-devel] [RFC PATCH 1/4] tests/test-vmstate.c: add save_buffer util func

[Halil Pasic]

Let us de-duplicate some code by introducing an utility function for
saving a chunk of bytes (used when testing load based on wire).

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
 tests/test-vmstate.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index d8da26f..d513dc6 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -83,6 +83,13 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)
     qemu_fclose(f);
 }
 
+static void save_buffer(const uint8_t *buf, size_t buf_size)
+{
+    QEMUFile *fsave = open_test_file(true);
+    qemu_put_buffer(fsave, buf, buf_size);
+    qemu_fclose(fsave);
+}
+
 static void compare_vmstate(uint8_t *wire, size_t size)
 {
     QEMUFile *f = open_test_file(false);
@@ -309,15 +316,13 @@ static const VMStateDescription vmstate_versioned = {
 
 static void test_load_v1(void)
 {
-    QEMUFile *fsave = open_test_file(true);
     uint8_t buf[] = {
         0, 0, 0, 10,             /* a */
         0, 0, 0, 30,             /* c */
         0, 0, 0, 0, 0, 0, 0, 40, /* d */
         QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
     };
-    qemu_put_buffer(fsave, buf, sizeof(buf));
-    qemu_fclose(fsave);
+    save_buffer(buf, sizeof(buf));
 
     QEMUFile *loading = open_test_file(false);
     TestStruct obj = { .b = 200, .e = 500, .f = 600 };
@@ -334,7 +339,6 @@ static void test_load_v1(void)
 
 static void test_load_v2(void)
 {
-    QEMUFile *fsave = open_test_file(true);
     uint8_t buf[] = {
         0, 0, 0, 10,             /* a */
         0, 0, 0, 20,             /* b */
@@ -344,8 +348,7 @@ static void test_load_v2(void)
         0, 0, 0, 0, 0, 0, 0, 60, /* f */
         QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
     };
-    qemu_put_buffer(fsave, buf, sizeof(buf));
-    qemu_fclose(fsave);
+    save_buffer(buf, sizeof(buf));
 
     QEMUFile *loading = open_test_file(false);
     TestStruct obj;
@@ -423,7 +426,6 @@ static void test_save_skip(void)
 
 static void test_load_noskip(void)
 {
-    QEMUFile *fsave = open_test_file(true);
     uint8_t buf[] = {
         0, 0, 0, 10,             /* a */
         0, 0, 0, 20,             /* b */
@@ -433,8 +435,7 @@ static void test_load_noskip(void)
         0, 0, 0, 0, 0, 0, 0, 60, /* f */
         QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
     };
-    qemu_put_buffer(fsave, buf, sizeof(buf));
-    qemu_fclose(fsave);
+    save_buffer(buf, sizeof(buf));
 
     QEMUFile *loading = open_test_file(false);
     TestStruct obj = { .skip_c_e = false };
@@ -451,7 +452,6 @@ static void test_load_noskip(void)
 
 static void test_load_skip(void)
 {
-    QEMUFile *fsave = open_test_file(true);
     uint8_t buf[] = {
         0, 0, 0, 10,             /* a */
         0, 0, 0, 20,             /* b */
@@ -459,8 +459,7 @@ static void test_load_skip(void)
         0, 0, 0, 0, 0, 0, 0, 60, /* f */
         QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
     };
-    qemu_put_buffer(fsave, buf, sizeof(buf));
-    qemu_fclose(fsave);
+    save_buffer(buf, sizeof(buf));
 
     QEMUFile *loading = open_test_file(false);
     TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
-- 
2.8.4

[Qemu-devel] [RFC PATCH 2/4] tests/test-vmstate.c: add array of pointer to struct

[Halil Pasic]

Increase test coverage by adding tests for the macro
VMSTATE_ARRAY_OF_POINTER_TO_STRUCT.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
 tests/test-vmstate.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index d513dc6..d2f529b 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -474,6 +474,76 @@ static void test_load_skip(void)
     qemu_fclose(loading);
 }
 
+
+typedef struct {
+    int32_t i;
+} TestStructTriv;
+
+const VMStateDescription vmsd_tst = {
+    .name = "test/tst",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(i, TestStructTriv),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+#define AR_SIZE 4
+
+typedef struct {
+    TestStructTriv *ar[AR_SIZE];
+} TestArrayOfPtrToStuct;
+
+const VMStateDescription vmsd_arps = {
+    .name = "test/arps",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(ar, TestArrayOfPtrToStuct,
+                AR_SIZE, 0, vmsd_tst, TestStructTriv),
+        VMSTATE_END_OF_LIST()
+    }
+};
+static void test_arr_ptr_str_no0_save(void)
+{
+    TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+    TestArrayOfPtrToStuct sample = {.ar = {&ar[0], &ar[1], &ar[2], &ar[3]} };
+    uint8_t wire_sample[] = {
+        0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x01,
+        0x00, 0x00, 0x00, 0x02,
+        0x00, 0x00, 0x00, 0x03,
+        QEMU_VM_EOF
+    };
+
+    save_vmstate(&vmsd_arps, &sample);
+    compare_vmstate(wire_sample, sizeof(wire_sample));
+}
+
+static void test_arr_ptr_str_no0_load(void)
+{
+    TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+    TestStructTriv ar[AR_SIZE] = {};
+    TestArrayOfPtrToStuct obj = {.ar = {&ar[0], &ar[1], &ar[2], &ar[3]} };
+    int idx;
+    uint8_t wire_sample[] = {
+        0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x01,
+        0x00, 0x00, 0x00, 0x02,
+        0x00, 0x00, 0x00, 0x03,
+        QEMU_VM_EOF
+    };
+
+    save_buffer(wire_sample, sizeof(wire_sample));
+    SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1,
+                          wire_sample, sizeof(wire_sample)));
+    for (idx = 0; idx < AR_SIZE; ++idx) {
+        /* compare the target array ar with the ground truth array ar_gt */
+        g_assert_cmpint(ar_gt[idx].i, ==, ar[idx].i);
+    }
+}
+
 int main(int argc, char **argv)
 {
     temp_fd = mkstemp(temp_file);
@@ -488,6 +558,10 @@ int main(int argc, char **argv)
     g_test_add_func("/vmstate/field_exists/load/skip", test_load_skip);
     g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
     g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
+    g_test_add_func("/vmstate/array/ptr/str/no0/save",
+                    test_arr_ptr_str_no0_save);
+    g_test_add_func("/vmstate/array/ptr/str/no0/load",
+                    test_arr_ptr_str_no0_load);
     g_test_run();
 
     close(temp_fd);
-- 
2.8.4

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

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

[Qemu-devel] [RFC PATCH 4/4] tests/test-vmstate.c: add array of pointers to struct with NULL

[Halil Pasic]

Increase coverage by testing VMSTATE_ARRAY_OF_POINTER_TO_STRUCT
with an array containing some NULL pointer.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 tests/test-vmstate.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index d2f529b..051be8f 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -544,6 +544,44 @@ static void test_arr_ptr_str_no0_load(void)
     }
 }
 
+static void test_arr_ptr_str_0_save(void)
+{
+    TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+    TestArrayOfPtrToStuct sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
+
+    save_vmstate(&vmsd_arps, &sample); /* fails with SEGFAULT with master */
+}
+
+static void test_arr_ptr_str_0_load(void)
+{
+    TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 0}, {.i = 2}, {.i = 3} };
+    TestStructTriv ar[AR_SIZE] = {};
+    TestArrayOfPtrToStuct obj = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
+    int idx;
+    uint8_t wire_sample[] = {
+        0x00, 0x00, 0x00, 0x00,
+        0x00, /* marker for the null pointer */
+        0x00, 0x00, 0x00, 0x02,
+        0x00, 0x00, 0x00, 0x03,
+        QEMU_VM_EOF
+    };
+
+    save_buffer(wire_sample, sizeof(wire_sample));
+    SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1,
+                          wire_sample, sizeof(wire_sample)));
+    for (idx = 0; idx < AR_SIZE; ++idx) {
+        /* compare the target array ar with the ground truth array ar_gt */
+        g_assert_cmpint(ar_gt[idx].i, ==, ar[idx].i);
+    }
+    for (idx = 0; idx < AR_SIZE; ++idx) {
+        if (idx == 1) {
+            g_assert_cmpint((uint64_t)(obj.ar[idx]), ==, 0);
+        } else {
+            g_assert_cmpint((uint64_t)(obj.ar[idx]), !=, 0);
+        }
+    }
+}
+
 int main(int argc, char **argv)
 {
     temp_fd = mkstemp(temp_file);
@@ -562,6 +600,9 @@ int main(int argc, char **argv)
                     test_arr_ptr_str_no0_save);
     g_test_add_func("/vmstate/array/ptr/str/no0/load",
                     test_arr_ptr_str_no0_load);
+    g_test_add_func("/vmstate/array/ptr/str/0/save", test_arr_ptr_str_0_save);
+    g_test_add_func("/vmstate/array/ptr/str/0/load",
+                    test_arr_ptr_str_0_load);
     g_test_run();
 
     close(temp_fd);
-- 
2.8.4

Re: [Qemu-devel] [RFC PATCH 1/4] tests/test-vmstate.c: add save_buffer util func

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Let us de-duplicate some code by introducing an utility function for
> saving a chunk of bytes (used when testing load based on wire).
> 
> 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>

> ---
>  tests/test-vmstate.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index d8da26f..d513dc6 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -83,6 +83,13 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)
>      qemu_fclose(f);
>  }
>  
> +static void save_buffer(const uint8_t *buf, size_t buf_size)
> +{
> +    QEMUFile *fsave = open_test_file(true);
> +    qemu_put_buffer(fsave, buf, buf_size);
> +    qemu_fclose(fsave);
> +}
> +
>  static void compare_vmstate(uint8_t *wire, size_t size)
>  {
>      QEMUFile *f = open_test_file(false);
> @@ -309,15 +316,13 @@ static const VMStateDescription vmstate_versioned = {
>  
>  static void test_load_v1(void)
>  {
> -    QEMUFile *fsave = open_test_file(true);
>      uint8_t buf[] = {
>          0, 0, 0, 10,             /* a */
>          0, 0, 0, 30,             /* c */
>          0, 0, 0, 0, 0, 0, 0, 40, /* d */
>          QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
>      };
> -    qemu_put_buffer(fsave, buf, sizeof(buf));
> -    qemu_fclose(fsave);
> +    save_buffer(buf, sizeof(buf));
>  
>      QEMUFile *loading = open_test_file(false);
>      TestStruct obj = { .b = 200, .e = 500, .f = 600 };
> @@ -334,7 +339,6 @@ static void test_load_v1(void)
>  
>  static void test_load_v2(void)
>  {
> -    QEMUFile *fsave = open_test_file(true);
>      uint8_t buf[] = {
>          0, 0, 0, 10,             /* a */
>          0, 0, 0, 20,             /* b */
> @@ -344,8 +348,7 @@ static void test_load_v2(void)
>          0, 0, 0, 0, 0, 0, 0, 60, /* f */
>          QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
>      };
> -    qemu_put_buffer(fsave, buf, sizeof(buf));
> -    qemu_fclose(fsave);
> +    save_buffer(buf, sizeof(buf));
>  
>      QEMUFile *loading = open_test_file(false);
>      TestStruct obj;
> @@ -423,7 +426,6 @@ static void test_save_skip(void)
>  
>  static void test_load_noskip(void)
>  {
> -    QEMUFile *fsave = open_test_file(true);
>      uint8_t buf[] = {
>          0, 0, 0, 10,             /* a */
>          0, 0, 0, 20,             /* b */
> @@ -433,8 +435,7 @@ static void test_load_noskip(void)
>          0, 0, 0, 0, 0, 0, 0, 60, /* f */
>          QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
>      };
> -    qemu_put_buffer(fsave, buf, sizeof(buf));
> -    qemu_fclose(fsave);
> +    save_buffer(buf, sizeof(buf));
>  
>      QEMUFile *loading = open_test_file(false);
>      TestStruct obj = { .skip_c_e = false };
> @@ -451,7 +452,6 @@ static void test_load_noskip(void)
>  
>  static void test_load_skip(void)
>  {
> -    QEMUFile *fsave = open_test_file(true);
>      uint8_t buf[] = {
>          0, 0, 0, 10,             /* a */
>          0, 0, 0, 20,             /* b */
> @@ -459,8 +459,7 @@ static void test_load_skip(void)
>          0, 0, 0, 0, 0, 0, 0, 60, /* f */
>          QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
>      };
> -    qemu_put_buffer(fsave, buf, sizeof(buf));
> -    qemu_fclose(fsave);
> +    save_buffer(buf, sizeof(buf));
>  
>      QEMUFile *loading = open_test_file(false);
>      TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

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

Hmm is this needed - I mean could you do this just by giving the vmsd
that defines the children of the array a '.needed' that tests if their
pointer is NULL?


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

This diff is quite confusing because a lot of it involves the
rename of 'addr' to 'curr_elem' at the same time as you change
the structure.  It would be better to split the renaming into
a separate patch to make this clearer or just leave the name
the same.

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

There's no need for the assert there, just return -EINVAL,
then we'll get a clear error.
Also, '0' is a bad value to use just as a check - if the field is wrong then
0 often appears in the next byte anyway; 

However, I'm not sure it's worth having the info_nullptr;
if we just leave out the whole info_nullptr then you should still
be protected by the section footer, although this may be
able to give a better error.



> +    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",

That 'name' field should be updated.

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

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

[Halil Pasic]

On 10/25/2016 12:13 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.
> Hmm is this needed - I mean could you do this just by giving the vmsd
> that defines the children of the array a '.needed' that tests if their
> pointer is NULL?
> 
> 

I do not think so: .needed is basically for subsections (also used
in migration/savevm.c via the exported vmstate_save_needed function),
and .field_exists is also no use for this (AFAIU). Have also tried
just to be sure, it did not work for me. 

If I did not convince you, a bit of a code proving me wrong would be
highly appreciated.

Thanks for the comment!

Halil

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

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 10/25/2016 12:13 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.
> > Hmm is this needed - I mean could you do this just by giving the vmsd
> > that defines the children of the array a '.needed' that tests if their
> > pointer is NULL?
> > 
> > 
> 
> I do not think so: .needed is basically for subsections (also used
> in migration/savevm.c via the exported vmstate_save_needed function),
> and .field_exists is also no use for this (AFAIU). Have also tried
> just to be sure, it did not work for me. 

Hmm yes you're right; I thought .needed was more general; and
field_exists does seem to be too late.

> If I did not convince you, a bit of a code proving me wrong would be
> highly appreciated.

Well, here's some untested code (on top of your code with the test);
it seems simple (if it works!)

Dave

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 0bc9f35..6d230ef 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -328,7 +328,9 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                     addr = *(void **)addr;
                 }
                 if (field->flags & VMS_STRUCT) {
-                    vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
+                    if (vmstate_save_needed(field->vmsd, addr)) {
+                        vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
+                    }
                 } else {
                     field->info->put(f, addr, size);
                 }

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index f8e7037..97919bb 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -497,6 +497,23 @@ const VMStateDescription vmsd_tst = {
     }
 };
 
+static bool tst_null_check(void *opaque)
+{
+    fprintf(stderr, "%s: %p\n", __func__, opaque);
+    return opaque != NULL;
+}
+
+const VMStateDescription vmsd_tst_null = {
+    .name = "test/tstnull",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = tst_null_check,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(i, TestStructTriv),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 #define AR_SIZE 4
 
 typedef struct {
@@ -513,6 +530,16 @@ const VMStateDescription vmsd_arps = {
         VMSTATE_END_OF_LIST()
     }
 };
+const VMStateDescription vmsd_arps_null = {
+    .name = "test/arpsnull",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(ar, TestArrayOfPtrToStuct,
+                AR_SIZE, 0, vmsd_tst_null, TestStructTriv),
+        VMSTATE_END_OF_LIST()
+    }
+};
 static void test_arr_ptr_str_no0_save(void)
 {
     TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
@@ -557,7 +584,7 @@ static void test_arr_ptr_str_0_save(void)
     TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
     TestArrayOfPtrToStuct sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
 
-    save_vmstate(&vmsd_arps, &sample); /* fails with SEGFAULT with master */
+    save_vmstate(&vmsd_arps_null, &sample); /* fails with SEGFAULT with master */
 }
 
 static void test_arr_ptr_str_0_load(void)
@@ -568,14 +595,13 @@ static void test_arr_ptr_str_0_load(void)
     int idx;
     uint8_t wire_sample[] = {
         0x00, 0x00, 0x00, 0x00,
-        0x00, /* marker for the null pointer */
         0x00, 0x00, 0x00, 0x02,
         0x00, 0x00, 0x00, 0x03,
         QEMU_VM_EOF
     };
 
     save_buffer(wire_sample, sizeof(wire_sample));
-    SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1,
+    SUCCESS(load_vmstate_one(&vmsd_arps_null, &obj, 1,
                           wire_sample, sizeof(wire_sample)));
     for (idx = 0; idx < AR_SIZE; ++idx) {
         /* compare the target array ar with the ground truth array ar_gt */

> Thanks for the comment!
> 
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Halil Pasic]

On 10/25/2016 12:13 PM, Dr. David Alan Gilbert wrote:
[..]
>>              for (i = 0; i < n_elems; i++) {
>> -                void *addr = base_addr + size * i;
>> +                void *curr_elem = first_elem + size * i;
> 
> This diff is quite confusing because a lot of it involves the
> rename of 'addr' to 'curr_elem' at the same time as you change
> the structure.  It would be better to split the renaming into
> a separate patch to make this clearer or just leave the name
> the same.
> 

You are absolutely right this is a Frankestein of a cleanup
patch and the actual patch. I will split the cleanup out.

The patch is also conceptually based on my remove .start patch
it's just that I wanted to make the RFC independent so it can
be tested more easily.

[..]
>> @@ -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);
> 
> There's no need for the assert there, just return -EINVAL,
> then we'll get a clear error.

Will do.

> Also, '0' is a bad value to use just as a check - if the field is wrong then
> 0 often appears in the next byte anyway; 
> 

Absolutely right. How about -1?

> However, I'm not sure it's worth having the info_nullptr;
> if we just leave out the whole info_nullptr then you should still
> be protected by the section footer, although this may be
> able to give a better error.
> 

IMHO this can (in some cases) guard against the case we have the
same number of elements on source and on target, but at different
positions (e.g. {ptr0, NULL, NULL} and {NULL, ptr1, NULL}. The footers
should not be able to detect this.

Thank you very much for the thorough review! I will wait a bit
to see if more discussion happens, and then send out a non RFC
version with the issues addressed.

Halil

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

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 10/25/2016 12:13 PM, Dr. David Alan Gilbert wrote:
> [..]
> >>              for (i = 0; i < n_elems; i++) {
> >> -                void *addr = base_addr + size * i;
> >> +                void *curr_elem = first_elem + size * i;
> > 
> > This diff is quite confusing because a lot of it involves the
> > rename of 'addr' to 'curr_elem' at the same time as you change
> > the structure.  It would be better to split the renaming into
> > a separate patch to make this clearer or just leave the name
> > the same.
> > 
> 
> You are absolutely right this is a Frankestein of a cleanup
> patch and the actual patch. I will split the cleanup out.
> 
> The patch is also conceptually based on my remove .start patch
> it's just that I wanted to make the RFC independent so it can
> be tested more easily.
> 
> [..]
> >> @@ -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);
> > 
> > There's no need for the assert there, just return -EINVAL,
> > then we'll get a clear error.
> 
> Will do.
> 
> > Also, '0' is a bad value to use just as a check - if the field is wrong then
> > 0 often appears in the next byte anyway; 
> > 
> 
> Absolutely right. How about -1?

-1 is OK (although you could use any character - e.g. N (for Null)).

> > However, I'm not sure it's worth having the info_nullptr;
> > if we just leave out the whole info_nullptr then you should still
> > be protected by the section footer, although this may be
> > able to give a better error.
> > 
> 
> IMHO this can (in some cases) guard against the case we have the
> same number of elements on source and on target, but at different
> positions (e.g. {ptr0, NULL, NULL} and {NULL, ptr1, NULL}. The footers
> should not be able to detect this.

Yes, you're right it does give that extra protection and is worth it.

Dave

> Thank you very much for the thorough review! I will wait a bit
> to see if more discussion happens, and then send out a non RFC
> version with the issues addressed.
> 
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH 1/4] tests/test-vmstate.c: add save_buffer util func

[Juan Quintela]

Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> Let us de-duplicate some code by introducing an utility function for
> saving a chunk of bytes (used when testing load based on wire).
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Included. Trivial enough.

Re: [Qemu-devel] [RFC PATCH 2/4] tests/test-vmstate.c: add array of pointer to struct

[Juan Quintela]

Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> Increase test coverage by adding tests for the macro
> VMSTATE_ARRAY_OF_POINTER_TO_STRUCT.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Included