Fix stack overflow in XMLElement::Accept for deeply nested trees

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<Frame, 10> 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 );
 }
 
 

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, "<child")) != nullptr; p += 6) {
+			++childTagCount;
+		}
+		XMLTest("Accept() on a deeply nested tree visits every level", depth, childTagCount);
+
+		for (int i = depth; i > 0; --i) {
+			chain[i - 1]->DeleteChild(chain[i]);
+		}
+		delete [] chain;
+	}
+
     // ----------- Performance tracking --------------
 	{
 #if defined( _MSC_VER )