[libvirt] [PATCH 3/3] qemu: agent: take monitor lock in qemuAgentNotifyEvent

[Nikolay Shirokovskiy]

On 08.12.2016 19:40, John Ferlan wrote:
> 
> 
> On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
>> qemuAgentNotifyEvent notify on a lock condition without taking
>> the lock. This works but it is a subject to race conditions.
>> ---
>>  src/qemu/qemu_agent.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
> 
> But the vm is locked prior to any priv->agent dereference and call - so
> what path could free priv->agent before we get into this NotifyEvent

This patche does not fix NULL or dangling pointer reference...

> code?  I suppose it wouldn't hurt, but we're not entering the agent and
> the AgentEOF would require vm lock to clear agent.

It is just we can not deliver signal reliably if lock is not taken. It is
race condition. Acessing monitor fields without lock in general is risky too.

For example imagine we are in the process of sending agent command, vm lock
is dropped, sending thread holds agent lock and at the same time reset comes -
it is possible as nobody holds vm lock, so we come here and now we have
dangerous simultaneous access from both threads - sending and that notifies us
of a reset.

Nikolay


> 
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index 5230cbc..ad031d0 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -1248,6 +1248,8 @@ qemuAgentMakeStringsArray(const char **strings, unsigned int len)
>>  void qemuAgentNotifyEvent(qemuAgentPtr mon,
>>                            qemuAgentEvent event)
>>  {
>> +    virObjectLock(mon);
>> +
>>      VIR_DEBUG("mon=%p event=%d await_event=%d", mon, event, mon->await_event);
>>      if (mon->await_event == event) {
>>          mon->await_event = QEMU_AGENT_EVENT_NONE;
>> @@ -1257,6 +1259,8 @@ void qemuAgentNotifyEvent(qemuAgentPtr mon,
>>              virCondSignal(&mon->notify);
>>          }
>>      }
>> +
>> +    virObjectUnlock(mon);
>>  }
>>  
>>  VIR_ENUM_DECL(qemuAgentShutdownMode);
>>