Changeset 52324 in vbox

Timestamp:

Aug 8, 2014 12:41:32 PM (10 years ago)

Author:

vboxsync

Message:

Main/Medium: fix deadlock between Medium::RefreshState and Medium::Close, lock order violation involving the event sem used in AutoCaller.

Location:

trunk/src/VBox/Main

Files:

• idl/VirtualBox.xidl (modified) (2 diffs)
• include/MediumImpl.h (modified) (2 diffs)
• src-server/MediumImpl.cpp (modified) (32 diffs)

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

@@ -13867 +13867 @@
     </attribute>
 
-    <method name="setIds">
+    <method name="setIds" wrap-hint-server="passcaller">
       <desc>
         Changes the UUID and parent UUID for a hard disk medium.
@@ -13903 +13903 @@
     </method>
 
-    <method name="refreshState">
+    <method name="refreshState" wrap-hint-server="passcaller">
       <desc>
         If the current medium state (see <link to="MediumState"/>) is one of

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

@@ -230 +230 @@
 
     // wrapped IMedium methods
-    HRESULT setIds(BOOL aSetImageId,
+    HRESULT setIds(AutoCaller &aAutoCaller,
+                   BOOL aSetImageId,
                    const com::Guid &aImageId,
                    BOOL aSetParentId,
                    const com::Guid &aParentId);
-    HRESULT refreshState(MediumState_T *aState);
+    HRESULT refreshState(AutoCaller &aAutoCaller,
+                         MediumState_T *aState);
     HRESULT getSnapshotIds(const com::Guid &aMachineId,
                            std::vector<com::Guid> &aSnapshotIds);
@@ -273 +275 @@
 
     // Private internal nmethods
-    HRESULT i_queryInfo(bool fSetImageId, bool fSetParentId);
+    HRESULT i_queryInfo(bool fSetImageId, bool fSetParentId, AutoCaller &autoCaller);
     HRESULT i_canClose();
     HRESULT i_unregisterWithVirtualBox();

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

@@ -131 +131 @@
 
     /** Special synchronization for operations which must wait for
-     * Medium::queryInfo in another thread to complete. Using a SemRW is
+     * Medium::i_queryInfo in another thread to complete. Using a SemRW is
      * not quite ideal, but at least it is subject to the lock validator,
      * unlike the SemEventMulti which we had here for many years. Catching
@@ -149 +149 @@
     bool autoReset : 1;
 
-    /** New UUID to be set on the next Medium::queryInfo call. */
+    /** New UUID to be set on the next Medium::i_queryInfo call. */
     const Guid uuidImage;
-    /** New parent UUID to be set on the next Medium::queryInfo call. */
+    /** New parent UUID to be set on the next Medium::i_queryInfo call. */
     const Guid uuidParentImage;
 
@@ -926 +926 @@
     m->hostDrive = false;
 
-    /* No storage unit is created yet, no need to call Medium::queryInfo */
+    /* No storage unit is created yet, no need to call Medium::i_queryInfo */
 
     rc = i_setFormat(aFormat);
@@ -1051 +1051 @@
         m->strLastAccessError = tr("Accessibility check was not yet performed");
 
-        /* Confirm a successful initialization before the call to queryInfo.
+        /* Confirm a successful initialization before the call to i_queryInfo.
          * Otherwise we can end up with a AutoCaller deadlock because the
          * medium becomes visible but is not marked as initialized. Causes
@@ -1064 +1064 @@
         return autoCaller.rc();
 
-    /* need to call queryInfo immediately to correctly place the medium in
+    /* need to call i_queryInfo immediately to correctly place the medium in
      * the respective media tree and update other information such as uuid */
-    rc = i_queryInfo(fForceNewUuid /* fSetImageId */, false /* fSetParentId */);
+    rc = i_queryInfo(fForceNewUuid /* fSetImageId */, false /* fSetParentId */,
+                     autoCaller);
     if (SUCCEEDED(rc))
     {
@@ -1086 +1087 @@
                        alock.release(); autoCaller.release(); uninit(); return E_FAIL);
 
-            /* storage format must be detected by Medium::queryInfo if the
+            /* storage format must be detected by Medium::i_queryInfo if the
              * medium is accessible */
             AssertStmt(!m->strFormat.isEmpty(),
@@ -1153 +1154 @@
     }
 
-    /* see below why we don't call Medium::queryInfo (and therefore treat
+    /* see below why we don't call Medium::i_queryInfo (and therefore treat
      * the medium as inaccessible for now */
     m->state = MediumState_Inaccessible;
@@ -1260 +1261 @@
                      m->strLocationFull.c_str(), m->strFormat.c_str(), m->id.raw()));
 
