[libvirt] [PATCH 1/2] Fix multiple potential NULL pointer references in monitor usage

[Eric Blake]

On 05/17/2010 05:53 AM, Daniel P. Berrange wrote:
> Any method which intends to invoke a monitor command must have
> a check for virDomainObjIsActive() before using the monitor to
> ensure that priv->mon != NULL.
> 
> There is one subtle edge case in this though. If a method invokes
> multiple monitor commands, and calls qemuDomainObjExitMonitor()
> in between two of these commands then there is no guarentee that
> priv->mon != NULL anymore. This is because the QEMU process may
> exit or die at any time, and because qemuDomainObjEnterMonitor()
> releases the lock on virDomainObj, it is possible for the background
> thread to close the monitor handle and thus qemuDomainObjExitMonitor
> will release the last reference allowing priv->mon to become NULL.

Nice analysis, and I didn't see anything obviously wrong in your patch.

ACK.

> @@ -5444,15 +5471,15 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus)
>      int i, rc;
>      int ret = -1;
>  
> +    qemuDomainObjEnterMonitor(vm);
> +
>      /* We need different branches here, because we want to offline
>       * in reverse order to onlining, so any partial fail leaves us in a
>       * reasonably sensible state */
>      if (nvcpus > vm->def->vcpus) {
>          for (i = vm->def->vcpus ; i < nvcpus ; i++) {
>              /* Online new CPU */
> -            qemuDomainObjEnterMonitor(vm);
>              rc = qemuMonitorSetCPU(priv->mon, i, 1);
> -            qemuDomainObjExitMonitor(vm);
>              if (rc == 0)
>                  goto unsupported;
>              if (rc < 0)
> @@ -5463,9 +5490,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus)
>      } else {
>          for (i = vm->def->vcpus - 1 ; i >= nvcpus ; i--) {
>              /* Offline old CPU */
> -            qemuDomainObjEnterMonitor(vm);
>              rc = qemuMonitorSetCPU(priv->mon, i, 0);
> -            qemuDomainObjExitMonitor(vm);
>              if (rc == 0)
>                  goto unsupported;
>              if (rc < 0)
> @@ -5478,6 +5503,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus)
>      ret = 0;
>  
>  cleanup:
> +    qemuDomainObjExitMonitor(vm);

This is a larger critical section now, but it didn't look like you were
doing anything that had potentially long-running actions that would
block such a large critical section.

-- 
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/20100517/9764c1f4/attachment-0001.sig>