Re: [PATCH v2] qemu: Add missing lock in qemuProcessHandleMonitorEOF

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(a)redhat.com&gt; >> Signed-off-by: Peng Liang <liangpeng10(a)huawei.com&gt; >> --- >> 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?