Changeset 46720 in vbox

Timestamp:

Jun 21, 2013 10:07:31 AM (12 years ago)

Author:

vboxsync

Message:

Main/xml/Settings.cpp: limit snapshot depth to 250, avoiding crashes
Main/Snapshot: limit snapshot depth to 250
Main/Medium: eliminate some spurious error messages when saving a VM config

Location:

trunk

Files:

• doc/manual/user_ChangeLogImpl.xml (modified) (1 diff)
• include/VBox/settings.h (modified) (3 diffs)
• src/VBox/Main/include/SnapshotImpl.h (modified) (1 diff)
• src/VBox/Main/src-server/MediumImpl.cpp (modified) (1 diff)
• src/VBox/Main/src-server/SnapshotImpl.cpp (modified) (4 diffs)
• src/VBox/Main/xml/Settings.cpp (modified) (7 diffs)

trunk/doc/manual/user_ChangeLogImpl.xml

@@ -27 +27 @@
         <para>Settings: global and per-VM default frontend configuration,
           useful to select the use of alternative VM frontends</para>
+      </listitem>
+
+      <listitem>
+        <para>Settings: limit depth of snapshot tree to 250 levels, as more
+          will lead to decreased performance and may trigger crashes</para>
       </listitem>
 

trunk/include/VBox/settings.h

@@ -50 +50 @@
 #include <list>
 #include <map>
+
+/**
+ * Maximum depth of the snapshot tree, to prevent stack overflows.
+ * XPCOM has a relatively low stack size for its workers, and we have
+ * to avoid crashes due to exceeding the limit both on reading and
+ * writing config files.
+ */
+#define SETTINGS_SNAPSHOT_DEPTH_MAX 250
 
 namespace xml
@@ -1196 +1204 @@
     void readAutostart(const xml::ElementNode *pElmAutostart, Autostart *pAutostart);
     void readGroups(const xml::ElementNode *elmGroups, StringsList *pllGroups);
-    void readSnapshot(const xml::ElementNode &elmSnapshot, Snapshot &snap);
+    void readSnapshot(uint32_t depth, const xml::ElementNode &elmSnapshot, Snapshot &snap);
     void convertOldOSType_pre1_5(com::Utf8Str &str);
     void readMachine(const xml::ElementNode &elmMachine);
@@ -1209 +1217 @@
     void buildAutostartXML(xml::ElementNode *pElmParent, const Autostart *pAutostart);
     void buildGroupsXML(xml::ElementNode *pElmParent, const StringsList *pllGroups);
-    void buildSnapshotXML(xml::ElementNode &elmParent, const Snapshot &snap);
+    void buildSnapshotXML(uint32_t depth, xml::ElementNode &elmParent, const Snapshot &snap);
 
     void bumpSettingsVersionIfNeeded();

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

@@ -101 +101 @@
     const Utf8Str& getStateFilePath() const;
 
+    uint32_t getDepth();
+
     ULONG getChildrenCount();
     ULONG getAllChildrenCount();

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

@@ -3426 +3426 @@
     }
 
