Changeset 52095 in vbox

Timestamp:

Jul 18, 2014 9:14:01 AM (11 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

95105

Message:

Main/Medium+AutoCaller+others: fix medium uninit deadlock caused by lock order violations, sometimes taking the caller before the media tree lock, sometimes after, plus a few other small fixes

Location:

trunk/src/VBox/Main

Files:

• idl/VirtualBox.xidl (modified) (3 diffs)
• include/AutoCaller.h (modified) (1 diff)
• include/VirtualBoxImpl.h (modified) (1 diff)
• src-all/AutoCaller.cpp (modified) (1 diff)
• src-server/MachineImplCloneVM.cpp (modified) (1 diff)
• src-server/MediumImpl.cpp (modified) (26 diffs)
• src-server/SnapshotImpl.cpp (modified) (1 diff)
• src-server/VirtualBoxImpl.cpp (modified) (10 diffs)

trunk/src/VBox/Main/idl/VirtualBox.xidl

@@ -13658 +13658 @@
     </attribute>
 
-    <attribute name="parent" type="IMedium" readonly="yes">
+    <attribute name="parent" type="IMedium" readonly="yes" wrap-hint-server="passcaller">
       <desc>
         Parent of this medium (the medium this medium is directly based
@@ -13668 +13668 @@
     </attribute>
 
-    <attribute name="children" type="IMedium" safearray="yes" readonly="yes">
+    <attribute name="children" type="IMedium" safearray="yes" readonly="yes" wrap-hint-server="passcaller">
       <desc>
         Children of this medium (all differencing media directly based
@@ -13676 +13676 @@
     </attribute>
 
-    <attribute name="base" type="IMedium" readonly="yes">
+    <attribute name="base" type="IMedium" readonly="yes" wrap-hint-server="passcaller">
       <desc>
         Base medium of this medium.

trunk/src/VBox/Main/include/AutoCaller.h

@@ -475 +475 @@
     bool uninitDone() { return mUninitDone; }
 
+    void setSucceeded();
+
 private:
 

trunk/src/VBox/Main/include/VirtualBoxImpl.h

@@ -214 +214 @@
     int i_calculateFullPath(const Utf8Str &strPath, Utf8Str &aResult);
     void i_copyPathRelativeToConfig(const Utf8Str &strSource, Utf8Str &strTarget);
-    HRESULT i_registerMedium(const ComObjPtr<Medium> &pMedium, ComObjPtr<Medium> *ppMedium, DeviceType_T argType);
+    HRESULT i_registerMedium(const ComObjPtr<Medium> &pMedium, ComObjPtr<Medium> *ppMedium, DeviceType_T argType, AutoWriteLock &mediaTreeLock);
     HRESULT i_unregisterMedium(Medium *pMedium);
     void i_pushMediumToListWithChildren(MediaList &llMedia, Medium *pMedium);

trunk/src/VBox/Main/src-all/AutoCaller.cpp

@@ -498 +498 @@
     mObj->getObjectState().autoUninitSpanDestructor();
 }
+
+/**
+ * Marks the uninitializion as succeeded.
+ *
+ * Same as the destructor, and makes the destructor do nothing.
+ */
+void AutoUninitSpan::setSucceeded()
+{
+    /* do nothing if already uninitialized */
+    if (mUninitDone)
+        return;
+
+    mObj->getObjectState().autoUninitSpanDestructor();
+    mUninitDone = true;
+}

trunk/src/VBox/Main/src-server/MachineImplCloneVM.cpp

@@ -1250 +1250 @@
                             AutoWriteLock tlock(p->mParent->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
                             rc = p->mParent->i_registerMedium(pTarget, &pTarget,
-                                                            DeviceType_HardDisk);
+                                                            DeviceType_HardDisk,
+                                                            tlock);
                             if (FAILED(rc)) throw rc;
                         }

trunk/src/VBox/Main/src-server/MediumImpl.cpp

@@ -934 +934 @@
     if (FAILED(rc)) return rc;
 
-    if (!(m->formatObj->i_getCapabilities() & (   MediumFormatCapabilities_CreateFixed
-                                              | MediumFormatCapabilities_CreateDynamic))
+    if (!(m->formatObj->i_getCapabilities() & (  MediumFormatCapabilities_CreateFixed
+                                               | MediumFormatCapabilities_CreateDynamic))
        )
     {
@@ -970 +970 @@
         }
 
-        rc = m->pVirtualBox->i_registerMedium(this, &pMedium, DeviceType_HardDisk);
+        rc = m->pVirtualBox->i_registerMedium(this, &pMedium, DeviceType_HardDisk, treeLock);
         Assert(this == pMedium || FAILED(rc));
     }
@@ -1266 +1266 @@
      * get the actual state and the rest of the data, the user will have to call
      * COMGETTER(State). */
-
-    AutoWriteLock treeLock(aVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     /* load all children */
@@ -1286 +1284 @@
         if (FAILED(rc)) break;
 
-        rc = m->pVirtualBox->i_registerMedium(pHD, &pHD, DeviceType_HardDisk);
+        AutoWriteLock treeLock(aVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+        rc = m->pVirtualBox->i_registerMedium(pHD, &pHD, DeviceType_HardDisk, treeLock);
         if (FAILED(rc)) break;
     }
@@ -1368 +1368 @@
 void Medium::uninit()
 {
+    /* It is possible that some previous/concurrent uninit has already cleared
+     * the pVirtualBox reference, and in this case we don't need to continue.
+     * Normally this would be handled through the AutoUninitSpan magic,
+     * however this cannot be done at this point as the media tree must be
+     * locked before reaching the AutoUninitSpan, otherwise deadlocks can
+     * happen due to*/
+    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
+    if (!pVirtualBox)
+        return;
+
+    /* Caller must not hold the object or media tree lock over uninit(). */
+    Assert(!isWriteLockOnCurrentThread());
+    Assert(!pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+
+    AutoWriteLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
     /* Enclose the state transition Ready->InUninit->NotReady */
     AutoUninitSpan autoUninitSpan(this);
@@ -1374 +1390 @@
 
     if (!m->formatObj.isNull())
-    {
-        /* remove the caller reference we added in setFormat() */
-        m->formatObj->getObjectState().releaseCaller();
         m->formatObj.setNull();
-    }
 
     if (m->state == MediumState_Deleting)
@@ -1388 +1400 @@
     else
     {
-        MediaList::iterator it;
-        for (it = m->llChildren.begin();
-            it != m->llChildren.end();
-            ++it)
-        {
-            Medium *pChild = *it;
+        MediaList llChildren(m->llChildren);
+        m->llChildren.clear();
+        autoUninitSpan.setSucceeded();
+
+        while (!llChildren.empty())
+        {
+            ComObjPtr<Medium> pChild = llChildren.front();
+            llChildren.pop_front();
             pChild->m->pParent.setNull();
+            treeLock.release();
             pChild->uninit();
-        }
-        m->llChildren.clear();          // this unsets all the ComPtrs and probably calls delete
+            treeLock.acquire();
+        }
 
         if (m->pParent)
@@ -1713 +1728 @@
 }
 
-HRESULT Medium::getParent(ComPtr<IMedium> &aParent)
-{
+HRESULT Medium::getParent(AutoCaller &autoCaller, ComPtr<IMedium> &aParent)
+{
+    autoCaller.release();
+
+    /* It is possible that some previous/concurrent uninit has already cleared
+     * the pVirtualBox reference, see #uninit(). */
+    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
+
     /* we access mParent */
-    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(!pVirtualBox.isNull() ? &pVirtualBox->i_getMediaTreeLockHandle() : NULL COMMA_LOCKVAL_SRC_POS);
+
+    autoCaller.add();
+    if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
     m->pParent.queryInterfaceTo(aParent.asOutParam());
@@ -1723 +1747 @@
 }
 
-HRESULT Medium::getChildren(std::vector<ComPtr<IMedium> > &aChildren)
-{
+HRESULT Medium::getChildren(AutoCaller &autoCaller, std::vector<ComPtr<IMedium> > &aChildren)
+{
+    autoCaller.release();
+
+    /* It is possible that some previous/concurrent uninit has already cleared
+     * the pVirtualBox reference, see #uninit(). */
+    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
+
     /* we access children */
-    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(!pVirtualBox.isNull() ? &pVirtualBox->i_getMediaTreeLockHandle() : NULL COMMA_LOCKVAL_SRC_POS);
+
+    autoCaller.add();
+    if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
     MediaList children(this->i_getChildren());
@@ -1736 +1769 @@
 }
 
-HRESULT Medium::getBase(ComPtr<IMedium> &aBase)
-{
+HRESULT Medium::getBase(AutoCaller &autoCaller, ComPtr<IMedium> &aBase)
+{
+    autoCaller.release();
+
     /* i_getBase() will do callers/locking */
     i_getBase().queryInterfaceTo(aBase.asOutParam());
@@ -3229 +3264 @@
     }
 
+    autoCaller.release();
+
     /* Save the error information now, the implicit restore when this goes
      * out of scope will throw away spurious additional errors created below. */
@@ -3523 +3560 @@
 {
     ComObjPtr<Medium> pBase;
-    uint32_t level;
+
+    /* it is possible that some previous/concurrent uninit has already cleared
+     * the pVirtualBox reference, and in this case we don't need to continue */
+    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
+    if (!pVirtualBox)
+        return pBase;
+
+    /* we access mParent */
+    AutoReadLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     AutoCaller autoCaller(this);
     AssertReturn(autoCaller.isOk(), pBase);
 
-    /* we access mParent */
-    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-
     pBase = this;
-    level = 0;
+    uint32_t level = 0;
 
     if (m->pParent)
@@ -3971 +4013 @@
 HRESULT Medium::i_close(AutoCaller &autoCaller)
 {
+    // must temporarily drop the caller, need the tree lock first
+    autoCaller.release();
+
     // we're accessing parent/child and backrefs, so lock the tree first, then ourselves
     AutoMultiWriteLock2 multilock(&m->pVirtualBox->i_getMediaTreeLockHandle(),
                                   this->lockHandle()
                                   COMMA_LOCKVAL_SRC_POS);
+
+    autoCaller.add();
+    if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
     LogFlowFunc(("ENTER for %s\n", i_getLocationFull().c_str()));
@@ -4010 +4058 @@
 
         multilock.release();
+        // Release the AutoCaller now, as otherwise uninit() will simply hang.
+        // Needs to be done before mark the registries as modified and saving
+        // the registry, as otherwise there may be a deadlock with someone else
+        // closing this object while we're in i_saveModifiedRegistries(), which
+        // needs the media tree lock, which the other thread holds until after
+        // uninit() below.
+        autoCaller.release();
         i_markRegistriesModified();
-        // Release the AutoCalleri now, as otherwise uninit() will simply hang.
-        // Needs to be done before saving the registry, as otherwise there
-        // may be a deadlock with someone else closing this object while we're
-        // in i_saveModifiedRegistries(), which needs the media tree lock, which
-        // the other thread holds until after uninit() below.
-        // @todo redesign the locking here, as holding the locks over uninit
-        // causes lock order trouble which the lock validator can't detect
-        autoCaller.release();
         m->pVirtualBox->i_saveModifiedRegistries();
-        multilock.acquire();
     }
     else
     {
+        multilock.release();
         // release the AutoCaller, as otherwise uninit() will simply hang
         autoCaller.release();
@@ -4079 +4126 @@
         LogFlowThisFunc(("aWait=%RTbool locationFull=%s\n", aWait, i_getLocationFull().c_str() ));
 
-        if (    !(m->formatObj->i_getCapabilities() & (   MediumFormatCapabilities_CreateDynamic
-                                                      | MediumFormatCapabilities_CreateFixed)))
+        if (    !(m->formatObj->i_getCapabilities() & (  MediumFormatCapabilities_CreateDynamic
+                                                       | MediumFormatCapabilities_CreateFixed)))
             throw setError(VBOX_E_NOT_SUPPORTED,
                            tr("Medium format '%s' does not support storage deletion"),
@@ -6176 +6223 @@
                             aFormat.c_str());
 
-        /* reference the format permanently to prevent its unexpected
-         * uninitialization */
-        HRESULT rc = m->formatObj->getObjectState().addCaller(m->formatObj);
-        AssertComRCReturnRC(rc);
-
         /* get properties (preinsert them as keys in the map). Note that the
          * map doesn't grow over the object life time since the set of
@@ -6676 +6718 @@
         AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
         ComObjPtr<Medium> pMedium;
-        rc = m->pVirtualBox->i_registerMedium(this, &pMedium, DeviceType_HardDisk);
+        rc = m->pVirtualBox->i_registerMedium(this, &pMedium, DeviceType_HardDisk, treeLock);
         Assert(this == pMedium);
     }
@@ -6863 +6905 @@
          * better than breaking media registry consistency) */
         ComObjPtr<Medium> pMedium;
-        mrc = m->pVirtualBox->i_registerMedium(pTarget, &pMedium, DeviceType_HardDisk);
+        mrc = m->pVirtualBox->i_registerMedium(pTarget, &pMedium, DeviceType_HardDisk, treeLock);
         Assert(pTarget == pMedium);
 
@@ -7098 +7140 @@
             ComObjPtr<Medium> pMedium;
             rc2 = m->pVirtualBox->i_registerMedium(pTarget, &pMedium,
-                                                 DeviceType_HardDisk);
+                                                   DeviceType_HardDisk,
+                                                   treeLock);
             AssertComRC(rc2);
         }
@@ -7182 +7225 @@
 
             if (task.isAsync() || pMedium != this)
+            {
+                treeLock.release();
                 pMedium->uninit();
+                treeLock.acquire();
+            }
         }
     }
@@ -7426 +7473 @@
     {
         /* we set mParent & children() */
-        AutoWriteLock alock2(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
         Assert(pTarget->m->pParent.isNull());
@@ -7443 +7490 @@
             ComObjPtr<Medium> pMedium;
             mrc = pParent->m->pVirtualBox->i_registerMedium(pTarget, &pMedium,
-                                                          DeviceType_HardDisk);
+                                                            DeviceType_HardDisk,
+                                                            treeLock);
             Assert(   FAILED(mrc)
                    || pTarget == pMedium);
@@ -7458 +7506 @@
             ComObjPtr<Medium> pMedium;
             mrc = m->pVirtualBox->i_registerMedium(pTarget, &pMedium,
-                                                 DeviceType_HardDisk);
+                                                   DeviceType_HardDisk,
+                                                   treeLock);
             Assert(   FAILED(mrc)
                    || pTarget == pMedium);
@@ -8214 +8263 @@
     {
         /* we set mParent & children() */
-        AutoWriteLock alock2(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
         Assert(m->pParent.isNull());
@@ -8231 +8280 @@
             ComObjPtr<Medium> pMedium;
             mrc = pParent->m->pVirtualBox->i_registerMedium(this, &pMedium,
-                                                          DeviceType_HardDisk);
+                                                            DeviceType_HardDisk,
+                                                            treeLock);
             Assert(this == pMedium);
             eik.fetch();
 
             if (FAILED(mrc))
+            {
+                treeLock.acquire();
                 /* break parent association on failure to register */
                 this->i_deparent();     // removes target from parent
+            }
         }
         else
@@ -8244 +8297 @@
             eik.restore();
             ComObjPtr<Medium> pMedium;
-            mrc = m->pVirtualBox->i_registerMedium(this, &pMedium, DeviceType_HardDisk);
+            mrc = m->pVirtualBox->i_registerMedium(this, &pMedium, DeviceType_HardDisk, treeLock);
             Assert(this == pMedium);
             eik.fetch();

trunk/src/VBox/Main/src-server/SnapshotImpl.cpp

@@ -3469 +3469 @@
 
         // then, register again
-        rc = mParent->i_registerMedium(pDeleteRec->mpTarget, &pDeleteRec->mpTarget, DeviceType_HardDisk);
+        rc = mParent->i_registerMedium(pDeleteRec->mpTarget, &pDeleteRec->mpTarget, DeviceType_HardDisk, treeLock);
         AssertComRC(rc);
+        treeLock.acquire();
     }
     else

trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp

@@ -675 +675 @@
         if (FAILED(rc)) return rc;
 
-        rc = i_registerMedium(pHardDisk, &pHardDisk, DeviceType_HardDisk);
+        rc = i_registerMedium(pHardDisk, &pHardDisk, DeviceType_HardDisk, treeLock);
         if (FAILED(rc)) return rc;
     }
@@ -695 +695 @@
         if (FAILED(rc)) return rc;
 
-        rc = i_registerMedium(pImage, &pImage, DeviceType_DVD);
+        rc = i_registerMedium(pImage, &pImage, DeviceType_DVD, treeLock);
         if (FAILED(rc)) return rc;
     }
@@ -715 +715 @@
         if (FAILED(rc)) return rc;
 
-        rc = i_registerMedium(pImage, &pImage, DeviceType_Floppy);
+        rc = i_registerMedium(pImage, &pImage, DeviceType_Floppy, treeLock);
         if (FAILED(rc)) return rc;
     }
@@ -1831 +1831 @@
         if (SUCCEEDED(rc))
         {
-            rc = i_registerMedium(pMedium, &pMedium, aDeviceType);
+            rc = i_registerMedium(pMedium, &pMedium, aDeviceType, treeLock);
 
             treeLock.release();
@@ -2948 +2948 @@
 
 /**
- *  @note Locks this object for reading.
+ *  @note Locks the list of other objects for reading.
  */
 ComObjPtr<GuestOSType> VirtualBox::i_getUnknownOSType()
 {
     ComObjPtr<GuestOSType> type;
-    AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), type);
 
     /* unknown type must always be the first */
@@ -4155 +4153 @@
  *                  created.
  * @param argType   Either DeviceType_HardDisk, DeviceType_DVD or DeviceType_Floppy.
+ * @param mediaTreeLock Reference to the AutoWriteLock holding the media tree
+ *                  lock, necessary to release it in the right spot.
  * @return
  */
 HRESULT VirtualBox::i_registerMedium(const ComObjPtr<Medium> &pMedium,
                                      ComObjPtr<Medium> *ppMedium,
-                                     DeviceType_T argType)
+                                     DeviceType_T argType,
+                                     AutoWriteLock &mediaTreeLock)
 {
     AssertReturn(pMedium != NULL, E_INVALIDARG);
     AssertReturn(ppMedium != NULL, E_INVALIDARG);
+
+    // caller must hold the media tree write lock
+    Assert(i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
 
     AutoCaller autoCaller(this);
@@ -4190 +4194 @@
     }
 
-    // caller must hold the media tree write lock
-    Assert(i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
-
     Guid id;
     Utf8Str strLocationFull;
@@ -4232 +4233 @@
             m->mapHardDisks[id] = pMedium;
 
+        mediumCaller.release();
+        mediaTreeLock.release();
         *ppMedium = pMedium;
     }
@@ -4241 +4244 @@
         // have a caller pending.
         mediumCaller.release();
+        // release media tree lock, must not be held at uninit time.
+        mediaTreeLock.release();
+        // must not hold the media tree write lock any more
+        Assert(!i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
         *ppMedium = pDupMedium;
     }
@@ -4800 +4807 @@
     AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
+    // need it below, in findDHCPServerByNetworkName (reading) and in
+    // m->allDHCPServers.addChild, so need to get it here to avoid lock
+    // order trouble with dhcpServerCaller
+    AutoWriteLock alock(m->allDHCPServers.getLockHandle() COMMA_LOCKVAL_SRC_POS);
+
     AutoCaller dhcpServerCaller(aDHCPServer);
     AssertComRCReturn(dhcpServerCaller.rc(), dhcpServerCaller.rc());