Re: [libvirt] [PATCH] qemu: fix libvirtd crash in migration after vm shutdown

On Tue, Aug 02, 2016 at 02:20:51 +0000, weifuqiang wrote: > [PATCH] qemu: fix libvirtd crash in migration after vm shutdown > > > > > > If we shutdown a guest, then migrate it without the arg XML, libvirtd will get crashed. > > > > The reason is that: > > 1 during shutdown callback, qemuProcessStop() , it points vm->def to vm->newDef > > 2 during migration, it frees persistentDef, which points to vm->newDef when the arg XML is NULL. > > However, because vm->newDef is now vm->def, what we IN FACT freed is vm->def. > > 3 it will refer to vm->def after step2, thus invalid read/write causes libvirtd crash > > > > We needn't to free persistentDef if persist_xml is NULL, because no extra def was alloced if persistent_xml is NULL. > > > --- > src/qemu/qemu_migration.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 6a683f7..3636c93 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -4915,7 +4915,7 @@ qemuMigrationRun(virQEMUDriverPtr driver, > VIR_WARN("Unable to encode migration cookie"); > } > - if (persistDef != vm->newDef) > + if (persist_xml && persistDef) > virDomainDefFree(persistDef); > qemuMigrationCookieFree(mig); If persist_xml != NULL then persistDef is it's parsed version (or NULL) and we need to free it. Otherwise it's just a copy of another pointer. In other words, your patch is correct (although checking for persist_xml is enough). An alternative solution would be to make sure we can always free persistDef by making a copy of vm->newDef rather than copying just the pointer and free it unconditionally. Anyway, your patch is good enough. ACK. I'll push it later.