-    /* Don't call Medium::queryInfo for registered media to prevent the calling
+    /* Don't call Medium::i_queryInfo for registered media to prevent the calling
      * thread (i.e. the VirtualBox server startup thread) from an unexpected
      * freeze but mark it as initially inaccessible instead. The vital UUID,
@@ -1855 +1856 @@
 }
 
-HRESULT Medium::setIds(BOOL aSetImageId,
+HRESULT Medium::setIds(AutoCaller &autoCaller,
+                       BOOL aSetImageId,
                        const com::Guid &aImageId,
                        BOOL aSetParentId,
@@ -1893 +1895 @@
     unconst(m->uuidParentImage) = parentId;
 
-    // must not hold any locks before calling Medium::queryInfo
+    // must not hold any locks before calling Medium::i_queryInfo
     alock.release();
 
     HRESULT rc = i_queryInfo(!!aSetImageId /* fSetImageId */,
-                           !!aSetParentId /* fSetParentId */);
+                             !!aSetParentId /* fSetParentId */,
+                             autoCaller);
 
     return rc;
 }
 
-HRESULT Medium::refreshState(MediumState_T *aState)
+HRESULT Medium::refreshState(AutoCaller &autoCaller, MediumState_T *aState)
 {
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -1914 +1917 @@
         case MediumState_LockedRead:
         {
-            // must not hold any locks before calling Medium::queryInfo
+            // must not hold any locks before calling Medium::i_queryInfo
             alock.release();
 
-            rc = i_queryInfo(false /* fSetImageId */, false /* fSetParentId */);
+            rc = i_queryInfo(false /* fSetImageId */, false /* fSetParentId */,
+                             autoCaller);
 
             alock.acquire();
@@ -1969 +1973 @@
 HRESULT Medium::lockRead(ComPtr<IToken> &aToken)
 {
-    /* Must not hold the object lock, as we need control over it below. */
-    Assert(!isWriteLockOnCurrentThread());
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    /* Wait for a concurrently running Medium::queryInfo to complete. */
+    /* Wait for a concurrently running Medium::i_queryInfo to complete. */
     if (m->queryInfoRunning)
     {
-        /* Must not hold the media tree lock, as Medium::queryInfo needs this
+        /* Must not hold the media tree lock, as Medium::i_queryInfo needs this
          * lock and thus we would run into a deadlock here. */
         Assert(!m->pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
@@ -1982 +1984 @@
         {
             alock.release();
+            /* must not hold the object lock now */
+            Assert(!isWriteLockOnCurrentThread());
             {
                 AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
@@ -2086 +2090 @@
 HRESULT Medium::lockWrite(ComPtr<IToken> &aToken)
 {
-    /* Must not hold the object lock, as we need control over it below. */
-    Assert(!isWriteLockOnCurrentThread());
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    /* Wait for a concurrently running Medium::queryInfo to complete. */
+    /* Wait for a concurrently running Medium::i_queryInfo to complete. */
     if (m->queryInfoRunning)
     {
-        /* Must not hold the media tree lock, as Medium::queryInfo needs this
+        /* Must not hold the media tree lock, as Medium::i_queryInfo needs this
          * lock and thus we would run into a deadlock here. */
         Assert(!m->pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
@@ -2099 +2101 @@
         {
             alock.release();
+            /* must not hold the object lock now */
+            Assert(!isWriteLockOnCurrentThread());
             {
                 AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
@@ -3794 +3798 @@
         {
             alock.release();
-            rc = pMedium->i_queryInfo(false /* fSetImageId */, false /* fSetParentId */);
+            rc = pMedium->i_queryInfo(false /* fSetImageId */, false /* fSetParentId */,
+                                      autoCaller);
             alock.acquire();
             if (FAILED(rc)) return rc;
@@ -5411 +5416 @@
  * @return
  */
-HRESULT Medium::i_queryInfo(bool fSetImageId, bool fSetParentId)
+HRESULT Medium::i_queryInfo(bool fSetImageId, bool fSetParentId, AutoCaller &autoCaller)
 {
     Assert(!isWriteLockOnCurrentThread());
@@ -5425 +5430 @@
     int vrc = VINF_SUCCESS;
 
-    /* check if a blocking queryInfo() call is in progress on some other thread,
+    /* check if a blocking i_queryInfo() call is in progress on some other thread,
      * and wait for it to finish if so instead of querying data ourselves */
     if (m->queryInfoRunning)
@@ -5435 +5440 @@
         {
             alock.release();
+            /* must not hold the object lock now */
+            Assert(!isWriteLockOnCurrentThread());
             {
                 AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
@@ -5467 +5474 @@
         uOpenFlags |= VD_OPEN_FLAGS_SHAREABLE;
 
-    /* Lock the medium, which makes the behavior much more consistent */
-    alock.release();
+    /* Lock the medium, which makes the behavior much more consistent, must be
+     * done before dropping the object lock and setting queryInfoRunning. */
     ComPtr<IToken> pToken;
     if (uOpenFlags & (VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_SHAREABLE))
@@ -5475 +5482 @@
         rc = LockWrite(pToken.asOutParam());
     if (FAILED(rc)) return rc;
-    alock.acquire();
 
     /* Copies of the input state fields which are not read-only,
@@ -5495 +5501 @@
     bool fRepairImageZeroParentUuid = false;
 
-    /* release the object lock before a lengthy operation, and take the
-     * opportunity to have a media tree lock, too, which isn't held initially */
+    ComObjPtr<VirtualBox> pVirtualBox = m->pVirtualBox;
+
+    /* must be set before leaving the object lock the first time */
     m->queryInfoRunning = true;
+
+    /* must leave object lock now, because a lock from a higher lock class
+     * is needed and also a lengthy operation is coming */
     alock.release();
-    Assert(!isWriteLockOnCurrentThread());
-    Assert(!m->pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
-    AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-    treeLock.release();
+    autoCaller.release();
 
     /* Note that taking the queryInfoSem after leaving the object lock above
-     * can lead to short spinning of the loops waiting for queryInfo() to
+     * can lead to short spinning of the loops waiting for i_queryInfo() to
      * complete. This is unavoidable since the other order causes a lock order
      * violation: here it would be requesting the object lock (at the beginning
      * of the method), then queryInfoSem, and below the other way round. */
     AutoWriteLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
+
+    /* take the opportunity to have a media tree lock, released initially */
+    Assert(!isWriteLockOnCurrentThread());
+    Assert(!pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    AutoWriteLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    treeLock.release();
+
+    /* re-take the caller, but not the object lock, to keep uninit away */
+    autoCaller.add();
+    if (FAILED(autoCaller.rc()))
+    {
+        m->queryInfoRunning = false;
+        return autoCaller.rc();
+    }
 
     try
@@ -5601 +5622 @@
                                 location.c_str(),
                                 mediumId.raw(),
-                                m->pVirtualBox->i_settingsFilePath().c_str());
+                                pVirtualBox->i_settingsFilePath().c_str());
                         throw S_OK;
                     }
@@ -5667 +5688 @@
                         rc = VBOX_E_OBJECT_NOT_FOUND;
                     else
-                        rc = m->pVirtualBox->i_findHardDiskById(Guid(parentId), false /* aSetError */, &pParent);
+                        rc = pVirtualBox->i_findHardDiskById(Guid(parentId), false /* aSetError */, &pParent);
                     if (FAILED(rc))
                     {
@@ -5689 +5710 @@
                             lastAccessError = Utf8StrFmt(tr("Parent medium with UUID {%RTuuid} of the medium '%s' is not found in the media registry ('%s')"),
                                                          &parentId, location.c_str(),
-                                                         m->pVirtualBox->i_settingsFilePath().c_str());
+                                                         pVirtualBox->i_settingsFilePath().c_str());
                             throw S_OK;
                         }
                     }
 
+                    /* must drop the caller before taking the tree lock */
+                    autoCaller.release();
                     /* we set mParent & children() */
                     treeLock.acquire();
+                    autoCaller.add();
+                    if (FAILED(autoCaller.rc()))
+                        throw autoCaller.rc();
 
                     if (m->pParent)
@@ -5705 +5731 @@
                 else
                 {
+                    /* must drop the caller before taking the tree lock */
+                    autoCaller.release();
                     /* we access mParent */
                     treeLock.acquire();
+                    autoCaller.add();
+                    if (FAILED(autoCaller.rc()))
+                        throw autoCaller.rc();
 
                     /* check that parent UUIDs match. Note that there's no need
@@ -5728 +5759 @@
                                 tr("Medium type of '%s' is differencing but it is not associated with any parent medium in the media registry ('%s')"),
                                 location.c_str(),
-                                m->pVirtualBox->settingsFilePath().c_str());
+                                pVirtualBox->settingsFilePath().c_str());
                         treeLock.release();
                         throw S_OK;
@@ -5735 +5766 @@
 
                     {
+                        autoCaller.release();
                         AutoReadLock parentLock(m->pParent COMMA_LOCKVAL_SRC_POS);
+                        autoCaller.add();
+                        if (FAILED(autoCaller.rc()))
+                            throw autoCaller.rc();
+
                         if (   !fRepairImageZeroParentUuid
                             && m->pParent->i_getState() != MediumState_Inaccessible
@@ -5745 +5781 @@
                                     &parentId, location.c_str(),
                                     m->pParent->i_getId().raw(),
-                                    m->pVirtualBox->i_settingsFilePath().c_str());
+                                    pVirtualBox->i_settingsFilePath().c_str());
                             parentLock.release();
                             treeLock.release();
@@ -5784 +5820 @@
     }
 
+    autoCaller.release();
     treeLock.acquire();
+    autoCaller.add();
+    if (FAILED(autoCaller.rc()))
+    {
+        m->queryInfoRunning = false;
+        return autoCaller.rc();
+    }
     alock.acquire();
 
@@ -5800 +5843 @@
                         rc, vrc));
     }
-
-    /* unblock anyone waiting for the queryInfo results */
-    qlock.release();
-    m->queryInfoRunning = false;
 
     /* Set the proper state according to the result of the check */
@@ -5810 +5849 @@
     else
         m->preLockState = MediumState_Inaccessible;
+
+    /* unblock anyone waiting for the i_queryInfo results */
+    qlock.release();
+    m->queryInfoRunning = false;
 
     pToken->Abandon();