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

17 May 2010


      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@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

    