Made the servers check for positive id in xml db loading.

Also fixed a memleak when loading an invalid monster attack.

Resolves: Mana-Mantis #215.

Reviewed-by: Thorbjorn.

7 files changed, 96 insertions, 45 deletions

diff --git a/src/game-server/attributemanager.cpp b/src/game-server/attributemanager.cpp
index 7e32d4dc..3c9fab23 100644
--- a/src/game-server/attributemanager.cpp
+++ b/src/game-server/attributemanager.cpp
@@ -61,23 +61,29 @@ void AttributeManager::reload()
     {
         if (xmlStrEqual(attributenode->name, BAD_CAST "attribute"))
         {
-            unsigned int id = XML::getProperty(attributenode, "id", 0);
+            int id = XML::getProperty(attributenode, "id", 0);
 
-            mAttributeMap[id] = std::pair< bool ,
-                              std::vector<struct AttributeInfoType> >
-                              (false , std::vector<struct AttributeInfoType>());
+            if (id <= 0)
+            {
+                LOG_WARN("Attribute manager: attribute '" << id
+                         << "' is invalid and will be ignored.");
+                continue;
+            }
+
+            mAttributeMap[id] = AttributeInfoMap(false,
+                                       std::vector<struct AttributeInfoType>());
 
             unsigned int layerCount = 0;
             for_each_xml_child_node(subnode, attributenode)
             {
                 if (xmlStrEqual(subnode->name, BAD_CAST "modifier"))
                 {
-                    std::string sType = utils::toUpper(XML::getProperty(subnode,
-                                                              "stacktype", ""));
-                    std::string eType = utils::toUpper(XML::getProperty(subnode,
-                                                              "modtype", ""));
-                    std::string tag = utils::toUpper(XML::getProperty(subnode,
-                                                              "tag", ""));
+                    std::string sType = utils::toUpper(
+                                    XML::getProperty(subnode, "stacktype", ""));
+                    std::string eType = utils::toUpper(
+                                    XML::getProperty(subnode, "modtype", ""));
+                    std::string tag = utils::toUpper(
+                                    XML::getProperty(subnode, "tag", ""));
                     AT_TY pSType;
                     AME_TY pEType;
                     if (!sType.empty())
@@ -93,11 +99,12 @@ void AttributeManager::reload()
                                 pSType = TY_NSTB;
                             else
                             {
-                                LOG_WARN("Attribute manager: attribute '" << id
-                                         << "' has unknown stack type '" << sType
-                                         << "', skipping modifier!");
+                                LOG_WARN("Attribute manager: attribute '"
+                                         << id << "' has unknown stack type '"
+                                         << sType << "', skipping modifier!");
                                 fail = true;
                             }
+
                             if (!fail)
                             {
                                 if (eType == "ADDITIVE")
@@ -106,36 +113,41 @@ void AttributeManager::reload()
                                     pEType = AME_MULT;
                                 else
                                 {
-                                    LOG_WARN("Attribute manager: attribute '" << id
-                                             << "' has unknown modification type '" << sType
-                                             << "', skipping modifier!");
+                                    LOG_WARN(
+                                          "Attribute manager: attribute '" << id
+                                          << "' has unknown modification type '"
+                                          << sType << "', skipping modifier!");
                                     fail = true;
                                 }
                                 if (!fail)
                                 {
-                                    mAttributeMap[id].second.push_back(AttributeInfoType(pSType, pEType));
-                                    std::string tag = XML::getProperty(subnode, "tag", "");
+                                    mAttributeMap[id].second.push_back(
+                                          AttributeInfoType(pSType, pEType));
+                                    std::string tag = XML::getProperty(
+                                                            subnode, "tag", "");
 
                                     if (!tag.empty())
-                                        mTagMap.insert(std::make_pair(tag,
-                                                                      std::make_pair(id, layerCount)));
+                                        mTagMap.insert(
+                                        std::make_pair(tag,
+                                            std::make_pair(id, layerCount)));
                                     ++layerCount;
                                 }
                             }
                         }
                         else
-                            LOG_WARN("Attribute manager: attribute '" << id <<
-                                     "' has undefined modification type, skipping modifier!");
+                            LOG_WARN("Attribute manager: attribute '" << id
+                                     << "' has undefined modification type, "
+                                     << "skipping modifier!");
                     }
                     else
                     {
                         LOG_WARN("Attribute manager: attribute '" << id <<
-                                 "' has undefined stack type, skipping modifier!");
+                              "' has undefined stack type, skipping modifier!");
                     }
                 }
             }
