[RFC,3/4] migration/vmstate: fix array of pointers to struct

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.

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(-)

* 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