[libvirt] [PATCH 0/2] Fix multiple NULL pointer problems in monitor usage
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
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 :|
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 :|
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake