From: Wlodek Wencel Date: Thu, 23 Jul 2026 11:24:11 +0000 (+0200) Subject: [#4640] Fix hierarchy bounds in mergeDiff/extend X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=84d41fdbd800f7c0e650af12b1f52e18836c5dab;p=thirdparty%2Fkea.git [#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 --- 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 index 0000000000..281610baa2 --- /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 0492cbc0ce..455ae18a23 100644 --- 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(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); + // 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); + } + 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->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->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(right); diff --git a/src/lib/cc/tests/data_unittests.cc b/src/lib/cc/tests/data_unittests.cc index a6f0b6f823..41a60903f6 100644 --- 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) {