qemu: agent: fix unsafe agent access

qemuDomainObjExitAgent is unsafe.

First it accesses domain object without domain lock.
Second it uses outdated logic that goes back to commit 79533da1 of
year 2009 when code was quite different. (unref function
instead of unreferencing only unlocked and disposed object
in case of last reference and leaved unlocking to the caller otherwise).
Nowadays this logic may lead to disposing locked object
i guess.

Another problem is that the callers of qemuDomainObjEnterAgent
use domain object again (namely priv->agent) without domain lock.

This patch address these two problems.

qemuDomainGetAgent is dropped as unused.

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5928b0225a0c3d0f50e07204bc701c2ec2d3e0df..137d4d51958443ca0cdaa172951f6b0009fc8390 100644 (file)
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3615,19 +3615,6 @@ qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
 }
 
 
-/**
- * qemuDomainGetAgent:
- * @vm: domain object
- *
- * Returns the agent pointer of @vm;
- */
-qemuAgentPtr
-qemuDomainGetAgent(virDomainObjPtr vm)
-{
-    return (((qemuDomainObjPrivatePtr)(vm->privateData))->agent);
-}
-
-
 /*
  * obj must be locked before calling
  *
@@ -3637,16 +3624,20 @@ qemuDomainGetAgent(virDomainObjPtr vm)
  *
  * To be followed with qemuDomainObjExitAgent() once complete
  */
-void
+qemuAgentPtr
 qemuDomainObjEnterAgent(virDomainObjPtr obj)
 {
     qemuDomainObjPrivatePtr priv = obj->privateData;
+    qemuAgentPtr agent = priv->agent;
 
     VIR_DEBUG("Entering agent (agent=%p vm=%p name=%s)",
               priv->agent, obj, obj->def->name);
-    virObjectLock(priv->agent);
-    virObjectRef(priv->agent);
+
+    virObjectLock(agent);
+    virObjectRef(agent);
     virObjectUnlock(obj);
+
+    return agent;
 }
 
 
@@ -3655,22 +3646,14 @@ qemuDomainObjEnterAgent(virDomainObjPtr obj)
  * Should be paired with an earlier qemuDomainObjEnterAgent() call
  */
 void
-qemuDomainObjExitAgent(virDomainObjPtr obj)
+qemuDomainObjExitAgent(virDomainObjPtr obj, qemuAgentPtr agent)
 {
-    qemuDomainObjPrivatePtr priv = obj->privateData;
-    bool hasRefs;
-
-    hasRefs = virObjectUnref(priv->agent);
-
-    if (hasRefs)
-        virObjectUnlock(priv->agent);
-
+    virObjectUnlock(agent);
+    virObjectUnref(agent);
     virObjectLock(obj);
-    VIR_DEBUG("Exited agent (agent=%p vm=%p name=%s)",
-              priv->agent, obj, obj->def->name);
 
-    if (!hasRefs)
-        priv->agent = NULL;
+    VIR_DEBUG("Exited agent (agent=%p vm=%p name=%s)",
+              agent, obj, obj->def->name);
 }
 
 void qemuDomainObjEnterRemote(virDomainObjPtr obj)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 42738234bd6e41799e50d729c2b7eaee33f96218..7650ff392d0449dbec2400cec4267d1f9c979a2c 100644 (file)
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -449,11 +449,10 @@ int qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 
 
-qemuAgentPtr qemuDomainGetAgent(virDomainObjPtr vm);
-void qemuDomainObjEnterAgent(virDomainObjPtr obj)
-    ATTRIBUTE_NONNULL(1);
-void qemuDomainObjExitAgent(virDomainObjPtr obj)
+qemuAgentPtr qemuDomainObjEnterAgent(virDomainObjPtr obj)
     ATTRIBUTE_NONNULL(1);
