diff mbox

[v4,26/30] savevm: fix potential segfault on invalid state

Message ID 1396275242-10810-27-git-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin March 31, 2014, 2:17 p.m. UTC 
savevm will segfault if version_id < vmsd->minimum_version_id &&
version_id >= vmsd->minimum_version_id_old

This calls through a NULL pointer.  This is a bug (should
exit not crash).

Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 vmstate.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Peter Maydell March 31, 2014, 4:04 p.m. UTC | #1 
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> savevm will segfault if version_id < vmsd->minimum_version_id &&
> version_id >= vmsd->minimum_version_id_old

...and the vmstate has no load_state_old handler.

> This calls through a NULL pointer.  This is a bug (should
> exit not crash).

I'd previously assumed that this was a vmstate description
bug if it happened (ie that a vmstate with
minimum_version_id_old < minimum_version_id but no
load_state_old wasn't allowed).

Rather than failing migration here, wouldn't it be better
to either:
 (a) diagnose the bug, by asserting at the earliest
     opportunity
 (b) define that the value of minimum_version_id_old is not
     relevant unless load_state_old is set

I would strongly prefer (b) -- this would allow us to
remove the now-pointless setting of minimum_version_id_old
in huge numbers of vmstate structures. (Only five devices
make use of load_state_old: acpi, apic, i440fx, pit and
the ppc cpu).

thanks
-- PMM
diff mbox

Patch

diff --git a/vmstate.c b/vmstate.c
index e1e9cae..5451fd2 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -67,6 +67,9 @@  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         return -EINVAL;
     }
     if  (version_id < vmsd->minimum_version_id) {
+        if (!vmsd->load_state_old) {
+            return -EINVAL;
+        }
         return vmsd->load_state_old(f, opaque, version_id);
     }
     if (vmsd->pre_load) {