+    /* Save the error information now, the implicit restore when this goes
+     * out of scope will throw away spurious additional errors created below. */
+    ErrorInfoKeeper eik;
     for (GuidList::const_iterator it = llRegistryIDs.begin();
          it != llRegistryIDs.end();

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

@@ -496 +496 @@
 
 /**
+ * Returns the depth in the snapshot tree for this snapshot.
+ *
+ * @note takes the snapshot tree lock
+ */
+
+uint32_t Snapshot::getDepth()
+{
+    AutoCaller autoCaller(this);
+    AssertComRC(autoCaller.rc());
+
+    // snapshots tree is protected by machine lock
+    AutoReadLock alock(m->pMachine COMMA_LOCKVAL_SRC_POS);
+
+    uint32_t cDepth = 0;
+    ComObjPtr<Snapshot> pSnap(this);
+    while (!pSnap.isNull())
+    {
+        pSnap = pSnap->m->pParent;
+        cDepth++;
+    }
+
+    return cDepth;
+}
+
+/**
  * Returns the number of direct child snapshots, without grandchildren.
  * Does not recurse.
@@ -791 +816 @@
              ++it)
         {
-            settings::Snapshot snap;
-            rc = (*it)->saveSnapshotImpl(snap, aAttrsOnly);
-            if (FAILED(rc)) return rc;
-
-            data.llChildSnapshots.push_back(snap);
+           // 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)->saveSnapshotImpl(*snap, aAttrsOnly);
+            if (FAILED(rc))
+            {
+                delete snap;
+                return rc;
+            }
+            data.llChildSnapshots.push_back(*snap);
+            delete snap;
         }
     }
@@ -1420 +1453 @@
     AssertReturn(mConsoleTaskData.mLastState == MachineState_Null, E_FAIL);
     AssertReturn(mConsoleTaskData.mSnapshot.isNull(), E_FAIL);
+
+    if (   mData->mCurrentSnapshot
+        && mData->mCurrentSnapshot->getDepth() >= SETTINGS_SNAPSHOT_DEPTH_MAX)
+    {
+        return setError(VBOX_E_INVALID_OBJECT_STATE,
+                        tr("Cannot take another snapshot for machine '%s', because it exceeds the maximum snapshot depth limit. Please delete some earlier snapshot which you no longer need"),
+                        mUserData->s.strName.c_str());
+    }
 
     if (    !fTakingSnapshotOnline
@@ -2108 +2149 @@
     if (childrenCount > 1)
         return setError(VBOX_E_INVALID_OBJECT_STATE,
-                        tr("Snapshot '%s' of the machine '%s' cannot be deleted. because it has %d child snapshots, which is more than the one snapshot allowed for deletion"),
+                        tr("Snapshot '%s' of the machine '%s' cannot be deleted, because it has %d child snapshots, which is more than the one snapshot allowed for deletion"),
                         pSnapshot->getName().c_str(),
                         mUserData->s.strName.c_str(),

trunk/src/VBox/Main/xml/Settings.cpp

@@ -3479 +3479 @@
  * Snapshot structure.
  *
+ * @param depth
  * @param elmSnapshot
  * @param snap
  */
-void MachineConfigFile::readSnapshot(const xml::ElementNode &elmSnapshot,
+void MachineConfigFile::readSnapshot(uint32_t depth,
+                                     const xml::ElementNode &elmSnapshot,
                                      Snapshot &snap)
 {
+    if (depth > SETTINGS_SNAPSHOT_DEPTH_MAX)
+        throw ConfigFileError(this, &elmSnapshot, N_("Maximum snapshot tree depth of %u exceeded"), depth);
+
     Utf8Str strTemp;
 
@@ -3531 +3536 @@
                 if (pelmChildSnapshot->nameEquals("Snapshot"))
                 {
-                    Snapshot child;
-                    readSnapshot(*pelmChildSnapshot, child);
-                    snap.llChildSnapshots.push_back(child);
+                    // 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.
+                    Snapshot *child = new Snapshot();
+                    readSnapshot(depth + 1, *pelmChildSnapshot, *child);
+                    snap.llChildSnapshots.push_back(*child);
+                    delete child;
                 }
             }
@@ -3670 +3680 @@
                 Snapshot snap;
                 // this will recurse into child snapshots, if necessary
-                readSnapshot(*pelmMachineChild, snap);
+                readSnapshot(1, *pelmMachineChild, snap);
                 llFirstSnapshot.push_back(snap);
             }
@@ -4551 +4561 @@
              && (sc.controllerType == StorageControllerType_I82078)
            )
-            // floppy controller already got written into <Hardware>/<FloppyController> in writeHardware()
+            // floppy controller already got written into <Hardware>/<FloppyController> in buildHardwareXML()
             // for pre-1.9 settings
             continue;
@@ -4751 +4761 @@
  * for the root snapshot of a machine, if present; elmParent then points to the <Snapshots> node under the
  * <Machine> node to which <Snapshot> must be added. This may then recurse for child snapshots.
+ *
+ * @param depth
  * @param elmParent
  * @param snap
  */
-void MachineConfigFile::buildSnapshotXML(xml::ElementNode &elmParent,
+void MachineConfigFile::buildSnapshotXML(uint32_t depth,
+                                         xml::ElementNode &elmParent,
                                          const Snapshot &snap)
 {
+    if (depth > SETTINGS_SNAPSHOT_DEPTH_MAX)
+        throw ConfigFileError(this, NULL, N_("Maximum snapshot tree depth of %u exceeded"), SETTINGS_SNAPSHOT_DEPTH_MAX);
+
     xml::ElementNode *pelmSnapshot = elmParent.createChild("Snapshot");
 
@@ -4788 +4804 @@
         {
             const Snapshot &child = *it;
-            buildSnapshotXML(*pelmChildren, child);
+            buildSnapshotXML(depth + 1, *pelmChildren, child);
         }
     }
@@ -4925 +4941 @@
     if (    (fl & BuildMachineXML_IncludeSnapshots)
          && llFirstSnapshot.size())
-        buildSnapshotXML(elmMachine, llFirstSnapshot.front());
+        buildSnapshotXML(1, elmMachine, llFirstSnapshot.front());
 
     buildHardwareXML(elmMachine, hardwareMachine, storageMachine);