[libvirt] [PATCH] snapshot: fix double free issue for qemuImgBinary value in qemu_driver
*src/qemu/qemu_driver.c: in qemuDomainSnapshotForEachQcow() it will free up the memory of qemu_driver->qemuImgBinary in the cleanup tag which leads to the garbage value of qemuImgBinary and libvirtd crash when running "virsh snapshot-create" command next time. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b94d1c4..bbe113f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1735,7 +1735,7 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver, ret = skipped ? 1 : 0; cleanup: - VIR_FREE(qemuimgarg[0]); + qemuimgarg[0] = NULL; return ret; } -- 1.7.1
*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(-) 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); - goto cleanup; + return -1; } qemuimgarg[4] = vm->def->disks[i]->src; @@ -1727,16 +1726,12 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver, skipped = true; continue; } - goto cleanup; + return -1; } } } - ret = skipped ? 1 : 0; - -cleanup: - VIR_FREE(qemuimgarg[0]); - return ret; + return skipped ? 1 : 0; } /* Discard one snapshot (or its metadata), without reparenting any children. */ -- 1.7.1
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Guannan Ren