Fix reference handling leak on qemuMonitor

The current code pattern requires that callers of qemuMonitorClose
check for the return value == 0, and if so, set priv->mon = NULL
and release the reference held on the associated virDomainObjPtr

The change d84bb6d6a3bd2fdd530184cc9743249ebddbee71 violated that
requirement, meaning that priv->mon never gets set to NULL, and
a reference count is leaked on virDomainObjPtr.

This design was a bad one, so remove the need to check the return
valueof qemuMonitorClose(). Instead allow registration of a
callback that's invoked just when the last reference on qemuMonitorPtr
is released.

Finally there was a potential reference leak in qemuConnectMonitor
in the failure path.

* src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h: Add a destroy
  callback invoked from qemuMonitorFree
* src/qemu/qemu_driver.c: Use the destroy callback to release the
  reference on virDomainObjPtr when the monitor is freed. Fix other
  potential reference count leak in connecting to monitor

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3f91e9fdb10c58ac25ef558ba7e561912a58c276..a7b3f2558b19238c8ec5b938f8b08650112190ae 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1173,7 +1173,17 @@ no_memory:
 }
 
 
+static void qemuHandleMonitorDestroy(qemuMonitorPtr mon,
+                                     virDomainObjPtr vm)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    if (priv->mon == mon)
+        priv->mon = NULL;
+    virDomainObjUnref(vm);
+}
+
 static qemuMonitorCallbacks monitorCallbacks = {
+    .destroy = qemuHandleMonitorDestroy,
     .eofNotify = qemuHandleMonitorEOF,
     .diskSecretLookup = findVolumeQcowPassphrase,
     .domainStop = qemuHandleDomainStop,
@@ -1190,10 +1200,6 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm)
     qemuDomainObjPrivatePtr priv = vm->privateData;
     int ret = -1;
 
-    /* Hold an extra reference because we can't allow 'vm' to be
-     * deleted while the monitor is active */
-    virDomainObjRef(vm);
-
     if ((driver->securityDriver &&
          driver->securityDriver->domainSetSecuritySocketLabel &&
          driver->securityDriver->domainSetSecuritySocketLabel(driver->securityDriver,vm)) < 0) {
@@ -1201,13 +1207,17 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm)
         goto error;
     }
 
-    if ((priv->mon = qemuMonitorOpen(vm,
-                                     priv->monConfig,
-                                     priv->monJSON,
-                                     &monitorCallbacks)) == NULL) {
-        VIR_ERROR(_("Failed to connect monitor for %s"), vm->def->name);
-        goto error;
-    }
+    /* Hold an extra reference because we can't allow 'vm' to be
+     * deleted while the monitor is active */
+    virDomainObjRef(vm);
+
+    priv->mon = qemuMonitorOpen(vm,
+                                priv->monConfig,
+                                priv->monJSON,
+                                &monitorCallbacks);
+
+    if (priv->mon == NULL)
+        virDomainObjUnref(vm);
 
     if ((driver->securityDriver &&
          driver->securityDriver->domainClearSecuritySocketLabel &&
@@ -1216,17 +1226,20 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm)
         goto error;
     }
 
+    if (priv->mon == NULL) {
+        VIR_INFO("Failed to connect monitor for %s", vm->def->name);
+        goto error;
+    }
+
+
     qemuDomainObjEnterMonitorWithDriver(driver, vm);
     ret = qemuMonitorSetCapabilities(priv->mon);
     qemuDomainObjExitMonitorWithDriver(driver, vm);
 
     ret = 0;
 error:
-    if (ret < 0) {
+    if (ret < 0)
         qemuMonitorClose(priv->mon);
-        priv->mon = NULL;
-        virDomainObjUnref(vm);
-    }
 
     return ret;
 }
@@ -3693,11 +3706,8 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver,
                              _("Failed to send SIGTERM to %s (%d)"),
                              vm->def->name, vm->pid);
 
-    if (priv->mon &&
-        qemuMonitorClose(priv->mon) == 0) {
-        virDomainObjUnref(vm);
-        priv->mon = NULL;
-    }
+    if (priv->mon)
+        qemuMonitorClose(priv->mon);
 
     if (priv->monConfig) {
         if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 3e18ac855b4624c95e9fe3f6a02a191d88e31811..ea0351ea828daf57b838f6ff4fb697dc13488cb4 100644 (file)
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -198,6 +198,8 @@ void qemuMonitorUnlock(qemuMonitorPtr mon)
 static void qemuMonitorFree(qemuMonitorPtr mon)
 {
     VIR_DEBUG("mon=%p", mon);
+    if (mon->cb->destroy)
+        (mon->cb->destroy)(mon, mon->vm);
     if (virCondDestroy(&mon->notify) < 0)
     {}
     virMutexDestroy(&mon->lock);
@@ -675,12 +677,12 @@ cleanup:
 }
 
 
-int qemuMonitorClose(qemuMonitorPtr mon)
+void qemuMonitorClose(qemuMonitorPtr mon)
 {
     int refs;
 
     if (!mon)
-        return 0;
+        return;
 
     VIR_DEBUG("mon=%p", mon);
 
@@ -700,7 +702,6 @@ int qemuMonitorClose(qemuMonitorPtr mon)
 
     if ((refs = qemuMonitorUnref(mon)) > 0)
         qemuMonitorUnlock(mon);
-    return refs;
 }
 
 

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 6c104f1bc9d69c4ed51737f245bc9e1a0bb9e3e9..b6a8f9f6c5aee0c7c3cfbb41a83b102e4eacad52 100644 (file)
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -63,6 +63,9 @@ struct _qemuMonitorMessage {
 typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks;
 typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr;
 struct _qemuMonitorCallbacks {
+    void (*destroy)(qemuMonitorPtr mon,
+                    virDomainObjPtr vm);
+
     void (*eofNotify)(qemuMonitorPtr mon,
                       virDomainObjPtr vm,
                       int withError);
@@ -120,7 +123,7 @@ qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm,
                                int json,
                                qemuMonitorCallbacksPtr cb);
 
-int qemuMonitorClose(qemuMonitorPtr mon);
+void qemuMonitorClose(qemuMonitorPtr mon);
 
 int qemuMonitorSetCapabilities(qemuMonitorPtr mon);