Fix some leaks in XML DynamicTypes Parser (#4717) (#4735)

* Fix some leaks in XML DynamicTypes Parser (#4717) * Refs #20732. Add regression test. Signed-off-by: Miguel Company <[email protected]> * Refs #20732. Fixes on parseXMLAliasDynamicType. Signed-off-by: Miguel Company <[email protected]> * Refs #20732. Fixes on parseXMLBitsetDynamicType. Signed-off-by: Miguel Company <[email protected]> * Refs #20732. Fixes on parseXMLBitmaskDynamicType. Signed-off-by: Miguel Company <[email protected]> * Refs #20732. Fixes on parseXMLEnumDynamicType. Signed-off-by: Miguel Company <[email protected]> * Refs #20732. Fixes on parseXMLStructDynamicType. Signed-off-by: Miguel Company <[email protected]> * Refs #20732. Fixes on parseXMLUnionDynamicType. Signed-off-by: Miguel Company <[email protected]> * Refs #20372. Return error when `insertDynamicTypeByName` fails. Signed-off-by: Miguel Company <[email protected]> * Refs #20372. Fail parsing for empty name attributes. Signed-off-by: Miguel Company <[email protected]> * Refs #20732. Move implementation of `DeleteInstance` to source file. Signed-off-by: Miguel Company <[email protected]> * Refs #20732. Delete registered dynamic type builders in `XMLProfileManager::DeleteInstance()`. Signed-off-by: Miguel Company <[email protected]> --------- Signed-off-by: Miguel Company <[email protected]> (cherry picked from commit 8cb447b) # Conflicts: # include/fastrtps/xmlparser/XMLProfileManager.h # test/unittest/xmlparser/XMLParserTests.cpp * Fix conflicts Signed-off-by: Miguel Company <[email protected]> --------- Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Miguel Company <[email protected]>
eProsima · May 9, 2024 · 04e90a2 · 04e90a2
1 parent 5eb48f6
commit 04e90a2
Show file tree

Hide file tree

Showing 5 changed files with 113 additions and 22 deletions.
diff --git a/include/fastrtps/xmlparser/XMLProfileManager.h b/include/fastrtps/xmlparser/XMLProfileManager.h
@@ -225,17 +225,7 @@ class XMLProfileManager
      * FastDDS's Domain calls this method automatically on its destructor, but
      * if using XMLProfileManager outside of FastDDS, it should be called manually.
      */
-    RTPS_DllAPI static void DeleteInstance()
-    {
-        participant_profiles_.clear();
-        publisher_profiles_.clear();
-        subscriber_profiles_.clear();
-        requester_profiles_.clear();
-        replier_profiles_.clear();
-        topic_profiles_.clear();
-        xml_files_.clear();
-        transport_profiles_.clear();
-    }
+    RTPS_DllAPI static void DeleteInstance();
 
     /**
      * Retrieves a DynamicPubSubType for the given dynamic type name.

diff --git a/src/cpp/rtps/xmlparser/XMLDynamicParser.cpp b/src/cpp/rtps/xmlparser/XMLDynamicParser.cpp
@@ -341,11 +341,23 @@ XMLP_ret XMLParser::parseXMLAliasDynamicType(
         if (valueBuilder != nullptr)
         {
             const char* name = p_root->Attribute(NAME);
-            if (name != nullptr)
+            if (name != nullptr && name[0] != '\0')
             {
-                p_dynamictypebuilder_t typeBuilder =
-                        types::DynamicTypeBuilderFactory::get_instance()->create_alias_builder(valueBuilder, name);
-                XMLProfileManager::insertDynamicTypeByName(name, typeBuilder);
+                if (nullptr == XMLProfileManager::getDynamicTypeByName(name))
+                {
+                    p_dynamictypebuilder_t typeBuilder =
+                            types::DynamicTypeBuilderFactory::get_instance()->create_alias_builder(valueBuilder, name);
+                    if (false == XMLProfileManager::insertDynamicTypeByName(name, typeBuilder))
+                    {
+                        types::DynamicTypeBuilderFactory::get_instance()->delete_builder(typeBuilder);
+                        ret = XMLP_ret::XML_ERROR;
+                    }
+                }
+                else
+                {
+                    EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing alias type: Type '" << name << "' already defined.");
+                    ret = XMLP_ret::XML_ERROR;
+                }
             }
             else
             {
@@ -394,11 +406,16 @@ XMLP_ret XMLParser::parseXMLBitsetDynamicType(
     uint32_t mId = 0;
 
     const char* name = p_root->Attribute(NAME);
-    if (nullptr == name)
+    if (nullptr == name || name[0] == '\0')
     {
         EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing 'bitsetDcl' type. No name attribute given.");
         return XMLP_ret::XML_ERROR;
     }
+    if (nullptr != XMLProfileManager::getDynamicTypeByName(name))
+    {
+        EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing 'bitsetDcl' type: Type '" << name << "' already defined.");
+        return XMLP_ret::XML_ERROR;
+    }
 
     const char* baseType = p_root->Attribute(BASE_TYPE);
     if (baseType != nullptr)
@@ -441,7 +458,11 @@ XMLP_ret XMLParser::parseXMLBitsetDynamicType(
         }
     }
 
-    XMLProfileManager::insertDynamicTypeByName(name, typeBuilder);
+    if (false == XMLProfileManager::insertDynamicTypeByName(name, typeBuilder))
+    {
+        types::DynamicTypeBuilderFactory::get_instance()->delete_builder(typeBuilder);
+        ret = XMLP_ret::XML_ERROR;
+    }
     return ret;
 }
 
@@ -629,6 +650,12 @@ XMLP_ret XMLParser::parseXMLBitmaskDynamicType(
     {
         return XMLP_ret::XML_ERROR;
     }
+    if (nullptr != XMLProfileManager::getDynamicTypeByName(name))
+    {
+        EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing 'bitmaskDcl' type: Type '" << name << "' already defined.");
+        return XMLP_ret::XML_ERROR;
+    }
+
     p_dynamictypebuilder_t typeBuilder =
             types::DynamicTypeBuilderFactory::get_instance()->create_bitmask_builder(bit_bound);
     typeBuilder->set_name(name);
@@ -653,7 +680,11 @@ XMLP_ret XMLParser::parseXMLBitmaskDynamicType(
         }
     }
 
-    XMLProfileManager::insertDynamicTypeByName(name, typeBuilder);
+    if (false == XMLProfileManager::insertDynamicTypeByName(name, typeBuilder))
+    {
+        types::DynamicTypeBuilderFactory::get_instance()->delete_builder(typeBuilder);
+        ret = XMLP_ret::XML_ERROR;
+    }
     return ret;
 }
 
@@ -678,11 +709,16 @@ XMLP_ret XMLParser::parseXMLEnumDynamicType(
     XMLP_ret ret = XMLP_ret::XML_OK;
     const char* enumName = p_root->Attribute(NAME);
 
-    if (enumName == nullptr)
+    if (enumName == nullptr || enumName[0] == '\0')
     {
         EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing 'enum' type. No name attribute given.");
         return XMLP_ret::XML_ERROR;
     }
+    if (nullptr != XMLProfileManager::getDynamicTypeByName(enumName))
+    {
+        EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing 'enum' type: Type '" << enumName << "' already defined.");
+        return XMLP_ret::XML_ERROR;
+    }
 
     p_dynamictypebuilder_t typeBuilder = types::DynamicTypeBuilderFactory::get_instance()->create_enum_builder();
     uint32_t currValue = 0;
@@ -704,7 +740,11 @@ XMLP_ret XMLParser::parseXMLEnumDynamicType(
         typeBuilder->add_empty_member(currValue++, name);
     }
 
-    XMLProfileManager::insertDynamicTypeByName(enumName, typeBuilder);
+    if (false == XMLProfileManager::insertDynamicTypeByName(enumName, typeBuilder))
+    {
+        types::DynamicTypeBuilderFactory::get_instance()->delete_builder(typeBuilder);
+        ret = XMLP_ret::XML_ERROR;
+    }
     return ret;
 }
 
@@ -729,6 +769,11 @@ XMLP_ret XMLParser::parseXMLStructDynamicType(
         EPROSIMA_LOG_ERROR(XMLPARSER, "Missing required attribute 'name' in 'structDcl'.");
         return XMLP_ret::XML_ERROR;
     }
+    if (nullptr != XMLProfileManager::getDynamicTypeByName(name))
+    {
+        EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing 'structDcl' type: Type '" << name << "' already defined.");
+        return XMLP_ret::XML_ERROR;
+    }
 
     p_dynamictypebuilder_t typeBuilder; // = types::DynamicTypeBuilderFactory::get_instance()->create_struct_builder();
     //typeBuilder->set_name(name);
@@ -774,7 +819,11 @@ XMLP_ret XMLParser::parseXMLStructDynamicType(
         }
     }
 
-    XMLProfileManager::insertDynamicTypeByName(name, typeBuilder);
+    if (false == XMLProfileManager::insertDynamicTypeByName(name, typeBuilder))
+    {
+        types::DynamicTypeBuilderFactory::get_instance()->delete_builder(typeBuilder);
+        ret = XMLP_ret::XML_ERROR;
+    }
 
     return ret;
 }
@@ -805,6 +854,17 @@ XMLP_ret XMLParser::parseXMLUnionDynamicType(
 
     XMLP_ret ret = XMLP_ret::XML_OK;
     const char* name = p_root->Attribute(NAME);
+    if (nullptr == name || name[0] == '\0')
+    {
+        EPROSIMA_LOG_ERROR(XMLPARSER, "Missing required attribute 'name' in 'unionDcl'.");
+        return XMLP_ret::XML_ERROR;
+    }
+    if (nullptr != XMLProfileManager::getDynamicTypeByName(name))
+    {
+        EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing 'unionDcl' type: Type '" << name << "' already defined.");
+        return XMLP_ret::XML_ERROR;
+    }
+
     tinyxml2::XMLElement* p_element = p_root->FirstChildElement(DISCRIMINATOR);
     if (p_element != nullptr)
     {
@@ -868,7 +928,11 @@ XMLP_ret XMLParser::parseXMLUnionDynamicType(
                 }
             }
 
-            XMLProfileManager::insertDynamicTypeByName(name, typeBuilder);
+            if (false == XMLProfileManager::insertDynamicTypeByName(name, typeBuilder))
+            {
+                types::DynamicTypeBuilderFactory::get_instance()->delete_builder(typeBuilder);
+                ret = XMLP_ret::XML_ERROR;
+            }
         }
     }
     else

diff --git a/src/cpp/rtps/xmlparser/XMLProfileManager.cpp b/src/cpp/rtps/xmlparser/XMLProfileManager.cpp
@@ -709,3 +709,26 @@ XMLP_ret XMLProfileManager::extractReplierProfile(
 
     return XMLP_ret::XML_OK;
 }
+
+void XMLProfileManager::DeleteInstance()
+{
+    participant_profiles_.clear();
+    publisher_profiles_.clear();
+    subscriber_profiles_.clear();
+    requester_profiles_.clear();
+    replier_profiles_.clear();
+    topic_profiles_.clear();
+    xml_files_.clear();
+    transport_profiles_.clear();
+
+    // Delete the registered dynamic types builders
+    {
+        namespace dyn_types = eprosima::fastrtps::types;
+        auto factory = dyn_types::DynamicTypeBuilderFactory::get_instance();
+        for (auto&& type : dynamic_types_)
+        {
+            factory->delete_builder(type.second);
+        }
+        dynamic_types_.clear();
+    }
+}
diff --git a/test/unittest/xmlparser/XMLParserTests.cpp b/test/unittest/xmlparser/XMLParserTests.cpp
@@ -66,6 +66,7 @@ TEST_F(XMLParserTests, regressions)
     EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/19851_profile_bin.xml", root));
     EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/simple_participant_profiles_nok.xml", root));
     EXPECT_EQ(XMLP_ret::XML_OK, XMLParser::loadXML("regressions/simple_participant_profiles_ok.xml", root));
+    EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/20732_profile_bin.xml", root));
 }
 
 TEST_F(XMLParserTests, NoFile)

diff --git a/test/unittest/xmlparser/regressions/20732_profile_bin.xml b/test/unittest/xmlparser/regressions/20732_profile_bin.xml