[libvirt] [PATCH] qemu_driver: don't leak a virDomainDeviceDef buffer

[Matthias Bolte]

2010/1/18 Jim Meyering <jim at meyering.net>:
> At first I was going to call virDomainDeviceDefFree only "if (dev)",
> but saw that it handles a NULL "dev" just fine, so it's better to skip
> the test altogether, just as we do for many other free-like functions.
>
> >From ea8511d709492f5cdc152a1eaccbccd05f036648 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Mon, 18 Jan 2010 16:55:36 +0100
> Subject: [PATCH] qemu_driver: don't leak a virDomainDeviceDef buffer
>
> * src/qemu/qemu_driver.c (qemudDomainAttachDevice): Don't leak "dev".
> ---
>  src/qemu/qemu_driver.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 365921f..1aa8af6 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6062,11 +6062,11 @@ cleanup:
>     if (cgroup)
>         virCgroupFree(&cgroup);
>
> -    if (ret < 0 && dev != NULL) {
> +    if (ret < 0 && dev != NULL)
>         if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0)
>             VIR_WARN0("Fail to restore disk device ownership");
> -        virDomainDeviceDefFree(dev);
> -    }
> +    virDomainDeviceDefFree(dev);
> +
>     if (vm)
>         virDomainObjUnlock(vm);
>     qemuDriverUnlock(driver);
> --
> 1.6.6.638.g2bc54
>
>

NACK. This will probably result in a segfault because you are freeing
memory that is still in use.

Yes the toplevel dev leaks here, but for example
qemudDomainAttachNetDevice some lines above takes parts from the dev
struct an assigns them to other structs _without_ copying them. I
found this leak some time ago too, but gave up on fixing it as I
noticed how entangled this code is.

Matthias