Changeset 54948 in vbox for trunk/src/VBox/Main/src-server

Timestamp:

Mar 25, 2015 4:56:48 PM (10 years ago)

Author:

vboxsync

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

99196

Message:

Main/Medium+Snapshot: make all code recursing over trees (objects containing lists of child objects) use as little stack as possible, and establish safe depth limits to avoid crashes, plus related cleanups in related code areas

Location:

trunk/src/VBox/Main/src-server

Files:

• MachineImpl.cpp (modified) (8 diffs)
• MachineImplCloneVM.cpp (modified) (1 diff)
• MediumImpl.cpp (modified) (31 diffs)
• SnapshotImpl.cpp (modified) (10 diffs)
• VirtualBoxImpl.cpp (modified) (9 diffs)

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

@@ -4273 +4273 @@
     }
 
+    /* Save modified registries, but skip this machine as it's the caller's
+     * job to save its settings like all other settings changes. */
+    mParent->i_unmarkRegistryModified(i_getId());
     mParent->i_saveModifiedRegistries();
 
@@ -4353 +4356 @@
     alock.release();
 
+    /* Save modified registries, but skip this machine as it's the caller's
+     * job to save its settings like all other settings changes. */
+    mParent->i_unmarkRegistryModified(i_getId());
     mParent->i_saveModifiedRegistries();
 
@@ -4794 +4800 @@
     multiLock.release();
 
+    /* Save modified registries, but skip this machine as it's the caller's
+     * job to save its settings like all other settings changes. */
+    mParent->i_unmarkRegistryModified(i_getId());
     mParent->i_saveModifiedRegistries();
 
@@ -5118 +5127 @@
     if (cSnapshots)
     {
-        // autoCleanup must be true here, or we would have failed above
-
         // add the media from the medium attachments of the snapshots to llMedia
         // as well, after the "main" machine media; Snapshot::uninitRecursively()
@@ -9278 +9285 @@
             if (puuidRegistry)
                 // caller wants registry ID to be set on all attached media (OVF import case)
-                medium->i_addRegistry(*puuidRegistry, false /* fRecurse */);
+                medium->i_addRegistry(*puuidRegistry);
         }
 
@@ -9963 +9970 @@
         if (mData->mFirstSnapshot)
         {
-            settings::Snapshot snapNew;
-            config.llFirstSnapshot.push_back(snapNew);
-
-            // get reference to the fresh copy of the snapshot on the list and
-            // work on that copy directly to avoid excessive copying later
-            settings::Snapshot &snap = config.llFirstSnapshot.front();
-
-            rc = mData->mFirstSnapshot->i_saveSnapshot(snap, false /*aAttrsOnly*/);
+            // the settings use a list for "the first snapshot"
+            config.llFirstSnapshot.push_back(settings::g_SnapshotEmpty);
+
+            // get reference to the snapshot on the list and work on that
+            // element straight in the list to avoid excessive copying later
+            rc = mData->mFirstSnapshot->i_saveSnapshot(config.llFirstSnapshot.back());
             if (FAILED(rc)) throw rc;
         }
@@ -10473 +10478 @@
         uuid = mParent->i_getGlobalRegistryId(); // VirtualBox global registry UUID
 
-    if (pMedium->i_addRegistry(uuid, false /* fRecurse */))
+    if (pMedium->i_addRegistry(uuid))
         mParent->i_markRegistryModified(uuid);
 
@@ -10480 +10485 @@
     if (pMedium != pBase)
     {
-        if (pBase->i_addRegistry(uuid, true /* fRecurse */))
+        /* Tree lock needed by Medium::addRegistry when recursing. */
+        AutoReadLock treeLock(&mParent->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        if (pBase->i_addRegistryRecursive(uuid))
+        {
+            treeLock.release();
             mParent->i_markRegistryModified(uuid);
+        }
     }
 }

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

@@ -1342 +1342 @@
                 mlock.acquire();
             }
-            pMedium->i_addRegistry(uuid, false /* fRecurse */);
+            pMedium->i_addRegistry(uuid);
         }
         /* Check if a snapshot folder is necessary and if so doesn't already

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

@@ -1213 +1213 @@
 /**
  * Initializes the medium object by loading its data from the given settings
- * node. In this mode, the medium will always be opened read/write.
+ * node. The medium will always be opened read/write.
  *
  * In this case, since we're loading from a registry, uuidMachineRegistry is
@@ -1219 +1219 @@
  * loading from a per-machine registry.
  *
- * @param aVirtualBox   VirtualBox object.
  * @param aParent       Parent medium disk or NULL for a root (base) medium.
  * @param aDeviceType   Device type of the medium.
  * @param uuidMachineRegistry The registry to which this medium should be
  *                            added (global registry UUID or machine UUID).
- * @param aNode         Configuration settings.
+ * @param data          Configuration settings.
  * @param strMachineFolder The machine folder with which to resolve relative paths;
  *                         if empty, then we use the VirtualBox home directory
@@ -1230 +1229 @@
  * @note Locks the medium tree for writing.
  */
