[libvirt] new NULL-dereference in qemu_driver.c
I ran clang on the very latest and it spotted this problem:
From qemu_driver.c, around line 11100,
else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */ if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } } if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0) goto endjob; } vm->state = snap->def->state; ret = 0; endjob: if (vm && qemuDomainObjEndJob(vm) == 0) vm = NULL; Note how "vm" is set to NULL. Then, it can be dereferenced both via qemuDomainSnapshotSetActive and via the vm->state = ... assignment.
On Tue, Apr 27, 2010 at 06:45:16PM +0200, Jim Meyering wrote:
I ran clang on the very latest and it spotted this problem:
From qemu_driver.c, around line 11100,
else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */
if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL;
This needs to add 'goto endjob' or possibly 'goto cleanup'
} }
if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0) goto endjob; }
vm->state = snap->def->state;
ret = 0;
endjob: if (vm && qemuDomainObjEndJob(vm) == 0) vm = NULL;
Note how "vm" is set to NULL. Then, it can be dereferenced both via qemuDomainSnapshotSetActive and via the vm->state = ... assignment.
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Tue, Apr 27, 2010 at 06:45:16PM +0200, Jim Meyering wrote:
I ran clang on the very latest and it spotted this problem:
From qemu_driver.c, around line 11100,
else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */
if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL;
This needs to add 'goto endjob' or possibly 'goto cleanup'
No point in endjob, since it does nothing when vm == NULL. Here's a tentative patch for that and another, similar problem (haven't even compiled it or run it through clang, but have to run). Will follow up tomorrow.
From c508c0228bbefe396532d16f6a8979cc219bedee Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 27 Apr 2010 22:35:32 +0200 Subject: [PATCH] qemuDomainSnapshotCreateXML: avoid NULL dereference
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): When setting "vm" to NULL, jump over vm-dereferencing code to "cleanup". (qemuDomainRevertToSnapshot): Likewise. --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2daf038..a164560 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10761,36 +10761,38 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, qemuimgarg[0], snap->def->name, vm->def->disks[i]->src); goto cleanup; } } } } else { qemuDomainObjPrivatePtr priv; int ret; if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) goto cleanup; priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorCreateSnapshot(priv->mon, def->name); qemuDomainObjExitMonitorWithDriver(driver, vm); - if (qemuDomainObjEndJob(vm) == 0) + if (qemuDomainObjEndJob(vm) == 0) { vm = NULL; + goto cleanup; + } if (ret < 0) goto cleanup; } snap->def->state = vm->state; /* FIXME: if we fail after this point, there's not a whole lot we can * do; we've successfully taken the snapshot, and we are now running * on it, so we have to go forward the best we can */ if (vm->current_snapshot) { def->parent = strdup(vm->current_snapshot->def->name); if (def->parent == NULL) { virReportOOMError(); goto cleanup; } @@ -11091,34 +11093,35 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */ if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL; + goto cleanup; } } if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0) goto endjob; } vm->state = snap->def->state; ret = 0; endjob: if (vm && qemuDomainObjEndJob(vm) == 0) vm = NULL; cleanup: if (event) -- 1.7.1.328.g9993c
On 04/27/2010 04:40 PM, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, Apr 27, 2010 at 06:45:16PM +0200, Jim Meyering wrote:
I ran clang on the very latest and it spotted this problem:
From qemu_driver.c, around line 11100,
else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */
if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL;
This needs to add 'goto endjob' or possibly 'goto cleanup'
No point in endjob, since it does nothing when vm == NULL.
Here's a tentative patch for that and another, similar problem (haven't even compiled it or run it through clang, but have to run). Will follow up tomorrow.
Yeah, this looks reasonable and is what I was going to submit. It would be good to give a test first, though. -- Chris Lalancette
Chris Lalancette wrote:
On 04/27/2010 04:40 PM, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, Apr 27, 2010 at 06:45:16PM +0200, Jim Meyering wrote:
I ran clang on the very latest and it spotted this problem:
From qemu_driver.c, around line 11100,
else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */
if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL;
This needs to add 'goto endjob' or possibly 'goto cleanup'
No point in endjob, since it does nothing when vm == NULL.
Here's a tentative patch for that and another, similar problem (haven't even compiled it or run it through clang, but have to run). Will follow up tomorrow.
Yeah, this looks reasonable and is what I was going to submit. It would be good to give a test first, though.
Can any of you easily test it? I can't right now.
On 04/28/2010 11:43 AM, Jim Meyering wrote:
Chris Lalancette wrote:
On 04/27/2010 04:40 PM, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, Apr 27, 2010 at 06:45:16PM +0200, Jim Meyering wrote:
I ran clang on the very latest and it spotted this problem:
From qemu_driver.c, around line 11100,
else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */
if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL;
This needs to add 'goto endjob' or possibly 'goto cleanup'
No point in endjob, since it does nothing when vm == NULL.
Here's a tentative patch for that and another, similar problem (haven't even compiled it or run it through clang, but have to run). Will follow up tomorrow.
Yeah, this looks reasonable and is what I was going to submit. It would be good to give a test first, though.
Can any of you easily test it? I can't right now.
Yep, this works fine with transient domains and snapshotting. ACK -- Chris Lalancette
Chris Lalancette wrote:
On 04/28/2010 11:43 AM, Jim Meyering wrote:
Chris Lalancette wrote:
On 04/27/2010 04:40 PM, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, Apr 27, 2010 at 06:45:16PM +0200, Jim Meyering wrote:
I ran clang on the very latest and it spotted this problem: >From qemu_driver.c, around line 11100,
else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */
if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL;
This needs to add 'goto endjob' or possibly 'goto cleanup'
No point in endjob, since it does nothing when vm == NULL.
Here's a tentative patch for that and another, similar problem (haven't even compiled it or run it through clang, but have to run). Will follow up tomorrow.
Yeah, this looks reasonable and is what I was going to submit. It would be good to give a test first, though.
Can any of you easily test it? I can't right now.
Yep, this works fine with transient domains and snapshotting.
Thanks for testing that, Chris. I'll rebase and push in 8 or 10 hours.
Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, Apr 27, 2010 at 06:45:16PM +0200, Jim Meyering wrote:
I ran clang on the very latest and it spotted this problem:
From qemu_driver.c, around line 11100,
else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, * then after a "loadvm" monitor command, the VM is set running * again. If the snapshot was taken offline, then after a "loadvm" * monitor command the VM is left paused. Unpausing it leads to * the memory state *before* the loadvm with the disk *after* the * loadvm, which obviously is bound to corrupt something. * Therefore we destroy the domain and set it to "off" in this case. */
if (virDomainObjIsActive(vm)) { qemudShutdownVMDaemon(driver, vm); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL;
This needs to add 'goto endjob' or possibly 'goto cleanup'
No point in endjob, since it does nothing when vm == NULL.
Here's a tentative patch for that and another, similar problem (haven't even compiled it or run it through clang, but have to run). Will follow up tomorrow. ... Subject: [PATCH] qemuDomainSnapshotCreateXML: avoid NULL dereference
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): When setting "vm" to NULL, jump over vm-dereferencing code to "cleanup". (qemuDomainRevertToSnapshot): Likewise.
Confirmed that the patch addresses the clang-reported problems and (of course) passes make check and syntax-check.
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Jim Meyering