Re: [PATCH v3] vmstate: validate VMStateDescription::fields upon registration

[Fabiano Rosas <farosas@suse.de>]

Roman Kiryanov <rkir@google.com> writes:

> The vmstate_save_state_v() function does not support
> NULL in VMStateDescription::fields and will crash if
> one is provided.
>
> This change prevents this situation from happening
> by explicitly crashing earlier.
>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Roman Kiryanov <rkir@google.com>
> ---
> v3:
>  - Also added assert to virtio.c to validate
>    VirtioDeviceClass instances which bypass
>    vmstate_register_with_alias_id.
>  - Updated the commit message to "Reviewed-by".
> v2:
>  - Moved the assert from vmstate_save_state_v to the registration
>    path (vmstate_register_with_alias_id) and the qtest validation path
>    (vmstate_check) to catch missing fields earlier.
> ---
>  hw/virtio/virtio.c | 6 ++++++
>  migration/savevm.c | 5 +++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0ba734d0bc..e4543de335 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
>      /* Devices should either use vmsd or the load/save methods */
>      assert(!vdc->vmsd || !vdc->load);
>  
> +    /*
> +     * If a device has vmsd, it either MUST have valid fields
> +     * or marked unmigratable.
> +     */
> +    assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields));
> +
>      if (vdc->realize != NULL) {
>          vdc->realize(dev, &err);
>          if (err != NULL) {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 197c89e0e6..20c2b99231 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription *vmsd)
>      const VMStateField *field = vmsd->fields;
>      const VMStateDescription * const *subsection = vmsd->subsections;
>  
> +    assert(field || vmsd->unmigratable);
> +
>      if (field) {
>          while (field->name) {
>              if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
>      /* If this triggers, alias support can be dropped for the vmsd. */
>      assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
>  
> +    /* If vmsd is migratable it MUST have valid fields. */
> +    assert(vmsd->fields || vmsd->unmigratable);
> +
>      se = g_new0(SaveStateEntry, 1);
>      se->version_id = vmsd->version_id;
>      se->section_id = savevm_state.global_section_id++;

Hi, this patch missed the train. It's crashing the
./scripts/device-crash-test

The vmstate_virtio_snd_device needs to be fixed first.

---
Some thoughts, no action needed:

I'm cautions after the issue with the stubs. We should expect that a
stub with all fields zeroed could still reach the vmstate code. Maybe
it'll be fine with the (eventual) fix for mips, but we need to test it
thoroughly.

There is also the -dump-vmstate command line option, which can certainly
process vmsds with incorrect fields as it takes them directly from the
device class. It doesn't crash, but prints stuff like:

"Fields": [{
    "field": "cpuhp_state",
    "version_id": 1,
    "field_exists": false,
    "size": 304,
    "Description": {
        "name": "(null)",  <---
        "version_id": 0,
        "minimum_version_id": 0
    }}]

Perhaps it should even do a validation pass and warn or add a comment to
the output.