Refactor VectorBuffer to use CheckedIterators.

Currently VectorBuffer allows the access of buffer[size]. This is
because callers are using buffer[size] to get buffer.end(). In order to
avoid this, use CheckedRandomAccessIterators in VectorBuffer. This
required creating iterator versions for the DestructRange and MoveRange
functions.

CheckedIterator's behavior now depends on the enable_checked_iterators
buildflag: when true, CheckedIterator will CHECK on spatial safety
violations; when false, CheckedIterator will merely DCHECK. This is
temporary, so that binary size regressions on Android can be studied
and explained: the eventual goal is to enable this universally and
remove the buildflag.

BUG=chromium:817982

Change-Id: I2037bdb542c4442011fc9ebffdc50e50ccfafa7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1674746Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685387}

base/BUILD.gn

@@ -46,6 +46,13 @@ declare_args() {
   # file name) is saved.
   enable_location_source = true
 
+  # When enabled, iterators will validate that they are not used in ways that
+  # violate spatial safety (i.e. out-of-bounds memory accesses).
+  # TODO(https://crbug.com/817982): currently guarded by a buildflag since
+  # there are non-trivial binary size regressions which are especially
+  # noticeable on Android.
+  enable_checked_iterators = !is_android
+
   # Unsafe developer build. Has developer-friendly features that may weaken or
   # disable security measures like sandboxing or ASLR.
   # IMPORTANT: Unsafe developer builds should never be distributed to end users.
@@ -1267,6 +1274,7 @@ jumbo_component("base") {
     ":build_date",
     ":cfi_buildflags",
     ":debugging_buildflags",
+    ":iterator_buildflags",
     ":logging_buildflags",
     ":orderfile_buildflags",
     ":partition_alloc_buildflags",
@@ -2081,6 +2089,12 @@ buildflag_header("debugging_buildflags") {
   ]
 }
 
+buildflag_header("iterator_buildflags") {
+  header = "iterator_buildflags.h"
+  header_dir = "base/containers"
+  flags = [ "ENABLE_CHECKED_ITERATORS=$enable_checked_iterators" ]
+}
+
 buildflag_header("logging_buildflags") {
   header = "logging_buildflags.h"
 

base/containers/checked_iterators.h

@@ -8,8 +8,20 @@
 #include <iterator>
 #include <memory>
 
+#include "base/containers/iterator_buildflags.h"
 #include "base/containers/util.h"
 #include "base/logging.h"
+#include "build/build_config.h"
+
+#if BUILDFLAG(ENABLE_CHECKED_ITERATORS)
+#define ITERATOR_CHECK CHECK
+#define ITERATOR_CHECK_EQ CHECK_EQ
+#define ITERATOR_CHECK_LE CHECK_LE
+#else
+#define ITERATOR_CHECK DCHECK
+#define ITERATOR_CHECK_EQ DCHECK_EQ
+#define ITERATOR_CHECK_LE DCHECK_LE
+#endif
 
 namespace base {
 
@@ -32,8 +44,8 @@ class CheckedRandomAccessIterator {
       : CheckedRandomAccessIterator(start, start, end) {}
   CheckedRandomAccessIterator(T* start, T* current, const T* end)
       : start_(start), current_(current), end_(end) {
-    CHECK(start <= current);
-    CHECK(current <= end);
+    ITERATOR_CHECK(start <= current);
+    ITERATOR_CHECK(current <= end);
   }
   CheckedRandomAccessIterator(const CheckedRandomAccessIterator& other) =
       default;
@@ -63,7 +75,7 @@ class CheckedRandomAccessIterator {
   }
 
   CheckedRandomAccessIterator& operator++() {
-    CHECK(current_ != end_);
+    ITERATOR_CHECK(current_ != end_);
     ++current_;
     return *this;
   }
@@ -75,7 +87,7 @@ class CheckedRandomAccessIterator {
   }
 
   CheckedRandomAccessIterator& operator--() {
-    CHECK(current_ != start_);
+    ITERATOR_CHECK(current_ != start_);
     --current_;
     return *this;
   }
@@ -88,9 +100,9 @@ class CheckedRandomAccessIterator {
 
   CheckedRandomAccessIterator& operator+=(difference_type rhs) {
     if (rhs > 0) {
-      CHECK_LE(rhs, end_ - current_);
+      ITERATOR_CHECK_LE(rhs, end_ - current_);
     } else {
-      CHECK_LE(-rhs, current_ - start_);
+      ITERATOR_CHECK_LE(-rhs, current_ - start_);
     }
     current_ += rhs;
     return *this;
@@ -104,9 +116,9 @@ class CheckedRandomAccessIterator {
 
   CheckedRandomAccessIterator& operator-=(difference_type rhs) {
     if (rhs < 0) {
-      CHECK_LE(rhs, end_ - current_);
+      ITERATOR_CHECK_LE(rhs, end_ - current_);
     } else {
-      CHECK_LE(-rhs, current_ - start_);
+      ITERATOR_CHECK_LE(-rhs, current_ - start_);
     }
     current_ -= rhs;
     return *this;
@@ -120,18 +132,18 @@ class CheckedRandomAccessIterator {
 
   friend difference_type operator-(const CheckedRandomAccessIterator& lhs,
                                    const CheckedRandomAccessIterator& rhs) {
-    CHECK(lhs.start_ == rhs.start_);
-    CHECK(lhs.end_ == rhs.end_);
+    ITERATOR_CHECK(lhs.start_ == rhs.start_);
+    ITERATOR_CHECK(lhs.end_ == rhs.end_);
     return lhs.current_ - rhs.current_;
   }
 
   reference operator*() const {
-    CHECK(current_ != end_);
+    ITERATOR_CHECK(current_ != end_);
     return *current_;
   }
 
   pointer operator->() const {
-    CHECK(current_ != end_);
+    ITERATOR_CHECK(current_ != end_);
     return current_;
   }
 
@@ -153,8 +165,8 @@ class CheckedRandomAccessIterator {
 
  private:
   void CheckComparable(const CheckedRandomAccessIterator& other) const {
-    CHECK_EQ(start_, other.start_);
-    CHECK_EQ(end_, other.end_);
+    ITERATOR_CHECK_EQ(start_, other.start_);
+    ITERATOR_CHECK_EQ(end_, other.end_);
   }
 
   const T* start_ = nullptr;
@@ -176,16 +188,16 @@ class CheckedRandomAccessConstIterator {
       : CheckedRandomAccessConstIterator(start, start, end) {}
   CheckedRandomAccessConstIterator(T* start, T* current, const T* end)
       : start_(start), current_(current), end_(end) {
-    CHECK(start <= current);
-    CHECK(current <= end);
+    ITERATOR_CHECK(start <= current);
+    ITERATOR_CHECK(current <= end);
   }
   CheckedRandomAccessConstIterator(
       const CheckedRandomAccessConstIterator& other) = default;
   CheckedRandomAccessConstIterator(const CheckedRandomAccessIterator<T>& other)
       : start_(other.start_), current_(other.current_), end_(other.end_) {
     // We explicitly don't delegate to the 3-argument constructor here. Its
-    // CHECKs would be redundant, since we expect |other| to maintain its own
-    // invariant. However, DCHECKs never hurt anybody. Presumably.
+    // ITERATOR_CHECKs would be redundant, since we expect |other| to maintain
+    // its own invariant. However, DCHECKs never hurt anybody. Presumably.
     DCHECK(other.start_ <= other.current_);
     DCHECK(other.current_ <= other.end_);
   }
@@ -218,7 +230,7 @@ class CheckedRandomAccessConstIterator {
   }
 
   CheckedRandomAccessConstIterator& operator++() {
-    CHECK(current_ != end_);
+    ITERATOR_CHECK(current_ != end_);
     ++current_;
     return *this;
   }
@@ -230,7 +242,7 @@ class CheckedRandomAccessConstIterator {
   }
 
   CheckedRandomAccessConstIterator& operator--() {
-    CHECK(current_ != start_);
+    ITERATOR_CHECK(current_ != start_);
     --current_;
     return *this;
   }
@@ -243,9 +255,9 @@ class CheckedRandomAccessConstIterator {
 
   CheckedRandomAccessConstIterator& operator+=(difference_type rhs) {
     if (rhs > 0) {
-      CHECK_LE(rhs, end_ - current_);
+      ITERATOR_CHECK_LE(rhs, end_ - current_);
     } else {
-      CHECK_LE(-rhs, current_ - start_);
+      ITERATOR_CHECK_LE(-rhs, current_ - start_);
     }
     current_ += rhs;
     return *this;
@@ -259,9 +271,9 @@ class CheckedRandomAccessConstIterator {
 
   CheckedRandomAccessConstIterator& operator-=(difference_type rhs) {
     if (rhs < 0) {
-      CHECK_LE(rhs, end_ - current_);
+      ITERATOR_CHECK_LE(rhs, end_ - current_);
     } else {
-      CHECK_LE(-rhs, current_ - start_);
+      ITERATOR_CHECK_LE(-rhs, current_ - start_);
     }
     current_ -= rhs;
     return *this;
@@ -276,18 +288,18 @@ class CheckedRandomAccessConstIterator {
   friend difference_type operator-(
       const CheckedRandomAccessConstIterator& lhs,
       const CheckedRandomAccessConstIterator& rhs) {
-    CHECK(lhs.start_ == rhs.start_);
-    CHECK(lhs.end_ == rhs.end_);
+    ITERATOR_CHECK(lhs.start_ == rhs.start_);
+    ITERATOR_CHECK(lhs.end_ == rhs.end_);
     return lhs.current_ - rhs.current_;
   }
 
   reference operator*() const {
-    CHECK(current_ != end_);
+    ITERATOR_CHECK(current_ != end_);
     return *current_;
   }
 
   pointer operator->() const {
-    CHECK(current_ != end_);
+    ITERATOR_CHECK(current_ != end_);
     return current_;
   }
 
@@ -309,8 +321,8 @@ class CheckedRandomAccessConstIterator {
 
  private:
   void CheckComparable(const CheckedRandomAccessConstIterator& other) const {
-    CHECK_EQ(start_, other.start_);
-    CHECK_EQ(end_, other.end_);
+    ITERATOR_CHECK_EQ(start_, other.start_);
+    ITERATOR_CHECK_EQ(end_, other.end_);
   }
 
   const T* start_ = nullptr;
@@ -320,4 +332,8 @@ class CheckedRandomAccessConstIterator {
 
 }  // namespace base
 
+#undef ITERATOR_CHECK
+#undef ITERATOR_CHECK_EQ
+#undef ITERATOR_CHECK_LE
+
 #endif  // BASE_CONTAINERS_CHECKED_ITERATORS_H_

base/containers/circular_deque.h

@@ -791,12 +791,12 @@ class circular_deque {
       return iterator(this, first.index_);
     } else if (first.index_ < last.index_) {
       // Contiguous range.
-      buffer_.DestructRange(&buffer_[first.index_], &buffer_[last.index_]);
+      buffer_.DestructRange(buffer_.begin() + first.index_,
+                            buffer_.begin() + last.index_);
     } else {
       // Deleted range wraps around.
-      buffer_.DestructRange(&buffer_[first.index_],
-                            &buffer_[buffer_.capacity()]);
-      buffer_.DestructRange(&buffer_[0], &buffer_[last.index_]);
+      buffer_.DestructRange(buffer_.begin() + first.index_, buffer_.end());
+      buffer_.DestructRange(buffer_.begin(), buffer_.begin() + last.index_);
     }
 
     if (first.index_ == begin_) {
@@ -812,9 +812,9 @@ class circular_deque {
     iterator move_src_end = end();
     iterator move_dest(this, first.index_);
     for (; move_src < move_src_end; move_src++, move_dest++) {
-      buffer_.MoveRange(&buffer_[move_src.index_],
-                        &buffer_[move_src.index_ + 1],
-                        &buffer_[move_dest.index_]);
+      buffer_.MoveRange(buffer_.begin() + move_src.index_,
+                        buffer_.begin() + move_src.index_ + 1,
+                        buffer_.begin() + move_dest.index_);
     }
 
     end_ = move_dest.index_;
@@ -860,7 +860,8 @@ class circular_deque {
 
   void pop_front() {
     DCHECK(size());
-    buffer_.DestructRange(&buffer_[begin_], &buffer_[begin_ + 1]);
+    buffer_.DestructRange(buffer_.begin() + begin_,
+                          buffer_.begin() + begin_ + 1);
     begin_++;
     if (begin_ == buffer_.capacity())
       begin_ = 0;
@@ -879,7 +880,7 @@ class circular_deque {
       end_ = buffer_.capacity() - 1;
     else
       end_--;
-    buffer_.DestructRange(&buffer_[end_], &buffer_[end_ + 1]);
+    buffer_.DestructRange(buffer_.begin() + end_, buffer_.begin() + end_ + 1);
 
     ShrinkCapacityIfNecessary();
 
@@ -917,17 +918,17 @@ class circular_deque {
     *to_begin = 0;
     if (from_begin < from_end) {
       // Contiguous.
-      from_buf.MoveRange(&from_buf[from_begin], &from_buf[from_end],
-                         to_buf->begin());
+      from_buf.MoveRange(from_buf.begin() + from_begin,
+                         from_buf.begin() + from_end, to_buf->begin());
       *to_end = from_end - from_begin;
     } else if (from_begin > from_end) {
       // Discontiguous, copy the right side to the beginning of the new buffer.
-      from_buf.MoveRange(&from_buf[from_begin], &from_buf[from_capacity],
+      from_buf.MoveRange(from_buf.begin() + from_begin, from_buf.end(),
                          to_buf->begin());
       size_t right_size = from_capacity - from_begin;
       // Append the left side.
-      from_buf.MoveRange(&from_buf[0], &from_buf[from_end],
-                         &(*to_buf)[right_size]);
+      from_buf.MoveRange(from_buf.begin(), from_buf.begin() + from_end,
+                         to_buf->begin() + right_size);
       *to_end = right_size + from_end;
     } else {
       // No items.
@@ -998,10 +999,10 @@ class circular_deque {
     if (end == begin) {
       return;
     } else if (end > begin) {
-      buffer_.DestructRange(&buffer_[begin], &buffer_[end]);
+      buffer_.DestructRange(buffer_.begin() + begin, buffer_.begin() + end);
     } else {
-      buffer_.DestructRange(&buffer_[begin], &buffer_[buffer_.capacity()]);
-      buffer_.DestructRange(&buffer_[0], &buffer_[end]);
+      buffer_.DestructRange(buffer_.begin() + begin, buffer_.end());
+      buffer_.DestructRange(buffer_.begin(), buffer_.begin() + end);
     }
   }
 
@@ -1035,8 +1036,9 @@ class circular_deque {
         break;
       --src;
       --dest;
-      buffer_.MoveRange(&buffer_[src.index_], &buffer_[src.index_ + 1],
-                        &buffer_[dest.index_]);
+      buffer_.MoveRange(buffer_.begin() + src.index_,
+                        buffer_.begin() + src.index_ + 1,
+                        buffer_.begin() + dest.index_);
     }
   }
 

base/containers/vector_buffer.h

@@ -11,10 +11,12 @@
 #include <type_traits>
 #include <utility>
 
+#include "base/containers/checked_iterators.h"
 #include "base/containers/util.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/numerics/checked_math.h"
+#include "build/build_config.h"
 
 namespace base {
 namespace internal {
@@ -40,6 +42,8 @@ template <typename T>
 class VectorBuffer {
  public:
   constexpr VectorBuffer() = default;
+  using iterator = CheckedRandomAccessIterator<T>;
+  using const_iterator = CheckedRandomAccessConstIterator<T>;
 
 #if defined(__clang__) && !defined(__native_client__)
   // This constructor converts an uninitialized void* to a T* which triggers
@@ -86,8 +90,12 @@ class VectorBuffer {
     return buffer_[i];
   }
 
-  T* begin() { return buffer_; }
-  T* end() { return &buffer_[capacity_]; }
+  iterator begin() const noexcept {
+    return iterator(buffer_, buffer_ + capacity_);
+  }
+  iterator end() const noexcept {
+    return iterator(buffer_, buffer_ + capacity_, buffer_ + capacity_);
+  }
 
   // DestructRange ------------------------------------------------------------
 
@@ -95,15 +103,15 @@ class VectorBuffer {
   template <typename T2 = T,
             typename std::enable_if<std::is_trivially_destructible<T2>::value,
                                     int>::type = 0>
-  void DestructRange(T* begin, T* end) {}
+  void DestructRange(iterator begin, iterator end) {}
 
   // Non-trivially destructible objects must have their destructors called
   // individually.
   template <typename T2 = T,
             typename std::enable_if<!std::is_trivially_destructible<T2>::value,
                                     int>::type = 0>
-  void DestructRange(T* begin, T* end) {
-    CHECK_LE(begin, end);
+  void DestructRange(iterator begin, iterator end) {
+    CHECK(begin <= end);
     while (begin != end) {
       begin->~T();
       begin++;
@@ -119,16 +127,15 @@ class VectorBuffer {
   // and the address of the first element to copy to. There must be sufficient
   // room in the destination for all items in the range [begin, end).
 
-  // Trivially copyable types can use memcpy. trivially copyable implies
-  // that there is a trivial destructor as we don't have to call it.
+  // Trivially copyable types can use memcpy. Trivially copyable implies that
+  // there is a trivial destructor as we don't have to call it.
   template <typename T2 = T,
             typename std::enable_if<base::is_trivially_copyable<T2>::value,
                                     int>::type = 0>
-  static void MoveRange(T* from_begin, T* from_end, T* to) {
-    CHECK(!RangesOverlap(from_begin, from_end, to));
-    memcpy(
-        to, from_begin,
-        CheckSub(get_uintptr(from_end), get_uintptr(from_begin)).ValueOrDie());
+  static void MoveRange(iterator from_begin, iterator from_end, iterator to) {
+    CHECK(iterator::IsRangeMoveSafe(from_begin, from_end, to));
+    memcpy(&(*to), &(*from_begin),
+           std::distance(from_begin, from_end) * sizeof(T));
   }
 
   // Not trivially copyable, but movable: call the move constructor and
@@ -137,10 +144,10 @@ class VectorBuffer {
             typename std::enable_if<std::is_move_constructible<T2>::value &&
                                         !base::is_trivially_copyable<T2>::value,
                                     int>::type = 0>
-  static void MoveRange(T* from_begin, T* from_end, T* to) {
-    CHECK(!RangesOverlap(from_begin, from_end, to));
+  static void MoveRange(iterator from_begin, iterator from_end, iterator to) {
+    CHECK(iterator::IsRangeMoveSafe(from_begin, from_end, to));
     while (from_begin != from_end) {
-      new (to) T(std::move(*from_begin));
+      new (&(*to)) T(std::move(*from_begin));
       from_begin->~T();
       from_begin++;
       to++;
@@ -153,10 +160,10 @@ class VectorBuffer {
             typename std::enable_if<!std::is_move_constructible<T2>::value &&
                                         !base::is_trivially_copyable<T2>::value,
                                     int>::type = 0>
-  static void MoveRange(T* from_begin, T* from_end, T* to) {
-    CHECK(!RangesOverlap(from_begin, from_end, to));
+  static void MoveRange(iterator from_begin, iterator from_end, iterator to) {
+    CHECK(iterator::IsRangeMoveSafe(from_begin, from_end, to));
     while (from_begin != from_end) {
-      new (to) T(*from_begin);
+      new (&(*to)) T(*from_begin);
       from_begin->~T();
       from_begin++;
       to++;
@@ -164,18 +171,6 @@ class VectorBuffer {
   }
 
  private:
-  static bool RangesOverlap(const T* from_begin,
-                            const T* from_end,
-                            const T* to) {
-    const auto from_begin_uintptr = get_uintptr(from_begin);
-    const auto from_end_uintptr = get_uintptr(from_end);
-    const auto to_uintptr = get_uintptr(to);
-    return !(
-        to >= from_end ||
-        CheckAdd(to_uintptr, CheckSub(from_end_uintptr, from_begin_uintptr))
-                .ValueOrDie() <= from_begin_uintptr);
-  }
-
   T* buffer_ = nullptr;
   size_t capacity_ = 0;
 

base/containers/vector_buffer_unittest.cc

@@ -15,26 +15,26 @@ TEST(VectorBuffer, DeletePOD) {
   constexpr int size = 10;
   VectorBuffer<int> buffer(size);
   for (int i = 0; i < size; i++)
-    buffer.begin()[i] = i + 1;
+    buffer[i] = i + 1;
 
   buffer.DestructRange(buffer.begin(), buffer.end());
 
   // Delete should do nothing.
   for (int i = 0; i < size; i++)
-    EXPECT_EQ(i + 1, buffer.begin()[i]);
+    EXPECT_EQ(i + 1, buffer[i]);
 }
 
 TEST(VectorBuffer, DeleteMoveOnly) {
   constexpr int size = 10;
   VectorBuffer<MoveOnlyInt> buffer(size);
   for (int i = 0; i < size; i++)
-    new (buffer.begin() + i) MoveOnlyInt(i + 1);
+    buffer[i] = MoveOnlyInt(i + 1);
 
   buffer.DestructRange(buffer.begin(), buffer.end());
 
   // Delete should have reset all of the values to 0.
   for (int i = 0; i < size; i++)
-    EXPECT_EQ(0, buffer.begin()[i].data());
+    EXPECT_EQ(0, buffer[i].data());
 }
 
 TEST(VectorBuffer, PODMove) {
@@ -43,11 +43,11 @@ TEST(VectorBuffer, PODMove) {
 
   VectorBuffer<int> original(size);
   for (int i = 0; i < size; i++)
-    original.begin()[i] = i + 1;
+    original[i] = i + 1;
 
   original.MoveRange(original.begin(), original.end(), dest.begin());
   for (int i = 0; i < size; i++)
-    EXPECT_EQ(i + 1, dest.begin()[i]);
+    EXPECT_EQ(i + 1, dest[i]);
 }
 
 TEST(VectorBuffer, MovableMove) {
@@ -56,14 +56,13 @@ TEST(VectorBuffer, MovableMove) {
 
   VectorBuffer<MoveOnlyInt> original(size);
   for (int i = 0; i < size; i++)
-    new (original.begin() + i) MoveOnlyInt(i + 1);
+    original[i] = MoveOnlyInt(i + 1);
 
   original.MoveRange(original.begin(), original.end(), dest.begin());
-
   // Moving from a MoveOnlyInt resets to 0.
   for (int i = 0; i < size; i++) {
-    EXPECT_EQ(0, original.begin()[i].data());
-    EXPECT_EQ(i + 1, dest.begin()[i].data());
+    EXPECT_EQ(0, original[i].data());
+    EXPECT_EQ(i + 1, dest[i].data());
   }
 }
 
@@ -73,15 +72,15 @@ TEST(VectorBuffer, CopyToMove) {
 
   VectorBuffer<CopyOnlyInt> original(size);
   for (int i = 0; i < size; i++)
-    new (original.begin() + i) CopyOnlyInt(i + 1);
+    new (&original[i]) CopyOnlyInt(i + 1);
 
   original.MoveRange(original.begin(), original.end(), dest.begin());
 
   // The original should have been destructed, which should reset the value to
   // 0. Technically this dereferences the destructed object.
   for (int i = 0; i < size; i++) {
-    EXPECT_EQ(0, original.begin()[i].data());
-    EXPECT_EQ(i + 1, dest.begin()[i].data());
+    EXPECT_EQ(0, original[i].data());
+    EXPECT_EQ(i + 1, dest[i].data());
   }
 }