[libvirt] [PATCH 1/2] qemu: Add missing lock of virDomainObj before calling virDomainUnref

[Eric Blake]

On 03/03/2011 12:47 PM, Laine Stump wrote:
> This was found while researching the root cause of:
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=670848
> 
> virDomainUnref should only be called with the lock held for the
> virDomainObj in question. However, when a transient qemu domain gets
> EOF on its monitor socket, it queues an event which frees the monitor,
> which unref's the virDomainObj without first locking it. If another
> thread has already locked the virDomainObj, the modification of the
> refcount could potentially be corrupted. In an extreme case, it could
> also be potentially unlocked by virDomainObjFree, thus left open to
> modification by anyone else who would have otherwise waited for the
> lock (not to mention the fact that they would be accessing freed
> data!).
> 
> The solution is to have qemuMonitorFree lock the domain object right
> before unrefing it. Since the caller to qemuMonitorFree doesn't expect
> this lock to be held, if the refcount doesn't go all the way to 0,
> qemuMonitorFree must unlock it after the unref.

Nice writeup.  However, just looking at this:

> ---
>  src/qemu/qemu_process.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c419c75..1d67b3c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -596,7 +596,9 @@ static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon,
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      if (priv->mon == mon)
>          priv->mon = NULL;

Hmm - we're modifying vm (by changing priv->mon)...

> -    virDomainObjUnref(vm);
> +    virDomainObjLock(vm);

...prior to obtaining the lock.  That sounds wrong.  Do things still
work for you if you move the virDomainObjLock(vm) prior to the point
where we change priv->mon?

> +    if (virDomainObjUnref(vm) > 0)
> +        virDomainObjUnlock(vm);
>  }
>  
>  static qemuMonitorCallbacks monitorCallbacks = {

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110303/46598515/attachment-0001.sig>