-            std::string scope = utils::toUpper(XML::getProperty(attributenode,
-                                                       "scope", std::string()));
+            std::string scope = utils::toUpper(
+                    XML::getProperty(attributenode, "scope", std::string()));
             if (scope.empty())
             {
                 // Give a warning unless scope has been explicitly set to "NONE"
@@ -165,7 +177,7 @@ void AttributeManager::reload()
                 LOG_DEBUG("Attribute manager: attribute '" << id
                           << "' set to have no default scope.");
             }
-        }
+        } // End 'attribute'
     }
 
     LOG_DEBUG("attribute map:");
diff --git a/src/game-server/attributemanager.hpp b/src/game-server/attributemanager.hpp
index 0afac022..c5f1d870 100644
--- a/src/game-server/attributemanager.hpp
+++ b/src/game-server/attributemanager.hpp
@@ -60,10 +60,15 @@ class AttributeManager
 
         const std::string *getTagFromInfo(unsigned int, unsigned int) const;
     private:
-        // attribute id -> { modifiable, { stackable type, effect type }[] }
-        typedef std::map< int, std::pair< bool, std::vector<struct AttributeInfoType> > > AttributeMap;
+        // modifiable, { stackable type, effect type }[]
+        typedef std::pair< bool,
+                       std::vector<struct AttributeInfoType> > AttributeInfoMap;
+
+        // Attribute id -> { modifiable, { stackable type, effect type }[] }
+        typedef std::map< int, AttributeInfoMap > AttributeMap;
         // tag name -> { attribute id, layer }
-        typedef std::map< std::string, std::pair<unsigned int, unsigned int> > TagMap;
+        typedef std::map< std::string,
+                          std::pair<unsigned int, unsigned int> > TagMap;
 
         // being type id -> (*{ stackable type, effect type })[]
         AttributeScopes mAttributeScopes[ATTR_MAX];
diff --git a/src/game-server/itemmanager.cpp b/src/game-server/itemmanager.cpp
index 802197d6..34a6170a 100644
--- a/src/game-server/itemmanager.cpp
+++ b/src/game-server/itemmanager.cpp
@@ -126,10 +126,10 @@ void ItemManager::reload()
             continue;
 
         int id = XML::getProperty(node, "id", 0);
-        if (!id)
+        if (id < 1)
         {
-            LOG_WARN("Item Manager: An item has no ID in "
-                     << mItemReferenceFile << ", and so has been ignored!");
+            LOG_WARN("Item Manager: Item ID: " << id << " is invalid in "
+                     << mItemReferenceFile << ", and will be ignored.");
             continue;
         }
 
diff --git a/src/game-server/mapmanager.cpp b/src/game-server/mapmanager.cpp
index bee43585..ab9b2dc5 100644
--- a/src/game-server/mapmanager.cpp
+++ b/src/game-server/mapmanager.cpp
@@ -70,7 +70,7 @@ int MapManager::initialize(const std::string &mapReferenceFile)
         std::string name = XML::getProperty(node, "name", std::string());
 
         // Test id and map name
-        if (id != 0 && !name.empty())
+        if (id > 0 && !name.empty())
         {
             // Testing if the file is actually in the maps folder
             std::string file = std::string("maps/") + name + ".tmx";
@@ -89,6 +89,18 @@ int MapManager::initialize(const std::string &mapReferenceFile)
                 ++loadedMaps;
             }
         }
+        else
+        {
+            if (name.empty())
+            {
+                LOG_WARN("Invalid unnamed map Id: " << id << '.');
+            }
+            else
+            {
+                LOG_WARN("Invalid map Id: " << id << " for map: "
+                         << name << '.');
+            }
+        }
     }
 
     if (loadedMaps > 0)
