Further xmlLoadString memleak fixes (#1577)

qaisjp · web-flow · commit c4cfbb012eeb · 2020-07-29T15:24:08.000+01:00
* Clean up BuildNode

* Move CXMLImpl::BuildNode to CXMLNodeImpl::BuildFromDocument()

* Also free TiXmlDocuments

* test destructors

* fix destructors

* remove test stuff
diff --git a/Client/mods/deathmatch/logic/lua/CLuaMain.cpp b/Client/mods/deathmatch/logic/lua/CLuaMain.cpp
@@ -57,11 +57,6 @@ CLuaMain::~CLuaMain()
 
     // Delete the timer manager
     delete m_pLuaTimerManager;
-    
-    for (auto& xmlNode : m_XMLNodes)
-    {
-        delete xmlNode;
-    }
 
     CClientPerfStatLuaMemory::GetSingleton()->OnLuaMainDestroy(this);
     CClientPerfStatLuaTiming::GetSingleton()->OnLuaMainDestroy(this);
@@ -370,10 +365,13 @@ CXMLFile* CLuaMain::CreateXML(const char* szFilename, bool bUseIDs, bool bReadOn
 
 CXMLNode* CLuaMain::ParseString(const char* strXmlContent)
 {
-    CXMLNode* xmlNode = g_pCore->GetXML()->ParseString(strXmlContent);
-    if (xmlNode)
-        m_XMLNodes.push_back(xmlNode);
-    return xmlNode;
+    auto xmlStringNode = g_pCore->GetXML()->ParseString(strXmlContent);
+    if (!xmlStringNode)
+        return nullptr;
+
+    auto node = xmlStringNode->node;
+    m_XMLStringNodes.emplace(std::move(xmlStringNode));
+    return node;
 }
 
 bool CLuaMain::DestroyXML(CXMLFile* pFile)
diff --git a/Client/mods/deathmatch/logic/lua/CLuaMain.h b/Client/mods/deathmatch/logic/lua/CLuaMain.h
@@ -95,8 +95,8 @@ class CLuaMain            //: public CClient
 
     class CResource* m_pResource;
 
-    std::list<CXMLFile*> m_XMLFiles;
-    std::list<CXMLNode*> m_XMLNodes;
+    std::list<CXMLFile*>                            m_XMLFiles;
+    std::unordered_set<std::unique_ptr<SXMLString>> m_XMLStringNodes;
     static SString       ms_strExpectedUndumpHash;
 
     bool m_bEnableOOP;
diff --git a/Server/mods/deathmatch/logic/lua/CLuaMain.cpp b/Server/mods/deathmatch/logic/lua/CLuaMain.cpp
@@ -77,11 +77,6 @@ CLuaMain::~CLuaMain()
         delete xmlFile;
     }
 
-    for (auto& xmlNode : m_XMLNodes)
-    {
-        delete xmlNode;
-    }
-
     // Eventually delete the text displays the LUA script didn't
     list<CTextDisplay*>::iterator iterDisplays = m_Displays.begin();
     for (; iterDisplays != m_Displays.end(); ++iterDisplays)
@@ -422,10 +417,13 @@ CXMLFile* CLuaMain::CreateXML(const char* szFilename, bool bUseIDs, bool bReadOn
 
 CXMLNode* CLuaMain::ParseString(const char* strXmlContent)
 {
-    CXMLNode* xmlNode = g_pServerInterface->GetXML()->ParseString(strXmlContent);
-    if (xmlNode)
-        m_XMLNodes.push_back(xmlNode);
-    return xmlNode;
+    auto xmlStringNode = g_pServerInterface->GetXML()->ParseString(strXmlContent);
+    if (!xmlStringNode)
+        return nullptr;
+
+    auto node = xmlStringNode->node;
+    m_XMLStringNodes.emplace(std::move(xmlStringNode));
+    return node;
 }
 
 bool CLuaMain::DestroyXML(CXMLFile* pFile)
diff --git a/Server/mods/deathmatch/logic/lua/CLuaMain.h b/Server/mods/deathmatch/logic/lua/CLuaMain.h
@@ -133,10 +133,10 @@ class CLuaMain            //: public CClient
     CVehicleManager*     m_pVehicleManager;
     CMapManager*         m_pMapManager;
 
-    list<CXMLFile*>     m_XMLFiles;
-    list<CXMLNode*>     m_XMLNodes;
-    list<CTextDisplay*> m_Displays;
-    list<CTextItem*>    m_TextItems;
+    list<CXMLFile*>                                 m_XMLFiles;
+    std::unordered_set<std::unique_ptr<SXMLString>> m_XMLStringNodes;
+    list<CTextDisplay*>                             m_Displays;
+    list<CTextItem*>                                m_TextItems;
 
     bool m_bEnableOOP;
 
diff --git a/Shared/XML/CXMLImpl.cpp b/Shared/XML/CXMLImpl.cpp
@@ -41,7 +41,7 @@ void CXMLImpl::DeleteXML(CXMLFile* pFile)
     delete pFile;
 }
 
-CXMLNode* CXMLImpl::ParseString(const char* strXmlContent)
+std::unique_ptr<SXMLString> CXMLImpl::ParseString(const char* strXmlContent)
 {
     TiXmlDocument* xmlDoc = new TiXmlDocument();
     if (xmlDoc)
@@ -51,29 +51,13 @@ CXMLNode* CXMLImpl::ParseString(const char* strXmlContent)
         {
             TiXmlElement* xmlDocumentRoot = xmlDoc->RootElement();
             CXMLNodeImpl* xmlBaseNode = new CXMLNodeImpl(nullptr, nullptr, *xmlDocumentRoot);
-            CXMLNode*     xmlRootNode = CXMLImpl::BuildNode(xmlBaseNode, xmlDocumentRoot);
-            return xmlRootNode;
+            xmlBaseNode->BuildFromDocument();
+            return std::unique_ptr<SXMLString>(new SXMLStringImpl(xmlDoc, xmlBaseNode));
         }
     }
     return nullptr;
 }
 
