Skip to content

Commit

Permalink
Fix some leaks in XML DynamicTypes Parser (#4717) (#4735)
Browse files Browse the repository at this point in the history 
* Fix some leaks in XML DynamicTypes Parser (#4717)

* Refs #20732. Add regression test.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20732. Fixes on parseXMLAliasDynamicType.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20732. Fixes on parseXMLBitsetDynamicType.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20732. Fixes on parseXMLBitmaskDynamicType.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20732. Fixes on parseXMLEnumDynamicType.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20732. Fixes on parseXMLStructDynamicType.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20732. Fixes on parseXMLUnionDynamicType.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20372. Return error when `insertDynamicTypeByName` fails.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20372. Fail parsing for empty name attributes.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20732. Move implementation of `DeleteInstance` to source file.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

* Refs #20732. Delete registered dynamic type builders in `XMLProfileManager::DeleteInstance()`.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

---------

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
(cherry picked from commit 8cb447b)

# Conflicts:
#	include/fastrtps/xmlparser/XMLProfileManager.h
#	test/unittest/xmlparser/XMLParserTests.cpp

* Fix conflicts

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

---------

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
(cherry picked from commit 04e90a2)
  • Loading branch information
@mergify
mergify[bot] committed May 9, 2024
1 parent cf40a06 commit 4c34978
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 22 deletions.
12 changes: 1 addition & 11 deletions include/fastrtps/xmlparser/XMLProfileManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,17 +226,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.
Expand Down
86 changes: 75 additions & 11 deletions src/cpp/rtps/xmlparser/XMLDynamicParser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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')
{
logError(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)
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -628,6 +649,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);
Expand All @@ -652,7 +679,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;
}

Expand All @@ -677,11 +708,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')
{
logError(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;
Expand All @@ -703,7 +739,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;
}

Expand All @@ -728,6 +768,11 @@ XMLP_ret XMLParser::parseXMLStructDynamicType(
logError(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);
Expand Down Expand Up @@ -773,7 +818,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;
}
Expand Down Expand Up @@ -804,6 +853,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)
{
Expand Down Expand Up @@ -867,7 +927,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
Expand Down
23 changes: 23 additions & 0 deletions src/cpp/rtps/xmlparser/XMLProfileManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,3 +692,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();
}
}
1 change: 1 addition & 0 deletions test/unittest/xmlparser/XMLParserTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
13 changes: 13 additions & 0 deletions test/unittest/xmlparser/regressions/20732_profile_bin.xml
View file

Large diffs are not rendered by default.

0 comments on commit 4c34978

Please sign in to comment.