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:). -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