-HRESULT Medium::init(VirtualBox *aVirtualBox,
-                     Medium *aParent,
-                     DeviceType_T aDeviceType,
-                     const Guid &uuidMachineRegistry,
-                     const settings::Medium &data,
-                     const Utf8Str &strMachineFolder)
-{
-    using namespace settings;
-
-    AssertReturn(aVirtualBox, E_INVALIDARG);
-
-    /* Enclose the state transition NotReady->InInit->Ready */
-    AutoInitSpan autoInitSpan(this);
-    AssertReturn(autoInitSpan.isOk(), E_FAIL);
-
-    HRESULT rc = S_OK;
-
-    unconst(m->pVirtualBox) = aVirtualBox;
+HRESULT Medium::initOne(Medium *aParent,
+                        DeviceType_T aDeviceType,
+                        const Guid &uuidMachineRegistry,
+                        const settings::Medium &data,
+                        const Utf8Str &strMachineFolder)
+{
+    HRESULT rc;
 
     if (uuidMachineRegistry.isValid() && !uuidMachineRegistry.isZero())
@@ -1258 +1246 @@
         // differencing medium: add to parent
         AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-        m->pParent = aParent;
-        aParent->m->llChildren.push_back(this);
+        // no need to check maximum depth as settings reading did it
+        i_setParent(aParent);
     }
 
@@ -1369 +1357 @@
                      m->strLocationFull.c_str(), m->strFormat.c_str(), m->id.raw()));
 
+    return S_OK;
+}
+
+/**
+ * Initializes the medium object and its children by loading its data from the
+ * given settings node. The medium will always be opened read/write.
+ *
+ * In this case, since we're loading from a registry, uuidMachineRegistry is
+ * always set: it's either the global registry UUID or a machine UUID when
+ * loading from a per-machine registry.
+ *
+ * @param aVirtualBox   VirtualBox object.
+ * @param aParent       Parent medium disk or NULL for a root (base) medium.
+ * @param aDeviceType   Device type of the medium.
+ * @param uuidMachineRegistry The registry to which this medium should be added (global registry UUID or machine UUID).
+ * @param data          Configuration settings.
+ * @param strMachineFolder The machine folder with which to resolve relative paths; if empty, then we use the VirtualBox home directory
+ *
+ * @note Locks the medium tree for writing.
+ */
+HRESULT Medium::init(VirtualBox *aVirtualBox,
+                     Medium *aParent,
+                     DeviceType_T aDeviceType,
+                     const Guid &uuidMachineRegistry,
+                     const settings::Medium &data,
+                     const Utf8Str &strMachineFolder,
+                     AutoWriteLock &mediaTreeLock)
+{
+    using namespace settings;
+
+    Assert(aVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    AssertReturn(aVirtualBox, E_INVALIDARG);
+
+    /* Enclose the state transition NotReady->InInit->Ready */
+    AutoInitSpan autoInitSpan(this);
+    AssertReturn(autoInitSpan.isOk(), E_FAIL);
+
+    unconst(m->pVirtualBox) = aVirtualBox;
+
+    // Do not inline this method call, as the purpose of having this separate
+    // is to save on stack size. Less local variables are the key for reaching
+    // deep recursion levels with small stack (XPCOM/g++ without optimization).
+    HRESULT rc = initOne(aParent, aDeviceType, uuidMachineRegistry, data, strMachineFolder);
+
+
     /* Don't call Medium::i_queryInfo for registered media to prevent the calling
      * thread (i.e. the VirtualBox server startup thread) from an unexpected
@@ -1390 +1423 @@
                            uuidMachineRegistry,
                            med,               // child data
-                           strMachineFolder);
+                           strMachineFolder,
+                           mediaTreeLock);
         if (FAILED(rc)) break;
 
-        AutoWriteLock treeLock(aVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-
-        rc = m->pVirtualBox->i_registerMedium(pMedium, &pMedium, treeLock);
+        rc = m->pVirtualBox->i_registerMedium(pMedium, &pMedium, mediaTreeLock);
         if (FAILED(rc)) break;
     }
@@ -3073 +3105 @@
                             m->strLocationFull.c_str(), m->backRefs.size());
 
-        if (m->llChildren.size() != 0)
+        if (i_getChildren().size() != 0)
             return setError(VBOX_E_INVALID_OBJECT_STATE,
                             tr("Cannot encrypt medium '%s' because it has %d children"),
-                            m->strLocationFull.c_str(), m->llChildren.size());
+                            m->strLocationFull.c_str(), i_getChildren().size());
 
         /* Build the medium lock list. */
@@ -3127 +3159 @@
                 break;
             }
-            else if (pMedium->m->llChildren.size() > 1)
+            else if (pMedium->i_getChildren().size() > 1)
             {
                 rc = setError(VBOX_E_INVALID_OBJECT_STATE,
                               tr("Cannot encrypt medium '%s' because it has %d children"),
-                              pMedium->m->strLocationFull.c_str(), pMedium->m->llChildren.size());
+                              pMedium->m->strLocationFull.c_str(), pMedium->i_getChildren().size());
                 break;
             }
