6:53 a.m.
These two patches address multiple problems in usage of the monitor
which lead to a crash on NULL pointer. The first patch fixes all the
actual root cause problems. The second patch provides a safety net
in case I missed any, or people introduce similar bugs in the future
6:53 a.m.
New subject: [libvirt] [PATCH 1/2] Fix multiple potential NULL pointer references in monitor usage
Any method which intends to invoke a monitor command must have
a check for virDomainObjIsActive() before using the monitor to
ensure that priv->mon != NULL.
There is one subtle edge case in this though. If a method invokes
multiple monitor commands, and calls qemuDomainObjExitMonitor()
in between two of these commands then there is no guarentee that
priv->mon != NULL anymore. This is because the QEMU process may
exit or die at any time, and because qemuDomainObjEnterMonitor()
releases the lock on virDomainObj, it is possible for the background
thread to close the monitor handle and thus qemuDomainObjExitMonitor
will release the last reference allowing priv->mon to become NULL.
This affects several methods, most notably migration but also some
hotplug methods. This patch takes a variety of approaches to solve
the problem, depending on the particular usage scenario. Generally
though it suffices to add an extra virDomainObjIsActive() check
if qemuDomainObjExitMonitor() was called during the method.
* src/qemu/qemu_driver.c: Fix multiple potential NULL pointer flaws
in usage of the monitor
---
src/qemu/qemu_driver.c | 77 ++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 71 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5f1d6b5..6d32d1b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4630,6 +4630,12 @@ qemuDomainWaitForMigrationComplete(struct qemud_driver *driver,
virDomainObjPtr
struct timeval now;
int rc;
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit during migration"));
+ goto cleanup;
+ }
+
if (priv->jobSignals & QEMU_JOB_SIGNAL_CANCEL) {
priv->jobSignals ^= QEMU_JOB_SIGNAL_CANCEL;
VIR_DEBUG0("Cancelling migration at client request");
@@ -4657,6 +4663,15 @@ qemuDomainWaitForMigrationComplete(struct qemud_driver *driver,
virDomainObjPtr
VIR_WARN0("Unable to set migration downtime");
}
+ /* Repeat check because the job signals might have caused
+ * guest to die
+ */
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit during migration"));
+ goto cleanup;
+ }
+
qemuDomainObjEnterMonitorWithDriver(driver, vm);
rc = qemuMonitorGetMigrationStatus(priv->mon,
&status,
@@ -4855,6 +4870,12 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
goto endjob;
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto endjob;
+ }
}
/* Get XML for the domain */
@@ -5374,6 +5395,12 @@ static int qemudDomainCoreDump(virDomainPtr dom,
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
paused = 1;
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto endjob;
+ }
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
@@ -5407,7 +5434,7 @@ endjob:
/* Since the monitor is always attached to a pty for libvirt, it
will support synchronous operations so we always get here after
the migration is complete. */
- else if (resume && paused) {
+ else if (resume && paused && priv->mon) {
qemuDomainObjEnterMonitorWithDriver(driver, vm);
if (qemuMonitorStartCPUs(priv->mon, dom->conn) < 0) {
if (virGetLastError() == NULL)
@@ -5444,15 +5471,15 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned
int nvcpus)
int i, rc;
int ret = -1;
+ qemuDomainObjEnterMonitor(vm);
+
/* We need different branches here, because we want to offline
* in reverse order to onlining, so any partial fail leaves us in a
* reasonably sensible state */
if (nvcpus > vm->def->vcpus) {
for (i = vm->def->vcpus ; i < nvcpus ; i++) {
/* Online new CPU */
- qemuDomainObjEnterMonitor(vm);
rc = qemuMonitorSetCPU(priv->mon, i, 1);
- qemuDomainObjExitMonitor(vm);
if (rc == 0)
goto unsupported;
if (rc < 0)
@@ -5463,9 +5490,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int
nvcpus)
} else {
for (i = vm->def->vcpus - 1 ; i >= nvcpus ; i--) {
/* Offline old CPU */
- qemuDomainObjEnterMonitor(vm);
rc = qemuMonitorSetCPU(priv->mon, i, 0);
- qemuDomainObjExitMonitor(vm);
if (rc == 0)
goto unsupported;
if (rc < 0)
@@ -5478,6 +5503,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int
nvcpus)
ret = 0;
cleanup:
+ qemuDomainObjExitMonitor(vm);
return ret;
unsupported:
@@ -7027,6 +7053,15 @@ qemuDomainFindOrCreateSCSIDiskController(struct qemud_driver
*driver,
VIR_FREE(cont);
return NULL;
}
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ /* cont doesn't need freeing here, since the reference
+ * now held in def->controllers */
+ return NULL;
+ }
+
return cont;
}
@@ -7308,6 +7343,12 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn,
goto cleanup;
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
}
/* FIXME - need to support vhost-net here (5th arg) */
@@ -7341,6 +7382,12 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn,
close(tapfd);
tapfd = -1;
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) {
if (!(nicstr = qemuBuildNicDevStr(net, vlan)))
goto try_remove;
@@ -7389,6 +7436,9 @@ cleanup:
return ret;
try_remove:
+ if (!priv->mon)
+ goto cleanup;
+
if (vlan < 0) {
if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) &&
(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) {
@@ -7418,6 +7468,9 @@ try_remove:
goto cleanup;
try_tapfd_close:
+ if (!priv->mon)
+ goto cleanup;
+
if (tapfd_name) {
qemuDomainObjEnterMonitorWithDriver(driver, vm);
if (qemuMonitorCloseFileHandle(priv->mon, tapfd_name) < 0)
@@ -10343,6 +10396,12 @@ static int doTunnelMigrate(virDomainPtr dom,
goto finish;
}
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
/* From this point onwards we *must* call cancel to abort the
* migration on source if anything goes wrong */
@@ -10378,7 +10437,7 @@ static int doTunnelMigrate(virDomainPtr dom,
retval = doTunnelSendAll(st, client_sock);
cancel:
- if (retval != 0) {
+ if (retval != 0 && priv->mon) {
qemuDomainObjEnterMonitorWithDriver(driver, vm);
qemuMonitorMigrateCancel(priv->mon);
qemuDomainObjExitMonitorWithDriver(driver, vm);
@@ -10653,6 +10712,12 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn,
* object, but if no, clean up the empty qemu process.
*/
if (retcode == 0) {
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
if (flags & VIR_MIGRATE_PERSIST_DEST) {
if (vm->persistent)
newVM = 0;
--
1.6.6.1
9:41 a.m.
New subject: [libvirt] [PATCH 1/2] Fix multiple potential NULL pointer references in monitor usage
On 05/17/2010 05:53 AM, Daniel P. Berrange wrote:
Any method which intends to invoke a monitor command must have
a check for virDomainObjIsActive() before using the monitor to
ensure that priv->mon != NULL.
There is one subtle edge case in this though. If a method invokes
multiple monitor commands, and calls qemuDomainObjExitMonitor()
in between two of these commands then there is no guarentee that
priv->mon != NULL anymore. This is because the QEMU process may
exit or die at any time, and because qemuDomainObjEnterMonitor()
releases the lock on virDomainObj, it is possible for the background
thread to close the monitor handle and thus qemuDomainObjExitMonitor
will release the last reference allowing priv->mon to become NULL.
Nice analysis, and I didn't see anything obviously wrong in your patch.
ACK.
@@ -5444,15 +5471,15 @@ static int
qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus)
int i, rc;
int ret = -1;
+ qemuDomainObjEnterMonitor(vm);
+
/* We need different branches here, because we want to offline
* in reverse order to onlining, so any partial fail leaves us in a
* reasonably sensible state */
if (nvcpus > vm->def->vcpus) {
for (i = vm->def->vcpus ; i < nvcpus ; i++) {
/* Online new CPU */
- qemuDomainObjEnterMonitor(vm);
rc = qemuMonitorSetCPU(priv->mon, i, 1);
- qemuDomainObjExitMonitor(vm);
if (rc == 0)
goto unsupported;
if (rc < 0)
@@ -5463,9 +5490,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int
nvcpus)
} else {
for (i = vm->def->vcpus - 1 ; i >= nvcpus ; i--) {
/* Offline old CPU */
- qemuDomainObjEnterMonitor(vm);
rc = qemuMonitorSetCPU(priv->mon, i, 0);
- qemuDomainObjExitMonitor(vm);
if (rc == 0)
goto unsupported;
if (rc < 0)
@@ -5478,6 +5503,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int
nvcpus)
ret = 0;
cleanup:
+ qemuDomainObjExitMonitor(vm);
This is a larger critical section now, but it didn't look like you were
doing anything that had potentially long-running actions that would
block such a large critical section.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:50 a.m.
New subject: [libvirt] [PATCH 1/2] Fix multiple potential NULL pointer references in monitor usage
On Mon, May 17, 2010 at 08:41:08AM -0600, Eric Blake wrote:
On 05/17/2010 05:53 AM, Daniel P. Berrange wrote:
> @@ -5444,15 +5471,15 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm,
unsigned int nvcpus)
> int i, rc;
> int ret = -1;
>
> + qemuDomainObjEnterMonitor(vm);
> +
> /* We need different branches here, because we want to offline
> * in reverse order to onlining, so any partial fail leaves us in a
> * reasonably sensible state */
> if (nvcpus > vm->def->vcpus) {
> for (i = vm->def->vcpus ; i < nvcpus ; i++) {
> /* Online new CPU */
> - qemuDomainObjEnterMonitor(vm);
> rc = qemuMonitorSetCPU(priv->mon, i, 1);
> - qemuDomainObjExitMonitor(vm);
> if (rc == 0)
> goto unsupported;
> if (rc < 0)
> @@ -5463,9 +5490,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm,
unsigned int nvcpus)
> } else {
> for (i = vm->def->vcpus - 1 ; i >= nvcpus ; i--) {
> /* Offline old CPU */
> - qemuDomainObjEnterMonitor(vm);
> rc = qemuMonitorSetCPU(priv->mon, i, 0);
> - qemuDomainObjExitMonitor(vm);
> if (rc == 0)
> goto unsupported;
> if (rc < 0)
> @@ -5478,6 +5503,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm,
unsigned int nvcpus)
> ret = 0;
>
> cleanup:
> + qemuDomainObjExitMonitor(vm);
This is a larger critical section now, but it didn't look like you were
doing anything that had potentially long-running actions that would
block such a large critical section.
Actually I've noticed a flaw in this block, not visible in the diff. My
change means that we're not holding the mutex when doing vm->def->vcpus++;
I'll have to fix this the more verbose way...
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 :|
12:41 p.m.
New subject: [libvirt] [PATCH] Fix multiple potential NULL pointer references in monitor usage (v2)
Any method which intends to invoke a monitor command must have
a check for virDomainObjIsActive() before using the monitor to
ensure that priv->mon != NULL.
There is one subtle edge case in this though. If a method invokes
multiple monitor commands, and calls qemuDomainObjExitMonitor()
in between two of these commands then there is no guarentee that
priv->mon != NULL anymore. This is because the QEMU process may
exit or die at any time, and because qemuDomainObjEnterMonitor()
releases the lock on virDomainObj, it is possible for the background
thread to close the monitor handle and thus qemuDomainObjExitMonitor
will release the last reference allowing priv->mon to become NULL.
This affects several methods, most notably migration but also some
hotplug methods. This patch takes a variety of approaches to solve
the problem, depending on the particular usage scenario. Generally
though it suffices to add an extra virDomainObjIsActive() check
if qemuDomainObjExitMonitor() was called during the method.
In v2:
- Revert the qemudDomainHotplugVcpus change and add IsActive
checks there instead to protect vm->def->vcpus update
* src/qemu/qemu_driver.c: Fix multiple potential NULL pointer flaws
in usage of the monitor
---
src/qemu/qemu_driver.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 80 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1d9b606..c537ed1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5095,6 +5095,12 @@ qemuDomainWaitForMigrationComplete(struct qemud_driver *driver,
virDomainObjPtr
struct timeval now;
int rc;
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit during migration"));
+ goto cleanup;
+ }
+
if (priv->jobSignals & QEMU_JOB_SIGNAL_CANCEL) {
priv->jobSignals ^= QEMU_JOB_SIGNAL_CANCEL;
VIR_DEBUG0("Cancelling migration at client request");
@@ -5122,6 +5128,15 @@ qemuDomainWaitForMigrationComplete(struct qemud_driver *driver,
virDomainObjPtr
VIR_WARN0("Unable to set migration downtime");
}
+ /* Repeat check because the job signals might have caused
+ * guest to die
+ */
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit during migration"));
+ goto cleanup;
+ }
+
qemuDomainObjEnterMonitorWithDriver(driver, vm);
rc = qemuMonitorGetMigrationStatus(priv->mon,
&status,
@@ -5320,6 +5335,12 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
goto endjob;
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto endjob;
+ }
}
/* Get XML for the domain */
@@ -5835,6 +5856,12 @@ static int qemudDomainCoreDump(virDomainPtr dom,
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
paused = 1;
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto endjob;
+ }
}
qemuDomainObjEnterMonitorWithDriver(driver, vm);
@@ -5867,7 +5894,7 @@ endjob:
/* Since the monitor is always attached to a pty for libvirt, it
will support synchronous operations so we always get here after
the migration is complete. */
- else if (resume && paused) {
+ else if (resume && paused && priv->mon) {
qemuDomainObjEnterMonitorWithDriver(driver, vm);
if (qemuMonitorStartCPUs(priv->mon, dom->conn) < 0) {
if (virGetLastError() == NULL)
@@ -5918,6 +5945,12 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int
nvcpus)
if (rc < 0)
goto cleanup;
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
vm->def->vcpus++;
}
} else {
@@ -5931,6 +5964,12 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int
nvcpus)
if (rc < 0)
goto cleanup;
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
vm->def->vcpus--;
}
}
@@ -7484,6 +7523,15 @@ qemuDomainFindOrCreateSCSIDiskController(struct qemud_driver
*driver,
VIR_FREE(cont);
return NULL;
}
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ /* cont doesn't need freeing here, since the reference
+ * now held in def->controllers */
+ return NULL;
+ }
+
return cont;
}
@@ -7765,6 +7813,12 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn,
goto cleanup;
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
}
/* FIXME - need to support vhost-net here (5th arg) */
@@ -7798,6 +7852,12 @@ static int qemudDomainAttachNetDevice(virConnectPtr conn,
close(tapfd);
tapfd = -1;
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) {
if (!(nicstr = qemuBuildNicDevStr(net, vlan)))
goto try_remove;
@@ -7846,6 +7906,9 @@ cleanup:
return ret;
try_remove:
+ if (!priv->mon)
+ goto cleanup;
+
if (vlan < 0) {
if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) &&
(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) {
@@ -7875,6 +7938,9 @@ try_remove:
goto cleanup;
try_tapfd_close:
+ if (!priv->mon)
+ goto cleanup;
+
if (tapfd_name) {
qemuDomainObjEnterMonitorWithDriver(driver, vm);
if (qemuMonitorCloseFileHandle(priv->mon, tapfd_name) < 0)
@@ -10856,6 +10922,12 @@ static int doTunnelMigrate(virDomainPtr dom,
goto finish;
}
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
/* From this point onwards we *must* call cancel to abort the
* migration on source if anything goes wrong */
@@ -10891,7 +10963,7 @@ static int doTunnelMigrate(virDomainPtr dom,
retval = doTunnelSendAll(st, client_sock);
cancel:
- if (retval != 0) {
+ if (retval != 0 && priv->mon) {
qemuDomainObjEnterMonitorWithDriver(driver, vm);
qemuMonitorMigrateCancel(priv->mon);
qemuDomainObjExitMonitorWithDriver(driver, vm);
@@ -11179,6 +11251,12 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn,
* object, but if no, clean up the empty qemu process.
*/
if (retcode == 0) {
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
if (flags & VIR_MIGRATE_PERSIST_DEST) {
if (vm->persistent)
newVM = 0;
--
1.6.6.1
3:42 p.m.
New subject: [libvirt] [PATCH] Fix multiple potential NULL pointer references in monitor usage (v2)
On 05/17/2010 11:41 AM, Daniel P. Berrange wrote:
Any method which intends to invoke a monitor command must have
a check for virDomainObjIsActive() before using the monitor to
ensure that priv->mon != NULL.
In v2:
- Revert the qemudDomainHotplugVcpus change and add IsActive
checks there instead to protect vm->def->vcpus update
Yes, that looks better.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:53 a.m.
New subject: [libvirt] [PATCH 2/2] Protect against NULL pointer flaws in monitor usage
History has shown that there are frequent bugs in the QEMU driver
code leading to the monitor being invoked with a NULL pointer.
Although the QEMU driver code should always report an error in
this case before invoking the monitor, as a safety net put in a
generic check in the monitor code entry points.
* src/qemu/qemu_monitor.c: Safety net to check for NULL monitor
object
---
src/qemu/qemu_monitor.c | 409 +++++++++++++++++++++++++++++++++++++++-------
1 files changed, 346 insertions(+), 63 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 2ce3d56..ec22c20 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -894,7 +894,13 @@ int qemuMonitorEmitGraphics(qemuMonitorPtr mon,
int qemuMonitorSetCapabilities(qemuMonitorPtr mon)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSetCapabilities(mon);
@@ -909,7 +915,13 @@ qemuMonitorStartCPUs(qemuMonitorPtr mon,
virConnectPtr conn)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONStartCPUs(mon, conn);
@@ -923,7 +935,13 @@ int
qemuMonitorStopCPUs(qemuMonitorPtr mon)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONStopCPUs(mon);
@@ -936,7 +954,13 @@ qemuMonitorStopCPUs(qemuMonitorPtr mon)
int qemuMonitorSystemPowerdown(qemuMonitorPtr mon)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSystemPowerdown(mon);
@@ -950,7 +974,13 @@ int qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
int **pids)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONGetCPUInfo(mon, pids);
@@ -963,7 +993,13 @@ int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon,
unsigned long *currmem)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONGetBalloonInfo(mon, currmem);
@@ -978,7 +1014,13 @@ int qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
unsigned int nr_stats)
{
int ret;
- DEBUG("mon=%p, fd=%d stats=%p nstats=%u", mon, mon->fd, stats,
nr_stats);
+ DEBUG("mon=%p stats=%p nstats=%u", mon, stats, nr_stats);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONGetMemoryStats(mon, stats, nr_stats);
@@ -997,7 +1039,13 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
long long *errs)
{
int ret;
- DEBUG("mon=%p, fd=%d dev=%s", mon, mon->fd, devname);
+ DEBUG("mon=%p dev=%s", mon, devname);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONGetBlockStatsInfo(mon, devname,
@@ -1017,7 +1065,14 @@ int qemuMonitorSetVNCPassword(qemuMonitorPtr mon,
const char *password)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p, password=%p",
+ mon, password);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (!password)
password = "";
@@ -1034,7 +1089,13 @@ int qemuMonitorSetBalloon(qemuMonitorPtr mon,
unsigned long newmem)
{
int ret;
- DEBUG("mon=%p, fd=%d newmem=%lu", mon, mon->fd, newmem);
+ DEBUG("mon=%p newmem=%lu", mon, newmem);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSetBalloon(mon, newmem);
@@ -1047,7 +1108,13 @@ int qemuMonitorSetBalloon(qemuMonitorPtr mon,
int qemuMonitorSetCPU(qemuMonitorPtr mon, int cpu, int online)
{
int ret;
- DEBUG("mon=%p, fd=%d cpu=%d online=%d", mon, mon->fd, cpu, online);
+ DEBUG("mon=%p cpu=%d online=%d", mon, cpu, online);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSetCPU(mon, cpu, online);
@@ -1061,7 +1128,13 @@ int qemuMonitorEjectMedia(qemuMonitorPtr mon,
const char *devname)
{
int ret;
- DEBUG("mon=%p, fd=%d devname=%s", mon, mon->fd, devname);
+ DEBUG("mon=%p devname=%s", mon, devname);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONEjectMedia(mon, devname);
@@ -1077,8 +1150,14 @@ int qemuMonitorChangeMedia(qemuMonitorPtr mon,
const char *format)
{
int ret;
- DEBUG("mon=%p, fd=%d devname=%s newmedia=%s format=%s",
- mon, mon->fd, devname, newmedia, format);
+ DEBUG("mon=%p devname=%s newmedia=%s format=%s",
+ mon, devname, newmedia, format);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONChangeMedia(mon, devname, newmedia, format);
@@ -1094,8 +1173,14 @@ int qemuMonitorSaveVirtualMemory(qemuMonitorPtr mon,
const char *path)
{
int ret;
- DEBUG("mon=%p, fd=%d offset=%llu length=%zu path=%s",
- mon, mon->fd, offset, length, path);
+ DEBUG("mon=%p offset=%llu length=%zu path=%s",
+ mon, offset, length, path);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSaveVirtualMemory(mon, offset, length, path);
@@ -1110,8 +1195,14 @@ int qemuMonitorSavePhysicalMemory(qemuMonitorPtr mon,
const char *path)
{
int ret;
- DEBUG("mon=%p, fd=%d offset=%llu length=%zu path=%s",
- mon, mon->fd, offset, length, path);
+ DEBUG("mon=%p offset=%llu length=%zu path=%s",
+ mon, offset, length, path);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSavePhysicalMemory(mon, offset, length, path);
@@ -1125,7 +1216,13 @@ int qemuMonitorSetMigrationSpeed(qemuMonitorPtr mon,
unsigned long bandwidth)
{
int ret;
- DEBUG("mon=%p, fd=%d bandwidth=%lu", mon, mon->fd, bandwidth);
+ DEBUG("mon=%p bandwidth=%lu", mon, bandwidth);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSetMigrationSpeed(mon, bandwidth);
@@ -1139,7 +1236,13 @@ int qemuMonitorSetMigrationDowntime(qemuMonitorPtr mon,
unsigned long long downtime)
{
int ret;
- DEBUG("mon=%p, fd=%d downtime=%llu", mon, mon->fd, downtime);
+ DEBUG("mon=%p downtime=%llu", mon, downtime);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSetMigrationDowntime(mon, downtime);
@@ -1156,7 +1259,13 @@ int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon,
unsigned long long *total)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONGetMigrationStatus(mon, status,
@@ -1178,8 +1287,14 @@ int qemuMonitorMigrateToHost(qemuMonitorPtr mon,
int port)
{
int ret;
- DEBUG("mon=%p, fd=%d hostname=%s port=%d",
- mon, mon->fd, hostname, port);
+ DEBUG("mon=%p hostname=%s port=%d",
+ mon, hostname, port);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONMigrateToHost(mon, background, hostname, port);
@@ -1194,8 +1309,14 @@ int qemuMonitorMigrateToCommand(qemuMonitorPtr mon,
const char * const *argv)
{
int ret;
- DEBUG("mon=%p, fd=%d argv=%p",
- mon, mon->fd, argv);
+ DEBUG("mon=%p argv=%p",
+ mon, argv);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONMigrateToCommand(mon, background, argv);
@@ -1211,8 +1332,14 @@ int qemuMonitorMigrateToFile(qemuMonitorPtr mon,
unsigned long long offset)
{
int ret;
- DEBUG("mon=%p, fd=%d argv=%p target=%s offset=%llu",
- mon, mon->fd, argv, target, offset);
+ DEBUG("mon=%p argv=%p target=%s offset=%llu",
+ mon, argv, target, offset);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1233,8 +1360,14 @@ int qemuMonitorMigrateToUnix(qemuMonitorPtr mon,
const char *unixfile)
{
int ret;
- DEBUG("mon=%p fd=%d unixfile=%s",
- mon, mon->fd, unixfile);
+ DEBUG("mon=%p, unixfile=%s",
+ mon, unixfile);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONMigrateToUnix(mon, background, unixfile);
@@ -1246,7 +1379,13 @@ int qemuMonitorMigrateToUnix(qemuMonitorPtr mon,
int qemuMonitorMigrateCancel(qemuMonitorPtr mon)
{
int ret;
- DEBUG("mon=%p fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONMigrateCancel(mon);
@@ -1259,7 +1398,13 @@ int qemuMonitorAddUSBDisk(qemuMonitorPtr mon,
const char *path)
{
int ret;
- DEBUG("mon=%p, fd=%d path=%s", mon, mon->fd, path);
+ DEBUG("mon=%p path=%s", mon, path);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONAddUSBDisk(mon, path);
@@ -1274,7 +1419,13 @@ int qemuMonitorAddUSBDeviceExact(qemuMonitorPtr mon,
int dev)
{
int ret;
- DEBUG("mon=%p, fd=%d bus=%d dev=%d", mon, mon->fd, bus, dev);
+ DEBUG("mon=%p bus=%d dev=%d", mon, bus, dev);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONAddUSBDeviceExact(mon, bus, dev);
@@ -1288,8 +1439,14 @@ int qemuMonitorAddUSBDeviceMatch(qemuMonitorPtr mon,
int product)
{
int ret;
- DEBUG("mon=%p, fd=%d vendor=%d product=%d",
- mon, mon->fd, vendor, product);
+ DEBUG("mon=%p vendor=%d product=%d",
+ mon, vendor, product);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONAddUSBDeviceMatch(mon, vendor, product);
@@ -1304,10 +1461,16 @@ int qemuMonitorAddPCIHostDevice(qemuMonitorPtr mon,
virDomainDevicePCIAddress *guestAddr)
{
int ret;
- DEBUG("mon=%p, fd=%d domain=%d bus=%d slot=%d function=%d",
- mon, mon->fd,
+ DEBUG("mon=%p domain=%d bus=%d slot=%d function=%d",
+ mon,
hostAddr->domain, hostAddr->bus, hostAddr->slot,
hostAddr->function);
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONAddPCIHostDevice(mon, hostAddr, guestAddr);
else
@@ -1322,8 +1485,14 @@ int qemuMonitorAddPCIDisk(qemuMonitorPtr mon,
virDomainDevicePCIAddress *guestAddr)
{
int ret;
- DEBUG("mon=%p, fd=%d path=%s bus=%s",
- mon, mon->fd, path, bus);
+ DEBUG("mon=%p path=%s bus=%s",
+ mon, path, bus);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONAddPCIDisk(mon, path, bus, guestAddr);
@@ -1338,7 +1507,13 @@ int qemuMonitorAddPCINetwork(qemuMonitorPtr mon,
virDomainDevicePCIAddress *guestAddr)
{
int ret;
- DEBUG("mon=%p, fd=%d nicstr=%s", mon, mon->fd, nicstr);
+ DEBUG("mon=%p nicstr=%s", mon, nicstr);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONAddPCINetwork(mon, nicstr, guestAddr);
@@ -1352,10 +1527,16 @@ int qemuMonitorRemovePCIDevice(qemuMonitorPtr mon,
virDomainDevicePCIAddress *guestAddr)
{
int ret;
- DEBUG("mon=%p, fd=%d domain=%d bus=%d slot=%d function=%d",
- mon, mon->fd, guestAddr->domain, guestAddr->bus,
+ DEBUG("mon=%p domain=%d bus=%d slot=%d function=%d",
+ mon, guestAddr->domain, guestAddr->bus,
guestAddr->slot, guestAddr->function);
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONRemovePCIDevice(mon, guestAddr);
else
@@ -1369,8 +1550,14 @@ int qemuMonitorSendFileHandle(qemuMonitorPtr mon,
int fd)
{
int ret;
- DEBUG("mon=%p, fd=%d fdname=%s fd=%d",
- mon, mon->fd, fdname, fd);
+ DEBUG("mon=%p, fdname=%s fd=%d",
+ mon, fdname, fd);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONSendFileHandle(mon, fdname, fd);
@@ -1384,8 +1571,14 @@ int qemuMonitorCloseFileHandle(qemuMonitorPtr mon,
const char *fdname)
{
int ret;
- DEBUG("mon=%p, fd=%d fdname=%s",
- mon, mon->fd, fdname);
+ DEBUG("mon=%p fdname=%s",
+ mon, fdname);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONCloseFileHandle(mon, fdname);
@@ -1399,8 +1592,14 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon,
const char *netstr)
{
int ret;
- DEBUG("mon=%p, fd=%d netstr=%s",
- mon, mon->fd, netstr);
+ DEBUG("mon=%p netstr=%s",
+ mon, netstr);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONAddHostNetwork(mon, netstr);
@@ -1415,8 +1614,14 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon,
const char *netname)
{
int ret;
- DEBUG("mon=%p, fd=%d netname=%s",
- mon, mon->fd, netname);
+ DEBUG("mon=%p netname=%s",
+ mon, netname);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONRemoveHostNetwork(mon, vlan, netname);
@@ -1430,8 +1635,14 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon,
const char *netdevstr)
{
int ret;
- DEBUG("mon=%p, fd=%d netdevstr=%s",
- mon, mon->fd, netdevstr);
+ DEBUG("mon=%p netdevstr=%s",
+ mon, netdevstr);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONAddNetdev(mon, netdevstr);
@@ -1445,8 +1656,14 @@ int qemuMonitorRemoveNetdev(qemuMonitorPtr mon,
const char *alias)
{
int ret;
- DEBUG("mon=%p, fd=%d alias=%s",
- mon, mon->fd, alias);
+ DEBUG("mon=%p alias=%s",
+ mon, alias);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONRemoveNetdev(mon, alias);
@@ -1460,8 +1677,14 @@ int qemuMonitorGetPtyPaths(qemuMonitorPtr mon,
virHashTablePtr paths)
{
int ret;
- DEBUG("mon=%p, fd=%d",
- mon, mon->fd);
+ DEBUG("mon=%p",
+ mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
if (mon->json)
ret = qemuMonitorJSONGetPtyPaths(mon, paths);
@@ -1475,9 +1698,15 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
const char *bus,
virDomainDevicePCIAddress *guestAddr)
{
- DEBUG("mon=%p, fd=%d type=%s", mon, mon->fd, bus);
+ DEBUG("mon=%p type=%s", mon, bus);
int ret;
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONAttachPCIDiskController(mon, bus, guestAddr);
else
@@ -1492,12 +1721,18 @@ int qemuMonitorAttachDrive(qemuMonitorPtr mon,
virDomainDevicePCIAddress *controllerAddr,
virDomainDeviceDriveAddress *driveAddr)
{
- DEBUG("mon=%p, fd=%d drivestr=%s domain=%d bus=%d slot=%d function=%d",
- mon, mon->fd, drivestr,
+ DEBUG("mon=%p drivestr=%s domain=%d bus=%d slot=%d function=%d",
+ mon, drivestr,
controllerAddr->domain, controllerAddr->bus,
controllerAddr->slot, controllerAddr->function);
int ret;
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONAttachDrive(mon, drivestr, controllerAddr, driveAddr);
else
@@ -1509,9 +1744,15 @@ int qemuMonitorAttachDrive(qemuMonitorPtr mon,
int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon,
qemuMonitorPCIAddress **addrs)
{
- DEBUG("mon=%p, fd=%d addrs=%p", mon, mon->fd, addrs);
+ DEBUG("mon=%p addrs=%p", mon, addrs);
int ret;
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONGetAllPCIAddresses(mon, addrs);
else
@@ -1522,9 +1763,15 @@ int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon,
int qemuMonitorDelDevice(qemuMonitorPtr mon,
const char *devalias)
{
- DEBUG("mon=%p, fd=%d devalias=%s", mon, mon->fd, devalias);
+ DEBUG("mon=%p devalias=%s", mon, devalias);
int ret;
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONDelDevice(mon, devalias);
else
@@ -1536,9 +1783,15 @@ int qemuMonitorDelDevice(qemuMonitorPtr mon,
int qemuMonitorAddDevice(qemuMonitorPtr mon,
const char *devicestr)
{
- DEBUG("mon=%p, fd=%d device=%s", mon, mon->fd, devicestr);
+ DEBUG("mon=%p device=%s", mon, devicestr);
int ret;
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONAddDevice(mon, devicestr);
else
@@ -1549,9 +1802,15 @@ int qemuMonitorAddDevice(qemuMonitorPtr mon,
int qemuMonitorAddDrive(qemuMonitorPtr mon,
const char *drivestr)
{
- DEBUG("mon=%p, fd=%d drive=%s", mon, mon->fd, drivestr);
+ DEBUG("mon=%p drive=%s", mon, drivestr);
int ret;
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONAddDrive(mon, drivestr);
else
@@ -1564,9 +1823,15 @@ int qemuMonitorSetDrivePassphrase(qemuMonitorPtr mon,
const char *alias,
const char *passphrase)
{
- DEBUG("mon=%p, fd=%d alias=%s passphrase=%p(value hidden)", mon,
mon->fd, alias, passphrase);
+ DEBUG("mon=%p alias=%s passphrase=%p(value hidden)", mon, alias,
passphrase);
int ret;
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONSetDrivePassphrase(mon, alias, passphrase);
else
@@ -1580,6 +1845,12 @@ int qemuMonitorCreateSnapshot(qemuMonitorPtr mon, const char
*name)
DEBUG("mon=%p, name=%s",mon,name);
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONCreateSnapshot(mon, name);
else
@@ -1593,6 +1864,12 @@ int qemuMonitorLoadSnapshot(qemuMonitorPtr mon, const char *name)
DEBUG("mon=%p, name=%s",mon,name);
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONLoadSnapshot(mon, name);
else
@@ -1606,6 +1883,12 @@ int qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char
*name)
DEBUG("mon=%p, name=%s",mon,name);
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
+
if (mon->json)
ret = qemuMonitorJSONDeleteSnapshot(mon, name);
else
--
1.6.6.1
9:44 a.m.
New subject: [libvirt] [PATCH 2/2] Protect against NULL pointer flaws in monitor usage
On 05/17/2010 05:53 AM, Daniel P. Berrange wrote:
History has shown that there are frequent bugs in the QEMU driver
code leading to the monitor being invoked with a NULL pointer.
Although the QEMU driver code should always report an error in
this case before invoking the monitor, as a safety net put in a
generic check in the monitor code entry points.
* src/qemu/qemu_monitor.c: Safety net to check for NULL monitor
object
---
src/qemu/qemu_monitor.c | 409 +++++++++++++++++++++++++++++++++++++++-------
1 files changed, 346 insertions(+), 63 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 2ce3d56..ec22c20 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -894,7 +894,13 @@ int qemuMonitorEmitGraphics(qemuMonitorPtr mon,
int qemuMonitorSetCapabilities(qemuMonitorPtr mon)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p", mon);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
Wouldn't it be better to move the DEBUG() to be after the (!mon) check,
so that we can still print mon->fd? (Throughout the patch).
@@ -1017,7 +1065,14 @@ int qemuMonitorSetVNCPassword(qemuMonitorPtr
mon,
const char *password)
{
int ret;
- DEBUG("mon=%p, fd=%d", mon, mon->fd);
+ DEBUG("mon=%p, password=%p",
+ mon, password);
+
+ if (!mon) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+ return -1;
+ }
Likewise. And while it was nice to add password=%p,...
if (!password)
password = "";
...you may have just dereferenced another NULL pointer (at least DEBUG
tends to only be used with glibc, where you get a sane "(null)" instead
of a crash).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:54 a.m.
New subject: [libvirt] [PATCH 2/2] Protect against NULL pointer flaws in monitor usage
On Mon, May 17, 2010 at 08:44:18AM -0600, Eric Blake wrote:
On 05/17/2010 05:53 AM, Daniel P. Berrange wrote:
> History has shown that there are frequent bugs in the QEMU driver
> code leading to the monitor being invoked with a NULL pointer.
> Although the QEMU driver code should always report an error in
> this case before invoking the monitor, as a safety net put in a
> generic check in the monitor code entry points.
>
> * src/qemu/qemu_monitor.c: Safety net to check for NULL monitor
> object
> ---
> src/qemu/qemu_monitor.c | 409 +++++++++++++++++++++++++++++++++++++++-------
> 1 files changed, 346 insertions(+), 63 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 2ce3d56..ec22c20 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -894,7 +894,13 @@ int qemuMonitorEmitGraphics(qemuMonitorPtr mon,
> int qemuMonitorSetCapabilities(qemuMonitorPtr mon)
> {
> int ret;
> - DEBUG("mon=%p, fd=%d", mon, mon->fd);
> + DEBUG("mon=%p", mon);
> +
> + if (!mon) {
> + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("monitor must not be NULL"));
> + return -1;
> + }
Wouldn't it be better to move the DEBUG() to be after the (!mon) check,
so that we can still print mon->fd? (Throughout the patch).
Yes & no. In practice I've never found a need to look at the 'fd'
parameter being logged. So I think it is more important to log
every call, so that we can see cases there 'mon=(null)' instead of
them being skipped.
> @@ -1017,7 +1065,14 @@ int qemuMonitorSetVNCPassword(qemuMonitorPtr mon,
> const char *password)
> {
> int ret;
> - DEBUG("mon=%p, fd=%d", mon, mon->fd);
> + DEBUG("mon=%p, password=%p",
> + mon, password);
> +
> + if (!mon) {
> + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("monitor must not be NULL"));
> + return -1;
> + }
Likewise. And while it was nice to add password=%p,...
>
> if (!password)
> password = "";
...you may have just dereferenced another NULL pointer (at least DEBUG
tends to only be used with glibc, where you get a sane "(null)" instead
of a crash).
Where is the NULL de-reference ? We're using %p, not %s for logging the
password because we don't want to actually expose the password string
in the logs & %p doesn't de-reference the pointer.
Regards,
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 :|
9:58 a.m.
New subject: [libvirt] [PATCH 2/2] Protect against NULL pointer flaws in monitor usage
On 05/17/2010 08:54 AM, Daniel P. Berrange wrote:
>> int ret;
>> - DEBUG("mon=%p, fd=%d", mon, mon->fd);
>> + DEBUG("mon=%p", mon);
>> +
>> + if (!mon) {
>> + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
>> + _("monitor must not be NULL"));
>> + return -1;
>> + }
>
> Wouldn't it be better to move the DEBUG() to be after the (!mon) check,
> so that we can still print mon->fd? (Throughout the patch).
Yes & no. In practice I've never found a need to look at the 'fd'
parameter being logged. So I think it is more important to log
every call, so that we can see cases there 'mon=(null)' instead of
them being skipped.
Fair enough; besides, the fact that mon is logged, a gdb session can
easily get at mon->fd from the logged pointer.
> Likewise. And while it was nice to add password=%p,...
>
>>
>> if (!password)
>> password = "";
>
> ...you may have just dereferenced another NULL pointer (at least DEBUG
> tends to only be used with glibc, where you get a sane "(null)" instead
> of a crash).
Where is the NULL de-reference ? We're using %p, not %s for logging the
password because we don't want to actually expose the password string
in the logs & %p doesn't de-reference the pointer.
Chalk it up to an early morning on my part. You're right, that %p is
safe on NULL. So, given your rebuttals, none of my points remain, and
you now have my:
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5304
days inactive
5304
days old
9 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Eric Blake