Changeset 106832 in vbox for trunk/src/VBox

Timestamp:

Nov 5, 2024 11:35:04 AM (3 months ago)

Author:

vboxsync

Message:

Main/{Machine,Medium,Snapshot}: Address broken reference counting of
medium back references when snapshots are involved which could cause
back references to get removed while the medium was still in use. Also
resolved some locking issues for various failure scenarios when creating
or deleting snapshots.

Location:

trunk/src/VBox/Main/src-server

Files:

• MachineImpl.cpp (modified) (14 diffs)
• MediumImpl.cpp (modified) (6 diffs)
• SnapshotImpl.cpp (modified) (3 diffs)

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

@@ -10582 +10582 @@
     AssertComRCReturn(autoCaller.hrc(), autoCaller.hrc());
 
-    AutoMultiWriteLock2 alock(this->lockHandle(),
-                              &mParent->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    /* We can't use AutoMultiWriteLock2 here as some error code paths acquire
+     * the media tree lock which means we need to be able to drop the media
+     * tree lock individually in those cases. */
+    AutoWriteLock aMachineLock(this->lockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock aTreeLock(&mParent->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     /* must be in a protective state because we release the lock below */
@@ -10621 +10624 @@
 
                     MediumLockList *pMediumLockList(new MediumLockList());
-                    alock.release();
+                    aTreeLock.release();
+                    aMachineLock.release();
                     hrc = pMedium->i_createMediumLockList(true /* fFailIfInaccessible */,
                                                           NULL /* pToLockWrite */,
@@ -10627 +10631 @@
                                                           NULL,
                                                           *pMediumLockList);
-                    alock.acquire();
+                    aMachineLock.acquire();
+                    aTreeLock.acquire();
                     if (FAILED(hrc))
                     {
@@ -10640 +10645 @@
 
             /* Now lock all media. If this fails, nothing is locked. */
-            alock.release();
+            aTreeLock.release();
+            aMachineLock.release();
             hrc = lockedMediaMap->Lock();
-            alock.acquire();
+            aMachineLock.acquire();
+            aTreeLock.acquire();
             if (FAILED(hrc))
                 throw setError(hrc, tr("Locking of attached media failed"));
@@ -10711 +10718 @@
                              uuidRegistryParent,
                              DeviceType_HardDisk);
-            if (FAILED(hrc)) throw hrc;
+            if (FAILED(hrc))
+            {
+                /* Throwing an exception here causes the 'diff' object to go out of scope
+                 * which triggers its destructor (ComObjPtr<Medium>::~ComObjPtr) which will
+                 * ultimately call Medium::uninit() which acquires the media tree lock
+                 * (VirtualBox::i_getMediaTreeLockHandle()) so drop the media tree lock here. */
+                aTreeLock.release();
+                throw hrc;
+            }
 
             /** @todo r=bird: How is the locking and diff image cleaned up if we fail before
@@ -10724 +10739 @@
             if (aOnline)
             {
-                alock.release();
+                aTreeLock.release();
+                aMachineLock.release();
                 /* The currently attached medium will be read-only, change
                  * the lock type to read. */
                 hrc = pMediumLockList->Update(pMedium, false);
-                alock.acquire();
+                aMachineLock.acquire();
+                aTreeLock.acquire();
                 AssertComRCThrowRC(hrc);
             }
 
             /* release the locks before the potentially lengthy operation */
-            alock.release();
+            aTreeLock.release();
+            aMachineLock.release();
             hrc = pMedium->i_createDiffStorage(diff,
                                                pMedium->i_getPreferredDiffVariant(),
@@ -10740 +10758 @@
                                                true /* aWait */,
                                                false /* aNotify */);
-            alock.acquire();
-            if (FAILED(hrc)) throw hrc;
+            aMachineLock.acquire();
+            aTreeLock.acquire();
+            if (FAILED(hrc))
+            {
+                /* As above, 'diff' will go out of scope via the 'throw' here resulting in
+                 * Medium::uninit() being called which acquires the media tree lock. */
+                aTreeLock.release();
+                throw hrc;
+            }
 
             /* actual lock list update is done in Machine::i_commitMedia */
@@ -10764 +10789 @@
                                    pAtt->i_getHotPluggable(),
                                    pAtt->i_getBandwidthGroup());
-            if (FAILED(hrc)) throw hrc;
+            if (FAILED(hrc)) {
+                /* As above, 'diff' will go out of scope via the 'throw' here resulting in
+                 * Medium::uninit() being called which acquires the media tree lock. */
+                aTreeLock.release();
+                throw hrc;
+            }
 
             hrc = lockedMediaMap->ReplaceKey(pAtt, attachment);
@@ -12646 +12676 @@
 #endif /* VBOX_WITH_USB */
 
-    // we need to lock this object in uninit() because the lock is shared
-    // with mPeer (as well as data we modify below). mParent lock is needed
-    // by several calls to it.
-    AutoMultiWriteLock2 multilock(mParent, this COMMA_LOCKVAL_SRC_POS);
+    /* We need to lock this object in uninit() because the lock is shared
+     * with mPeer (as well as data we modify below). mParent lock is needed
+     * by several calls to it.
+     * We can't use AutoMultiWriteLock2 here as some error code paths
+     * acquire the machine lock so we need to be able to drop the machine
+     * lock individually in those cases. */
+    AutoWriteLock aVBoxLock(mParent COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock aMachineLock(this COMMA_LOCKVAL_SRC_POS);
 
 #ifdef VBOX_WITH_RESOURCE_USAGE_API
@@ -12690 +12724 @@
     {
         Log1WarningThisFunc(("Discarding unsaved settings changes!\n"));
-        i_rollback(false /* aNotify */);
+        aMachineLock.release();
+        discardSettings();
+        mParent->i_unmarkRegistryModified(i_getId());
+        aMachineLock.acquire();
     }
 
@@ -12718 +12755 @@
             ComPtr<IInternalSessionControl> pControl = *it;
             mData->mSession.mRemoteControls.erase(it);
-            multilock.release();
+            aMachineLock.release();
+            aVBoxLock.release();
             LogFlowThisFunc(("  Calling remoteControl->Uninitialize()...\n"));
             HRESULT hrc = pControl->Uninitialize();
@@ -12724 +12762 @@
             if (FAILED(hrc))
                 Log1WarningThisFunc(("Forgot to close the remote session?\n"));
-            multilock.acquire();
+            aVBoxLock.acquire();
+            aMachineLock.acquire();
         }
         mData->mSession.mRemoteControls.clear();
@@ -12750 +12789 @@
                 if (SUCCEEDED(hrc))
                 {
-                    multilock.release();
+                    aMachineLock.release();
+                    aVBoxLock.release();
                     Utf8Str strName(name);
                     LogRel(("VM '%s' stops using NAT network '%s'\n",
                             mUserData->s.strName.c_str(), strName.c_str()));
                     mParent->i_natNetworkRefDec(strName);
-                    multilock.acquire();
+                    aVBoxLock.acquire();
+                    aMachineLock.acquire();
                 }
             }
