| Message ID | 20161021143741.8597-4-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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
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
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
* 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
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 */