-CXMLNode* CXMLImpl::BuildNode(CXMLNodeImpl* xmlParent, TiXmlNode* xmlNode)
-{
-    TiXmlNode*    xmlChild = nullptr;
-    TiXmlElement* xmlChildElement;
-    CXMLNodeImpl* xmlChildNode;
-    while (xmlChild = xmlNode->IterateChildren(xmlChild))
-    {
-        xmlChildElement = xmlChild->ToElement();
-        if (xmlChildElement)
-        {
-            xmlChildNode = new CXMLNodeImpl(nullptr, xmlParent, *xmlChildElement);
-            CXMLImpl::BuildNode(xmlChildNode, xmlChildElement);
-        }
-    }
-    return xmlParent;
-}
 
 CXMLNode* CXMLImpl::CreateDummyNode()
 {
diff --git a/Shared/XML/CXMLImpl.h b/Shared/XML/CXMLImpl.h
@@ -13,16 +13,26 @@
 
 #include <xml/CXML.h>
 
+typedef struct SXMLStringImpl : SXMLString
+{
+    TiXmlDocument* doc;
+    SXMLStringImpl(TiXmlDocument* d, CXMLNode* n) : doc(d) { node = n; };
+    ~SXMLStringImpl()
+    {
+        delete node;
+        delete doc;
+    };
+} SXMLStringImpl;
+
 class CXMLImpl : public CXML
 {
 public:
     CXMLImpl();
     virtual ~CXMLImpl();
 
-    CXMLFile* CreateXML(const char* szFilename, bool bUseIDs, bool bReadOnly);
-    CXMLNode* ParseString(const char* strXmlContent);
-    CXMLNode* BuildNode(CXMLNodeImpl* xmlParent, TiXmlNode* xmlNode);
-    void      DeleteXML(CXMLFile* pFile);
+    CXMLFile*                   CreateXML(const char* szFilename, bool bUseIDs, bool bReadOnly);
+    std::unique_ptr<SXMLString> ParseString(const char* strXmlContent);
+    void                        DeleteXML(CXMLFile* pFile);
 
     CXMLNode* CreateDummyNode();
 
diff --git a/Shared/XML/CXMLNodeImpl.cpp b/Shared/XML/CXMLNodeImpl.cpp
@@ -74,6 +74,20 @@ CXMLNodeImpl::~CXMLNodeImpl()
     }
 }
 
