[libvirt] [PATCH v2 0/3] qemu: fix racy paths in agent related code
Changes from v1: 1. patch 2: make vm def copy instead of making qemuAgentGetFSInfo independent of domain conf. 2. patch 3: provide more relevant commit message Nikolay Shirokovskiy (3): qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo qemu: don't use vm when lock is dropped in qemuDomainGetFSInfo qemu: agent: take monitor lock in qemuAgentNotifyEvent src/qemu/qemu_agent.c | 5 +++++ src/qemu/qemu_driver.c | 12 +++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) -- 1.8.3.1
In case of 0 filesystems *info is not set while according to virDomainGetFSInfo contract user should call free on it even in case of 0 filesystems. Thus we need to properly set it. NULL will be enough as free eats NULLs ok. --- src/qemu/qemu_agent.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 1d677f7..c50f760 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1872,6 +1872,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, ndata = virJSONValueArraySize(data); if (!ndata) { ret = 0; + *info = NULL; goto cleanup; } if (VIR_ALLOC_N(info_ret, ndata) < 0) -- 1.8.3.1
Current call to qemuAgentGetFSInfo in qemuDomainGetFSInfo is unsafe. Domain lock is dropped and we use vm->def. Let's make def copy to fix that. --- src/qemu/qemu_driver.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 842de0a..976ccb5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19749,6 +19749,8 @@ qemuDomainGetFSInfo(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuAgentPtr agent; + virCapsPtr caps = NULL; + virDomainDefPtr def = NULL; int ret = -1; virCheckFlags(0, ret); @@ -19771,8 +19773,14 @@ qemuDomainGetFSInfo(virDomainPtr dom, if (!qemuDomainAgentAvailable(vm, true)) goto endjob; + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto endjob; + + if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, NULL, false))) + goto endjob; + agent = qemuDomainObjEnterAgent(vm); - ret = qemuAgentGetFSInfo(agent, info, vm->def); + ret = qemuAgentGetFSInfo(agent, info, def); qemuDomainObjExitAgent(vm, agent); endjob: @@ -19780,6 +19788,8 @@ qemuDomainGetFSInfo(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); + virDomainDefFree(def); + virObjectUnref(caps); return ret; } -- 1.8.3.1
qemuAgentNotifyEvent accesses monitor structure and is called on qemu reset/shutdown/suspend events under domain lock. Other monitor functions on the other hand take monitor lock and don't hold domain lock. Thus it is possible to have risky simultaneous access to the structure from 2 threads. Let's take monitor lock here to make access exclusive. --- John, I decided to formulate patch purpuse this way as I doubt we can have actual signalling race here becase shutdown/suspend functions first set await_event and then send message to agent which in turn causes the event that qemuAgentNotifyEvent handles. src/qemu/qemu_agent.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index c50f760..46cad53 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1248,6 +1248,8 @@ qemuAgentMakeStringsArray(const char **strings, unsigned int len) void qemuAgentNotifyEvent(qemuAgentPtr mon, qemuAgentEvent event) { + virObjectLock(mon); + VIR_DEBUG("mon=%p event=%d await_event=%d", mon, event, mon->await_event); if (mon->await_event == event) { mon->await_event = QEMU_AGENT_EVENT_NONE; @@ -1257,6 +1259,8 @@ void qemuAgentNotifyEvent(qemuAgentPtr mon, virCondSignal(&mon->notify); } } + + virObjectUnlock(mon); } VIR_ENUM_DECL(qemuAgentShutdownMode); -- 1.8.3.1
On 12/12/2016 04:13 AM, Nikolay Shirokovskiy wrote:
Changes from v1: 1. patch 2: make vm def copy instead of making qemuAgentGetFSInfo independent of domain conf. 2. patch 3: provide more relevant commit message
Nikolay Shirokovskiy (3): qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo qemu: don't use vm when lock is dropped in qemuDomainGetFSInfo qemu: agent: take monitor lock in qemuAgentNotifyEvent
src/qemu/qemu_agent.c | 5 +++++ src/qemu/qemu_driver.c | 12 +++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)
ACK series and pushed. John BTW: Try to keep in mind for future patches we'd like to have features, enhancements, fixes, etc. logged in news.html.in If you think what I've pushed already should have some text, then create a patch for that...
On 13.12.2016 01:58, John Ferlan wrote:
On 12/12/2016 04:13 AM, Nikolay Shirokovskiy wrote:
Changes from v1: 1. patch 2: make vm def copy instead of making qemuAgentGetFSInfo independent of domain conf. 2. patch 3: provide more relevant commit message
Nikolay Shirokovskiy (3): qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo qemu: don't use vm when lock is dropped in qemuDomainGetFSInfo qemu: agent: take monitor lock in qemuAgentNotifyEvent
src/qemu/qemu_agent.c | 5 +++++ src/qemu/qemu_driver.c | 12 +++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)
ACK series and pushed.
Thanxs!
John
BTW: Try to keep in mind for future patches we'd like to have features, enhancements, fixes, etc. logged in news.html.in
Ok.
If you think what I've pushed already should have some text, then create a patch for that...
Looks like these changes don't need to be mentioned there. Nikolay
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy