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

[Jiri Denemark]

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.

Jirka