From 4cdd2d69d58cb6921c6cd0b4bd9d964309f81c22 Mon Sep 17 00:00:00 2001 From: afonsojanu Date: Thu, 3 Sep 2026 10:14:49 +0100 Subject: [PATCH] Stop DeepClone, DeepCopy and node teardown recursing per tree level A tree built up at runtime through the normal Insert*Child API isn't bounded by XMLDocument::DepthTracker the way a parsed one is, so a long enough chain of single-child nodes can push both operations past their stack limit. DeepClone (and DeepCopy, which just calls it per top level node) walked children recursively, and even after the destructor got its sibling-iteration cleanup in #1075, tearing down a node still recursed once per level through DeleteNode for anything nested more than a couple of siblings deep. Both now gather the subtree with an explicit, heap backed stack instead. For teardown, every descendant has its own _firstChild link cleared before it's actually destroyed, so each one's derived destructor runs without trying to recurse back into children that are already queued up in the same pass. Added a test with a 60000 level deep tree that reliably overflowed the stack before this change and now runs in well under a second. Reproduces the crash reported in #839 for both DeepClone/DeepCopy and plain destruction; Accept() has the same recursive shape but is a separate code path and isn't touched here. --- tinyxml2.cpp | 71 ++++++++++++++++++++++++++++++++++++++++++---------- xmltest.cpp | 34 ++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/tinyxml2.cpp b/tinyxml2.cpp index 69d93ef0..4ec30c7d 100644 --- a/tinyxml2.cpp +++ b/tinyxml2.cpp @@ -806,15 +806,41 @@ XMLNode::XMLNode( XMLDocument* doc ) : XMLNode::~XMLNode() { - // Fast path: this node is dying, so maintaining _firstChild/_lastChild and - // sibling _prev/_next links is unnecessary. Only _parent must be zeroed to - // satisfy the MarkInUse assertion inside DeleteNode. - XMLNode *currentChild = _firstChild; - while (currentChild != NULL) { - XMLNode *next = currentChild->_next; - currentChild->_parent = 0; - DeleteNode(currentChild); - currentChild = next; + // This node is dying, so maintaining _firstChild/_lastChild and sibling + // _prev/_next links on the way down is unnecessary. + // + // The subtree below is gathered with an explicit, heap-backed stack + // rather than by recursing into each child's own destructor. A tree + // built up at runtime isn't bounded by XMLDocument::DepthTracker the way + // a parsed one is, so a long enough chain of single-child nodes used to + // take down the whole call stack here, one frame per level. Every + // descendant gets its _firstChild cleared before anything is actually + // destroyed, so when DeleteNode later runs each one's own (possibly + // virtual, derived-class) destructor, that destructor finds no children + // left to recurse into, since they're already queued for destruction + // in this same pass. + if ( _firstChild ) { + DynArray toVisit; + DynArray descendants; + for ( XMLNode* child = _firstChild; child; child = child->_next ) { + toVisit.Push( child ); + } + while ( toVisit.Size() > 0 ) { + XMLNode* node = toVisit.Pop(); + descendants.Push( node ); + for ( XMLNode* child = node->_firstChild; child; child = child->_next ) { + toVisit.Push( child ); + } + } + + for ( size_t i = 0; i < descendants.Size(); ++i ) { + descendants[i]->_firstChild = 0; + descendants[i]->_parent = 0; + } + for ( size_t i = 0; i < descendants.Size(); ++i ) { + DeleteNode( descendants[i] ); + } + _firstChild = _lastChild = 0; } if ( _parent ) { @@ -873,10 +899,29 @@ XMLNode* XMLNode::DeepClone(XMLDocument* target) const XMLNode* clone = this->ShallowClone(target); if (!clone) return 0; - for (const XMLNode* child = this->FirstChild(); child; child = child->NextSibling()) { - XMLNode* childClone = child->DeepClone(target); - TIXMLASSERT(childClone); - clone->InsertEndChild(childClone); + // Walk the tree with an explicit, heap-backed stack instead of recursing + // per level. A tree built up at runtime (rather than parsed, where depth + // is bounded by XMLDocument::DepthTracker) can be nested arbitrarily deep, + // and one call frame per level is enough to blow the stack long before + // that. The two stacks below always stay in lockstep: srcStack holds a + // node whose clone has already been made and linked in, dstStack holds + // that clone, and we still owe it a pass over its own children. + DynArray srcStack; + DynArray dstStack; + srcStack.Push(this); + dstStack.Push(clone); + + while (srcStack.Size() > 0) { + const XMLNode* srcNode = srcStack.Pop(); + XMLNode* dstNode = dstStack.Pop(); + + for (const XMLNode* child = srcNode->FirstChild(); child; child = child->NextSibling()) { + XMLNode* childClone = child->ShallowClone(target); + TIXMLASSERT(childClone); + dstNode->InsertEndChild(childClone); + srcStack.Push(child); + dstStack.Push(childClone); + } } return clone; } diff --git a/xmltest.cpp b/xmltest.cpp index db9df56c..c862e007 100644 --- a/xmltest.cpp +++ b/xmltest.cpp @@ -2787,7 +2787,39 @@ int main( int argc, const char ** argv ) } } } - + + { + // A tree assembled at runtime isn't bounded by the parser's + // DepthTracker, so a long chain of single-child elements used to + // blow the stack in DeepClone/DeepCopy (both walked it recursively, + // one call frame per level). 60000 levels reliably overflowed the + // default thread stack before the fix and completes in well under + // a second after it. + const int depth = 60000; + XMLDocument doc; + XMLElement* parent = doc.NewElement("root"); + doc.InsertEndChild(parent); + for (int i = 0; i < depth; ++i) { + parent = parent->InsertNewChildElement("child"); + } + + XMLDocument cloned; + doc.DeepCopy(&cloned); + int clonedDepth = 0; + for (XMLElement* e = cloned.FirstChildElement(); e; e = e->FirstChildElement()) { + ++clonedDepth; + } + XMLTest("DeepCopy of a deeply nested tree doesn't overflow the stack", depth + 1, clonedDepth); + + XMLDocument cloneTarget; + XMLNode* clonedSubtree = doc.FirstChildElement()->DeepClone(&cloneTarget); + int subtreeDepth = 0; + for (XMLNode* n = clonedSubtree; n; n = n->FirstChild()) { + ++subtreeDepth; + } + XMLTest("DeepClone of a deeply nested subtree doesn't overflow the stack", depth + 1, subtreeDepth); + } + // ----------- Performance tracking -------------- { #if defined( _MSC_VER )