Merge pull request #266 from AsherGlick/missing_name_segfault

Missing "name" segfault fix
AsherGlick · Jan 20, 2024 · a742a4d · a742a4d
2 parents 622a391 + f895ed4
commit a742a4d
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 1 deletion.
diff --git a/xml_converter/integration_tests/test_cases/category_name_invalid/input/pack/xml_file.xml b/xml_converter/integration_tests/test_cases/category_name_invalid/input/pack/xml_file.xml
@@ -0,0 +1,4 @@
+<OverlayData>
+    <MarkerCategory DisplayName="My Missing Category" />
+    <MarkerCategory DisplayName="My Empty Category" Name="" />
+</OverlayData>
diff --git a/xml_converter/integration_tests/test_cases/category_name_invalid/output_proto/markers.bin b/xml_converter/integration_tests/test_cases/category_name_invalid/output_proto/markers.bin
diff --git a/xml_converter/integration_tests/test_cases/category_name_invalid/output_xml/xml_file.xml b/xml_converter/integration_tests/test_cases/category_name_invalid/output_xml/xml_file.xml
@@ -0,0 +1,10 @@
+<OverlayData>
+<MarkerCategory  DisplayName="My Missing Category">
+</MarkerCategory>
+
+<MarkerCategory  DisplayName="My Empty Category" Name="">
+</MarkerCategory>
+
+<POIs>
+</POIs>
+</OverlayData>
diff --git a/xml_converter/integration_tests/test_cases/category_name_invalid/testcase.yaml b/xml_converter/integration_tests/test_cases/category_name_invalid/testcase.yaml
@@ -0,0 +1,13 @@
+input_paths: 
+    "pack": "xml"
+expected_stdout: |
+    Error: Category attribute 'name' is missing or is an empty string when it should be a non-empty string
+    test_cases/category_name_invalid/input/pack/xml_file.xml
+    2 |    <MarkerCategory DisplayName="My Missing Category" />
+      |     ^^^^^^^^^^^^^^
+    Error: Category attribute 'name' is missing or is an empty string when it should be a non-empty string
+    test_cases/category_name_invalid/input/pack/xml_file.xml
+    3 |    <MarkerCategory DisplayName="My Empty Category" Name="" />
+      |     ^^^^^^^^^^^^^^
+expected_stderr: |
+expected_returncode: 0
diff --git a/xml_converter/src/packaging_xml.cpp b/xml_converter/src/packaging_xml.cpp
@@ -13,15 +13,27 @@ using namespace std;
 ////////////////////////////////// SERIALIZE ///////////////////////////////////
 ////////////////////////////////////////////////////////////////////////////////
 
+unsigned int UNKNOWN_CATEGORY_COUNTER = 0;
 void parse_marker_categories(rapidxml::xml_node<>* node, map<string, Category>* marker_categories, vector<XMLError*>* errors, string base_dir, int depth = 0) {
     if (get_node_name(node) == "MarkerCategory") {
+        // TODO: Eventually this process should be removed. Instead we should be
+        //       grabbing the name during the parsing of all the atrributes to
+        //       avoid doing a secondary search through the attributes, and then
+        //       we should have a method of applying the parsed node on top of
+        //       its hirearchy probably using the is_set flags.
         string name = lowercase(find_attribute_value(node, "name"));
 
         XMLReaderState state = {
             base_dir,
             marker_categories,
         };
 
+        if (name == "") {
+            errors->push_back(new XMLNodeNameError("Category attribute 'name' is missing or is an empty string when it should be a non-empty string", node));
+            name = "UNKNOWN_CATEGORY_" + to_string(UNKNOWN_CATEGORY_COUNTER);
+            UNKNOWN_CATEGORY_COUNTER++;
+        }
+
         Category* this_category = &(*marker_categories)[name];
         this_category->init_from_xml(node, errors, &state);
         for (rapidxml::xml_node<>* child_node = node->first_node(); child_node; child_node = child_node->next_sibling()) {

diff --git a/xml_converter/src/rapid_helpers.cpp b/xml_converter/src/rapid_helpers.cpp
@@ -9,8 +9,20 @@
 
 using namespace std;
 
+////////////////////////////////////////////////////////////////////////////////
+// find_attribute_value (depricated)
+//
+// This function does a linear search over an xml node to try and find an
+// attribute with the specified name. It the attribute is not found then an
+// empty string is returned.
+//
+// This function is depricated and should not be used by any new code.
+////////////////////////////////////////////////////////////////////////////////
 string find_attribute_value(rapidxml::xml_node<>* node, string attribute_name) {
-    auto attribute = node->first_attribute(attribute_name.data(), attribute_name.size(), false);
+    rapidxml::xml_attribute<char>* attribute = node->first_attribute(attribute_name.data(), attribute_name.size(), false);
+    if (attribute == nullptr) {
+        return "";
+    }
 
     return string(attribute->value(), attribute->value_size());
 }