From 816d3fa0cd3217d0521f6eb25c3f9768d1f28d84 Mon Sep 17 00:00:00 2001 From: Lee Thomason Date: Mon, 5 Jun 2017 14:35:55 -0700 Subject: [PATCH 1/2] Fix string leaking (and destructors not getting called) when there are XMLNodes that aren't in the document tree --- tinyxml2.cpp | 34 +++++++++++++++++++++++++++++++--- tinyxml2.h | 18 ++++++++++++++++++ xmltest.cpp | 14 ++++++++++++++ 3 files changed, 63 insertions(+), 3 deletions(-) diff --git a/tinyxml2.cpp b/tinyxml2.cpp index 152e3d2..9855dcf 100755 --- a/tinyxml2.cpp +++ b/tinyxml2.cpp @@ -802,6 +802,8 @@ void XMLNode::Unlink( XMLNode* child ) child->_next->_prev = child->_prev; } child->_parent = 0; + child->_next = 0; + child->_prev = 0; } @@ -811,6 +813,9 @@ void XMLNode::DeleteChild( XMLNode* node ) TIXMLASSERT( node->_document == _document ); TIXMLASSERT( node->_parent == this ); Unlink( node ); + TIXMLASSERT(node->_prev == 0); + TIXMLASSERT(node->_next == 0); + TIXMLASSERT(node->_parent == 0); DeleteNode( node ); } @@ -1055,11 +1060,16 @@ char* XMLNode::ParseDeep( char* p, StrPair* parentEndTag, int* curLineNumPtr ) return 0; } -void XMLNode::DeleteNode( XMLNode* node ) +/*static*/ void XMLNode::DeleteNode( XMLNode* node ) { if ( node == 0 ) { return; } + TIXMLASSERT(node->_document); + if (!node->ToDocument()) { + node->_document->MarkInUse(node); + } + MemPool* pool = node->_memPool; node->~XMLNode(); pool->Free( node ); @@ -1070,10 +1080,13 @@ void XMLNode::InsertChildPreamble( XMLNode* insertThis ) const TIXMLASSERT( insertThis ); TIXMLASSERT( insertThis->_document == _document ); - if ( insertThis->_parent ) + if (insertThis->_parent) { insertThis->_parent->Unlink( insertThis ); - else + } + else { + insertThis->_document->MarkInUse(insertThis); insertThis->_memPool->SetTracked(); + } } const XMLElement* XMLNode::ToElementWithName( const char* name ) const @@ -1979,9 +1992,24 @@ XMLDocument::~XMLDocument() } +void XMLDocument::MarkInUse(XMLNode* node) +{ + TIXMLASSERT(node->_parent == 0); + + for (int i = 0; i < _unlinked.Size(); ++i) { + if (node == _unlinked[i]) { + _unlinked.SwapRemove(i); + break; + } + } +} + void XMLDocument::Clear() { DeleteChildren(); + while( _unlinked.Size()) { + DeleteNode(_unlinked[0]); // Will remove from _unlinked as part of delete. + } #ifdef DEBUG const bool hadError = Error(); diff --git a/tinyxml2.h b/tinyxml2.h index 94c9c6f..a740dc3 100755 --- a/tinyxml2.h +++ b/tinyxml2.h @@ -264,6 +264,12 @@ public: return _allocated; } + void SwapRemove(int i) { + TIXMLASSERT(i < _size); + _mem[i] = _mem[_size - 1]; + --_size; + } + const T* Mem() const { TIXMLASSERT( _mem ); return _mem; @@ -1802,6 +1808,9 @@ public: // internal char* Identify( char* p, XMLNode** node ); + // internal + void MarkInUse(XMLNode*); + virtual XMLNode* ShallowClone( XMLDocument* /*document*/ ) const { return 0; } @@ -1822,6 +1831,13 @@ private: int _errorLineNum; char* _charBuffer; int _parseCurLineNum; + // Memory tracking does add some overhead. + // However, the code assumes that you don't + // have a bunch of unlinked nodes around. + // Therefore it takes less memory to track + // in the document vs. a linked list in the XMLNode, + // and the performance is the same. + DynArray _unlinked; MemPoolT< sizeof(XMLElement) > _elementPool; MemPoolT< sizeof(XMLAttribute) > _attributePool; @@ -1844,6 +1860,8 @@ inline NodeType* XMLDocument::CreateUnlinkedNode( MemPoolT& poo NodeType* returnNode = new (pool.Alloc()) NodeType( this ); TIXMLASSERT( returnNode ); returnNode->_memPool = &pool; + + _unlinked.Push(returnNode); return returnNode; } diff --git a/xmltest.cpp b/xmltest.cpp index f49f2e8..a580756 100644 --- a/xmltest.cpp +++ b/xmltest.cpp @@ -1641,6 +1641,20 @@ int main( int argc, const char ** argv ) } } + { + // Oh those memory leaks. + // Only way to see these is in the (Windows) allocator tracking. + { + XMLDocument doc; + doc.NewElement("LEAK 1"); + } + { + XMLDocument doc; + XMLElement* ele = doc.NewElement("LEAK 2"); + doc.DeleteNode(ele); + } + } + // ----------- Line Number Tracking -------------- { struct TestUtil: XMLVisitor From b754ddf0fbd86ef1d2e9da536bc29d50488691f2 Mon Sep 17 00:00:00 2001 From: Lee Thomason Date: Wed, 14 Jun 2017 15:02:38 -0700 Subject: [PATCH 2/2] address feedback from review --- tinyxml2.cpp | 4 ++-- tinyxml2.h | 1 + xmltest.cpp | 19 +++++++++++++++++-- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/tinyxml2.cpp b/tinyxml2.cpp index 9855dcf..3182c5a 100755 --- a/tinyxml2.cpp +++ b/tinyxml2.cpp @@ -797,13 +797,13 @@ void XMLNode::Unlink( XMLNode* child ) if ( child->_prev ) { child->_prev->_next = child->_next; + child->_prev = 0; } if ( child->_next ) { child->_next->_prev = child->_prev; + child->_next = 0; } child->_parent = 0; - child->_next = 0; - child->_prev = 0; } diff --git a/tinyxml2.h b/tinyxml2.h index a740dc3..864c8b9 100755 --- a/tinyxml2.h +++ b/tinyxml2.h @@ -265,6 +265,7 @@ public: } void SwapRemove(int i) { + TIXMLASSERT(i >= 0); TIXMLASSERT(i < _size); _mem[i] = _mem[_size - 1]; --_size; diff --git a/xmltest.cpp b/xmltest.cpp index a580756..b86e5af 100644 --- a/xmltest.cpp +++ b/xmltest.cpp @@ -1642,8 +1642,23 @@ int main( int argc, const char ** argv ) } { - // Oh those memory leaks. - // Only way to see these is in the (Windows) allocator tracking. + // Evil memory leaks. + // If an XMLElement (etc) is allocated via NewElement() (etc.) + // and NOT added to the XMLDocument, what happens? + // + // Previously (buggy): + // The memory would be free'd when the XMLDocument is + // destructed. But the destructor wasn't called, so that + // memory allocated by the XMLElement would not be free'd. + // In practice this meant strings allocated by the XMLElement + // would leak. An edge case, but annoying. + // Now: + // The destructor is called. But the list of unlinked nodes + // has to be tracked. This has a minor performance impact + // that can become significant if you have a lot. (But why + // would you do that?) + // The only way to see this bug is in a leak tracker. This + // is compiled in by default on Windows Debug. { XMLDocument doc; doc.NewElement("LEAK 1");