[#4637] Fix MapElement::remove out-of-range UB

Bounds-check the index so out-of-range remove is a no-op, matching
the documented contract and avoiding segfaults.

Closes #4637

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

diff --git a/changelog_unreleased/4637-mapelement-remove-int-has-undefined-behavior-on-out-of-range-index b/changelog_unreleased/4637-mapelement-remove-int-has-undefined-behavior-on-out-of-range-index
new file mode 100644 (file)
index 0000000..ce49923
--- /dev/null
+++ b/changelog_unreleased/4637-mapelement-remove-int-has-undefined-behavior-on-out-of-range-index
@@ -0,0 +1,4 @@
+[bug]          wlodek
+       Fixed MapElement::remove() undefined behavior on out-of-range
+       indices so it matches the documented no-op contract.
+       (Gitlab #4637)

diff --git a/src/lib/cc/data.h b/src/lib/cc/data.h
index 8735f91c0b1bf1cb34fbd87ae6d47f30cba7cde1..aa30818d8407bb985e05cd09d5eff665e61c1982 100644 (file)
--- a/src/lib/cc/data.h
+++ b/src/lib/cc/data.h
@@ -997,11 +997,15 @@ public:
 
     /// @brief Remove the i-th element from the map.
     ///
+    /// If the index is out of bounds, nothing happens.
+    ///
     /// @param i the position of the element you want to remove
     void remove(int const i) override {
-        auto it(m.begin());
-        std::advance(it, i);
-        m.erase(it);
+        if (i >= 0 && static_cast<size_t>(i) < m.size()) {
+            auto it(m.begin());
+            std::advance(it, i);
+            m.erase(it);
+        }
     }
 
     bool contains(const std::string& s) const override {

diff --git a/src/lib/cc/tests/data_unittests.cc b/src/lib/cc/tests/data_unittests.cc
index 2d896eca14b99206afc931387185a1682861c292..6321aacadfe4d1b91bf481e7934745d193de6d0c 100644 (file)
--- a/src/lib/cc/tests/data_unittests.cc
+++ b/src/lib/cc/tests/data_unittests.cc
@@ -839,6 +839,39 @@ TEST(Element, mapElement) {
     EXPECT_EQ("{ \"value\": None }", el->str());
 }
 
+// Verifies that MapElement::remove(int) is a no-op for out-of-range indices
+// rather than invoking undefined behavior (Gitlab #4637).
+TEST(Element, mapElementRemoveOutOfRange) {
+    ElementPtr el = Element::fromJSON("{ \"a\": 1, \"b\": 2 }");
+    ASSERT_EQ(2, static_cast<int>(el->size()));
+
+    // Index past the end should be a no-op.
+    EXPECT_NO_THROW(el->remove(5));
+    EXPECT_EQ(2, static_cast<int>(el->size()));
+    EXPECT_EQ("{ \"a\": 1, \"b\": 2 }", el->str());
+
+    // Index equal to size should be a no-op.
+    EXPECT_NO_THROW(el->remove(2));
+    EXPECT_EQ(2, static_cast<int>(el->size()));
+    EXPECT_EQ("{ \"a\": 1, \"b\": 2 }", el->str());
+
+    // Negative index should be a no-op.
+    EXPECT_NO_THROW(el->remove(-1));
+    EXPECT_EQ(2, static_cast<int>(el->size()));
+    EXPECT_EQ("{ \"a\": 1, \"b\": 2 }", el->str());
+
+    // Empty map: any remove should be a no-op.
+    ElementPtr empty = Element::createMap();
+    EXPECT_NO_THROW(empty->remove(0));
+    EXPECT_TRUE(empty->empty());
+
+    // In-range remove still works (ordered map: index 0 is key "a").
+    EXPECT_NO_THROW(el->remove(0));
+    EXPECT_EQ(1, static_cast<int>(el->size()));
+    EXPECT_EQ(2, el->get("b")->intValue());
+    EXPECT_TRUE(isNull(el->get("a")));
+}
+
 TEST(Element, toAndFromWire) {
     // Wire format is now plain JSON.
     EXPECT_EQ("1", Element::create(1)->toWire());