core/circular_buffer: fix iterator comparison for wrapped-around indexes

Indexes into a circular buffer can wrap-around (under or overflow) by design. The code knows about this and it is handled correctly. One area which managed to shelter a related bug undetected is iterator comparison. Iterators are just thin wrappers around an index. The comparison operators simply compare the two indexes. This breaks when one of the two compared iterators wrapped around but the other didn't. To fix, we compare offsets compared to the begin iterator. A unit test reproducing the problem is also added. Signed-off-by: Botond Dénes <[email protected]> Message-Id: <[email protected]>
shawnstone2022 · Mar 25, 2022 · b62f871 · b62f871
1 parent 3b4e42d
commit b62f871
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 4 deletions.
diff --git a/include/seastar/core/circular_buffer.hh b/include/seastar/core/circular_buffer.hh
@@ -122,6 +122,7 @@ private:
     void expand(size_t);
     void maybe_expand(size_t nr = 1);
     size_t mask(size_t idx) const;
+    size_t offset(size_t idx) const;
 
     template<typename CB, typename ValueType>
     struct cbiterator {
@@ -176,16 +177,16 @@ private:
             return idx != rhs.idx;
         }
         bool operator<(const cbiterator<CB, ValueType>& rhs) const noexcept {
-            return idx < rhs.idx;
+            return cb->offset(idx) < cb->offset(rhs.idx);
         }
         bool operator>(const cbiterator<CB, ValueType>& rhs) const noexcept {
-            return idx > rhs.idx;
+            return cb->offset(idx) > cb->offset(rhs.idx);
         }
         bool operator>=(const cbiterator<CB, ValueType>& rhs) const noexcept {
-            return idx >= rhs.idx;
+            return cb->offset(idx) >= cb->offset(rhs.idx);
         }
         bool operator<=(const cbiterator<CB, ValueType>& rhs) const noexcept {
-            return idx <= rhs.idx;
+            return cb->offset(idx) <= cb->offset(rhs.idx);
         }
        difference_type operator-(const cbiterator<CB, ValueType>& rhs) const noexcept {
             return idx - rhs.idx;
@@ -230,6 +231,13 @@ circular_buffer<T, Alloc>::mask(size_t idx) const {
     return idx & (_impl.capacity - 1);
 }
 
+template <typename T, typename Alloc>
+inline
+size_t
+circular_buffer<T, Alloc>::offset(size_t idx) const {
+    return mask(idx) - mask(_impl.begin);
+}
+
 template <typename T, typename Alloc>
 inline
 bool

diff --git a/tests/unit/circular_buffer_test.cc b/tests/unit/circular_buffer_test.cc
@@ -107,3 +107,27 @@ BOOST_AUTO_TEST_CASE(test_erasing_in_the_middle) {
     BOOST_REQUIRE_EQUAL(*i++, 9);
     BOOST_REQUIRE(i == buf.end());
 }
+
+BOOST_AUTO_TEST_CASE(test_underflow_index_iterator_comparison) {
+    circular_buffer<int> buf;
+
+    // We want to craft a buffer such that push_front() wraps around.
+    buf.reserve(20);
+    for (int i = 0; i < 10; ++i) {
+        buf.push_back(i);
+    }
+    for (int i = 0; i < 10; ++i) {
+        buf.push_front(i);
+    }
+
+    const auto it1 = buf.begin() + 5;
+    const auto it2 = it1 + 10;
+    const auto it3 = it2;
+
+    BOOST_REQUIRE(it1 < it2);
+    BOOST_REQUIRE(it2 > it1);
+    BOOST_REQUIRE(it1 <= it2);
+    BOOST_REQUIRE(it2 <= it3);
+    BOOST_REQUIRE(it2 >= it1);
+    BOOST_REQUIRE(it3 >= it2);
+}