@@ -12834 +12875 @@
     mData.free();
 
-    /* release the exclusive lock before setting the below two to NULL */
-    multilock.release();
+    /* release the exclusive locks before setting the below two to NULL */
+    aMachineLock.release();
+    aVBoxLock.release();
 
     unconst(mParent) = NULL;

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

@@ -4617 +4617 @@
     AssertReturn(aMachineId.isValid(), E_FAIL);
 
-    LogFlowThisFunc(("ENTER, aMachineId: {%RTuuid}, aSnapshotId: {%RTuuid}\n", aMachineId.raw(), aSnapshotId.raw()));
-
     AutoCaller autoCaller(this);
     AssertComRCReturnRC(autoCaller.hrc());
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    LogFlowThisFunc(("ENTER: medium: '%s' aMachineId: {%RTuuid}, aSnapshotId: {%RTuuid}\n",
+        i_getLocationFull().c_str(), aMachineId.raw(), aSnapshotId.raw()));
 
     switch (m->state)
@@ -4744 +4745 @@
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
+    LogFlowThisFunc(("ENTER: medium: '%s' aMachineId: {%RTuuid}, aSnapshotId: {%RTuuid}\n",
+        i_getLocationFull().c_str(), aMachineId.raw(), aSnapshotId.raw()));
+
     BackRefList::iterator it =
         std::find_if(m->backRefs.begin(), m->backRefs.end(),
@@ -4749 +4753 @@
     AssertReturn(it != m->backRefs.end(), E_FAIL);
 
+    bool fDvd = false;
+    {
+        AutoReadLock arlock(this COMMA_LOCKVAL_SRC_POS);
+        /*
+         *  Check the medium is DVD and readonly. It's for the case if DVD
+         *  will be able to be writable sometime in the future.
+         */
+        fDvd = m->type == MediumType_Readonly && m->devType == DeviceType_DVD;
+    }
+
     if (aSnapshotId.isZero())
     {
+        /* Attached removable media which are part of a snapshot can be added to a
+         * back reference more than once (as described above in i_addBackReference())
+         * so we need to balance out the reference counting for DVDs here. */
+        if (fDvd && it->iRefCnt > 1)
+            it->iRefCnt--;
         it->iRefCnt--;
         if (it->iRefCnt > 0)
             return S_OK;
 
-        /* remove the current state attachment */
+        /* Mark the medium as no longer in use by the current machine although it
+         * may still be included in one or more snapshots (it->llSnapshotIds). */
         it->fInCurState = false;
     }
@@ -4773 +4793 @@
 
         it->llSnapshotIds.erase(jt);
+
+        /* The BackRef() constructor in i_addBackReference() creates back references
+         * for attached hard disk mediums associated with a snapshot with the reference
+         * count set to '1' and fInCurState=false before adding the snapshot to
+         * it->llSnapshotIds.  When removing that snapshot reference here we need to
+         * decrement the reference count since there won't be a separate
+         * i_removeBackReference() call with aSnapshotId.isZero() since this hard disk
+         * medium is only associated with this snapshot.  Removable media like DVDs
+         * associated with a snapshot on the other hand can be attached twice, once for
+         * the snapshot and once for the machine (aSnapshotId.isZero()=true) and their
+         * reference counting is balanced out in the machine case above. */
+        if (!fDvd && it->fInCurState == false && it->iRefCnt == 1 && it->llSnapshotIds.size() == 0)
+            it->iRefCnt--;
     }
 
     /* if the backref becomes empty, remove it */
