Re: [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct

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

* 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.

Please document that limitation in a comment in the vmstate.h near the
macros that use it.

> 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).
> ---
>  include/migration/vmstate.h |  5 +++++
>  migration/vmstate.c         | 32 ++++++++++++++++++++++++++++++--
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 5940db0..86d4aca 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16;
>  extern const VMStateInfo vmstate_info_uint32;
>  extern const VMStateInfo vmstate_info_uint64;
>  
> +/** Put this in the stream when migrating a null pointer.*/
> +#define VMS_NULLPTR_MARKER ((int8_t) -1)

You seem to be making things harder by using a signed int everywhere
when you just want a byte;  I suggest using '0'

> +extern const VMStateInfo vmstate_info_nullptr;
> +
>  extern const VMStateInfo vmstate_info_float64;
>  extern const VMStateInfo vmstate_info_cpudouble;
>  
> @@ -453,6 +457,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,                              \

What's this change for?

>  }
>  
>  #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 10a7645..ce3490a 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>                      curr_elem = *(void **)curr_elem;
>                  }
> -                if (field->flags & VMS_STRUCT) {
> +                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 {
> @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                      assert(curr_elem);
>                      curr_elem = *(void **)curr_elem;
>                  }
> -                if (field->flags & VMS_STRUCT) {
> +                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, curr_elem, size);
> @@ -708,6 +716,26 @@ 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);

Now that I've suggested using just a character you can turn that
into:

  return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;

> +    return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> +}
> +
> +static void put_nullptr(QEMUFile *f, void *pv, size_t size)
> +{
> +    int8_t tmp = VMS_NULLPTR_MARKER;
> +    assert(pv == NULL);
> +    qemu_put_s8s(f, &tmp);

and similarly put_byte.

Dave

> +}
> +
> +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 */
>  
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK