* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>
>
> On 10/25/2016 12:13 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@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 / dgilb...@redhat.com / Manchester, UK
