[PATCH] qemuMonitorUnregister: Fix use-after-free of mon->watch

[Michal Privoznik]

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 at 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