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@xxxxxxxxxx> >> Signed-off-by: Peng Liang <liangpeng10@xxxxxxxxxx> >> --- >> 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 > > .
