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.

To avoid similar problem in the future, use
g_atomic_pointer_compare_and_exchange instead of adding a lock in the
missing cases.

Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 src/qemu/qemu_monitor.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 0476d606f553..f4d05cd951c2 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -853,10 +853,11 @@ qemuMonitorRegister(qemuMonitorPtr mon)
 void
 qemuMonitorUnregister(qemuMonitorPtr mon)
 {
-    if (mon->watch) {
-        g_source_destroy(mon->watch);
-        g_source_unref(mon->watch);
-        mon->watch = NULL;
+    GSource *watch = mon->watch;
+
+    if (watch && g_atomic_pointer_compare_and_exchange(&mon->watch, watch, NULL)) {
+        g_source_destroy(watch);
+        g_source_unref(watch);
     }
 }
 
-- 
2.29.2

On 2/18/21 8:04 AM, 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.
> 
> To avoid similar problem in the future, use
> g_atomic_pointer_compare_and_exchange instead of adding a lock in the
> missing cases.
> 
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>   src/qemu/qemu_monitor.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 0476d606f553..f4d05cd951c2 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -853,10 +853,11 @@ qemuMonitorRegister(qemuMonitorPtr mon)
>   void
>   qemuMonitorUnregister(qemuMonitorPtr mon)
>   {
> -    if (mon->watch) {
> -        g_source_destroy(mon->watch);
> -        g_source_unref(mon->watch);
> -        mon->watch = NULL;
> +    GSource *watch = mon->watch;
> +
> +    if (watch && g_atomic_pointer_compare_and_exchange(&mon->watch, watch, NULL)) {
> +        g_source_destroy(watch);
> +        g_source_unref(watch);
>       }
>   }
>   
> 

This is not safe either, is it? I mean, the problem is with this patch 
we would be mixing two approaches - and if there is a function that 
locks the monitor it may see mon->watch disappear for no obvious reason 
- simply because this functioned overwrote it. I think the proper fix is 
to lock the monitor in parent functions. And looking into the code I 
think the only problematic function is where the monitor is unlocked and 
yet qemuMonitorUnregister() is called. I believe wrapping that call with 
lock is the way to go. And also, documenting that 
qemuMonitorUnregister() expects the monitor object to be locked.

But hey! Nice catch!

Michal