[PATCH v2] qemu: Add missing lock in qemuProcessHandleMonitorEOF

[Peng Liang]

On 2/24/2021 9:59 PM, Michal Privoznik wrote:
> On 2/24/21 12:28 PM, Peng Liang wrote:
>> qemuMonitorUnregister will be called in multiple threads (e.g. threads
>> in rpc worker pool and the vm event thread).  In some cases, it isn't
>> protected by the monitor lock, which may lead to call g_source_unref
>> more than one time and a use-after-free problem eventually.
>>
>> Add the missing lock in qemuProcessHandleMonitorEOF (which is the only
>> position missing lock of monitor I found).
>>
>> Suggested-by: Michal Privoznik <mprivozn at redhat.com>
>> Signed-off-by: Peng Liang <liangpeng10 at huawei.com>
>> ---
>> This patch is v2 of
>> https://listman.redhat.com/archives/libvir-list/2021-February/msg00945.html.
>>
>>
>> v1 -> v2:
>> Locking monitor in qemuProcessHandleMonitorEOF instead of using aotmic
>> function in qemuMonitorUnregister.
>>
>>   src/qemu/qemu_monitor.h | 1 +
>>   src/qemu/qemu_process.c | 2 ++
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>> index d25c26343a7f..14e6b1fe9626 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -427,6 +427,7 @@ bool qemuMonitorWasDisposed(void);
>>     void qemuMonitorRegister(qemuMonitorPtr mon)
>>       ATTRIBUTE_NONNULL(1);
>> +/* Must be called with monitor locked. */
>>   void qemuMonitorUnregister(qemuMonitorPtr mon)
> 
> From this it's not very clear which function the comment belongs to. We
> tend to put comments into .c because that's where tags usually take you
> first. So you get the memo first.

Hi Michal,

So, do I need to send another patch to document it in
src/qemu/qemu_monitor.c?

Thanks,
Peng

> 
>>       ATTRIBUTE_NONNULL(1);
>>   void qemuMonitorClose(qemuMonitorPtr mon);
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index d930ff9a74f6..bfa742577f32 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -318,7 +318,9 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
>>       /* We don't want this EOF handler to be called over and over
>> while the
>>        * thread is waiting for a job.
>>        */
>> +    virObjectLock(mon);
>>       qemuMonitorUnregister(mon);
>> +    virObjectUnlock(mon);
>>         /* We don't want any cleanup from EOF handler (or any other
>>        * thread) to enter qemu namespace. */
>>
> 
> ACK to this hunk. And I'll be pushing it in a few moments.
> 
> Michal
> 
> .