+void CXMLNodeImpl::BuildFromDocument()
+{
+    TiXmlNode*    xmlChild = nullptr;
+    while (xmlChild = m_pNode->IterateChildren(xmlChild))
+    {
+        auto xmlChildElement = xmlChild->ToElement();
+        if (xmlChildElement)
+        {
+            auto xmlChildNode = new CXMLNodeImpl(nullptr, this, *xmlChildElement);
+            xmlChildNode->BuildFromDocument();
+        }
+    }
+}
+
 CXMLNode* CXMLNodeImpl::CreateSubNode(const char* szTagName, CXMLNode* pInsertAfter)
 {
     TiXmlElement* pNewNode;
diff --git a/Shared/XML/CXMLNodeImpl.h b/Shared/XML/CXMLNodeImpl.h
@@ -24,6 +24,12 @@ class CXMLNodeImpl : public CXMLNode
     CXMLNodeImpl(class CXMLFileImpl* pFile, CXMLNodeImpl* pParent, TiXmlElement& Node);
     ~CXMLNodeImpl();
 
+    // BuildFromDocument recursively builds child CXMLNodeImpl from the underlying TiXmlElement.
+    //
+    // This is **only** used for xmlLoadString right now.
+    // Look elsewhere if you're thinking about XML files. It does things a different way.
+    void BuildFromDocument();
+
     CXMLNode* CreateSubNode(const char* szTagName, CXMLNode* pInsertBefore = nullptr);
     void      DeleteSubNode(CXMLNode* pNode) { delete pNode; };
     void      DeleteAllSubNodes();
diff --git a/Shared/sdk/xml/CXML.h b/Shared/sdk/xml/CXML.h
@@ -14,14 +14,19 @@
 class CXMLNode;
 class CXMLFile;
 class CXMLAttribute;
+typedef struct SXMLString
+{
+    CXMLNode* node;
+    virtual ~SXMLString(){};
+} SXMLString;
 
 class CXML
 {
 public:
-    virtual CXMLFile* CreateXML(const char* szFilename, bool bUseIDs = false, bool bReadOnly = false) = 0;
-    virtual CXMLNode* ParseString(const char* strXmlContent) = 0;
-    virtual void      DeleteXML(CXMLFile* pFile) = 0;
-    virtual CXMLNode* CreateDummyNode() = 0;
+    virtual CXMLFile*                   CreateXML(const char* szFilename, bool bUseIDs = false, bool bReadOnly = false) = 0;
+    virtual std::unique_ptr<SXMLString> ParseString(const char* strXmlContent) = 0;
+    virtual void                        DeleteXML(CXMLFile* pFile) = 0;
+    virtual CXMLNode*                   CreateDummyNode() = 0;
 
     virtual CXMLAttribute* GetAttrFromID(unsigned long ulID) = 0;
     virtual CXMLFile*      GetFileFromID(unsigned long ulID) = 0;

Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,7 @@ void CXMLImpl::DeleteXML(CXMLFile* pFile)`
`41`	`41`	`delete pFile;`
`42`	`42`	`}`
`43`	`43`
`44`		`-CXMLNode* CXMLImpl::ParseString(const char* strXmlContent)`
	`44`	`+std::unique_ptr<SXMLString> CXMLImpl::ParseString(const char* strXmlContent)`
`45`	`45`	`{`
`46`	`46`	`TiXmlDocument* xmlDoc = new TiXmlDocument();`
`47`	`47`	`if (xmlDoc)`
`@@ -51,29 +51,13 @@ CXMLNode* CXMLImpl::ParseString(const char* strXmlContent)`
`51`	`51`	`{`
`52`	`52`	`TiXmlElement* xmlDocumentRoot = xmlDoc->RootElement();`
`53`	`53`	`CXMLNodeImpl* xmlBaseNode = new CXMLNodeImpl(nullptr, nullptr, *xmlDocumentRoot);`
`54`		`- CXMLNode* xmlRootNode = CXMLImpl::BuildNode(xmlBaseNode, xmlDocumentRoot);`
`55`		`- return xmlRootNode;`
	`54`	`+ xmlBaseNode->BuildFromDocument();`
	`55`	`+ return std::unique_ptr<SXMLString>(new SXMLStringImpl(xmlDoc, xmlBaseNode));`
`56`	`56`	`}`
`57`	`57`	`}`
`58`	`58`	`return nullptr;`
`59`	`59`	`}`
`60`	`60`
`61`		`-CXMLNode* CXMLImpl::BuildNode(CXMLNodeImpl* xmlParent, TiXmlNode* xmlNode)`
`62`		`-{`
`63`		`- TiXmlNode* xmlChild = nullptr;`
`64`		`- TiXmlElement* xmlChildElement;`
`65`		`- CXMLNodeImpl* xmlChildNode;`
`66`		`- while (xmlChild = xmlNode->IterateChildren(xmlChild))`
`67`		`- {`
`68`		`- xmlChildElement = xmlChild->ToElement();`
`69`		`- if (xmlChildElement)`
`70`		`- {`
`71`		`- xmlChildNode = new CXMLNodeImpl(nullptr, xmlParent, *xmlChildElement);`
`72`		`- CXMLImpl::BuildNode(xmlChildNode, xmlChildElement);`
`73`		`- }`
`74`		`- }`
`75`		`- return xmlParent;`
`76`		`-}`
`77`	`61`
`78`	`62`	`CXMLNode* CXMLImpl::CreateDummyNode()`
`79`	`63`	`{`