[libvirt] [PATCH 0/2] qemu: agent: fix unsafe agent access
The series is about second patch actually. First is just a small cleanup. Nikolay Shirokovskiy (2): qemu: drop write-only agentStart qemu: agent: fix unsafe agent access src/qemu/qemu_domain.c | 43 ++++---------- src/qemu/qemu_domain.h | 8 +-- src/qemu/qemu_driver.c | 153 ++++++++++++++++++++++++------------------------ src/qemu/qemu_process.c | 2 - 4 files changed, 91 insertions(+), 115 deletions(-) -- 1.8.3.1
--- src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_process.c | 2 -- 3 files changed, 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8cba755..f121152 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3657,7 +3657,6 @@ qemuDomainObjEnterAgent(virDomainObjPtr obj) priv->agent, obj, obj->def->name); virObjectLock(priv->agent); virObjectRef(priv->agent); - ignore_value(virTimeMillisNow(&priv->agentStart)); virObjectUnlock(obj); } @@ -3681,7 +3680,6 @@ qemuDomainObjExitAgent(virDomainObjPtr obj) VIR_DEBUG("Exited agent (agent=%p vm=%p name=%s)", priv->agent, obj, obj->def->name); - priv->agentStart = 0; if (!hasRefs) priv->agent = NULL; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2ee1829..8d3688c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -178,7 +178,6 @@ struct _qemuDomainObjPrivate { qemuAgentPtr agent; bool agentError; - unsigned long long agentStart; bool gotShutdown; bool beingDestroyed; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b67aee..35fd8c3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -231,7 +231,6 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) * deleted while the agent is active */ virObjectRef(vm); - ignore_value(virTimeMillisNow(&priv->agentStart)); virObjectUnlock(vm); agent = qemuAgentOpen(vm, @@ -239,7 +238,6 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) &agentCallbacks); virObjectLock(vm); - priv->agentStart = 0; if (agent == NULL) virObjectUnref(vm); -- 1.8.3.1
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. --- I wonder should we make qemuDomainObj(Enter|Exit)Agent be macros. So we don't need to declare a variable to hold agent reference and zero it out at the end for safety. Also the qemu monitor code has probably same problems as the agent monitor code seems to be copied from there. src/qemu/qemu_domain.c | 41 ++++--------- src/qemu/qemu_domain.h | 7 +-- src/qemu/qemu_driver.c | 153 ++++++++++++++++++++++++------------------------- 3 files changed, 91 insertions(+), 110 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f121152..e272699 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3626,19 +3626,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 * @@ -3648,16 +3635,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; } @@ -3666,22 +3657,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; + agent, obj, obj->def->name); } void qemuDomainObjEnterRemote(virDomainObjPtr obj) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8d3688c..e675bb2 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -445,11 +445,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 38c8414..20c419c 100644 --- 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 @@ -4685,6 +4688,7 @@ qemuDomainSetVcpusAgent(virDomainObjPtr vm, unsigned int nvcpus) { qemuAgentCPUInfoPtr cpuinfo = NULL; + qemuAgentPtr agent; int ncpuinfo; int ret = -1; @@ -4699,9 +4703,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; @@ -4712,9 +4717,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); @@ -5487,11 +5492,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; @@ -5503,8 +5508,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; @@ -5525,9 +5528,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); @@ -13461,15 +13464,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; } @@ -13480,20 +13483,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); @@ -18044,6 +18047,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; + qemuAgentPtr agent; int ret = -1; virCheckFlags(0, -1); @@ -18109,9 +18113,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); @@ -18199,15 +18203,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; @@ -18223,9 +18225,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); @@ -18294,8 +18296,8 @@ qemuDomainFSTrim(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; + qemuAgentPtr agent; int ret = -1; - qemuDomainObjPrivatePtr priv; virCheckFlags(0, -1); @@ -18309,8 +18311,6 @@ qemuDomainFSTrim(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; - if (virDomainFSTrimEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -18326,9 +18326,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); @@ -18487,7 +18487,7 @@ qemuDomainGetTime(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - qemuDomainObjPrivatePtr priv; + qemuAgentPtr agent; int ret = -1; int rv; @@ -18499,8 +18499,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; @@ -18513,9 +18511,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; @@ -18539,6 +18537,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; @@ -18577,9 +18576,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; @@ -19676,8 +19675,8 @@ qemuDomainGetFSInfo(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; + qemuAgentPtr agent; int ret = -1; virCheckFlags(0, ret); @@ -19688,8 +19687,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; @@ -19702,9 +19699,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); @@ -19721,8 +19718,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); @@ -19730,8 +19727,6 @@ qemuDomainInterfaceAddresses(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; - if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -19753,9 +19748,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); @@ -19879,8 +19874,8 @@ qemuDomainSetUserPassword(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; + qemuAgentPtr agent; int ret = -1; int rv; @@ -19892,8 +19887,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; @@ -19906,10 +19899,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; @@ -20139,6 +20132,7 @@ qemuDomainGetGuestVcpus(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; + qemuAgentPtr agent; qemuAgentCPUInfoPtr info = NULL; int ninfo = 0; int ret = -1; @@ -20157,9 +20151,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; @@ -20189,6 +20183,7 @@ qemuDomainSetGuestVcpus(virDomainPtr dom, virDomainObjPtr vm = NULL; virBitmapPtr map = NULL; qemuAgentCPUInfoPtr info = NULL; + qemuAgentPtr agent; int ninfo = 0; size_t i; int ret = -1; @@ -20215,9 +20210,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; @@ -20246,9 +20242,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); -- 1.8.3.1
On 14.11.2016 15:24, Nikolay Shirokovskiy wrote:
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.
Well, I agree that the order of those two steps should be reversed, so ACK to that.
Another problem is that the callers of qemuDomainObjEnterAgent use domain object again (namely priv->agent) without domain lock.
But why is this a problem?
This patch address these two problems.
qemuDomainGetAgent is dropped as unused. ---
I wonder should we make qemuDomainObj(Enter|Exit)Agent be macros. So we don't need to declare a variable to hold agent reference and zero it out at the end for safety.
Also the qemu monitor code has probably same problems as the agent monitor code seems to be copied from there.
Well, qemu monitor can't come and go. I mean, if it does, the domain is killed. That's not the case with agent monitor.
src/qemu/qemu_domain.c | 41 ++++--------- src/qemu/qemu_domain.h | 7 +-- src/qemu/qemu_driver.c | 153 ++++++++++++++++++++++++------------------------- 3 files changed, 91 insertions(+), 110 deletions(-)
Michal
On 22.11.2016 17:49, Michal Privoznik wrote:
On 14.11.2016 15:24, Nikolay Shirokovskiy wrote:
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.
Well, I agree that the order of those two steps should be reversed, so ACK to that.
Another problem is that the callers of qemuDomainObjEnterAgent use domain object again (namely priv->agent) without domain lock.
But why is this a problem?
qemuProcessHandleAgentEOF for example can zero out priv->agent after we call qemuDomainObjEnterAgent and before we call qemuAgentGetTime(priv->agent, seconds, nseconds). Because we drop domain lock in qemuDomainObjEnterAgent and qemuProcessHandleAgentEOF does not acquire job condition, only domain lock. At the same time qemuAgentGetTime is not ready for NULL agent and will crash. It could get even worse as priv->agent is not atomic and instead of NULL we can get garbage there. Long story short we just should not access domain state without lock. Thats why I change all the places so we get copy of priv->agent before we drop domain lock.
This patch address these two problems.
qemuDomainGetAgent is dropped as unused. ---
I wonder should we make qemuDomainObj(Enter|Exit)Agent be macros. So we don't need to declare a variable to hold agent reference and zero it out at the end for safety.
Also the qemu monitor code has probably same problems as the agent monitor code seems to be copied from there.
Well, qemu monitor can't come and go. I mean, if it does, the domain is killed. That's not the case with agent monitor.
Nevertheless in the same situation as described above we can get undefined behaviour on domain destroy. Nikolay
On 23.11.2016 08:08, Nikolay Shirokovskiy wrote:
On 22.11.2016 17:49, Michal Privoznik wrote:
On 14.11.2016 15:24, Nikolay Shirokovskiy wrote:
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.
Well, I agree that the order of those two steps should be reversed, so ACK to that.
Another problem is that the callers of qemuDomainObjEnterAgent use domain object again (namely priv->agent) without domain lock.
But why is this a problem?
qemuProcessHandleAgentEOF for example can zero out priv->agent after we call qemuDomainObjEnterAgent and before we call qemuAgentGetTime(priv->agent, seconds, nseconds). Because we drop domain lock in qemuDomainObjEnterAgent and qemuProcessHandleAgentEOF does not acquire job condition, only domain lock. At the same time qemuAgentGetTime is not ready for NULL agent and will crash. It could get even worse as priv->agent is not atomic and instead of NULL we can get garbage there. Long story short we just should not access domain state without lock. Thats why I change all the places so we get copy of priv->agent before we drop domain lock.
Ah. Sounds reasonable. Mind adding it to the commit message? ACK then. Michal
On 23.11.2016 10:50, Michal Privoznik wrote:
On 23.11.2016 08:08, Nikolay Shirokovskiy wrote:
On 22.11.2016 17:49, Michal Privoznik wrote:
On 14.11.2016 15:24, Nikolay Shirokovskiy wrote:
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.
Well, I agree that the order of those two steps should be reversed, so ACK to that.
Another problem is that the callers of qemuDomainObjEnterAgent use domain object again (namely priv->agent) without domain lock.
But why is this a problem?
qemuProcessHandleAgentEOF for example can zero out priv->agent after we call qemuDomainObjEnterAgent and before we call qemuAgentGetTime(priv->agent, seconds, nseconds). Because we drop domain lock in qemuDomainObjEnterAgent and qemuProcessHandleAgentEOF does not acquire job condition, only domain lock. At the same time qemuAgentGetTime is not ready for NULL agent and will crash. It could get even worse as priv->agent is not atomic and instead of NULL we can get garbage there. Long story short we just should not access domain state without lock. Thats why I change all the places so we get copy of priv->agent before we drop domain lock.
Ah. Sounds reasonable. Mind adding it to the commit message?
Of course not.
ACK then.
Thanx! I have no push rights so can you push this and another agent series you ACKed recently instead of me? (Sorry this will take changing commit message too:).
23-Nov-16 11:00, Nikolay Shirokovskiy пишет:
On 23.11.2016 10:50, Michal Privoznik wrote:
On 23.11.2016 08:08, Nikolay Shirokovskiy wrote:
On 22.11.2016 17:49, Michal Privoznik wrote:
On 14.11.2016 15:24, Nikolay Shirokovskiy wrote:
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. Well, I agree that the order of those two steps should be reversed, so ACK to that.
Another problem is that the callers of qemuDomainObjEnterAgent use domain object again (namely priv->agent) without domain lock. But why is this a problem? qemuProcessHandleAgentEOF for example can zero out priv->agent after we call qemuDomainObjEnterAgent and before we call qemuAgentGetTime(priv->agent, seconds, nseconds). Because we drop domain lock in qemuDomainObjEnterAgent and qemuProcessHandleAgentEOF does not acquire job condition, only domain lock. At the same time qemuAgentGetTime is not ready for NULL agent and will crash. It could get even worse as priv->agent is not atomic and instead of NULL we can get garbage there. Long story short we just should not access domain state without lock. Thats why I change all the places so we get copy of priv->agent before we drop domain lock.
Ah. Sounds reasonable. Mind adding it to the commit message? Of course not.
ACK then.
Thanx! I have no push rights so can you push this and another agent series you ACKed recently instead of me? (Sorry this will take changing commit message too:).
I went ahead and pushed this and another series you mentioned. Maxim
participants (3)
-
Maxim Nestratov -
Michal Privoznik -
Nikolay Shirokovskiy