Merge bitcoin/bitcoin#32490: refactor: Remove UB in prevector reverse iterators

faf9082 test: Fix whitespace in prevector_tests.cpp (MarcoFalke)
fa7f04c refactor: Remove UB in prevector reverse iterators (MarcoFalke)

Pull request description:

  `rend()` creates a pointer with offset `-1`. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:

      When an expression J that has integral type is added to [...] an
      expression P of pointer type, the result has the type of P.

      ... if P points to a (possibly-hypothetical) array element i of an
      array object x with n elements [...] the expressions P + J and J + P
      (where J has the value j) point to the (possibly-hypothetical) array
      element i+j of x if 0≤i+j≤n [...]

      Otherwise, the behavior is undefined.

  Also, it is unclear why the functions exist at all, when stdlib utils such as `std::reverse_iterator{it}` or `std::views::reverse` can be used out of the box.

  So remove them, along with the ubsan suppressions, that are no longer used.

  I've tagged this a refactor, because the code was always dead (unused outside of tests). And since commit 2925bd5 it was completely dead. Also, I could not find a sanitizer that detects this type of UB.

ACKs for top commit:
  l0rinc:
    tested ACK faf9082
  achow101:
    ACK faf9082
  stickies-v:
    ACK faf9082, nice find.
  theuni:
    utACK faf9082

Tree-SHA512: 31511d520a1c0fdd65c2e5f1a8ef6fd17464303b6bff88a5d9d9577adfee849d431deb510882b6f4e15e8fb7168861bc0d26fca3bed4278f57a9d6e7b1235dce

Author: achow101

src/prevector.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2015-2022 The Bitcoin Core developers
+// Copyright (c) 2015-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -77,26 +77,6 @@ class prevector {
         bool operator<(iterator x) const { return ptr < x.ptr; }
     };
 