diff --git a/src/game-server/monstermanager.cpp b/src/game-server/monstermanager.cpp
index f59799c3..1444e17b 100644
--- a/src/game-server/monstermanager.cpp
+++ b/src/game-server/monstermanager.cpp
@@ -79,10 +79,10 @@ void MonsterManager::reload()
         if (!xmlStrEqual(node->name, BAD_CAST "monster"))
             continue;
 
-        int id = XML::getProperty(node, "id", -1);
+        int id = XML::getProperty(node, "id", 0);
         std::string name = XML::getProperty(node, "name", "unnamed");
 
-        if (id == -1)
+        if (id < 1)
         {
             LOG_WARN("Monster Manager: There is a monster ("
                      << name << ") without ID in "
@@ -210,19 +210,32 @@ void MonsterManager::reload()
                 std::string sElement = XML::getProperty(subnode, "element", "neutral");
                 att->element = elementFromString(sElement);
                 std::string sType = XML::getProperty(subnode, "type", "physical");
-                if (sType == "physical") {att->type = DAMAGE_PHYSICAL; }
-                else if (sType == "magical" || sType == "magic") {att->type = DAMAGE_MAGICAL; }
-                else if (sType == "other") {att->type = DAMAGE_OTHER; }
-                else {
+
+                bool validMonsterAttack = true;
+                if (sType == "physical")
+                {
+                    att->type = DAMAGE_PHYSICAL;
+                }
+                else if (sType == "magical" || sType == "magic")
+                {
+                    att->type = DAMAGE_MAGICAL;
+                }
+                else if (sType == "other")
+                {
+                    att->type = DAMAGE_OTHER;
+                }
+                else
+                {
                     LOG_WARN("Monster manager " << mMonsterReferenceFile
                               <<  ": unknown damage type '" << sType << "'.");
                 }
 
-                if (att->id == 0)
+                if (att->id < 1)
                 {
                     LOG_WARN(mMonsterReferenceFile
                              << ": Attack without ID for monster #"
                              << id << " (" << name << ") - attack ignored");
+                    validMonsterAttack = false;
                 }
                 else if (att->element == ELEMENT_ILLEGAL)
                 {
@@ -230,17 +243,25 @@ void MonsterManager::reload()
                              << ": Attack with unknown element \""
                              << sElement << "\" for monster #" << id
                              << " (" << name << ") - attack ignored");
+                    validMonsterAttack = false;
                 }
                 else if (att->type == -1)
                 {
                     LOG_WARN(mMonsterReferenceFile
                              << ": Attack with unknown type \"" << sType << "\""
                              << " for monster #" << id << " (" << name << ")");
+                    validMonsterAttack = false;
                 }
-                else
+
+                if (validMonsterAttack)
                 {
                     monster->addAttack(att);
                 }
+                else
+                {
+                    delete att;
+                    att = 0;
+                }
 
             }
             else if (xmlStrEqual(subnode->name, BAD_CAST "script"))
diff --git a/src/game-server/skillmanager.cpp b/src/game-server/skillmanager.cpp
index 99c03f0e..2cfcba58 100644
--- a/src/game-server/skillmanager.cpp
+++ b/src/game-server/skillmanager.cpp
@@ -90,7 +90,7 @@ void SkillManager::reload()
                                                     std::string());
                 name = utils::toUpper(name);
                 int id = XML::getProperty(skillnode, "id", 0);
-                if (id && !name.empty())
+                if (id > 0 && !name.empty())
                 {
                     bool duplicateKey = false;
                     for (SkillMap::iterator i = skillMap.begin();
diff --git a/src/game-server/statusmanager.cpp b/src/game-server/statusmanager.cpp
index 21c396e7..008e9827 100644
--- a/src/game-server/statusmanager.cpp
+++ b/src/game-server/statusmanager.cpp
@@ -65,10 +65,11 @@ void StatusManager::reload()
             continue;
 
         int id = XML::getProperty(node, "id", 0);
-        if (id == 0)
+        if (id < 1)
         {
-            LOG_WARN("Status Manager: An (ignored) Status has no ID in "
-                     << statusReferenceFile << "!");
+            LOG_WARN("Status Manager: The status ID: " << id << " in "
+                     << statusReferenceFile
+                     << " is invalid and will be ignored.");
             continue;
         }