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

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