From: Andrei Pavel Date: Mon, 10 Aug 2026 10:16:58 +0000 (+0300) Subject: [#4640] Review X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7b3c93d1238595dc1c8003ee2d8777078ee60df5;p=thirdparty%2Fkea.git [#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 index 281610baa2..0000000000 --- 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 455ae18a23..2340e4ea37 100644 --- 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(right); - for (auto const& left : element->listValue()) { - ElementPtr mutable_left = boost::const_pointer_cast(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(right); + for (auto const& left : element->listValue()) { + ElementPtr mutable_left = boost::const_pointer_cast(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->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->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 41a60903f6..32d4648ef2 100644 --- a/src/lib/cc/tests/data_unittests.cc +++ b/src/lib/cc/tests/data_unittests.cc @@ -11,9 +11,9 @@ #include #include -#include - +#include #include +#include #include #include @@ -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"); } }