mirror of
https://gitlab.com/libvirt/libvirt.git
synced 2024-08-28 11:31:16 +00:00
qemu: Fix @vm locking issue when connecting to the monitor
When connecting to qemu's monitor the @vm object is unlocked. This is justified - connecting may take a long time and we don't want to wait with the domain object locked. However, just before the domain object is locked again, the monitor's FD is registered in the event loop. Therefore, there is a small window where the event loop has a chance to call a handler for an event that occurred on the monitor FD but vm is not initalized properly just yet (i.e. priv->mon is not set). For instance, if there's an incoming migration, qemu creates its socket but then fails to initialize (for various reasons, I'm reproducing this by using hugepages but leaving the HP pool empty) then the following may happen: 1) qemuConnectMonitor() unlocks @vm 2) qemuMonitorOpen() connects to the monitor socket and by calling qemuMonitorOpenInternal() which subsequently calls qemuMonitorRegister() the event handler is installed 3) qemu fails to initialize and exit()-s, which closes the monitor 4) The even loop sees EOF on the monitor and the control gets to qemuProcessEventHandler() which locks @vm and calls processMonitorEOFEvent() which then calls qemuMonitorLastError(priv->mon). But priv->mon is not set just yet. 5) qemuMonitorLastError() dereferences NULL pointer The solution is to unlock the domain object for a shorter time and most importantly, register event handler with domain object locked so that any possible event processing is done only after @vm's private data was properly initialized. This issue is also mentioned in v4.2.0-99-ga5a777a8ba. Since we are unlocking @vm and locking it back, another thread might have destroyed the domain meanwhile. Therefore we have to check if domain is still active, and we have to do it at the same place where domain lock is acquired back, i.e. in qemuMonitorOpen(). This creates a small problem for our test suite which calls qemuMonitorOpen() directly and passes @vm which has no definition. This makes virDomainObjIsActive() call crash. Fortunately, allocating empty domain definition is sufficient. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This commit is contained in:
parent
db873ab3bc
commit
75dd595861
@ -810,35 +810,52 @@ qemuMonitorOpen(virDomainObjPtr vm,
|
||||
qemuMonitorCallbacksPtr cb,
|
||||
void *opaque)
|
||||
{
|
||||
int fd;
|
||||
int fd = -1;
|
||||
bool hasSendFD = false;
|
||||
qemuMonitorPtr ret;
|
||||
qemuMonitorPtr ret = NULL;
|
||||
|
||||
timeout += QEMU_DEFAULT_MONITOR_WAIT;
|
||||
|
||||
/* Hold an extra reference because we can't allow 'vm' to be
|
||||
* deleted until the monitor gets its own reference. */
|
||||
virObjectRef(vm);
|
||||
|
||||
switch (config->type) {
|
||||
case VIR_DOMAIN_CHR_TYPE_UNIX:
|
||||
hasSendFD = true;
|
||||
if ((fd = qemuMonitorOpenUnix(config->data.nix.path,
|
||||
vm->pid, retry, timeout)) < 0)
|
||||
return NULL;
|
||||
virObjectUnlock(vm);
|
||||
fd = qemuMonitorOpenUnix(config->data.nix.path,
|
||||
vm->pid, retry, timeout);
|
||||
virObjectLock(vm);
|
||||
break;
|
||||
|
||||
case VIR_DOMAIN_CHR_TYPE_PTY:
|
||||
if ((fd = qemuMonitorOpenPty(config->data.file.path)) < 0)
|
||||
return NULL;
|
||||
virObjectUnlock(vm);
|
||||
fd = qemuMonitorOpenPty(config->data.file.path);
|
||||
virObjectLock(vm);
|
||||
break;
|
||||
|
||||
default:
|
||||
virReportError(VIR_ERR_INTERNAL_ERROR,
|
||||
_("unable to handle monitor type: %s"),
|
||||
virDomainChrTypeToString(config->type));
|
||||
return NULL;
|
||||
break;
|
||||
}
|
||||
|
||||
if (fd < 0)
|
||||
goto cleanup;
|
||||
|
||||
if (!virDomainObjIsActive(vm)) {
|
||||
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
|
||||
_("domain is not running"));
|
||||
goto cleanup;
|
||||
}
|
||||
|
||||
ret = qemuMonitorOpenInternal(vm, fd, hasSendFD, cb, opaque);
|
||||
cleanup:
|
||||
if (!ret)
|
||||
VIR_FORCE_CLOSE(fd);
|
||||
virObjectUnref(vm);
|
||||
return ret;
|
||||
}
|
||||
|
||||
|
@ -1955,13 +1955,8 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
|
||||
* 1GiB of guest RAM. */
|
||||
timeout = vm->def->mem.total_memory / (1024 * 1024);
|
||||
|
||||
/* Hold an extra reference because we can't allow 'vm' to be
|
||||
* deleted until the monitor gets its own reference. */
|
||||
virObjectRef(vm);
|
||||
|
||||
ignore_value(virTimeMillisNow(&priv->monStart));
|
||||
monConfig = virObjectRef(priv->monConfig);
|
||||
virObjectUnlock(vm);
|
||||
|
||||
mon = qemuMonitorOpen(vm,
|
||||
monConfig,
|
||||
@ -1978,15 +1973,8 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
|
||||
qemuProcessMonitorLogFree);
|
||||
}
|
||||
|
||||
virObjectLock(vm);
|
||||
virObjectUnref(monConfig);
|
||||
virObjectUnref(vm);
|
||||
priv->monStart = 0;
|
||||
|
||||
if (!virDomainObjIsActive(vm)) {
|
||||
qemuMonitorClose(mon);
|
||||
mon = NULL;
|
||||
}
|
||||
priv->mon = mon;
|
||||
|
||||
if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0) {
|
||||
|
@ -1085,6 +1085,8 @@ qemuMonitorCommonTestNew(virDomainXMLOptionPtr xmlopt,
|
||||
test->vm = virDomainObjNew(xmlopt);
|
||||
if (!test->vm)
|
||||
goto error;
|
||||
if (!(test->vm->def = virDomainDefNew()))
|
||||
goto error;
|
||||
}
|
||||
|
||||
if (virNetSocketNewListenUNIX(path, 0700, geteuid(), getegid(),
|
||||
|
Loading…
Reference in New Issue
Block a user