@@ -3473 +3505 @@
  * See getFirstRegistryMachineId() for details.
  *
- * If fRecurse == true, then the media tree lock must be held for reading.
- *
  * @param id
- * @param fRecurse If true, recurses into child media to make sure the whole tree has registries in sync.
  * @return true if the registry was added; false if the given id was already on the list.
  */
-bool Medium::i_addRegistry(const Guid& id, bool fRecurse)
+bool Medium::i_addRegistry(const Guid& id)
 {
     AutoCaller autoCaller(this);
@@ -3511 +3540 @@
         m->llRegistryIDs.push_back(id);
 
-    if (fRecurse)
-    {
-        // Get private list of children and release medium lock straight away.
-        MediaList llChildren(m->llChildren);
-        alock.release();
-
-        for (MediaList::iterator it = llChildren.begin();
-             it != llChildren.end();
-             ++it)
-        {
-            Medium *pChild = *it;
-            fAdd |= pChild->i_addRegistry(id, true);
-        }
-    }
-
     return fAdd;
 }
 
 /**
- * Removes the given UUID from the list of media registry UUIDs. Returns true
- * if found or false if not.
- *
- * If fRecurse == true, then the media tree lock must be held for reading.
+ * This adds the given UUID to the list of media registries in which this
+ * medium should be registered. The UUID can either be a machine UUID,
+ * to add a machine registry, or the global registry UUID as returned by
+ * VirtualBox::getGlobalRegistryId(). This recurses over all children.
+ *
+ * Note that for hard disks, this method does nothing if the medium is
+ * already in another registry to avoid having hard disks in more than
+ * one registry, which causes trouble with keeping diff images in sync.
+ * See getFirstRegistryMachineId() for details.
+ *
+ * @note the caller must hold the media tree lock for reading.
  *
  * @param id
- * @param fRecurse If true, recurses into child media to make sure the whole tree has registries in sync.
- * @return
- */
-bool Medium::i_removeRegistry(const Guid& id, bool fRecurse)
+ * @return true if the registry was added; false if the given id was already on the list.
+ */
+bool Medium::i_addRegistryRecursive(const Guid &id)
+{
+    AutoCaller autoCaller(this);
+    if (FAILED(autoCaller.rc()))
+        return false;
+
+    bool fAdd = i_addRegistry(id);
+
+    // protected by the medium tree lock held by our original caller
+    for (MediaList::const_iterator it = i_getChildren().begin();
+         it != i_getChildren().end();
+         ++it)
+    {
+        Medium *pChild = *it;
+        fAdd |= pChild->i_addRegistryRecursive(id);
+    }
+
+    return fAdd;
+}
+
+/**
+ * Removes the given UUID from the list of media registry UUIDs of this medium.
+ *
+ * @param id
+ * @return true if the UUID was found or false if not.
+ */
+bool Medium::i_removeRegistry(const Guid &id)
 {
     AutoCaller autoCaller(this);
@@ -3548 +3594 @@
     bool fRemove = false;
 
+    // @todo r=klaus eliminate this code, replace it by using find.
     for (GuidList::iterator it = m->llRegistryIDs.begin();
          it != m->llRegistryIDs.end();
@@ -3554 +3601 @@
         if ((*it) == id)
         {
+            // getting away with this as the iterator isn't used after
             m->llRegistryIDs.erase(it);
             fRemove = true;
@@ -3560 +3608 @@
     }
 
-    if (fRecurse)
-    {
-        // Get private list of children and release medium lock straight away.
-        MediaList llChildren(m->llChildren);
-        alock.release();
-
-        for (MediaList::iterator it = llChildren.begin();
-             it != llChildren.end();
-             ++it)
-        {
-            Medium *pChild = *it;
-            fRemove |= pChild->i_removeRegistry(id, true);
-        }
+    return fRemove;
+}
+
+/**
+ * Removes the given UUID from the list of media registry UUIDs, for this
+ * medium and all its children recursively.
+ *
+ * @note the caller must hold the media tree lock for reading.
+ *
+ * @param id
+ * @return true if the UUID was found or false if not.
+ */
+bool Medium::i_removeRegistryRecursive(const Guid &id)
+{
+    AutoCaller autoCaller(this);
+    if (FAILED(autoCaller.rc()))
+        return false;
+
+    bool fRemove = i_removeRegistry(id);
+
+    // protected by the medium tree lock held by our original caller
+    for (MediaList::const_iterator it = i_getChildren().begin();
+         it != i_getChildren().end();
+         ++it)
+    {
+        Medium *pChild = *it;
+        fRemove |= pChild->i_removeRegistryRecursive(id);
     }
 
@@ -3586 +3648 @@
  * @return
  */
-bool Medium::i_isInRegistry(const Guid& id)
-{
+bool Medium::i_isInRegistry(const Guid &id)
+{
+    // @todo r=klaus eliminate this code, replace it by using find.
     for (GuidList::const_iterator it = m->llRegistryIDs.begin();
          it != m->llRegistryIDs.end();
@@ -3832 +3895 @@
         return &m->backRefs.front().machineId;
 
-    for (MediaList::iterator it = m->llChildren.begin();
-         it != m->llChildren.end();
+    for (MediaList::const_iterator it = i_getChildren().begin();
+         it != i_getChildren().end();
          ++it)
     {
@@ -3981 +4044 @@
 
     return pBase;
+}
+
+/**
+ * Returns the depth of this medium in the media chain.
+ *
+ * @note Locks medium tree for reading.
+ */
+uint32_t Medium::i_getDepth()
+{
+    /* 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 1;
+
+    /* we access mParent */
+    AutoReadLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+    uint32_t cDepth = 0;
+    ComObjPtr<Medium> pMedium(this);
+    while (!pMedium.isNull())
+    {
+        AutoCaller autoCaller(this);
+        AssertReturn(autoCaller.isOk(), cDepth + 1);
+
+        pMedium = pMedium->m->pParent;
+        cDepth++;
+    }
+
+    return cDepth;
 }
 
@@ -4041 +4134 @@
 
 /**
- * Saves medium data by appending a new child node to the given
- * parent XML settings node.
+ * Saves the settings of one medium.
+ *
+ * @note Caller MUST take care of the medium tree lock and caller.
  *
  * @param data      Settings struct to be updated.
  * @param strHardDiskFolder Folder for which paths should be relative.
- *
- * @note Locks this object, medium tree and children for reading.
- */
-HRESULT Medium::i_saveSettings(settings::Medium &data,
-                               const Utf8Str &strHardDiskFolder)
-{
-    AutoCaller autoCaller(this);
-    if (FAILED(autoCaller.rc())) return autoCaller.rc();
-
-    /* we access mParent */
-    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-
+ */
+void Medium::i_saveSettingsOne(settings::Medium &data, const Utf8Str &strHardDiskFolder)
+{
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -4117 +4202 @@
     if (m->pParent.isNull())
         data.hdType = m->type;
+}
+
+/**
+ * Saves medium data by putting it into the provided data structure.
+ * Recurses over all children to save their settings, too.
+ *
+ * @param data      Settings struct to be updated.
+ * @param strHardDiskFolder Folder for which paths should be relative.
+ *
+ * @note Locks this object, medium tree and children for reading.
+ */
+HRESULT Medium::i_saveSettings(settings::Medium &data,
+                               const Utf8Str &strHardDiskFolder)
+{
+    AutoCaller autoCaller(this);
+    if (FAILED(autoCaller.rc())) return autoCaller.rc();
+
+    /* we access mParent */
+    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+    i_saveSettingsOne(data, strHardDiskFolder);
 
     /* save all children */
+    settings::MediaList &llSettingsChildren = data.llChildren;
     for (MediaList::const_iterator it = i_getChildren().begin();
          it != i_getChildren().end();
          ++it)
     {
-        settings::Medium med;
-        HRESULT rc = (*it)->i_saveSettings(med, strHardDiskFolder);
-        AssertComRCReturnRC(rc);
-        data.llChildren.push_back(med);
+        // Use the element straight in the list to reduce both unnecessary
+        // deep copying (when unwinding the recursion the entire medium
+        // settings sub-tree is copied) and the stack footprint (the settings
+        // need almost 1K, and there can be VMs with long image chains.
+        llSettingsChildren.push_back(settings::g_MediumEmpty);
+        HRESULT rc = (*it)->i_saveSettings(llSettingsChildren.back(), strHardDiskFolder);
+        if (FAILED(rc))
+        {
+            llSettingsChildren.pop_back();
+            return rc;
+        }
     }
 
@@ -6120 +6234 @@
                     if (m->pParent)
                         i_deparent();
+
+                    if (!pParent.isNull())
+                        if (pParent->i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+                        {
+                            AutoReadLock plock(pParent COMMA_LOCKVAL_SRC_POS);
+                            throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                                           tr("Cannot open differencing image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                                           pParent->m->strLocationFull.c_str());
+                        }
                     i_setParent(pParent);
 
@@ -6353 +6476 @@
         {
             // re-associate with the parent as we are still relatives in the registry
-            m->pParent = pParentBackup;
-            m->pParent->m->llChildren.push_back(this);
+            i_setParent(pParentBackup);
         }
     }
@@ -7346 +7468 @@
     try
     {
+        if (i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+        {
+            AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+            throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                           tr("Cannot create differencing image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                           m->strLocationFull.c_str());
+        }
+
         /* Lock both in {parent,child} order. */
         AutoMultiWriteLock2 mediaLock(this, pTarget COMMA_LOCKVAL_SRC_POS);
@@ -7467 +7597 @@
         Assert(pTarget->m->pParent.isNull());
 
-        /* associate the child with the parent */
-        pTarget->m->pParent = this;
-        m->llChildren.push_back(pTarget);
-
-        /** @todo r=klaus neither target nor base() are locked,
-            * potential race! */
+        /* associate child with the parent, maximum depth was checked above */
+        pTarget->i_setParent(this);
+
         /* diffs for immutable media are auto-reset by default */
-        pTarget->m->autoReset = (i_getBase()->m->type == MediumType_Immutable);
+        bool fAutoReset;
+        {
+            ComObjPtr<Medium> pBase = i_getBase();
+            AutoReadLock block(pBase COMMA_LOCKVAL_SRC_POS);
+            fAutoReset = (pBase->m->type == MediumType_Immutable);
+        }
+        {
+            AutoWriteLock tlock(pTarget COMMA_LOCKVAL_SRC_POS);
+            pTarget->m->autoReset = fAutoReset;
+        }
 
         /* register with mVirtualBox as the last step and move to
@@ -7548 +7684 @@
     try
     {
+        if (!task.mParentForTarget.isNull())
+            if (task.mParentForTarget->i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+            {
+                AutoReadLock plock(task.mParentForTarget COMMA_LOCKVAL_SRC_POS);
+                throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                               tr("Cannot merge image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                               task.mParentForTarget->m->strLocationFull.c_str());
+            }
+
         PVBOXHDD hdd;
         int vrc = VDCreate(m->vdDiskIfaces, i_convertDeviceType(), &hdd);
@@ -7702 +7847 @@
             AssertComRC(rc2);
 
-            /* then, reparent it and disconnect the deleted branch at
-             * both ends (chain->parent() is source's parent) */
+            /* then, reparent it and disconnect the deleted branch at both ends
+             * (chain->parent() is source's parent). Depth check above. */
             pTarget->i_deparent();
-            pTarget->m->pParent = task.mParentForTarget;
-            if (pTarget->m->pParent)
-            {
-                pTarget->m->pParent->m->llChildren.push_back(pTarget);
+            pTarget->i_setParent(task.mParentForTarget);
+            if (task.mParentForTarget)
                 i_deparent();
-            }
 
             /* then, register again */
@@ -7743 +7885 @@
 
                     pMedium->i_deparent();  // removes pMedium from source
+                    // no depth check, reduces depth
                     pMedium->i_setParent(pTarget);
                 }
@@ -7859 +8002 @@
     try
     {
+        if (!pParent.isNull())
+            if (pParent->i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+            {
+                AutoReadLock plock(pParent COMMA_LOCKVAL_SRC_POS);
+                throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                               tr("Cannot clone image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                               pParent->m->strLocationFull.c_str());
+            }
+
         /* Lock all in {parent,child} order. The lock is also used as a
          * signal from the task initiator (which releases it only after
@@ -7983 +8135 @@
                 }
 
-                /** @todo r=klaus target isn't locked, race getting the state */
+                /* target isn't locked, but no changing data is accessed */
                 if (task.midxSrcImageSame == UINT32_MAX)
                 {
@@ -8053 +8205 @@
         if (pParent)
         {
-            /* associate the clone with the parent and deassociate
-             * from VirtualBox */
-            pTarget->m->pParent = pParent;
-            pParent->m->llChildren.push_back(pTarget);
+            /* Associate the clone with the parent and deassociate
+             * from VirtualBox. Depth check above. */
+            pTarget->i_setParent(pParent);
 
             /* register with mVirtualBox as the last step and move to
@@ -8692 +8843 @@
     try
     {
+        if (!pParent.isNull())
+            if (pParent->i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+            {
+                AutoReadLock plock(pParent COMMA_LOCKVAL_SRC_POS);
+                throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                               tr("Cannot import image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                               pParent->m->strLocationFull.c_str());
+            }
+
         /* Lock all in {parent,child} order. The lock is also used as a
          * signal from the task initiator (which releases it only after
@@ -8799 +8959 @@
                 }
 
-                /** @todo r=klaus target isn't locked, race getting the state */
                 vrc = VDCopy(hdd,
                              VD_LAST_IMAGE,
@@ -8848 +9007 @@
         if (pParent)
         {
-            /* associate the imported medium with the parent and deassociate
-             * from VirtualBox */
-            m->pParent = pParent;
-            pParent->m->llChildren.push_back(this);
+            /* Associate the imported medium with the parent and deassociate
+             * from VirtualBox. Depth check above. */
+            i_setParent(pParent);
 
             /* register with mVirtualBox as the last step and move to

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

@@ -1 +1 @@
 /* $Id$ */
 /** @file
- *
  * COM class implementation for Snapshot and SnapshotMachine in VBoxSVC.
  */
@@ -175 +174 @@
     m->llChildren.clear();          // this unsets all the ComPtrs and probably calls delete
 
+    // since there is no guarantee anyone holds a reference to us except the
+    // list of children in our parent, make sure that the reference count
+    // will not drop to 0 before we've declared ourselves as uninitialized,
+    // otherwise there will be another uninit call which causes a self-deadlock
+    // because this uninit isn't complete yet.
+    ComObjPtr<Snapshot> pSnapshot(this);
     if (m->pParent)
         i_deparent();
@@ -186 +191 @@
     delete m;
     m = NULL;
+
+    autoUninitSpan.setSucceeded();
+    // see above, now the refcount may reach 0
+    pSnapshot.setNull();
 }
 
@@ -725 +734 @@
 
 /**
- * Internal implementation for Snapshot::saveSnapshot (below). Caller has
- * requested the snapshots tree (machine) lock.
- *
- * @param aNode
- * @param aAttrsOnly
+ * Saves the settings attributes of one snapshot.
+ *
+ * @param data      Target for saving snapshot settings.
  * @return
  */
-HRESULT Snapshot::i_saveSnapshotImpl(settings::Snapshot &data, bool aAttrsOnly)
+HRESULT Snapshot::i_saveSnapshotImplOne(settings::Snapshot &data) const
 {
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -740 +747 @@
     data.timestamp = m->timeStamp;
     data.strDescription = m->strDescription;
-
-    if (aAttrsOnly)
-        return S_OK;
 
     // state file (only if this snapshot is online)
@@ -756 +760 @@
     if (FAILED(rc)) return rc;
 
-    alock.release();
-
-    data.llChildSnapshots.clear();
-
-    if (m->llChildren.size())
-    {
-        for (SnapshotsList::const_iterator it = m->llChildren.begin();
-             it != m->llChildren.end();
-             ++it)
+    return S_OK;
+}
+
+/**
+ * Internal implementation for Snapshot::saveSnapshot (below). Caller has
+ * requested the snapshots tree (machine) lock.
+ *
+ * @param data      Target for saving snapshot settings.
+ * @return
+ */
+HRESULT Snapshot::i_saveSnapshotImpl(settings::Snapshot &data) const
+{
+    HRESULT rc = i_saveSnapshotImplOne(data);
+    if (FAILED(rc))
+        return rc;
+
+    settings::SnapshotsList &llSettingsChildren = data.llChildSnapshots;
+    for (SnapshotsList::const_iterator it = m->llChildren.begin();
+         it != m->llChildren.end();
+         ++it)
+    {
+        // Use the heap (indirectly through the list container) to reduce the
+        // stack footprint, avoiding local settings objects on the stack which
+        // need a lot of stack space. There can be VMs with deeply nested
+        // snapshots. The stack can be quite small, especially with XPCOM.
+        llSettingsChildren.push_back(settings::g_SnapshotEmpty);
+        Snapshot *pSnap = *it;
+        rc = pSnap->i_saveSnapshotImpl(llSettingsChildren.back());
+        if (FAILED(rc))
         {
-           // Use the heap to reduce the stack footprint. Each recursion needs
-           // over 1K, and there can be VMs with deeply nested snapshots. The
-           // stack can be quite small, especially with XPCOM.
-
-            settings::Snapshot *snap = new settings::Snapshot();
-            rc = (*it)->i_saveSnapshotImpl(*snap, aAttrsOnly);
-            if (FAILED(rc))
-            {
-                delete snap;
-                return rc;
-            }
-            data.llChildSnapshots.push_back(*snap);
-            delete snap;
+            llSettingsChildren.pop_back();
+            return rc;
         }
     }
@@ -786 +799 @@
 
 /**
- *  Saves the given snapshot and all its children (unless \a aAttrsOnly is true).
- *  It is assumed that the given node is empty (unless \a aAttrsOnly is true).
- *
- *  @param aNode        <Snapshot> node to save the snapshot to.
- *  @param aSnapshot    Snapshot to save.
- *  @param aAttrsOnly   If true, only update user-changeable attrs.
- */
-HRESULT Snapshot::i_saveSnapshot(settings::Snapshot &data, bool aAttrsOnly)
+ * Saves the given snapshot and all its children.
+ * It is assumed that the given node is empty.
+ *
+ * @param data      Target for saving snapshot settings.
+ */
+HRESULT Snapshot::i_saveSnapshot(settings::Snapshot &data) const
 {
     // snapshots tree is protected by machine lock
     AutoReadLock alock(m->pMachine COMMA_LOCKVAL_SRC_POS);
 
-    return i_saveSnapshotImpl(data, aAttrsOnly);
+    return i_saveSnapshotImpl(data);
 }
 
@@ -804 +815 @@
  * Part of the cleanup engine of Machine::Unregister().
  *
- * This recursively removes all medium attachments from the snapshot's machine
- * and returns the snapshot's saved state file name, if any, and then calls
- * uninit() on "this" itself.
- *
- * This recurses into children first, so the given MediaList receives child
- * media first before their parents. If the caller wants to close all media,
- * they should go thru the list from the beginning to the end because media
- * cannot be closed if they have children.
- *
- * This calls uninit() on itself, so the snapshots tree (beginning with a machine's pFirstSnapshot) becomes invalid after this.
- * It does not alter the main machine's snapshot pointers (pFirstSnapshot, pCurrentSnapshot).
+ * This removes all medium attachments from the snapshot's machine and returns
+ * the snapshot's saved state file name, if any, and then calls uninit() on
+ * "this" itself.
  *
  * Caller must hold the machine write lock (which protects the snapshots tree!)
@@ -824 +827 @@
  * @return
  */
-HRESULT Snapshot::i_uninitRecursively(AutoWriteLock &writeLock,
-                                      CleanupMode_T cleanupMode,
-                                      MediaList &llMedia,
-                                      std::list<Utf8Str> &llFilenames)
-{
-    Assert(m->pMachine->isWriteLockOnCurrentThread());
-
-    HRESULT rc = S_OK;
-
-    // make a copy of the Guid for logging before we uninit ourselves
-#ifdef LOG_ENABLED
-    Guid uuid = i_getId();
-    Utf8Str name = i_getName();
-    LogFlowThisFunc(("Entering for snapshot '%s' {%RTuuid}\n", name.c_str(), uuid.raw()));
-#endif
-
-    // recurse into children first so that the child media appear on
-    // the list first; this way caller can close the media from the
-    // beginning to the end because parent media can't be closed if
-    // they have children
-
-    // make a copy of the children list since uninit() modifies it
-    SnapshotsList llChildrenCopy(m->llChildren);
-    for (SnapshotsList::iterator it = llChildrenCopy.begin();
-         it != llChildrenCopy.end();
-         ++it)
-    {
-        Snapshot *pChild = *it;
-        rc = pChild->i_uninitRecursively(writeLock, cleanupMode, llMedia, llFilenames);
-        if (FAILED(rc))
-            return rc;
-    }
-
+HRESULT Snapshot::i_uninitOne(AutoWriteLock &writeLock,
+                              CleanupMode_T cleanupMode,
+                              MediaList &llMedia,
+                              std::list<Utf8Str> &llFilenames)
+{
     // now call detachAllMedia on the snapshot machine
-    rc = m->pMachine->i_detachAllMedia(writeLock,
-                                       this /* pSnapshot */,
-                                       cleanupMode,
-                                       llMedia);
+    HRESULT rc = m->pMachine->i_detachAllMedia(writeLock,
+                                               this /* pSnapshot */,
+                                               cleanupMode,
+                                               llMedia);
     if (FAILED(rc))
         return rc;
@@ -884 +859 @@
     }
 
-    this->i_beginSnapshotDelete();
-    this->uninit();
-
+    i_beginSnapshotDelete();
+    uninit();
+
+    return S_OK;
+}
+
+/**
+ * Part of the cleanup engine of Machine::Unregister().
+ *
+ * This recursively removes all medium attachments from the snapshot's machine
+ * and returns the snapshot's saved state file name, if any, and then calls
+ * uninit() on "this" itself.
+ *
+ * This recurses into children first, so the given MediaList receives child
+ * media first before their parents. If the caller wants to close all media,
+ * they should go thru the list from the beginning to the end because media
+ * cannot be closed if they have children.
+ *
+ * This calls uninit() on itself, so the snapshots tree (beginning with a machine's pFirstSnapshot) becomes invalid after this.
+ * It does not alter the main machine's snapshot pointers (pFirstSnapshot, pCurrentSnapshot).
+ *
+ * Caller must hold the machine write lock (which protects the snapshots tree!)
+ *
+ * @param writeLock Machine write lock, which can get released temporarily here.
+ * @param cleanupMode Cleanup mode; see Machine::detachAllMedia().
+ * @param llMedia List of media returned to caller, depending on cleanupMode.
+ * @param llFilenames
+ * @return
+ */
+HRESULT Snapshot::i_uninitRecursively(AutoWriteLock &writeLock,
+                                      CleanupMode_T cleanupMode,
+                                      MediaList &llMedia,
+                                      std::list<Utf8Str> &llFilenames)
+{
+    Assert(m->pMachine->isWriteLockOnCurrentThread());
+
+    HRESULT rc = S_OK;
+
+    // make a copy of the Guid for logging before we uninit ourselves
 #ifdef LOG_ENABLED
-    LogFlowThisFunc(("Leaving for snapshot '%s' {%RTuuid}\n", name.c_str(), uuid.raw()));
+    Guid uuid = i_getId();
+    Utf8Str name = i_getName();
+    LogFlowThisFunc(("Entering for snapshot '%s' {%RTuuid}\n", name.c_str(), uuid.raw()));
 #endif
 
-    return S_OK;
+    // Recurse into children first so that the child media appear on the list
+    // first; this way caller can close the media from the beginning to the end
+    // because parent media can't be closed if they have children and
+    // additionally it postpones the uninit() call until we no longer need
+    // anything from the list. Oh, and remember that the child removes itself
+    // from the list, so keep the iterator at the beginning.
+    for (SnapshotsList::const_iterator it = m->llChildren.begin();
+         it != m->llChildren.end();
+         it = m->llChildren.begin())
+    {
+        Snapshot *pChild = *it;
+        rc = pChild->i_uninitRecursively(writeLock, cleanupMode, llMedia, llFilenames);
+        if (FAILED(rc))
+            break;
+    }
+
+    if (SUCCEEDED(rc))
+        rc = i_uninitOne(writeLock, cleanupMode, llMedia, llFilenames);
+
+#ifdef LOG_ENABLED
+    LogFlowThisFunc(("Leaving for snapshot '%s' {%RTuuid}: %Rhrc\n", name.c_str(), uuid.raw(), rc));
+#endif
+
+    return rc;
 }
 

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

@@ -650 +650 @@
  */
 HRESULT VirtualBox::initMedia(const Guid &uuidRegistry,
-                              const settings::MediaRegistry mediaRegistry,
+                              const settings::MediaRegistry &mediaRegistry,
                               const Utf8Str &strMachineFolder)
 {
@@ -674 +674 @@
                                  uuidRegistry,
                                  xmlHD,         // XML data; this recurses to processes the children
-                                 strMachineFolder);
+                                 strMachineFolder,
+                                 treeLock);
         if (FAILED(rc)) return rc;
 
@@ -694 +695 @@
                               uuidRegistry,
                               xmlDvd,
-                              strMachineFolder);
+                              strMachineFolder,
+                              treeLock);
         if (FAILED(rc)) return rc;
 
@@ -714 +716 @@
                               uuidRegistry,
                               xmlFloppy,
-                              strMachineFolder);
+                              strMachineFolder,
+                              treeLock);
         if (FAILED(rc)) return rc;
 
@@ -4014 +4017 @@
             if (pMedium->i_isInRegistry(uuidRegistry))
             {
-                settings::Medium med;
-                rc = pMedium->i_saveSettings(med, strMachineFolder);     // this recurses into child hard disks
-                if (FAILED(rc)) throw rc;
-                llTarget.push_back(med);
+                llTarget.push_back(settings::g_MediumEmpty);
+                rc = pMedium->i_saveSettings(llTarget.back(), strMachineFolder);     // this recurses into child hard disks
+                if (FAILED(rc))
+                {
+                    llTarget.pop_back();
+                    throw rc;
+                }
             }
         }
@@ -4482 +4488 @@
             AutoWriteLock mlock(pMedium COMMA_LOCKVAL_SRC_POS);
 
-            if (pMedium->i_removeRegistry(id, true /* fRecurse */))
+            if (pMedium->i_removeRegistryRecursive(id))
             {
                 // machine ID was found in base medium's registry list:
@@ -4491 +4497 @@
                 {
                     // 2) better registry found: then use that
-                    pMedium->i_addRegistry(*puuidBetter, true /* fRecurse */);
+                    pMedium->i_addRegistryRecursive(*puuidBetter);
                     // 3) and make sure the registry is saved below
                     mlock.release();
@@ -4497 +4503 @@
                     i_markRegistryModified(*puuidBetter);
                     tlock.acquire();
-                    mlock.release();
+                    mlock.acquire();
                 }
             }
@@ -4533 +4539 @@
             if (SUCCEEDED(machineCaller.rc()))
                 ASMAtomicIncU64(&pMachine->uRegistryNeedsSaving);
+        }
+    }
+}
+
+/**
+ * Marks the registry for @a uuid as unmodified, so that it's not saved in
+ * a later call to saveModifiedRegistries().
+ *
+ * @param uuid
+ */
+void VirtualBox::i_unmarkRegistryModified(const Guid &uuid)
+{
+    uint64_t uOld;
+    if (uuid == i_getGlobalRegistryId())
+    {
+        for (;;)
+        {
+            uOld = ASMAtomicReadU64(&m->uRegistryNeedsSaving);
+            if (!uOld)
+                break;
+            if (ASMAtomicCmpXchgU64(&m->uRegistryNeedsSaving, 0, uOld))
+                break;
+            ASMNopPause();
+        }
+    }
+    else
+    {
+        ComObjPtr<Machine> pMachine;
+        HRESULT rc = i_findMachine(uuid,
+                                   false /* fPermitInaccessible */,
+                                   false /* aSetError */,
+                                   &pMachine);
+        if (SUCCEEDED(rc))
+        {
+            AutoCaller machineCaller(pMachine);
+            if (SUCCEEDED(machineCaller.rc()))
+            {
+                for (;;)
+                {
+                    uOld = ASMAtomicReadU64(&pMachine->uRegistryNeedsSaving);
+                    if (!uOld)
+                        break;
+                    if (ASMAtomicCmpXchgU64(&pMachine->uRegistryNeedsSaving, 0, uOld))
+                        break;
+                    ASMNopPause();
+                }
+            }
         }
     }