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

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.