[libvirt] [PATCH v2] snapshot:Fix double memory free to the qemuImgBinary field in qemu_driver struct

[Eric Blake]

On 09/10/2011 11:43 PM, Guannan Ren wrote:

Your subject line is rather long; I trimmed it to:

snapshot: fix double free of qemuImgBinary

>       *src/qemu/qemu_driver.c: In qemuDomainSnapshotForEachQcow()
>            it free up the memory of qemu_driver->qemuImgBinary in the
>            cleanup tag which leads to the garbage value of qemuImgBinary
>            in qemu_driver struct and libvirtd crash when running
>            "virsh snapshot-create" command at second time.
> ---
>   src/qemu/qemu_driver.c |   13 ++++---------
>   1 files changed, 4 insertions(+), 9 deletions(-)

Thanks for finding this - I had encountered this bug last Thursday, but 
then was unable to investigate root cause.  Your patch is almost 
perfect.  I added mention of the commit id where the bug was introduced 
(3881a470 added qemu-img caching incorrectly).  The regression stems 
from the fact that I wrote my patch for qemu-img caching first, then 
wrote my patch to refactor things to add qemuDomainSnapshotForEachQcow2 
(commit 25fb3ef), but then rebased things to apply the patches in the 
opposite order, failing to notice that my rebase left behind a spurious 
VIR_FREE in the new function.

>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b94d1c4..d5a2bc0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1681,14 +1681,13 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
>                                  bool try_all)
>   {
>       const char *qemuimgarg[] = { NULL, "snapshot", NULL, NULL, NULL, NULL };
> -    int ret = -1;
>       int i;
>       bool skipped = false;
>
>       qemuimgarg[0] = qemuFindQemuImgBinary(driver);
>       if (qemuimgarg[0] == NULL) {
>           /* qemuFindQemuImgBinary set the error */
> -        goto cleanup;
> +        return -1;
>       }
>
>       qemuimgarg[2] = op;
> @@ -1715,7 +1714,7 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
>                                   _("Disk device '%s' does not support"
>                                     " snapshotting"),
>                                   vm->def->disks[i]->info.alias);

Except that it is missing one additional improvement - here, 
vm->def->disks[i]->info.alias can be NULL (I first noticed the log had a 
literal "(null)", thanks to glibc, but other platforms would crash on a 
NULL deref).  But we are guaranteed that disks[i]->dst is always 
non-NULL, and still a reasonably useful string to log.  Besides, the 
info.alias field is internal to libvirt (no way for the user to set it 
in their xml), while the dst field comes straight from the user.

I'm pushing with this squashed in:

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index d5a2bc0..321b07b 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -1706,14 +1706,14 @@ qemuDomainSnapshotForEachQcow2(struct 
qemud_driver *driver,
                       * disks in this VM may have the same snapshot name.
                       */
                      VIR_WARN("skipping snapshot action on %s",
-                             vm->def->disks[i]->info.alias);
+                             vm->def->disks[i]->dst);
                      skipped = true;
                      continue;
                  }
                  qemuReportError(VIR_ERR_OPERATION_INVALID,
                                  _("Disk device '%s' does not support"
                                    " snapshotting"),
-                                vm->def->disks[i]->info.alias);
+                                vm->def->disks[i]->dst);
                  return -1;
              }

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org