[libvirt] [PATCH 2/2] qemu: avoid corruption of domain hashtable and misuse of freed domains

[Daniel P. Berrange]

On Thu, Mar 03, 2011 at 02:47:38PM -0500, Laine Stump wrote:
> This was also found while investigating
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=670848
> 
> An EOF on a domain's monitor socket results in an event being queued
> to handle the EOF. The handler calls qemuProcessHandleMonitorEOF. If
> it is a transient domain, this leads to a call to
> virDomainRemoveInactive, which removes the domain from the driver's
> hashtable and unref's it. Nowhere in this code is the qemu driver lock
> acquired.
> 
> However, all modifications to the driver's domain hashtable *must* be
> done while holding the driver lock, otherwise the hashtable can become
> corrupt, and (even more likely) another thread could call a different
> hashtable function and acquire a pointer to the domain that is in the
> process of being destroyed.
> 
> To prevent such a disaster, qemuProcessHandleMonitorEOF must get the
> qemu driver lock *before* it gets the DomainObj's lock, and hold it
> until it is finished with the DomainObj. This guarantees that nobody
> else modifies the hashtable at the same time, and that anyone who had
> already gotten the DomainObj from the hashtable prior to this call has
> finished with it before we remove/destroy it.
> ---
>  src/qemu/qemu_process.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1d67b3c..6a65a33 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -107,11 +107,13 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>  
>      VIR_DEBUG("Received EOF on %p '%s'", vm, vm->def->name);
>  
> +    qemuDriverLock(driver);
>      virDomainObjLock(vm);
>  
>      if (!virDomainObjIsActive(vm)) {
>          VIR_DEBUG("Domain %p is not active, ignoring EOF", vm);
>          virDomainObjUnlock(vm);
> +        qemuDriverUnlock(driver);
>          return;
>      }
>  
> @@ -137,10 +139,9 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>          virDomainObjUnlock(vm);
>  
>      if (event) {
> -        qemuDriverLock(driver);
>          qemuDomainEventQueue(driver, event);
> -        qemuDriverUnlock(driver);
>      }
> +    qemuDriverUnlock(driver);
>  }

ACK

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|