Further xmlLoadString memleak fixes (multitheftauto#1577)

* Clean up BuildNode * Move CXMLImpl::BuildNode to CXMLNodeImpl::BuildFromDocument() * Also free TiXmlDocuments * test destructors * fix destructors * remove test stuff
Grafu · Jul 29, 2020 · c4cfbb0 · c4cfbb0
1 parent 06b6322
commit c4cfbb0
Show file tree

Hide file tree

Showing 9 changed files with 66 additions and 51 deletions.
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;