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>

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 58de26a276073dd16f55b0b2a37aa60beead44a7..a53d3b1d609b8aa1deeee90256aeea22cf0e6715 100644 (file)
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -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;
 }
 

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7774a82972f73181f79689d3f8c2e90f59ff1447..0d20427b9f0ee6a48200511e253e18487423ceb1 100644 (file)
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -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) {

diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index e9dff123f8177934d84ffb7fa1632989987e5d1d..c7580c5f28fa8fa50c77ea591126523d78c9ab80 100644 (file)
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -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(),