-    if (it->fInCurState == false && it->llSnapshotIds.size() == 0)
+    if (it->fInCurState == false && it->iRefCnt == 0 && it->llSnapshotIds.size() == 0)
         m->backRefs.erase(it);
 
@@ -4868 +4901 @@
     {
         const BackRef &ref = *it2;
-        LogFlowThisFunc(("  Backref from machine {%RTuuid} (fInCurState: %d, iRefCnt: %d)\n", ref.machineId.raw(), ref.fInCurState, ref.iRefCnt));
+        LogFlowThisFunc(("  Backref from machine={%RTuuid} (fInCurState=%d, iRefCnt=%d)\n", ref.machineId.raw(), ref.fInCurState, ref.iRefCnt));
 
         for (std::list<SnapshotRef>::const_iterator jt2 = it2->llSnapshotIds.begin();
@@ -4875 +4908 @@
         {
             const Guid &id = jt2->snapshotId;
-            LogFlowThisFunc(("  Backref from snapshot {%RTuuid} (iRefCnt = %d)\n", id.raw(), jt2->iRefCnt));
+            LogFlowThisFunc(("  Backref from snapshot={%RTuuid} (iRefCnt=%d)\n", id.raw(), jt2->iRefCnt));
         }
     }

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

@@ -1859 +1859 @@
         mMediumAttachments.backup();
 
+        /* We need to set fBeganTakingSnapshot=true here to handle not only the
+         * scenario of i_createImplicitDiffs() succeeding but also the failure case
+         * since differencing hard disks can still be created which then need to be
+         * cleaned up and deleted via i_finishTakingSnapshot() below. */
+        fBeganTakingSnapshot = true;
+
         alock.release();
         /* create new differencing hard disks and attach them to this machine */
@@ -1864 +1870 @@
                                     1,            // operation weight; must be the same as in Machine::TakeSnapshot()
                                     task.m_fTakingSnapshotOnline);
+        /* If i_createImplicitDiffs() fails the 'catch' block below calls
+         * Snapshot::uninit() which requires the machine to be write locked
+         * so reacquire alock here before testing for failure. */
+        alock.acquire();
         if (FAILED(hrc))
             throw hrc;
-        alock.acquire();
 
         // MUST NOT save the settings or the media registry here, because
         // this causes trouble with rolling back settings if the user cancels
         // taking the snapshot after the diff images have been created.
-
-        fBeganTakingSnapshot = true;
 
         // STEP 3: save the VM state (if online)
@@ -2608 +2615 @@
         ErrorInfoKeeper eik;
 
+        /* If any MediumAttachments have been modified then Machine::i_rollback() will
+         * go through Machine::i_rollbackMedia() -> Machine::i_deleteImplicitDiffs() ->
+         * Medium::i_deleteStorage() -> Medium::i_markRegistriesModified() ->
+         * VirtualBox::i_markRegistryModified() -> VirtualBox::i_findMachine() which
+         * attempts to acquire the Machine object lock for 'VirtualBox::allMachines' but
+         * the locking order when accessing the MachinesOList type (aka ObjectsList<Machine>)
+         * requires that the machines lock list (LOCKCLASS_LISTOFMACHINES) be
+         * acquired first and then the Machine object lock (LOCKCLASS_MACHINEOBJECT).
+         */
+        AutoReadLock alockMachines(mParent->i_getMachinesListLockHandle() COMMA_LOCKVAL_SRC_POS);
+
         /* undo all changes on failure */
         i_rollback(false /* aNotify */);
-
     }