Index: /trunk/doc/manual/user_ChangeLogImpl.xml =================================================================== --- /trunk/doc/manual/user_ChangeLogImpl.xml (revision 46719) +++ /trunk/doc/manual/user_ChangeLogImpl.xml (revision 46720) @@ -27,4 +27,9 @@ Settings: global and per-VM default frontend configuration, useful to select the use of alternative VM frontends + + + + Settings: limit depth of snapshot tree to 250 levels, as more + will lead to decreased performance and may trigger crashes Index: /trunk/include/VBox/settings.h =================================================================== --- /trunk/include/VBox/settings.h (revision 46719) +++ /trunk/include/VBox/settings.h (revision 46720) @@ -50,4 +50,12 @@ #include #include + +/** + * 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,5 +1204,5 @@ 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,5 +1217,5 @@ 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(); Index: /trunk/src/VBox/Main/include/SnapshotImpl.h =================================================================== --- /trunk/src/VBox/Main/include/SnapshotImpl.h (revision 46719) +++ /trunk/src/VBox/Main/include/SnapshotImpl.h (revision 46720) @@ -101,4 +101,6 @@ const Utf8Str& getStateFilePath() const; + uint32_t getDepth(); + ULONG getChildrenCount(); ULONG getAllChildrenCount(); Index: /trunk/src/VBox/Main/src-server/MediumImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/MediumImpl.cpp (revision 46719) +++ /trunk/src/VBox/Main/src-server/MediumImpl.cpp (revision 46720) @@ -3426,4 +3426,7 @@ } + /* 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(); Index: /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp (revision 46719) +++ /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp (revision 46720) @@ -496,4 +496,29 @@ /** + * 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 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,9 +816,17 @@ ++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,4 +1453,12 @@ 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,5 +2149,5 @@ 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(), Index: /trunk/src/VBox/Main/xml/Settings.cpp =================================================================== --- /trunk/src/VBox/Main/xml/Settings.cpp (revision 46719) +++ /trunk/src/VBox/Main/xml/Settings.cpp (revision 46720) @@ -3479,10 +3479,15 @@ * 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,7 +3536,12 @@ 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,5 +3680,5 @@ Snapshot snap; // this will recurse into child snapshots, if necessary - readSnapshot(*pelmMachineChild, snap); + readSnapshot(1, *pelmMachineChild, snap); llFirstSnapshot.push_back(snap); } @@ -4551,5 +4561,5 @@ && (sc.controllerType == StorageControllerType_I82078) ) - // floppy controller already got written into / in writeHardware() + // floppy controller already got written into / in buildHardwareXML() // for pre-1.9 settings continue; @@ -4751,10 +4761,16 @@ * for the root snapshot of a machine, if present; elmParent then points to the node under the * node to which 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,5 +4804,5 @@ { const Snapshot &child = *it; - buildSnapshotXML(*pelmChildren, child); + buildSnapshotXML(depth + 1, *pelmChildren, child); } } @@ -4925,5 +4941,5 @@ if ( (fl & BuildMachineXML_IncludeSnapshots) && llFirstSnapshot.size()) - buildSnapshotXML(elmMachine, llFirstSnapshot.front()); + buildSnapshotXML(1, elmMachine, llFirstSnapshot.front()); buildHardwareXML(elmMachine, hardwareMachine, storageMachine);