fix iterator invalidation issue by storing indices not iterators

Author: MungoG

include/boost/buffers/slice.hpp

@@ -64,9 +64,12 @@ class slice_of
     using iter_type = decltype(
         std::declval<BufferSequence const&>().begin());
 
+    using difference_type =
+        typename std::iterator_traits<iter_type>::difference_type;
+
     BufferSequence bs_;
-    iter_type begin_;
-    iter_type end_;
+    difference_type begin_ = 0; // index of first buffer in sequence
+    difference_type end_ = 0;   // 1 + index of last buffer in sequence
     std::size_t len_ = 0;       // length of bs_
     std::size_t size_ = 0;      // total bytes
     std::size_t prefix_ = 0;    // used prefix bytes
@@ -92,11 +95,12 @@ class slice_of
     slice_of(
         BufferSequence const& bs)
         : bs_(bs)
-        , begin_(buffers::begin(bs_))
-        , end_(buffers::end(bs_))
     {
-        auto it = begin_;
-        while(it != end_)
+        iter_type it = buffers::begin(bs_);
+        iter_type eit = buffers::end(bs_);
+        begin_ = 0;
+        end_ = std::distance(it, eit);
+        while(it != eit)
         {
             value_type b(*it);
             size_ += b.size();
@@ -127,18 +131,39 @@ class slice_of
     }
 
 private:
+    iter_type
+    begin_iter_impl() const noexcept
+    {
+        iter_type it = buffers::begin(bs_);
+        std::advance(it, begin_);
+        return it;
+    }
+
+    iter_type
+    end_iter_impl() const noexcept
+    {
+        iter_type it = buffers::begin(bs_);
+        std::advance(it, end_);
+        return it;
+    }
+
     void
     remove_prefix_impl(
         std::size_t n)
     {
+        if(n > size_)
+            n = size_;
+
         // nice hack to simplify the loop (M. Nejati)
         n += prefix_;
         size_ += prefix_;
         prefix_ = 0;
 
+        iter_type it = begin_iter_impl();
+
         while(n > 0 && begin_ != end_)
         {
-            value_type b = *begin_;
+            value_type b = *it;
             if(n < b.size())
             {
                 prefix_ = n;
@@ -148,6 +173,7 @@ class slice_of
             n -= b.size();
             size_ -= b.size();
             ++begin_;
+            ++it;
             --len_;
         }
     }
@@ -162,12 +188,19 @@ class slice_of
             return;
         }
         BOOST_ASSERT(begin_ != end_);
+
+        if(n > size_)
+            n = size_;
+
         n += suffix_;
         size_ += suffix_;
         suffix_ = 0;
-        iter_type it = end_;
-        --it;
-        while(it != begin_)
+
+        iter_type bit = begin_iter_impl();
+        iter_type it = end_iter_impl();
+        it--;
+
+        while(it != bit)
         {
             value_type b = *it;
             if(n < b.size())
@@ -192,6 +225,7 @@ class slice_of
         }
         end_ = begin_;
         len_ = 0;
+        size_ = 0;
     }
 
     void
@@ -376,7 +410,7 @@ begin() const noexcept ->
     const_iterator
 {
     return const_iterator(
-        this->begin_, prefix_, suffix_, 0, len_);
+        begin_iter_impl(), prefix_, suffix_, 0, len_);
 }
 
 template<class BufferSequence>
@@ -386,7 +420,7 @@ end() const noexcept ->
     const_iterator
 {
     return const_iterator(
-        this->end_, prefix_, suffix_, len_, len_);
+        end_iter_impl(), prefix_, suffix_, len_, len_);
 }
 
 //------------------------------------------------

test/unit/CMakeLists.txt

@@ -7,7 +7,9 @@
 # Official repository: https://github.com/cppalliance/buffers
 #
 
-add_subdirectory(../../../url/extra/test_suite test_suite)
+if (NOT TARGET boost_url_test_suite)
+    add_subdirectory(../../../url/extra/test_suite test_suite)
+endif()
 
 file(GLOB_RECURSE PFILES CONFIGURE_DEPENDS *.cpp *.hpp)
 list(APPEND PFILES

test/unit/slice.cpp

@@ -17,6 +17,7 @@
 #include <boost/static_assert.hpp>
 
 #include <array>
+#include <vector>
 
 #include "test_buffers.hpp"
 #include "test_suite.hpp"
@@ -119,7 +120,8 @@ struct slice_test
         test::check_iterators(b, s);
     }
 
