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

.