From bd367917bd9e3bda6a931a082400117738bd706b Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Wed, 12 Jun 2024 09:51:33 +0200 Subject: [PATCH 1/3] Refs #21153: Regression and fix Signed-off-by: Mario Dominguez --- src/cpp/xmlparser/XMLDynamicParser.cpp | 210 +++++++++++------- test/unittest/xmlparser/XMLParserTests.cpp | 1 + .../regressions/21153_profile_bin.xml | 1 + 3 files changed, 127 insertions(+), 85 deletions(-) create mode 100644 test/unittest/xmlparser/regressions/21153_profile_bin.xml diff --git a/src/cpp/xmlparser/XMLDynamicParser.cpp b/src/cpp/xmlparser/XMLDynamicParser.cpp index a699aad3e23..9d89a054bd0 100644 --- a/src/cpp/xmlparser/XMLDynamicParser.cpp +++ b/src/cpp/xmlparser/XMLDynamicParser.cpp @@ -569,29 +569,37 @@ XMLP_ret XMLParser::parseXMLBitsetDynamicType( DynamicTypeBuilder::_ref_type type_builder = DynamicTypeBuilderFactory::get_instance()->create_type(bitset_descriptor); - const char* element_name {nullptr}; - MemberId position {0}; - for (tinyxml2::XMLElement* p_element = p_root->FirstChildElement(); - p_element != nullptr; p_element = p_element->NextSiblingElement()) + if (nullptr != type_builder) { - element_name = p_element->Name(); - if (strcmp(element_name, BITFIELD) == 0) + const char* element_name {nullptr}; + MemberId position {0}; + for (tinyxml2::XMLElement* p_element = p_root->FirstChildElement(); + p_element != nullptr; p_element = p_element->NextSiblingElement()) { - ret = parseXMLBitfieldDynamicType(p_element, type_builder, position); + element_name = p_element->Name(); + if (strcmp(element_name, BITFIELD) == 0) + { + ret = parseXMLBitfieldDynamicType(p_element, type_builder, position); + } + else + { + EPROSIMA_LOG_ERROR(XMLPARSER, "Invalid element found into 'bitsetDcl'. Name: " << element_name); + return XMLP_ret::XML_ERROR; + } } - else + + if (XMLP_ret::XML_OK == ret) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Invalid element found into 'bitsetDcl'. Name: " << element_name); - return XMLP_ret::XML_ERROR; + if (false == XMLProfileManager::insertDynamicTypeByName(name, type_builder->build())) + { + ret = XMLP_ret::XML_ERROR; + } } } - - if (XMLP_ret::XML_OK == ret) + else { - if (false == XMLProfileManager::insertDynamicTypeByName(name, type_builder->build())) - { - ret = XMLP_ret::XML_ERROR; - } + EPROSIMA_LOG_ERROR(XMLPARSER, "Error creating bitset type: " << name); + ret = XMLP_ret::XML_ERROR; } return ret; @@ -792,27 +800,35 @@ XMLP_ret XMLParser::parseXMLBitmaskDynamicType( DynamicTypeBuilderFactory::get_instance()->create_type(bitmask_descriptor)}; uint16_t position = 0; - const char* element_name = nullptr; - for (tinyxml2::XMLElement* p_element = p_root->FirstChildElement(); - p_element != nullptr; p_element = p_element->NextSiblingElement()) + if (nullptr != type_builder) { - element_name = p_element->Name(); - if (strcmp(element_name, BIT_VALUE) == 0) + const char* element_name = nullptr; + for (tinyxml2::XMLElement* p_element = p_root->FirstChildElement(); + p_element != nullptr; p_element = p_element->NextSiblingElement()) { - if (parseXMLBitvalueDynamicType(p_element, type_builder, position) != XMLP_ret::XML_OK) + element_name = p_element->Name(); + if (strcmp(element_name, BIT_VALUE) == 0) + { + if (parseXMLBitvalueDynamicType(p_element, type_builder, position) != XMLP_ret::XML_OK) + { + return XMLP_ret::XML_ERROR; + } + } + else { + EPROSIMA_LOG_ERROR(XMLPARSER, "Invalid element found into 'bitmaskDcl'. Name: " << element_name); return XMLP_ret::XML_ERROR; } } - else + + if (false == XMLProfileManager::insertDynamicTypeByName(name, type_builder->build())) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Invalid element found into 'bitmaskDcl'. Name: " << element_name); - return XMLP_ret::XML_ERROR; + ret = XMLP_ret::XML_ERROR; } } - - if (false == XMLProfileManager::insertDynamicTypeByName(name, type_builder->build())) + else { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error creating bitmask type: " << name); ret = XMLP_ret::XML_ERROR; } @@ -861,31 +877,39 @@ XMLP_ret XMLParser::parseXMLEnumDynamicType( DynamicTypeBuilder::_ref_type type_builder {DynamicTypeBuilderFactory::get_instance()->create_type( enum_descriptor)}; - for (tinyxml2::XMLElement* literal = p_root->FirstChildElement(ENUMERATOR); - literal != nullptr; literal = literal->NextSiblingElement(ENUMERATOR)) + if(nullptr != type_builder) { - const char* name = literal->Attribute(NAME); - if (name == nullptr) + for (tinyxml2::XMLElement* literal = p_root->FirstChildElement(ENUMERATOR); + literal != nullptr; literal = literal->NextSiblingElement(ENUMERATOR)) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing enum type: Literals must have name."); - return XMLP_ret::XML_ERROR; - } + const char* name = literal->Attribute(NAME); + if (name == nullptr) + { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing enum type: Literals must have name."); + return XMLP_ret::XML_ERROR; + } - MemberDescriptor::_ref_type md {traits::make_shared()}; - md->type(DynamicTypeBuilderFactory::get_instance()->get_primitive_type(TK_INT32)); - md->name(name); + MemberDescriptor::_ref_type md {traits::make_shared()}; + md->type(DynamicTypeBuilderFactory::get_instance()->get_primitive_type(TK_INT32)); + md->name(name); - const char* value = literal->Attribute(VALUE); - if (value != nullptr) - { - md->default_value(value); + const char* value = literal->Attribute(VALUE); + if (value != nullptr) + { + md->default_value(value); + } + + type_builder->add_member(md); } - type_builder->add_member(md); + if (false == XMLProfileManager::insertDynamicTypeByName(enumName, type_builder->build())) + { + ret = XMLP_ret::XML_ERROR; + } } - - if (false == XMLProfileManager::insertDynamicTypeByName(enumName, type_builder->build())) + else { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error creating enum type: " << enumName); ret = XMLP_ret::XML_ERROR; } @@ -950,27 +974,35 @@ XMLP_ret XMLParser::parseXMLStructDynamicType( DynamicTypeBuilder::_ref_type type_builder = DynamicTypeBuilderFactory::get_instance()->create_type( structure_descriptor); - const char* element_name = nullptr; - for (tinyxml2::XMLElement* p_element = p_root->FirstChildElement(); - p_element != nullptr; p_element = p_element->NextSiblingElement()) + if (nullptr != type_builder) { - element_name = p_element->Name(); - if (strcmp(element_name, MEMBER) == 0) + const char* element_name = nullptr; + for (tinyxml2::XMLElement* p_element = p_root->FirstChildElement(); + p_element != nullptr; p_element = p_element->NextSiblingElement()) { - if (!parseXMLMemberDynamicType(p_element, type_builder, mId++)) + element_name = p_element->Name(); + if (strcmp(element_name, MEMBER) == 0) + { + if (!parseXMLMemberDynamicType(p_element, type_builder, mId++)) + { + return XMLP_ret::XML_ERROR; + } + } + else { + EPROSIMA_LOG_ERROR(XMLPARSER, "Invalid element found into 'structDcl'. Name: " << element_name); return XMLP_ret::XML_ERROR; } } - else + + if (false == XMLProfileManager::insertDynamicTypeByName(name, type_builder->build())) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Invalid element found into 'structDcl'. Name: " << element_name); - return XMLP_ret::XML_ERROR; + ret = XMLP_ret::XML_ERROR; } } - - if (false == XMLProfileManager::insertDynamicTypeByName(name, type_builder->build())) + else { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error creating struct type: " << name); ret = XMLP_ret::XML_ERROR; } @@ -1037,53 +1069,61 @@ XMLP_ret XMLParser::parseXMLUnionDynamicType( DynamicTypeBuilder::_ref_type type_builder {DynamicTypeBuilderFactory::get_instance()-> create_type(union_descriptor)}; - MemberId mId{1}; - for (p_element = p_root->FirstChildElement(CASE); - p_element != nullptr; p_element = p_element->NextSiblingElement(CASE)) + if (nullptr != type_builder) { - std::string valuesStr = ""; - for (tinyxml2::XMLElement* caseValue = p_element->FirstChildElement(CASE_DISCRIMINATOR); - caseValue != nullptr; caseValue = caseValue->NextSiblingElement(CASE_DISCRIMINATOR)) + MemberId mId{1}; + for (p_element = p_root->FirstChildElement(CASE); + p_element != nullptr; p_element = p_element->NextSiblingElement(CASE)) { - const char* values = caseValue->Attribute(VALUE); - if (values == nullptr) + std::string valuesStr = ""; + for (tinyxml2::XMLElement* caseValue = p_element->FirstChildElement(CASE_DISCRIMINATOR); + caseValue != nullptr; caseValue = caseValue->NextSiblingElement(CASE_DISCRIMINATOR)) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing union case value: Not found."); - return XMLP_ret::XML_ERROR; + const char* values = caseValue->Attribute(VALUE); + if (values == nullptr) + { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing union case value: Not found."); + return XMLP_ret::XML_ERROR; + } + + if (valuesStr.empty()) + { + valuesStr = values; + } + else + { + valuesStr += std::string(",") + values; + } } - if (valuesStr.empty()) + tinyxml2::XMLElement* caseElement = p_element->FirstChildElement(); + while (caseElement != nullptr && + strncmp(caseElement->Value(), CASE_DISCRIMINATOR, CASE_DISCRIMINATOR_len) == 0) { - valuesStr = values; + caseElement = caseElement->NextSiblingElement(); } - else + if (caseElement != nullptr) { - valuesStr += std::string(",") + values; + if (!parseXMLMemberDynamicType(caseElement, type_builder, mId++, valuesStr)) + { + return XMLP_ret::XML_ERROR; + } } - } - - tinyxml2::XMLElement* caseElement = p_element->FirstChildElement(); - while (caseElement != nullptr && - strncmp(caseElement->Value(), CASE_DISCRIMINATOR, CASE_DISCRIMINATOR_len) == 0) - { - caseElement = caseElement->NextSiblingElement(); - } - if (caseElement != nullptr) - { - if (!parseXMLMemberDynamicType(caseElement, type_builder, mId++, valuesStr)) + else { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing union case member: Not found."); return XMLP_ret::XML_ERROR; } } - else + + if (false == XMLProfileManager::insertDynamicTypeByName(name, type_builder->build())) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing union case member: Not found."); - return XMLP_ret::XML_ERROR; + ret = XMLP_ret::XML_ERROR; } } - - if (false == XMLProfileManager::insertDynamicTypeByName(name, type_builder->build())) + else { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error creating union type: " << name); ret = XMLP_ret::XML_ERROR; } } diff --git a/test/unittest/xmlparser/XMLParserTests.cpp b/test/unittest/xmlparser/XMLParserTests.cpp index 726bbe36f45..f8e638636ae 100644 --- a/test/unittest/xmlparser/XMLParserTests.cpp +++ b/test/unittest/xmlparser/XMLParserTests.cpp @@ -78,6 +78,7 @@ TEST_F(XMLParserTests, regressions) EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/20608_profile_bin.xml", root)); EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/20610_profile_bin.xml", root)); EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/20732_profile_bin.xml", root)); + EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/21153_profile_bin.xml", root)); } TEST_F(XMLParserTests, NoFile) diff --git a/test/unittest/xmlparser/regressions/21153_profile_bin.xml b/test/unittest/xmlparser/regressions/21153_profile_bin.xml new file mode 100644 index 00000000000..cb15ed9acfa --- /dev/null +++ b/test/unittest/xmlparser/regressions/21153_profile_bin.xml @@ -0,0 +1 @@ + \ No newline at end of file From ad5508658213d7ceb685a6614f35ad14c51a8a83 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Wed, 12 Jun 2024 09:52:14 +0200 Subject: [PATCH 2/3] Refs #21154: Regression and fix Signed-off-by: Mario Dominguez --- src/cpp/xmlparser/XMLDynamicParser.cpp | 41 ++++++++++++++++--- test/unittest/xmlparser/XMLParserTests.cpp | 2 + .../regressions/21154_profile_bin.xml | 1 + 3 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 test/unittest/xmlparser/regressions/21154_profile_bin.xml diff --git a/src/cpp/xmlparser/XMLDynamicParser.cpp b/src/cpp/xmlparser/XMLDynamicParser.cpp index 9d89a054bd0..11a5ef1d148 100644 --- a/src/cpp/xmlparser/XMLDynamicParser.cpp +++ b/src/cpp/xmlparser/XMLDynamicParser.cpp @@ -434,7 +434,15 @@ XMLP_ret XMLParser::parseXMLAliasDynamicType( const char* boundStr = p_root->Attribute(STR_MAXLENGTH); if (boundStr != nullptr) { - bound = static_cast(std::stoul(boundStr)); + try + { + bound = static_cast(std::stoul(boundStr)); + } + catch (const std::exception&) + { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); + return XMLP_ret::XML_ERROR; + } } value_type = getDiscriminatorTypeBuilder(type, bound); } @@ -540,10 +548,17 @@ XMLP_ret XMLParser::parseXMLBitsetDynamicType( EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing bitfield bit_bound: Not found."); return XMLP_ret::XML_ERROR; } - if (nullptr != member_name) { - bitset_descriptor->bound().push_back(static_cast(std::stoul(bit_bound))); + try + { + bitset_descriptor->bound().push_back(static_cast(std::stoul(bit_bound))); + } + catch (const std::exception&) + { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing bitfield type bound: '" << BIT_BOUND << "' out of bounds."); + return XMLP_ret::XML_ERROR; + } } } //}}} @@ -1595,7 +1610,15 @@ DynamicType::_ref_type XMLParser:: parseXMLMemberDynamicType( if (nullptr != boundStr) { - bound = static_cast(std::stoul(boundStr)); + try + { + bound = static_cast(std::stoul(boundStr)); + } + catch (const std::exception&) + { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); + return {}; + } } DynamicTypeBuilder::_ref_type string_builder = factory->create_string_type(bound); @@ -1620,7 +1643,15 @@ DynamicType::_ref_type XMLParser:: parseXMLMemberDynamicType( if (nullptr != boundStr) { - bound = static_cast(std::stoul(boundStr)); + try + { + bound = static_cast(std::stoul(boundStr)); + } + catch (const std::exception&) + { + EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); + return {}; + } } DynamicTypeBuilder::_ref_type wstring_builder = factory->create_wstring_type(bound); diff --git a/test/unittest/xmlparser/XMLParserTests.cpp b/test/unittest/xmlparser/XMLParserTests.cpp index f8e638636ae..396a0b37c5a 100644 --- a/test/unittest/xmlparser/XMLParserTests.cpp +++ b/test/unittest/xmlparser/XMLParserTests.cpp @@ -79,6 +79,8 @@ TEST_F(XMLParserTests, regressions) EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/20610_profile_bin.xml", root)); EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/20732_profile_bin.xml", root)); EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/21153_profile_bin.xml", root)); + EXPECT_EQ(XMLP_ret::XML_ERROR, XMLParser::loadXML("regressions/21154_profile_bin.xml", root)); + Log::Flush(); } TEST_F(XMLParserTests, NoFile) diff --git a/test/unittest/xmlparser/regressions/21154_profile_bin.xml b/test/unittest/xmlparser/regressions/21154_profile_bin.xml new file mode 100644 index 00000000000..19fd380b4ac --- /dev/null +++ b/test/unittest/xmlparser/regressions/21154_profile_bin.xml @@ -0,0 +1 @@ + \ No newline at end of file From 5891a84c2d5df400d4c328182d99a38670fdd035 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Wed, 12 Jun 2024 10:00:05 +0200 Subject: [PATCH 3/3] Refs #21154: Linter Signed-off-by: Mario Dominguez --- src/cpp/xmlparser/XMLDynamicParser.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/cpp/xmlparser/XMLDynamicParser.cpp b/src/cpp/xmlparser/XMLDynamicParser.cpp index 11a5ef1d148..9363692bc52 100644 --- a/src/cpp/xmlparser/XMLDynamicParser.cpp +++ b/src/cpp/xmlparser/XMLDynamicParser.cpp @@ -440,7 +440,8 @@ XMLP_ret XMLParser::parseXMLAliasDynamicType( } catch (const std::exception&) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); + EPROSIMA_LOG_ERROR(XMLPARSER, + "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); return XMLP_ret::XML_ERROR; } } @@ -556,7 +557,8 @@ XMLP_ret XMLParser::parseXMLBitsetDynamicType( } catch (const std::exception&) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing bitfield type bound: '" << BIT_BOUND << "' out of bounds."); + EPROSIMA_LOG_ERROR(XMLPARSER, + "Error parsing bitfield type bound: '" << BIT_BOUND << "' out of bounds."); return XMLP_ret::XML_ERROR; } } @@ -892,7 +894,7 @@ XMLP_ret XMLParser::parseXMLEnumDynamicType( DynamicTypeBuilder::_ref_type type_builder {DynamicTypeBuilderFactory::get_instance()->create_type( enum_descriptor)}; - if(nullptr != type_builder) + if (nullptr != type_builder) { for (tinyxml2::XMLElement* literal = p_root->FirstChildElement(ENUMERATOR); literal != nullptr; literal = literal->NextSiblingElement(ENUMERATOR)) @@ -1616,7 +1618,8 @@ DynamicType::_ref_type XMLParser:: parseXMLMemberDynamicType( } catch (const std::exception&) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); + EPROSIMA_LOG_ERROR(XMLPARSER, + "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); return {}; } } @@ -1649,7 +1652,8 @@ DynamicType::_ref_type XMLParser:: parseXMLMemberDynamicType( } catch (const std::exception&) { - EPROSIMA_LOG_ERROR(XMLPARSER, "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); + EPROSIMA_LOG_ERROR(XMLPARSER, + "Error parsing alias type bound: '" << STR_MAXLENGTH << "' out of bounds."); return {}; } }