From c4b2a93907357cff31178cd5bc388fc1a0d033be Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 17 May 2010 07:38:05 -0400 Subject: [PATCH] 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 file changed, 71 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 948ab5b417..04fd8d563c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4655,6 +4655,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"); @@ -4682,6 +4688,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, @@ -4881,6 +4896,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 */ @@ -5401,6 +5422,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); @@ -5434,7 +5461,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) @@ -5471,15 +5498,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) @@ -5490,9 +5517,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) @@ -5505,6 +5530,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus) ret = 0; cleanup: + qemuDomainObjExitMonitor(vm); return ret; unsupported: @@ -7054,6 +7080,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; } @@ -7335,6 +7370,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) */ @@ -7368,6 +7409,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; @@ -7416,6 +7463,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)) { @@ -7445,6 +7495,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) @@ -10370,6 +10423,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 */ @@ -10405,7 +10464,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); @@ -10680,6 +10739,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; -- GitLab