-    class reverse_iterator {
-        T* ptr{};
-    public:
-        typedef Diff difference_type;
-        typedef T value_type;
-        typedef T* pointer;
-        typedef T& reference;
-        typedef std::bidirectional_iterator_tag iterator_category;
-        reverse_iterator() = default;
-        reverse_iterator(T* ptr_) : ptr(ptr_) {}
-        T& operator*() const { return *ptr; }
-        T* operator->() const { return ptr; }
-        reverse_iterator& operator--() { ptr++; return *this; }
-        reverse_iterator& operator++() { ptr--; return *this; }
-        reverse_iterator operator++(int) { reverse_iterator copy(*this); ++(*this); return copy; }
-        reverse_iterator operator--(int) { reverse_iterator copy(*this); --(*this); return copy; }
-        bool operator==(reverse_iterator x) const { return ptr == x.ptr; }
-        bool operator!=(reverse_iterator x) const { return ptr != x.ptr; }
-    };
-
     class const_iterator {
         const T* ptr{};
     public:
@@ -129,27 +109,6 @@ class prevector {
         bool operator<(const_iterator x) const { return ptr < x.ptr; }
     };
 
-    class const_reverse_iterator {
-        const T* ptr{};
-    public:
-        typedef Diff difference_type;
-        typedef const T value_type;
-        typedef const T* pointer;
-        typedef const T& reference;
-        typedef std::bidirectional_iterator_tag iterator_category;
-        const_reverse_iterator() = default;
-        const_reverse_iterator(const T* ptr_) : ptr(ptr_) {}
-        const_reverse_iterator(reverse_iterator x) : ptr(&(*x)) {}
-        const T& operator*() const { return *ptr; }
-        const T* operator->() const { return ptr; }
-        const_reverse_iterator& operator--() { ptr++; return *this; }
-        const_reverse_iterator& operator++() { ptr--; return *this; }
-        const_reverse_iterator operator++(int) { const_reverse_iterator copy(*this); ++(*this); return copy; }
-        const_reverse_iterator operator--(int) { const_reverse_iterator copy(*this); --(*this); return copy; }
-        bool operator==(const_reverse_iterator x) const { return ptr == x.ptr; }
-        bool operator!=(const_reverse_iterator x) const { return ptr != x.ptr; }
-    };
-
 private:
 #pragma pack(push, 1)
     union direct_or_indirect {
@@ -304,11 +263,6 @@ class prevector {
     iterator end() { return iterator(item_ptr(size())); }
     const_iterator end() const { return const_iterator(item_ptr(size())); }
 
-    reverse_iterator rbegin() { return reverse_iterator(item_ptr(size() - 1)); }
-    const_reverse_iterator rbegin() const { return const_reverse_iterator(item_ptr(size() - 1)); }
-    reverse_iterator rend() { return reverse_iterator(item_ptr(-1)); }
-    const_reverse_iterator rend() const { return const_reverse_iterator(item_ptr(-1)); }
-
     size_t capacity() const {
         if (is_direct()) {
             return N;

src/test/prevector_tests.cpp

@@ -1,4 +1,4 @@
-// Copyright (c) 2015-2022 The Bitcoin Core developers
+// Copyright (c) 2015-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -15,8 +15,9 @@
 
 BOOST_FIXTURE_TEST_SUITE(prevector_tests, TestingSetup)
 
-template<unsigned int N, typename T>
-class prevector_tester {
+template <unsigned int N, typename T>
+class prevector_tester
+{
     typedef std::vector<T> realtype;
     realtype real_vector;
     realtype real_vector_alt;
@@ -31,36 +32,37 @@ class prevector_tester {
 
 
     template <typename A, typename B>
-        void local_check_equal(A a, B b)
-        {
-            local_check(a == b);
-        }
+    void local_check_equal(A a, B b)
+    {
+        local_check(a == b);
+    }
     void local_check(bool b)
     {
         passed &= b;
     }
-    void test() {
+    void test()
+    {
         const pretype& const_pre_vector = pre_vector;
         local_check_equal(real_vector.size(), pre_vector.size());
         local_check_equal(real_vector.empty(), pre_vector.empty());
         for (Size s = 0; s < real_vector.size(); s++) {
-             local_check(real_vector[s] == pre_vector[s]);
-             local_check(&(pre_vector[s]) == &(pre_vector.begin()[s]));
-             local_check(&(pre_vector[s]) == &*(pre_vector.begin() + s));
-             local_check(&(pre_vector[s]) == &*((pre_vector.end() + s) - real_vector.size()));
+            local_check(real_vector[s] == pre_vector[s]);
+            local_check(&(pre_vector[s]) == &(pre_vector.begin()[s]));
+            local_check(&(pre_vector[s]) == &*(pre_vector.begin() + s));
+            local_check(&(pre_vector[s]) == &*((pre_vector.end() + s) - real_vector.size()));
         }
         // local_check(realtype(pre_vector) == real_vector);
         local_check(pretype(real_vector.begin(), real_vector.end()) == pre_vector);
         local_check(pretype(pre_vector.begin(), pre_vector.end()) == pre_vector);
         size_t pos = 0;
         for (const T& v : pre_vector) {
-             local_check(v == real_vector[pos++]);
+            local_check(v == real_vector[pos++]);
         }
         for (const T& v : pre_vector | std::views::reverse) {
             local_check(v == real_vector[--pos]);
         }
         for (const T& v : const_pre_vector) {
-             local_check(v == real_vector[pos++]);
+            local_check(v == real_vector[pos++]);
         }
         for (const T& v : const_pre_vector | std::views::reverse) {
             local_check(v == real_vector[--pos]);
@@ -76,90 +78,105 @@ class prevector_tester {
     }
 
 public:
-    void resize(Size s) {
+    void resize(Size s)
+    {
         real_vector.resize(s);
         local_check_equal(real_vector.size(), s);
         pre_vector.resize(s);
         local_check_equal(pre_vector.size(), s);
         test();
     }
 
-    void reserve(Size s) {
+    void reserve(Size s)
+    {
         real_vector.reserve(s);
         local_check(real_vector.capacity() >= s);
         pre_vector.reserve(s);
         local_check(pre_vector.capacity() >= s);
         test();
     }
 
-    void insert(Size position, const T& value) {
+    void insert(Size position, const T& value)
+    {
         real_vector.insert(real_vector.begin() + position, value);
         pre_vector.insert(pre_vector.begin() + position, value);
         test();
     }
 
-    void insert(Size position, Size count, const T& value) {
+    void insert(Size position, Size count, const T& value)
+    {
         real_vector.insert(real_vector.begin() + position, count, value);
         pre_vector.insert(pre_vector.begin() + position, count, value);
         test();
     }
 
-    template<typename I>
-    void insert_range(Size position, I first, I last) {
+    template <typename I>
+    void insert_range(Size position, I first, I last)
+    {
         real_vector.insert(real_vector.begin() + position, first, last);
         pre_vector.insert(pre_vector.begin() + position, first, last);
         test();
     }
 
-    void erase(Size position) {
+    void erase(Size position)
+    {
         real_vector.erase(real_vector.begin() + position);
         pre_vector.erase(pre_vector.begin() + position);
         test();
     }
 
-    void erase(Size first, Size last) {
+    void erase(Size first, Size last)
+    {
         real_vector.erase(real_vector.begin() + first, real_vector.begin() + last);
         pre_vector.erase(pre_vector.begin() + first, pre_vector.begin() + last);
         test();
     }
 
-    void update(Size pos, const T& value) {
+    void update(Size pos, const T& value)
+    {
         real_vector[pos] = value;
         pre_vector[pos] = value;
         test();
     }
 
-    void push_back(const T& value) {
+    void push_back(const T& value)
+    {
         real_vector.push_back(value);
         pre_vector.push_back(value);
         test();
     }
 
-    void pop_back() {
+    void pop_back()
+    {
         real_vector.pop_back();
         pre_vector.pop_back();
         test();
     }
 
-    void clear() {
+    void clear()
+    {
         real_vector.clear();
         pre_vector.clear();
     }
 
-    void assign(Size n, const T& value) {
+    void assign(Size n, const T& value)
+    {
         real_vector.assign(n, value);
         pre_vector.assign(n, value);
     }
 
-    Size size() const {
+    Size size() const
+    {
         return real_vector.size();
     }
 
-    Size capacity() const {
+    Size capacity() const
+    {
         return pre_vector.capacity();
     }
 
-    void shrink_to_fit() {
+    void shrink_to_fit()
+    {
         pre_vector.shrink_to_fit();
         test();
     }
@@ -171,19 +188,22 @@ class prevector_tester {
         test();
     }
 
-    void move() {
+    void move()
+    {
         real_vector = std::move(real_vector_alt);
         real_vector_alt.clear();
         pre_vector = std::move(pre_vector_alt);
         pre_vector_alt.clear();
     }
 
-    void copy() {
+    void copy()
+    {
         real_vector = real_vector_alt;
         pre_vector = pre_vector_alt;
     }
 
-    void resize_uninitialized(realtype values) {
+    void resize_uninitialized(realtype values)
+    {
         size_t r = values.size();
         size_t s = real_vector.size() / 2;
         if (real_vector.capacity() < s + r) {
@@ -203,11 +223,13 @@ class prevector_tester {
         test();
     }
 
-    ~prevector_tester() {
+    ~prevector_tester()
+    {
         BOOST_CHECK_MESSAGE(passed, "insecure_rand: " + rand_seed.ToString());
     }
 
-    prevector_tester(FastRandomContext& rng) {
+    prevector_tester(FastRandomContext& rng)
+    {
         rand_seed = rng.rand256();
         rng.Reseed(rand_seed);
     }

test/sanitizer_suppressions/ubsan

@@ -54,15 +54,13 @@ unsigned-integer-overflow:DecompressAmount
 unsigned-integer-overflow:crypto/
 unsigned-integer-overflow:MurmurHash3
 unsigned-integer-overflow:TxConfirmStats::EstimateMedianVal
-unsigned-integer-overflow:prevector.h
 unsigned-integer-overflow:InsecureRandomContext::rand64
 unsigned-integer-overflow:InsecureRandomContext::SplitMix64
 unsigned-integer-overflow:bitset_detail::PopCount
 implicit-integer-sign-change:SetStdinEcho
 implicit-integer-sign-change:compressor.h
 implicit-integer-sign-change:crypto/
 implicit-integer-sign-change:TxConfirmStats::removeTx
-implicit-integer-sign-change:prevector.h
 implicit-integer-sign-change:verify_flags
 implicit-integer-sign-change:EvalScript
 implicit-integer-sign-change:serialize.h