[#4636] Fix ListElement::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 #4636

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

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

diff --git a/src/lib/cc/data.h b/src/lib/cc/data.h
index 698edd406e829709a3c30fdf20382577648229a8..8735f91c0b1bf1cb34fbd87ae6d47f30cba7cde1 100644 (file)
--- a/src/lib/cc/data.h
+++ b/src/lib/cc/data.h
@@ -456,7 +456,7 @@ public:
     virtual void add(ElementPtr element);
 
     /// @brief Removes the element at the given position. If the index is out
-    /// of nothing happens.
+    /// of bounds, nothing happens.
     /// @param i The index of the element to remove.
     virtual void remove(const int i);
 
@@ -925,7 +925,14 @@ public:
     }
     void add(ElementPtr e) { l.push_back(e); }
     using Element::remove;
-    void remove(int i) { l.erase(l.begin() + i); }
+    /// @brief Removes the element at the given position.
+    ///
+    /// If the index is out of bounds, nothing happens.
+    void remove(int i) {
+        if (i >= 0 && static_cast<size_t>(i) < l.size()) {
+            l.erase(l.begin() + i);
+        }
+    }
     void toJSON(std::ostream& ss,
                 unsigned level = MAX_NESTING_LEVEL) const;
     size_t size() const { return (l.size()); }

diff --git a/src/lib/cc/tests/data_unittests.cc b/src/lib/cc/tests/data_unittests.cc
index d8dff3e6f8df48fa8ce1ae1c50cb5cf533a9af31..57d7405c199a11cdca343cf63c5955ebf1b49010 100644 (file)
--- a/src/lib/cc/tests/data_unittests.cc
+++ b/src/lib/cc/tests/data_unittests.cc
@@ -747,6 +747,38 @@ TEST(Element, listElement) {
     EXPECT_ANY_THROW(el->set(3, Element::create(0)));
 }
 
+// Verifies that ListElement::remove() is a no-op for out-of-range indices
+// rather than invoking undefined behavior (Gitlab #4636).
+TEST(Element, listElementRemoveOutOfRange) {
+    ElementPtr el = Element::fromJSON("[ 1, 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("[ 1, 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("[ 1, 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("[ 1, 2 ]", el->str());
+
+    // Empty list: any remove should be a no-op.
+    ElementPtr empty = Element::createList();
+    EXPECT_NO_THROW(empty->remove(0));
+    EXPECT_TRUE(empty->empty());
+
+    // In-range remove still works.
+    EXPECT_NO_THROW(el->remove(0));
+    EXPECT_EQ(1, static_cast<int>(el->size()));
+    EXPECT_EQ(2, el->get(0)->intValue());
+}
+
 TEST(Element, mapElement) {
     // this function checks the specific functions for ListElements
     ElementPtr el = Element::fromJSON("{ \"name\": \"foo\", "