11:45 a.m.
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.
11:53 a.m.
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 :|
3:40 p.m.
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(a)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
4:38 p.m.
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
10:43 a.m.
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.
3:46 p.m.
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
4:02 p.m.
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.
4:59 a.m.
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.
5322
days inactive
5323
days old
7 comments
3 participants
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Jim Meyering