Re: [libvirt] [PATCH 2/2] qemu: agent: fix unsafe agent access

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:).