-    using seq_type = std::array<const_buffer, 3>;
+    // Use a vector so that iterator invalidation is observable during testing.
+    using seq_type = std::vector<const_buffer>;
 
     void
     grind_back(
@@ -176,8 +178,8 @@ struct slice_test
     run()
     {
         std::string s;
-        seq_type bs =  make_buffers(s,
-            "boost.", "buffers.", "slice_" );
+        auto a = make_buffers(s, "boost.", "buffers.", "slice_");
+        seq_type bs(a.begin(), a.end());
         test::check_sequence(bs, s);
         //check(bs, s);
         //grind(bs, s);

test/unit/test_buffers.hpp

@@ -17,6 +17,7 @@
 #include <boost/core/detail/string_view.hpp>
 #include <boost/assert.hpp>
 #include <boost/static_assert.hpp>
+
 #include <string>
 
 // Trick boostdep into requiring URL
@@ -255,7 +256,8 @@ template<class ConstBufferSequence>
 void
 grind_front(
     ConstBufferSequence const& bs0,
-    core::string_view pat0)
+    core::string_view pat0,
+    int levels)
 {
     for(std::size_t n = 0; n <= pat0.size() + 1; ++n)
     {
@@ -265,13 +267,37 @@ grind_front(
             remove_prefix(bs, n);
             check_eq(bs, pat);
             check_iterators(bs, pat);
+
+            if(levels)
+            {
+                // Take a copy, blank out the original to invalidate any
+                // iterators, and redo the test
+                slice_type<ConstBufferSequence> bsc(bs);
+                {
+                    slice_type<ConstBufferSequence> dummy{};
+                    std::swap(bs, dummy);
+                }
+                grind_front(bsc, pat, levels-1);
+            }
         }
         {
             auto pat = kept_front(pat0, n);
             slice_type<ConstBufferSequence> bs(bs0);
             keep_prefix(bs, n);
             check_eq(bs, pat);
             check_iterators(bs, pat);
+
+            if(levels)
+            {
+                // Take a copy, blank out the original to invalidate any
+                // iterators, and redo the test
+                slice_type<ConstBufferSequence> bsc(bs);
+                {
+                    slice_type<ConstBufferSequence> dummy{};
+                    std::swap(bs, dummy);
+                }
+                grind_front(bsc, pat, levels-1);
+            }
         }
     }
 }
@@ -280,7 +306,8 @@ template<class ConstBufferSequence>
 void
 grind_back(
     ConstBufferSequence const& bs0,
-    core::string_view pat0)
+    core::string_view pat0,
+    int levels)
 {
     for(std::size_t n = 0; n <= pat0.size() + 1; ++n)
     {
@@ -290,13 +317,89 @@ grind_back(
             remove_suffix(bs, n);
             check_eq(bs, pat);
             check_iterators(bs, pat);
+
+            if(levels)
+            {
+                // Take a copy, blank out the original to invalidate any
+                // iterators, and redo the test
+                slice_type<ConstBufferSequence> bsc(bs);
+                {
+                    slice_type<ConstBufferSequence> dummy{};
+                    std::swap(bs, dummy);
+                }
+                grind_back(bsc, pat, levels-1);
+            }
         }
         {
             auto pat = kept_back(pat0, n);
             slice_type<ConstBufferSequence> bs(bs0);
             keep_suffix(bs, n);
             check_eq(bs, pat);
             check_iterators(bs, pat);
+
+            if(levels)
+            {
+                // Take a copy, blank out the original to invalidate any
+                // iterators, and redo the test
+                slice_type<ConstBufferSequence> bsc(bs);
+                {
+                    slice_type<ConstBufferSequence> dummy{};
+                    std::swap(bs, dummy);
+                }
+                grind_back(bsc, pat, levels-1);
+            }
+        }
+    }
+}
+
+template<class ConstBufferSequence>
+void
+grind_both(
+    ConstBufferSequence const& bs0,
+    core::string_view pat0,
+    int levels)
+{
+    for(std::size_t n = 0; n <= pat0.size() / 2 + 2; ++n)
+    {
+        {
+            auto pat = trimmed_back(trimmed_front(pat0, n), n);
+            slice_type<ConstBufferSequence> bs(bs0);
+            remove_prefix(bs, n);
+            remove_suffix(bs, n);
+            check_eq(bs, pat);
+            check_iterators(bs, pat);
+
+            if(levels)
+            {
+                // Take a copy, blank out the original to invalidate any
+                // iterators, and redo the test
+                slice_type<ConstBufferSequence> bsc(bs);
+                {
+                    slice_type<ConstBufferSequence> dummy{};
+                    std::swap(bs, dummy);
+                }
+                grind_both(bsc, pat, levels - 1);
+            }
+        }
+        {
+            auto pat = kept_back(kept_front(pat0, n), n);
+            slice_type<ConstBufferSequence> bs(bs0);
+            keep_prefix(bs, n);
+            keep_suffix(bs, n);
+            check_eq(bs, pat);
+            check_iterators(bs, pat);
+
+            if(levels)
+            {
+                // Take a copy, blank out the original to invalidate any
+                // iterators, and redo the test
+                slice_type<ConstBufferSequence> bsc(bs);
+                {
+                    slice_type<ConstBufferSequence> dummy{};
+                    std::swap(bs, dummy);
+                }
+                grind_both(bsc, pat, levels - 1);
+            }
         }
     }
 }
@@ -307,8 +410,9 @@ check_slice(
     ConstBufferSequence const& bs,
     core::string_view pat)
 {
-    grind_front(bs, pat);
-    grind_back(bs, pat);
+    grind_front(bs, pat, 1);
+    grind_back(bs, pat, 1);
+    grind_both(bs, pat, 1);
 }
 
 // Test API and behavior of a BufferSequence
@@ -320,6 +424,7 @@ check_sequence(
     BOOST_STATIC_ASSERT(is_const_buffer_sequence<T>::value);
 
     check_iterators(t, pat);
+
     check_slice(t, pat);
 }