diff --git a/tinyxml2.cpp b/tinyxml2.cpp index 69d93ef0..d94c73f0 100644 --- a/tinyxml2.cpp +++ b/tinyxml2.cpp @@ -2149,14 +2149,52 @@ bool XMLElement::ShallowEqual( const XMLNode* compare ) const bool XMLElement::Accept( XMLVisitor* visitor ) const { TIXMLASSERT( visitor ); - if ( visitor->VisitEnter( *this, _rootAttribute ) ) { - for ( const XMLNode* node=FirstChild(); node; node=node->NextSibling() ) { - if ( !node->Accept( visitor ) ) { - break; + + // A chain of nested elements walked through here the same way DeepClone's + // did before #1091: one call frame per level, recursing through this + // function via node->Accept() for every element child. A tree built up at + // runtime isn't bounded by XMLDocument::DepthTracker the way a parsed one + // is, so it can be nested deep enough to overflow the stack. + // + // Only element children can nest further (text/comment/declaration/unknown + // are always leaves), so those are still visited directly through their own + // Accept(); an explicit, heap-backed stack takes over just for descending + // into element children, standing in for the call stack a recursive + // version would have used. + struct Frame { + const XMLElement* elem; + const XMLNode* next; + }; + DynArray stack; + + const XMLElement* elem = this; + const XMLNode* next = visitor->VisitEnter( *elem, elem->_rootAttribute ) ? elem->FirstChild() : 0; + + for (;;) { + while ( next ) { + const XMLElement* childElem = next->ToElement(); + if ( !childElem ) { + if ( !next->Accept( visitor ) ) { + next = 0; + break; + } + next = next->NextSibling(); + continue; } + stack.Push( Frame{ elem, next->NextSibling() } ); + elem = childElem; + next = visitor->VisitEnter( *elem, elem->_rootAttribute ) ? elem->FirstChild() : 0; + } + + const bool ok = visitor->VisitExit( *elem ); + if ( stack.Size() == 0 ) { + return ok; } + + const Frame parent = stack.Pop(); + elem = parent.elem; + next = ok ? parent.next : 0; } - return visitor->VisitExit( *this ); } diff --git a/xmltest.cpp b/xmltest.cpp index db9df56c..b7187d95 100644 --- a/xmltest.cpp +++ b/xmltest.cpp @@ -2787,7 +2787,44 @@ int main( int argc, const char ** argv ) } } } - + + { + // Accept() recurses into element children the same way DeepClone did + // before #1091 fixed that (see #839): one call frame per level, so a + // chain of single-child elements assembled at runtime (and thus not + // bounded by the parser's DepthTracker) could overflow the stack. + // 60000 levels reliably crashed the old recursive version and now + // completes without issue. + // + // Node teardown has its own, separate recursion (tracked by #1091, + // not fixed here), so the chain is unwound one leaf at a time below + // rather than left for the destructor to walk recursively when doc + // goes out of scope - that would crash regardless of this fix. + const int depth = 60000; + XMLDocument doc; + XMLElement** chain = new XMLElement*[depth + 1]; + chain[0] = doc.NewElement("root"); + doc.InsertEndChild(chain[0]); + for (int i = 0; i < depth; ++i) { + chain[i + 1] = chain[i]->InsertNewChildElement("child"); + } + + XMLPrinter printer(0, true); // compact: avoids O(depth^2) output from indentation + const bool acceptResult = doc.Accept(&printer); + XMLTest("Accept() on a deeply nested tree doesn't overflow the stack", true, acceptResult); + + int childTagCount = 0; + for (const char* p = printer.CStr(); (p = strstr(p, " 0; --i) { + chain[i - 1]->DeleteChild(chain[i]); + } + delete [] chain; + } + // ----------- Performance tracking -------------- { #if defined( _MSC_VER )