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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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