[#4640] Review

- Throw exception instead of a silent behavior.
- Remove changelog entry since it's not a user-visible change.

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
deleted file mode 100644 (file)
index 281610b..0000000
--- a/changelog_unreleased/4640-mergediffadd-del-extend-access-hierarchy-idx-without-bounds-check
+++ /dev/null
@@ -1,4 +0,0 @@
-[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 455ae18a2339953de1c2a866da481f16238ce58f..2340e4ea37f4df4d37608aa8d2b196c38f6318eb 100644 (file)
--- a/src/lib/cc/data.cc
+++ b/src/lib/cc/data.cc
@@ -1314,30 +1314,31 @@ 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. 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);
+            // hierarchy.
+            if (hierarchy.size() <= idx) {
+                isc_throw(OutOfRange, "Attempt at accessing index "
+                                          << idx << " in hierarchy of size " << 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);
                     }
-                    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()) {
@@ -1385,33 +1386,32 @@ 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. 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)) {
+                // configuration hierarchy.
+                if (hierarchy.size() <= idx) {
+                    isc_throw(OutOfRange, "Attempt at accessing index " << idx
+                                                                        << " in hierarchy of size "
+                                                                        << 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 {
-                                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);
@@ -1445,15 +1445,18 @@ mergeDiffDel(ElementPtr& element, ElementPtr& other,
                     } else {
                         // Check if we have any description of the key in the
                         // configuration hierarchy.
-                        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);
-                                }
+                        if (hierarchy.size() <= idx) {
+                            isc_throw(OutOfRange, "Attempt at accessing index "
+                                                      << idx << " in hierarchy of size "
+                                                      << 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);
@@ -1488,9 +1491,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. Missing or too-shallow descriptors skip matching.
-            if (idx >= hierarchy.size()) {
-                continue;
+            // hierarchy.
+            if (hierarchy.size() <= idx) {
+                isc_throw(OutOfRange, "Attempt at accessing index "
+                                          << idx << " in hierarchy of size " << hierarchy.size());
             }
             auto f = hierarchy[idx].find(key);
             if (f != hierarchy[idx].end()) {

diff --git a/src/lib/cc/tests/data_unittests.cc b/src/lib/cc/tests/data_unittests.cc
index 41a60903f6b6049ba6333e5781f5eb02d0db0686..32d4648ef2ea435eb1015cc8fff3be2a2af17858 100644 (file)
--- a/src/lib/cc/tests/data_unittests.cc
+++ b/src/lib/cc/tests/data_unittests.cc
@@ -11,9 +11,9 @@
 #include <boost/pointer_cast.hpp>
 #include <boost/assign/std/vector.hpp>
 
-#include <climits>
-
+#include <exceptions/exceptions.h>
 #include <cc/data.h>
+#include <testutils/gtest_utils.h>
 #include <util/unittests/check_valgrind.h>
 #include <util/bigints.h>
 
@@ -2110,52 +2110,37 @@ 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();
+        ElementPtr left = Element::fromJSON("[{\"id\": 1}]");
+        ElementPtr right = Element::fromJSON("[{\"id\": 2}]");
+        EXPECT_THROW_MSG(mergeDiffAdd(left, right, hierarchy, "pools"), isc::OutOfRange,
+                         "Attempt at accessing index 0 in hierarchy of size 0");
     }
     {
         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();
+        ElementPtr left = Element::fromJSON("[{\"id\": 1},{\"id\": 2}]");
+        ElementPtr right = Element::fromJSON("[{\"id\": 2}]");
+        EXPECT_THROW_MSG(mergeDiffDel(left, right, hierarchy, "pools"), isc::OutOfRange,
+                         "Attempt at accessing index 0 in hierarchy of size 0");
     }
     {
         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 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();
+        EXPECT_THROW_MSG(extend("pools", "name", left, right, hierarchy, "pools"), isc::OutOfRange,
+                         "Attempt at accessing index 0 in hierarchy of size 0");
     }
     {
         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();
+        ElementPtr left = Element::fromJSON("{\"items\":[{\"id\": 1, \"v\": \"a\"}]}");
+        ElementPtr right = Element::fromJSON("{\"items\":[{\"id\": 2, \"v\": \"b\"}]}");
+        EXPECT_THROW_MSG(mergeDiffAdd(left, right, hierarchy, "root"), isc::OutOfRange,
+                         "Attempt at accessing index 1 in hierarchy of size 1");
     }
 }