highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-08-09 08:12:21 +02:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
faf9082a5f test: Fix whitespace in prevector_tests.cpp (MarcoFalke)
fa7f04c8a7 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 2925bd537c it was completely dead. Also, I could not find a sanitizer that detects this type of UB.

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

Tree-SHA512: 31511d520a1c0fdd65c2e5f1a8ef6fd17464303b6bff88a5d9d9577adfee849d431deb510882b6f4e15e8fb7168861bc0d26fca3bed4278f57a9d6e7b1235dce
This commit is contained in:
Ava Chow
2025-05-14 14:34:32 -07:00
parent 53eb5593f0 faf9082a5f
commit d5786bc19a
3 changed files with 58 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
src/prevector.h
View File

@@ -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 @@ public:
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 @@ public:
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 @@ public:
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;

92
src/test/prevector_tests.cpp
View File

@@ -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,7 +78,8 @@ 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);
@@ -84,7 +87,8 @@ public:
test();
}
void reserve(Size s) {
void reserve(Size s)
{
real_vector.reserve(s);
local_check(real_vector.capacity() >= s);
pre_vector.reserve(s);
@@ -92,74 +96,87 @@ public:
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 @@ public:
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 @@ public:
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);
}

2
test/sanitizer_suppressions/ubsan
View File

@@ -54,7 +54,6 @@ 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
@@ -62,7 +61,6 @@ 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