+void qemuDomainObjExitAgent(virDomainObjPtr obj, qemuAgentPtr agent)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 
 void qemuDomainObjEnterRemote(virDomainObjPtr obj)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ab030fe4f67df16fdb5c13dd3f0477a5cd8be90f..2f420d52c0d36ba603519d5ce4492db7ee7cc0c2 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2013,10 +2013,11 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
 
 
     if (useAgent) {
+        qemuAgentPtr agent;
         qemuDomainSetFakeReboot(driver, vm, false);
-        qemuDomainObjEnterAgent(vm);
-        ret = qemuAgentShutdown(priv->agent, agentFlag);
-        qemuDomainObjExitAgent(vm);
+        agent = qemuDomainObjEnterAgent(vm);
+        ret = qemuAgentShutdown(agent, agentFlag);
+        qemuDomainObjExitAgent(vm, agent);
     }
 
     /* If we are not enforced to use just an agent, try ACPI
@@ -2106,10 +2107,12 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
     }
 
     if (useAgent) {
+        qemuAgentPtr agent;
+
         qemuDomainSetFakeReboot(driver, vm, false);
-        qemuDomainObjEnterAgent(vm);
-        ret = qemuAgentShutdown(priv->agent, agentFlag);
-        qemuDomainObjExitAgent(vm);
+        agent = qemuDomainObjEnterAgent(vm);
+        ret = qemuAgentShutdown(agent, agentFlag);
+        qemuDomainObjExitAgent(vm, agent);
     }
 
     /* If we are not enforced to use just an agent, try ACPI
@@ -4679,6 +4682,7 @@ qemuDomainSetVcpusAgent(virDomainObjPtr vm,
                         unsigned int nvcpus)
 {
     qemuAgentCPUInfoPtr cpuinfo = NULL;
+    qemuAgentPtr agent;
     int ncpuinfo;
     int ret = -1;
 
@@ -4693,9 +4697,10 @@ qemuDomainSetVcpusAgent(virDomainObjPtr vm,
         goto cleanup;
     }
 
-    qemuDomainObjEnterAgent(vm);
-    ncpuinfo = qemuAgentGetVCPUs(qemuDomainGetAgent(vm), &cpuinfo);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    ncpuinfo = qemuAgentGetVCPUs(agent, &cpuinfo);
+    qemuDomainObjExitAgent(vm, agent);
+    agent = NULL;
 
     if (ncpuinfo < 0)
         goto cleanup;
@@ -4706,9 +4711,9 @@ qemuDomainSetVcpusAgent(virDomainObjPtr vm,
     if (!qemuDomainAgentAvailable(vm, true))
         goto cleanup;
 
-    qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentSetVCPUs(qemuDomainGetAgent(vm), cpuinfo, ncpuinfo);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentSetVCPUs(agent, cpuinfo, ncpuinfo);
+    qemuDomainObjExitAgent(vm, agent);
 
  cleanup:
     VIR_FREE(cpuinfo);
@@ -5481,11 +5486,11 @@ static int
 qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
-    qemuDomainObjPrivatePtr priv;
     virDomainObjPtr vm;
     virDomainDefPtr def;
     int ret = -1;
     qemuAgentCPUInfoPtr cpuinfo = NULL;
+    qemuAgentPtr agent;
     int ncpuinfo = -1;
     size_t i;
 
@@ -5497,8 +5502,6 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
     if (!(vm = qemuDomObjFromDomain(dom)))
         return -1;
 
-    priv = vm->privateData;
-
     if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
@@ -5519,9 +5522,9 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
         if (!qemuDomainAgentAvailable(vm, true))
             goto endjob;
 
-        qemuDomainObjEnterAgent(vm);
-        ncpuinfo = qemuAgentGetVCPUs(priv->agent, &cpuinfo);
-        qemuDomainObjExitAgent(vm);
+        agent = qemuDomainObjEnterAgent(vm);
+        ncpuinfo = qemuAgentGetVCPUs(agent, &cpuinfo);
+        qemuDomainObjExitAgent(vm, agent);
 
  endjob:
         qemuDomainObjEndJob(driver, vm);
@@ -13458,15 +13461,15 @@ qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
                            const char **mountpoints,
                            unsigned int nmountpoints)
 {
-    qemuDomainObjPrivatePtr priv = vm->privateData;
+    qemuAgentPtr agent;
     int frozen;
 
     if (!qemuDomainAgentAvailable(vm, true))
         return -1;
 
-    qemuDomainObjEnterAgent(vm);
-    frozen = qemuAgentFSFreeze(priv->agent, mountpoints, nmountpoints);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    frozen = qemuAgentFSFreeze(agent, mountpoints, nmountpoints);
+    qemuDomainObjExitAgent(vm, agent);
     return frozen < 0 ? -2 : frozen;
 }
 
@@ -13477,20 +13480,20 @@ qemuDomainSnapshotFSThaw(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
                          virDomainObjPtr vm,
                          bool report)
 {
-    qemuDomainObjPrivatePtr priv = vm->privateData;
+    qemuAgentPtr agent;
     int thawed;
     virErrorPtr err = NULL;
 
     if (!qemuDomainAgentAvailable(vm, report))
         return -1;
 
-    qemuDomainObjEnterAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
     if (!report)
         err = virSaveLastError();
-    thawed = qemuAgentFSThaw(priv->agent);
+    thawed = qemuAgentFSThaw(agent);
     if (!report)
         virSetError(err);
-    qemuDomainObjExitAgent(vm);
+    qemuDomainObjExitAgent(vm, agent);
 
     virFreeError(err);
 
@@ -18041,6 +18044,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
     virQEMUDriverPtr driver = dom->conn->privateData;
     qemuDomainObjPrivatePtr priv;
     virDomainObjPtr vm;
+    qemuAgentPtr agent;
     int ret = -1;
 
     virCheckFlags(0, -1);
@@ -18106,9 +18110,9 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentSuspend(priv->agent, target);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentSuspend(agent, target);
+    qemuDomainObjExitAgent(vm, agent);
 
  endjob:
     qemuDomainObjEndJob(driver, vm);
@@ -18196,15 +18200,13 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
     virDomainObjPtr vm;
     int ret = -1;
     char *result = NULL;
-    qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
 
     virCheckFlags(0, NULL);
 
     if (!(vm = qemuDomObjFromDomain(domain)))
         goto cleanup;
 
-    priv = vm->privateData;
-
     if (virDomainQemuAgentCommandEnsureACL(domain->conn, vm->def) < 0)
         goto cleanup;
 
@@ -18220,9 +18222,9 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentArbitraryCommand(priv->agent, cmd, &result, timeout);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentArbitraryCommand(agent, cmd, &result, timeout);
+    qemuDomainObjExitAgent(vm, agent);
     if (ret < 0)
         VIR_FREE(result);
 
@@ -18291,8 +18293,8 @@ qemuDomainFSTrim(virDomainPtr dom,
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     virDomainObjPtr vm;
+    qemuAgentPtr agent;
     int ret = -1;
-    qemuDomainObjPrivatePtr priv;
 
     virCheckFlags(0, -1);
 
@@ -18306,8 +18308,6 @@ qemuDomainFSTrim(virDomainPtr dom,
     if (!(vm = qemuDomObjFromDomain(dom)))
         goto cleanup;
 
-    priv = vm->privateData;
-
     if (virDomainFSTrimEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
@@ -18323,9 +18323,9 @@ qemuDomainFSTrim(virDomainPtr dom,
         goto endjob;
     }
 
-    qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentFSTrim(priv->agent, minimum);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentFSTrim(agent, minimum);
+    qemuDomainObjExitAgent(vm, agent);
 
  endjob:
     qemuDomainObjEndJob(driver, vm);
@@ -18484,7 +18484,7 @@ qemuDomainGetTime(virDomainPtr dom,
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     virDomainObjPtr vm = NULL;
-    qemuDomainObjPrivatePtr priv;
+    qemuAgentPtr agent;
     int ret = -1;
     int rv;
 
@@ -18496,8 +18496,6 @@ qemuDomainGetTime(virDomainPtr dom,
     if (virDomainGetTimeEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
-    priv = vm->privateData;
-
     if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
         goto cleanup;
 
@@ -18510,9 +18508,9 @@ qemuDomainGetTime(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjEnterAgent(vm);
-    rv = qemuAgentGetTime(priv->agent, seconds, nseconds);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    rv = qemuAgentGetTime(agent, seconds, nseconds);
+    qemuDomainObjExitAgent(vm, agent);
 
     if (rv < 0)
         goto endjob;
@@ -18536,6 +18534,7 @@ qemuDomainSetTime(virDomainPtr dom,
     virQEMUDriverPtr driver = dom->conn->privateData;
     qemuDomainObjPrivatePtr priv;
     virDomainObjPtr vm;
+    qemuAgentPtr agent;
     bool rtcSync = flags & VIR_DOMAIN_TIME_SYNC;
     int ret = -1;
     int rv;
@@ -18574,9 +18573,9 @@ qemuDomainSetTime(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjEnterAgent(vm);
-    rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    rv = qemuAgentSetTime(agent, seconds, nseconds, rtcSync);
+    qemuDomainObjExitAgent(vm, agent);
 
     if (rv < 0)
         goto endjob;
@@ -19679,8 +19678,8 @@ qemuDomainGetFSInfo(virDomainPtr dom,
                     unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
-    qemuDomainObjPrivatePtr priv;
     virDomainObjPtr vm;
+    qemuAgentPtr agent;
     int ret = -1;
 
     virCheckFlags(0, ret);
@@ -19691,8 +19690,6 @@ qemuDomainGetFSInfo(virDomainPtr dom,
     if (virDomainGetFSInfoEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
-    priv = vm->privateData;
-
     if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
         goto cleanup;
 
@@ -19705,9 +19702,9 @@ qemuDomainGetFSInfo(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentGetFSInfo(priv->agent, info, vm->def);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentGetFSInfo(agent, info, vm->def);
+    qemuDomainObjExitAgent(vm, agent);
 
  endjob:
     qemuDomainObjEndJob(driver, vm);
@@ -19724,8 +19721,8 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
                              unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
-    qemuDomainObjPrivatePtr priv = NULL;
     virDomainObjPtr vm = NULL;
+    qemuAgentPtr agent;
     int ret = -1;
 
     virCheckFlags(0, -1);
@@ -19733,8 +19730,6 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
     if (!(vm = qemuDomObjFromDomain(dom)))
         goto cleanup;
 
-    priv = vm->privateData;
-
     if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
@@ -19756,9 +19751,9 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
         if (!qemuDomainAgentAvailable(vm, true))
             goto endjob;
 
-        qemuDomainObjEnterAgent(vm);
-        ret = qemuAgentGetInterfaces(priv->agent, ifaces);
-        qemuDomainObjExitAgent(vm);
+        agent = qemuDomainObjEnterAgent(vm);
+        ret = qemuAgentGetInterfaces(agent, ifaces);
+        qemuDomainObjExitAgent(vm, agent);
 
     endjob:
         qemuDomainObjEndJob(driver, vm);
@@ -19882,8 +19877,8 @@ qemuDomainSetUserPassword(virDomainPtr dom,
                           unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
-    qemuDomainObjPrivatePtr priv;
     virDomainObjPtr vm;
+    qemuAgentPtr agent;
     int ret = -1;
     int rv;
 
@@ -19895,8 +19890,6 @@ qemuDomainSetUserPassword(virDomainPtr dom,
     if (virDomainSetUserPasswordEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
-    priv = vm->privateData;
-
     if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
         goto cleanup;
 
@@ -19909,10 +19902,10 @@ qemuDomainSetUserPassword(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjEnterAgent(vm);
-    rv = qemuAgentSetUserPassword(priv->agent, user, password,
+    agent = qemuDomainObjEnterAgent(vm);
+    rv = qemuAgentSetUserPassword(agent, user, password,
                                   flags & VIR_DOMAIN_PASSWORD_ENCRYPTED);
-    qemuDomainObjExitAgent(vm);
+    qemuDomainObjExitAgent(vm, agent);
 
     if (rv < 0)
         goto endjob;
@@ -20142,6 +20135,7 @@ qemuDomainGetGuestVcpus(virDomainPtr dom,
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
     virDomainObjPtr vm = NULL;
+    qemuAgentPtr agent;
     qemuAgentCPUInfoPtr info = NULL;
     int ninfo = 0;
     int ret = -1;
@@ -20160,9 +20154,9 @@ qemuDomainGetGuestVcpus(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjEnterAgent(vm);
-    ninfo = qemuAgentGetVCPUs(qemuDomainGetAgent(vm), &info);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    ninfo = qemuAgentGetVCPUs(agent, &info);
+    qemuDomainObjExitAgent(vm, agent);
 
     if (ninfo < 0)
         goto endjob;
@@ -20192,6 +20186,7 @@ qemuDomainSetGuestVcpus(virDomainPtr dom,
     virDomainObjPtr vm = NULL;
     virBitmapPtr map = NULL;
     qemuAgentCPUInfoPtr info = NULL;
+    qemuAgentPtr agent;
     int ninfo = 0;
     size_t i;
     int ret = -1;
@@ -20218,9 +20213,10 @@ qemuDomainSetGuestVcpus(virDomainPtr dom,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjEnterAgent(vm);
-    ninfo = qemuAgentGetVCPUs(qemuDomainGetAgent(vm), &info);
-    qemuDomainObjExitAgent(vm);
+    agent = qemuDomainObjEnterAgent(vm);
+    ninfo = qemuAgentGetVCPUs(agent, &info);
+    qemuDomainObjExitAgent(vm, agent);
+    agent = NULL;
 
     if (ninfo < 0)
         goto endjob;
@@ -20249,9 +20245,12 @@ qemuDomainSetGuestVcpus(virDomainPtr dom,
         goto endjob;
     }
 
-    qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentSetVCPUs(qemuDomainGetAgent(vm), info, ninfo);
-    qemuDomainObjExitAgent(vm);
+    if (!qemuDomainAgentAvailable(vm, true))
+        goto endjob;
+
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentSetVCPUs(agent, info, ninfo);
+    qemuDomainObjExitAgent(vm, agent);
 
  endjob:
     qemuDomainObjEndJob(driver, vm);