[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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"),

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;
                                 _("Disk device '%s' does not support"
                                   " snapshotting"),
-                                vm->def->disks[i]->info.alias);
+                                vm->def->disks[i]->dst);
                 return -1;

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]