* 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?
> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hu...@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 / dgilb...@redhat.com / Manchester, UK
