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

[Dr. David Alan Gilbert]

* Halil Pasic (address@hidden) 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 <address@hidden>
> Reviewed-by: Guenther Hutzl <address@hidden>
> ---
> 
> 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 / address@hidden / Manchester, UK