[libvirt] [PATCH] qemu: monitor: fix unsafe monitor access

[Michal Privoznik]

On 02/28/2018 06:09 PM, Peng Hao wrote:
> From: root <root at localhost.localdomain>

Don't you want to take credit for the patch?

git config --global user.name "Mona Lisa"
git config --global user.email "email at example.com"

> 
> qemuDomainObjExitMonitor is unsafe
> 
> domain lock released when qemuDomainObjEnterMonitor finish,
> So other thread (qemuProcessStop) has chance to modify priv->mon
> to NULL. qemuDomainObjExitMonitor will never release the mon->lock,
> 
> that may cause problem:
> thread get monitor ptr early, and then try to get mon->lock,
> it will block forerver cause mon->lock not released by
> qemuDomainObjExitMonitor.

I don't quite understand. There can be only one thread talking to
monitor, and actually only one thread calling
qemuDomainObjEnterMonitor(). Because in order to call EnterMonitor() the
caller needs to have domainObj lock. So there can be only one thread
holding the domainObj lock and another which holds the monitor lock. I'm
failing to see why not unlocking monitor would be a problem. Do you
perhaps have a reproducer?

> 
> Signed-off-by: Wang Yechao <wang.yechao255 at zte.com.cn>
> Signed-off-by: Peng Hao <peng.hao2 at zte.com.cn>
> ---
>  src/qemu/THREADS.txt             |  12 +-
>  src/qemu/qemu_block.c            |   5 +-
>  src/qemu/qemu_domain.c           |  64 +++++----
>  src/qemu/qemu_domain.h           |  12 +-
>  src/qemu/qemu_driver.c           | 258 ++++++++++++++++++++--------------
>  src/qemu/qemu_hotplug.c          | 296 ++++++++++++++++++++++-----------------
>  src/qemu/qemu_migration.c        | 104 ++++++++------
>  src/qemu/qemu_migration_cookie.c |   5 +-
>  src/qemu/qemu_process.c          | 108 ++++++++------
>  9 files changed, 507 insertions(+), 357 deletions(-)

This fails to apply cleanly. Can you please rebase?

>  mode change 100644 => 100755 src/qemu/THREADS.txt
>  mode change 100644 => 100755 src/qemu/qemu_block.c
>  mode change 100644 => 100755 src/qemu/qemu_domain.c
>  mode change 100644 => 100755 src/qemu/qemu_domain.h
>  mode change 100644 => 100755 src/qemu/qemu_driver.c
>  mode change 100644 => 100755 src/qemu/qemu_hotplug.c
>  mode change 100644 => 100755 src/qemu/qemu_migration.c
>  mode change 100644 => 100755 src/qemu/qemu_migration_cookie.c
>  mode change 100644 => 100755 src/qemu/qemu_process.c

No. This is not what we want. We don't need *.c files executable.

Michal