[#4640] Fix hierarchy bounds in mergeDiff/extend

Guard hierarchy[idx] access so empty or too-shallow
HierarchyDescriptor values fall back to flat list
behavior instead of crashing.

Closes #4640

Co-authored-by: Cursor <cursoragent@cursor.com>

diff --git a/changelog_unreleased/4640-mergediffadd-del-extend-access-hierarchy-idx-without-bounds-check b/changelog_unreleased/4640-mergediffadd-del-extend-access-hierarchy-idx-without-bounds-check
new file mode 100644 (file)
index 0000000..281610b
--- /dev/null
+++ b/changelog_unreleased/4640-mergediffadd-del-extend-access-hierarchy-idx-without-bounds-check
@@ -0,0 +1,4 @@
+[bug]          wlodek
+       Fixed mergeDiffAdd/Del and extend to bounds-check
+       hierarchy descriptors before indexing.
+       (Gitlab #4640)

diff --git a/src/lib/cc/data.cc b/src/lib/cc/data.cc
index 0492cbc0ce4432cb85079da39a32e5c84b7cc488..455ae18a2339953de1c2a866da481f16238ce58f 100644 (file)
--- a/src/lib/cc/data.cc
+++ b/src/lib/cc/data.cc
@@ -1314,27 +1314,30 @@ mergeDiffAdd(ElementPtr& element, ElementPtr& other,
         ElementPtr new_elements = Element::createList();
         for (auto const& right : other->listValue()) {
             // Check if we have any description of the key in the configuration
-            // hierarchy.
-            auto f = hierarchy[idx].find(key);
-            if (f != hierarchy[idx].end()) {
-                bool found = false;
-                ElementPtr mutable_right = boost::const_pointer_cast<Element>(right);
-                for (auto const& left : element->listValue()) {
-                    ElementPtr mutable_left = boost::const_pointer_cast<Element>(left);
-                    // Check if the elements refer to the same configuration
-                    // entity.
-                    if (f->second.match_(mutable_left, mutable_right)) {
-                        found = true;
-                        mergeDiffAdd(mutable_left, mutable_right, hierarchy,
-                                     key, idx, level - 1);
+            // hierarchy. Missing or too-shallow descriptors are treated as a
+            // flat list append.
+            if (idx < hierarchy.size()) {
+                auto f = hierarchy[idx].find(key);
+                if (f != hierarchy[idx].end()) {
+                    bool found = false;
+                    ElementPtr mutable_right = boost::const_pointer_cast<Element>(right);
+                    for (auto const& left : element->listValue()) {
+                        ElementPtr mutable_left = boost::const_pointer_cast<Element>(left);
+                        // Check if the elements refer to the same configuration
+                        // entity.
+                        if (f->second.match_(mutable_left, mutable_right)) {
+                            found = true;
+                            mergeDiffAdd(mutable_left, mutable_right, hierarchy,
+                                         key, idx, level - 1);
+                        }
                     }
+                    if (!found) {
+                        new_elements->add(right);
+                    }
+                    continue;
                 }
-                if (!found) {
-                    new_elements->add(right);
-                }
-            } else {
-                new_elements->add(right);
             }
+            new_elements->add(right);
         }
         // Finally add the new elements.
         for (auto const& right : new_elements->listValue()) {
@@ -1382,27 +1385,33 @@ mergeDiffDel(ElementPtr& element, ElementPtr& other,
             for (uint32_t iter = 0; iter < element->listValue().size();) {
                 bool removed = false;
                 // Check if we have any description of the key in the
-                // configuration hierarchy.
-                auto f = hierarchy[idx].find(key);
-                if (f != hierarchy[idx].end()) {
-                    ElementPtr mutable_left = boost::const_pointer_cast<Element>(element->listValue().at(iter));
-                    // Check if the elements refer to the same configuration
-                    // entity.
-                    if (f->second.match_(mutable_left, mutable_right)) {
-                        // Check if the user supplied data only contains
-                        // identification information, so the intent is to
-                        // delete the element, not just element data.
-                        if (f->second.no_data_(mutable_right)) {
-                            element->remove(iter);
-                            removed = true;
-                        } else {
-                            mergeDiffDel(mutable_left, mutable_right,
-                                         hierarchy, key, idx, level - 1);
-                            if (mutable_left->empty()) {
+                // configuration hierarchy. Missing or too-shallow descriptors
+                // fall back to equality-based removal.
+                if (idx < hierarchy.size()) {
+                    auto f = hierarchy[idx].find(key);
+                    if (f != hierarchy[idx].end()) {
+                        ElementPtr mutable_left = boost::const_pointer_cast<Element>(element->listValue().at(iter));
+                        // Check if the elements refer to the same configuration
+                        // entity.
+                        if (f->second.match_(mutable_left, mutable_right)) {
+                            // Check if the user supplied data only contains
+                            // identification information, so the intent is to
+                            // delete the element, not just element data.
+                            if (f->second.no_data_(mutable_right)) {
                                 element->remove(iter);
                                 removed = true;
+                            } else {
+                                mergeDiffDel(mutable_left, mutable_right,
+                                             hierarchy, key, idx, level - 1);
+                                if (mutable_left->empty()) {
+                                    element->remove(iter);
+                                    removed = true;
+                                }
                             }
                         }
+                    } else if (element->listValue().at(iter)->equals(*value)) {
+                        element->remove(iter);
+                        removed = true;
                     }
                 } else if (element->listValue().at(iter)->equals(*value)) {
                     element->remove(iter);
@@ -1436,13 +1445,15 @@ mergeDiffDel(ElementPtr& element, ElementPtr& other,
                     } else {
                         // Check if we have any description of the key in the
                         // configuration hierarchy.
-                        auto f = hierarchy[idx].find(key);
-                        if (f != hierarchy[idx].end()) {
-                            // Check if the key is used for element
-                            // identification.
-                            if (f->second.is_key_(current_key)) {
-                                // Store the key parameter.
-                                new_elements->set(current_key, mutable_element);
+                        if (idx < hierarchy.size()) {
+                            auto f = hierarchy[idx].find(key);
+                            if (f != hierarchy[idx].end()) {
+                                // Check if the key is used for element
+                                // identification.
+                                if (f->second.is_key_(current_key)) {
+                                    // Store the key parameter.
+                                    new_elements->set(current_key, mutable_element);
+                                }
                             }
                         }
                         element->remove(current_key);
@@ -1477,7 +1488,10 @@ extend(const std::string& container, const std::string& extension,
     if (element->getType() == Element::list) {
         for (auto const& right : other->listValue()) {
             // Check if we have any description of the key in the configuration
-            // hierarchy.
+            // hierarchy. Missing or too-shallow descriptors skip matching.
+            if (idx >= hierarchy.size()) {
+                continue;
+            }
             auto f = hierarchy[idx].find(key);
             if (f != hierarchy[idx].end()) {
                 ElementPtr mutable_right = boost::const_pointer_cast<Element>(right);

diff --git a/src/lib/cc/tests/data_unittests.cc b/src/lib/cc/tests/data_unittests.cc
index a6f0b6f8237aea549c550dacfc7a4f4b9e0942bd..41a60903f6b6049ba6333e5781f5eb02d0db0686 100644 (file)
--- a/src/lib/cc/tests/data_unittests.cc
+++ b/src/lib/cc/tests/data_unittests.cc
@@ -2103,6 +2103,62 @@ TEST(Element, mergeDiffAdd) {
     }
 }
 
+/// @brief Test that empty or too-shallow HierarchyDescriptor does not cause
+/// out-of-bounds access in mergeDiffAdd, mergeDiffDel, or extend.
+TEST(Element, hierarchyIndexBounds) {
+    {
+        SCOPED_TRACE("mergeDiffAdd empty hierarchy list");
+        // Reproduces Gitlab #4640: empty hierarchy used to crash on list merge.
+        isc::data::HierarchyDescriptor hierarchy;
+        ElementPtr left = Element::fromJSON("[{\"id\":1}]");
+        ElementPtr right = Element::fromJSON("[{\"id\":2}]");
+        ASSERT_NO_THROW(mergeDiffAdd(left, right, hierarchy, "pools"));
+        ElementPtr expected = Element::fromJSON("[{\"id\":1},{\"id\":2}]");
+        EXPECT_TRUE(isc::data::isEquivalent(left, expected))
+            << "Actual: " << left->str()
+            << "\nExpected: " << expected->str();
+    }
+    {
+        SCOPED_TRACE("mergeDiffDel empty hierarchy list");
+        isc::data::HierarchyDescriptor hierarchy;
+        ElementPtr left = Element::fromJSON("[{\"id\":1},{\"id\":2}]");
+        ElementPtr right = Element::fromJSON("[{\"id\":2}]");
+        ASSERT_NO_THROW(mergeDiffDel(left, right, hierarchy, "pools"));
+        ElementPtr expected = Element::fromJSON("[{\"id\":1}]");
+        EXPECT_TRUE(isc::data::isEquivalent(left, expected))
+            << "Actual: " << left->str()
+            << "\nExpected: " << expected->str();
+    }
+    {
+        SCOPED_TRACE("extend empty hierarchy list");
+        isc::data::HierarchyDescriptor hierarchy;
+        ElementPtr left = Element::fromJSON("[{\"id\":1,\"name\":\"a\"}]");
+        ElementPtr right = Element::fromJSON("[{\"id\":1,\"name\":\"b\"}]");
+        ElementPtr expected = copy(left);
+        ASSERT_NO_THROW(extend("pools", "name", left, right, hierarchy,
+                               "pools"));
+        EXPECT_TRUE(isc::data::isEquivalent(left, expected))
+            << "Actual: " << left->str()
+            << "\nExpected: " << expected->str();
+    }
+    {
+        SCOPED_TRACE("mergeDiffAdd shallow hierarchy nested list");
+        // hierarchy size 1; nested list is processed at idx 1 (out of range).
+        isc::data::HierarchyDescriptor hierarchy = createHierarchy();
+        hierarchy.resize(1);
+        ElementPtr left = Element::fromJSON(
+            "{\"items\":[{\"id\":1,\"v\":\"a\"}]}");
+        ElementPtr right = Element::fromJSON(
+            "{\"items\":[{\"id\":2,\"v\":\"b\"}]}");
+        ASSERT_NO_THROW(mergeDiffAdd(left, right, hierarchy, "root"));
+        ElementPtr expected = Element::fromJSON(
+            "{\"items\":[{\"id\":1,\"v\":\"a\"},{\"id\":2,\"v\":\"b\"}]}");
+        EXPECT_TRUE(isc::data::isEquivalent(left, expected))
+            << "Actual: " << left->str()
+            << "\nExpected: " << expected->str();
+    }
+}
+
 /// @brief Test which checks that mergeDiffDel throws if called with wrong
 /// element types.
 TEST(Element, mergeDiffDelBadParams) {