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

[Maxim Nestratov]

23-Nov-16 11:00, 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:).
>
>

I went ahead and pushed this and another series you mentioned.

Maxim