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++;

Looks like we'll also need to fix some stubs that define empty vmsds:

qemu-system-mips64: ../migration/savevm.c:864: void vmstate_check(const
VMStateDescription *): Assertion `field || vmsd->unmigratable' failed.

Maybe something like the following. Peter, what do you think?

-- >8 --
From 9bd63109e970ff231eb321e627f622910f9977ca Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Mon, 16 Mar 2026 11:16:22 -0300
Subject: [PATCH] fixup! vmstate: validate VMStateDescription::fields upon
 registration

---
 hw/acpi/acpi-cpu-hotplug-stub.c | 4 +++-
 hw/acpi/acpi-mem-hotplug-stub.c | 4 +++-
 hw/acpi/acpi-pci-hotplug-stub.c | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
index 72c5f05f5c..4332f3fb7d 100644
--- a/hw/acpi/acpi-cpu-hotplug-stub.c
+++ b/hw/acpi/acpi-cpu-hotplug-stub.c
@@ -3,7 +3,9 @@
 #include "hw/acpi/cpu.h"
 
 /* Following stubs are all related to ACPI cpu hotplug */
-const VMStateDescription vmstate_cpu_hotplug;
+const VMStateDescription vmstate_cpu_hotplug = {
+    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
+};
 
 void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
                          CPUHotplugState *state, hwaddr base_addr)
diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c
index 7ad0fdcdf2..36c12a4e5f 100644
--- a/hw/acpi/acpi-mem-hotplug-stub.c
+++ b/hw/acpi/acpi-mem-hotplug-stub.c
@@ -2,7 +2,9 @@
 #include "hw/acpi/memory_hotplug.h"
 #include "migration/vmstate.h"
 
-const VMStateDescription vmstate_memory_hotplug;
+const VMStateDescription vmstate_memory_hotplug = {
+    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
+};
 
 void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
                               MemHotplugState *state, hwaddr io_base)
diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
index d58ea726a8..3e3a484d3e 100644
--- a/hw/acpi/acpi-pci-hotplug-stub.c
+++ b/hw/acpi/acpi-pci-hotplug-stub.c
@@ -2,7 +2,9 @@
 #include "hw/acpi/pcihp.h"
 #include "migration/vmstate.h"
 
-const VMStateDescription vmstate_acpi_pcihp_pci_status;
+const VMStateDescription vmstate_acpi_pcihp_pci_status = {
+    .fields = (const VMStateField[]) { VMSTATE_END_OF_LIST() },
+};
 
 void acpi_pcihp_init(Object *owner, AcpiPciHpState *s,
                      MemoryRegion *address_space_io, uint16_t io_base)
-- 
2.51.0