[libvirt] [PATCH] qemu: fix msg could be a wild pointer in qemuMonitorIOProcess()

[Peter Krempa]

On Tue, Jun 05, 2018 at 10:14:39 +0800, Shannon Zhao wrote:
> From: Weilun Zhu <zhuweilun at huawei.com>
> 
> As qemuMonitorJSONIOProcess() will unlock the qemu monitor, there is
> some extreme situation, eg qemu send message to monitor twice in a short
> time, where the local viriable 'msg' of qemuMonitorIOProcess() could be
> a wild point:
> 
> 1. qemuMonitorSend() assign mon->msg to parameter 'msg', which is alse a
> local variable of its caller qemuMonitorJSONCommandWithFd(), cause
> eventloop to send message to monitor, then wait condition.
> 2. qemu send message to monitor for the first time immediately.
> 3. qemuMonitorIOProcess() is called, then wake up the qemuMonitorSend()
> thread, but the qemuMonitorSend() thread stuck for a while, which means

If you wake up a tread via signalling a condition it _must_ have the
mutex locked prior to executing ....

Note that after waking up from virCondWait you have the mutex which was
passed to it locked.

> the qemu monitor is still unlocked.
> 4. qemu send message to monitor for the second time, such as RTC_CHANGE
> event
> 5. qemuMonitorIOProcess() is called, the local viriable 'msg' is
> assigned to mon->msg.
> 6. qemuMonitorIOProcess() call qemuMonitorJSONIOProcess() to deal with
> the message
> 7. qemuMonitorJSONIOProcess() unlock the qemu monitor, qemuMonitorSend()
> thread get the lock and free the mon->msg, assign mon->msg to NULL.

The monitor is unlocked in qemuMonitorIO() after finishing processing
from the event loop. There is no point where qemuMonitorIOProcess()
would not hold the mutex locked.

The event loop or the thread waiting for the message to be processed can
start executing only after unlocking the mutex.

> so the local viriable 'msg' of qemuMonitorIOProcess() is a wild pointer
> now.
> 
> AFAIK, it is not harmful to call again virCondBroadcast() while msg is a
> wild pointer, but just in case,  we fix it in this patch.
> ---
>  src/qemu/qemu_monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 43f1d2f..464f200 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -454,7 +454,7 @@ qemuMonitorIOProcess(qemuMonitorPtr mon)
>  #if DEBUG_IO
>      VIR_DEBUG("Process done %d used %d", (int)mon->bufferOffset, len);
>  #endif
> -    if (msg && msg->finished)
> +    if (msg && msg == mon->msg && msg->finished)

qemuMonitorIOProcess is executed when the monitor is locked so this is
impossible to happen. If the mon->msg object would be overwritten at
this point in any way, this certainly is not the correct fix as
something overwrote the pointer while the lock was held.

>          virCondBroadcast(&mon->notify);
>      return len;
>  }
> -- 
> 1.8.3.1
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180605/679e610f/attachment-0001.sig>