Skip to content

Commit 49d9168

Browse files
davidbendavidben
committed
[libc++][hardening] Check bounds on arithmetic in __bounded_iter
Previously, __bounded_iter only checked operator*. It allowed the pointer to go out of bounds with operator++, etc., and relied on operator* (which checked begin <= current < end) to handle everything. This has several unfortunate consequences: First, pointer arithmetic is UB if it goes out of bounds. So by the time operator* checks, it may be too late and the optimizer may have done something bad. Checking both operations is safer. Second, std::copy and friends currently bypass bounded iterator checks. I think the only hope we have to fix this is to key on iter + n doing a check. See #78771 for further discussion. Note this PR is not sufficient to fix this. It adds the output bounds check, but ends up doing it after the memmove, which is too late. Finally, doing these checks is actually *more* optimizable. See #78829, which is fixed by this PR. Keeping the iterator always in bounds means operator* can rely on some invariants and only needs to check current != end. This aligns better with the ways that callers tend to condition iterator operations. At least for the common case of iterating over the entire See https://godbolt.org/z/vEWrWEf8h for how this new __bounded_iter impacts compiler output. The old __bounded_iter injected checks inside the loops for all the sum() functions, which not only added a check inside a loop, but also impeded Clang's vectorization. The new __bounded_iter allows all the checks to be optimized out and we emit the same code as if it wasn't here. Not everything is ideal however. add_and_deref ends up emitting two comparisons now instead of one. This is because a missed optimization in Clang. I've filed #78875 for that. I suspect (with no data) that this PR is still a net performance win because impeding ranged-for loops is particularly egregious. But ideally we'd fix the optimizer and make add_and_deref fine too. There's also something funny going on with std::ranges::find which I have not yet figured out yet, but I suspect there are some further missed optimization opportunities. Fixes #78829.
1 parent b1d4265 commit 49d9168

File tree

4 files changed

+292
-137
lines changed

4 files changed

+292
-137
lines changed

‎libcxx/include/__iterator/bounded_iter.h

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,10 @@ _LIBCPP_BEGIN_NAMESPACE_STD
3131
// Iterator wrapper that carries the valid range it is allowed to access.
3232
//
3333
// This is a simple iterator wrapper for contiguous iterators that points
34-
// within a [begin, end) range and carries these bounds with it. The iterator
35-
// ensures that it is pointing within that [begin, end) range when it is
36-
// dereferenced.
37-
//
38-
// Arithmetic operations are allowed and the bounds of the resulting iterator
39-
// are not checked. Hence, it is possible to create an iterator pointing outside
40-
// its range, but it is not possible to dereference it.
34+
// within a [begin, end] range and carries these bounds with it. The iterator
35+
// ensures that it is pointing within [begin, end) range when it is
36+
// dereferenced. It also ensures that it is never iterated outside of
37+
// [begin, end].
4138
template <class _Iterator, class = __enable_if_t< __libcpp_is_contiguous_iterator<_Iterator>::value > >
4239
struct __bounded_iter {
4340
using value_type = typename iterator_traits<_Iterator>::value_type;
@@ -70,18 +67,20 @@ struct __bounded_iter {
7067

7168
private:
7269
// Create an iterator wrapping the given iterator, and whose bounds are described
73-
// by the provided [begin, end) range.
70+
// by the provided [begin, end] range.
7471
//
75-
// This constructor does not check whether the resulting iterator is within its bounds.
76-
// However, it does check that the provided [begin, end) range is a valid range (that
77-
// is, begin <= end).
72+
// Except in debug builds, the constructor does not check whether the resulting iterator
73+
// is within its bounds. The container is assumed to be self-consistent.
7874
//
7975
// Since it is non-standard for iterators to have this constructor, __bounded_iter must
8076
// be created via `std::__make_bounded_iter`.
8177
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 explicit __bounded_iter(
8278
_Iterator __current, _Iterator __begin, _Iterator __end)
8379
: __current_(__current), __begin_(__begin), __end_(__end) {
84-
_LIBCPP_ASSERT_INTERNAL(__begin <= __end, "__bounded_iter(current, begin, end): [begin, end) is not a valid range");
80+
_LIBCPP_ASSERT_INTERNAL(
81+
__begin <= __current, "__bounded_iter(current, begin, end): current and begin are inconsistent");
82+
_LIBCPP_ASSERT_INTERNAL(
83+
__current <= __end, "__bounded_iter(current, begin, end): current and end are inconsistent");
8584
}
8685

8786
template <class _It>
@@ -91,21 +90,25 @@ struct __bounded_iter {
9190
// Dereference and indexing operations.
9291
//
9392
// These operations check that the iterator is dereferenceable, that is within [begin, end).
93+
// The iterator is always within [begin, end], so we only need to check for end. This is
94+
// easier to optimize out. See https://github.com/llvm/llvm-project/issues/78829
9495
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference operator*() const _NOEXCEPT {
9596
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
96-
__in_bounds(__current_), "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
97+
__current_ != __end_, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
9798
return *__current_;
9899
}
99100

100101
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pointer operator->() const _NOEXCEPT {
101102
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
102-
__in_bounds(__current_), "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
103+
__current_ != __end_, "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
103104
return std::__to_address(__current_);
104105
}
105106

106107
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference operator[](difference_type __n) const _NOEXCEPT {
107108
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
108-
__in_bounds(__current_ + __n), "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
109+
__n >= __begin_ - __current_, "__bounded_iter::operator[]: Attempt to index an iterator past the start");
110+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
111+
__n < __end_ - __current_, "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
109112
return __current_[__n];
110113
}
111114

@@ -114,6 +117,8 @@ struct __bounded_iter {
114117
// These operations do not check that the resulting iterator is within the bounds, since that
115118
// would make it impossible to create a past-the-end iterator.
116119
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator++() _NOEXCEPT {
120+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
121+
__current_ != __end_, "__bounded_iter::operator++: Attempt to advance an iterator past the end");
117122
++__current_;
118123
return *this;
119124
}
@@ -124,6 +129,8 @@ struct __bounded_iter {
124129
}
125130

126131
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator--() _NOEXCEPT {
132+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
133+
__current_ != __begin_, "__bounded_iter::operator--: Attempt to rewind an iterator past the start");
127134
--__current_;
128135
return *this;
129136
}
@@ -134,6 +141,10 @@ struct __bounded_iter {
134141
}
135142

136143
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator+=(difference_type __n) _NOEXCEPT {
144+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
145+
__n >= __begin_ - __current_, "__bounded_iter::operator+=: Attempt to rewind an iterator past the start");
146+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
147+
__n <= __end_ - __current_, "__bounded_iter::operator+=: Attempt to advance an iterator past the end");
137148
__current_ += __n;
138149
return *this;
139150
}
@@ -151,6 +162,10 @@ struct __bounded_iter {
151162
}
152163

153164
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __bounded_iter& operator-=(difference_type __n) _NOEXCEPT {
165+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
166+
__n <= __current_ - __begin_, "__bounded_iter::operator-=: Attempt to rewind an iterator past the start");
167+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
168+
__n >= __current_ - __end_, "__bounded_iter::operator-=: Attempt to advance an iterator past the end");
154169
__current_ -= __n;
155170
return *this;
156171
}
@@ -197,15 +212,10 @@ struct __bounded_iter {
197212
}
198213

199214
private:
200-
// Return whether the given iterator is in the bounds of this __bounded_iter.
201-
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __in_bounds(_Iterator const& __iter) const {
202-
return __iter >= __begin_ && __iter < __end_;
203-
}
204-
205215
template <class>
206216
friend struct pointer_traits;
207217
_Iterator __current_; // current iterator
208-
_Iterator __begin_, __end_; // valid range represented as [begin, end)
218+
_Iterator __begin_, __end_; // valid range represented as [begin, end]
209219
};
210220

211221
template <class _It>

‎libcxx/test/libcxx/containers/views/views.span/debug.iterator-indexing.pass.cpp

Lines changed: 127 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -20,77 +20,156 @@ struct Foo {
2020
int x;
2121
};
2222

23+
template <typename Iter>
24+
void test_iterator(Iter begin, Iter end, bool reverse) {
25+
ptrdiff_t distance = std::distance(begin, end);
26+
27+
// Dereferencing an iterator at the end.
28+
{
29+
TEST_LIBCPP_ASSERT_FAILURE(
30+
*end,
31+
reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
32+
: "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
33+
#if _LIBCPP_STD_VER >= 20
34+
// In C++20 mode, std::reverse_iterator implements operator->, but not operator*, with
35+
// std::prev instead of operator--. std::prev ultimately calls operator+
36+
TEST_LIBCPP_ASSERT_FAILURE(
37+
end->x,
38+
reverse ? "__bounded_iter::operator+=: Attempt to rewind an iterator past the start"
39+
: "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
40+
#else
41+
TEST_LIBCPP_ASSERT_FAILURE(
42+
end->x,
43+
reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
44+
: "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
45+
#endif
46+
}
47+
48+
// Incrementing an iterator past the end.
49+
{
50+
[[maybe_unused]] const char* msg =
51+
reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
52+
: "__bounded_iter::operator++: Attempt to advance an iterator past the end";
53+
auto it = end;
54+
TEST_LIBCPP_ASSERT_FAILURE(it++, msg);
55+
it = end;
56+
TEST_LIBCPP_ASSERT_FAILURE(++it, msg);
57+
}
58+
59+
// Decrementing an iterator past the start.
60+
{
61+
[[maybe_unused]] const char* msg =
62+
reverse ? "__bounded_iter::operator++: Attempt to advance an iterator past the end"
63+
: "__bounded_iter::operator--: Attempt to rewind an iterator past the start";
64+
auto it = begin;
65+
TEST_LIBCPP_ASSERT_FAILURE(it--, msg);
66+
it = begin;
67+
TEST_LIBCPP_ASSERT_FAILURE(--it, msg);
68+
}
69+
70+
// Advancing past the end with operator+= and operator+.
71+
{
72+
[[maybe_unused]] const char* msg =
73+
reverse ? "__bounded_iter::operator-=: Attempt to rewind an iterator past the start"
74+
: "__bounded_iter::operator+=: Attempt to advance an iterator past the end";
75+
auto it = end;
76+
TEST_LIBCPP_ASSERT_FAILURE(it += 1, msg);
77+
TEST_LIBCPP_ASSERT_FAILURE(end + 1, msg);
78+
it = begin;
79+
TEST_LIBCPP_ASSERT_FAILURE(it += (distance + 1), msg);
80+
TEST_LIBCPP_ASSERT_FAILURE(begin + (distance + 1), msg);
81+
}
82+
83+
// Advancing past the end with operator-= and operator-.
84+
{
85+
[[maybe_unused]] const char* msg =
86+
reverse ? "__bounded_iter::operator+=: Attempt to rewind an iterator past the start"
87+
: "__bounded_iter::operator-=: Attempt to advance an iterator past the end";
88+
auto it = end;
89+
TEST_LIBCPP_ASSERT_FAILURE(it -= (-1), msg);
90+
TEST_LIBCPP_ASSERT_FAILURE(end - (-1), msg);
91+
it = begin;
92+
TEST_LIBCPP_ASSERT_FAILURE(it -= (-distance - 1), msg);
93+
TEST_LIBCPP_ASSERT_FAILURE(begin - (-distance - 1), msg);
94+
}
95+
96+
// Rewinding past the start with operator+= and operator+.
97+
{
98+
[[maybe_unused]] const char* msg =
99+
reverse ? "__bounded_iter::operator-=: Attempt to advance an iterator past the end"
100+
: "__bounded_iter::operator+=: Attempt to rewind an iterator past the start";
101+
auto it = begin;
102+
TEST_LIBCPP_ASSERT_FAILURE(it += (-1), msg);
103+
TEST_LIBCPP_ASSERT_FAILURE(begin + (-1), msg);
104+
it = end;
105+
TEST_LIBCPP_ASSERT_FAILURE(it += (-distance - 1), msg);
106+
TEST_LIBCPP_ASSERT_FAILURE(end + (-distance - 1), msg);
107+
}
108+
109+
// Rewinding past the start with operator-= and operator-.
110+
{
111+
[[maybe_unused]] const char* msg =
112+
reverse ? "__bounded_iter::operator+=: Attempt to advance an iterator past the end"
113+
: "__bounded_iter::operator-=: Attempt to rewind an iterator past the start";
114+
auto it = begin;
115+
TEST_LIBCPP_ASSERT_FAILURE(it -= 1, msg);
116+
TEST_LIBCPP_ASSERT_FAILURE(begin - 1, msg);
117+
it = end;
118+
TEST_LIBCPP_ASSERT_FAILURE(it -= (distance + 1), msg);
119+
TEST_LIBCPP_ASSERT_FAILURE(end - (distance + 1), msg);
120+
}
121+
122+
// Out-of-bounds operator[].
123+
{
124+
[[maybe_unused]] const char* end_msg =
125+
reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start"
126+
: "__bounded_iter::operator[]: Attempt to index an iterator at or past the end";
127+
[[maybe_unused]] const char* past_end_msg =
128+
reverse ? "__bounded_iter::operator-=: Attempt to rewind an iterator past the start"
129+
: "__bounded_iter::operator[]: Attempt to index an iterator at or past the end";
130+
[[maybe_unused]] const char* past_start_msg =
131+
reverse ? "__bounded_iter::operator-=: Attempt to advance an iterator past the end"
132+
: "__bounded_iter::operator[]: Attempt to index an iterator past the start";
133+
TEST_LIBCPP_ASSERT_FAILURE(begin[distance], end_msg);
134+
TEST_LIBCPP_ASSERT_FAILURE(begin[distance + 1], past_end_msg);
135+
TEST_LIBCPP_ASSERT_FAILURE(begin[-1], past_start_msg);
136+
TEST_LIBCPP_ASSERT_FAILURE(begin[-99], past_start_msg);
137+
138+
auto it = begin + 1;
139+
TEST_LIBCPP_ASSERT_FAILURE(it[distance - 1], end_msg);
140+
TEST_LIBCPP_ASSERT_FAILURE(it[distance], past_end_msg);
141+
TEST_LIBCPP_ASSERT_FAILURE(it[-2], past_start_msg);
142+
TEST_LIBCPP_ASSERT_FAILURE(it[-99], past_start_msg);
143+
}
144+
}
145+
23146
int main(int, char**) {
24147
// span<T>::iterator
25148
{
26149
Foo array[] = {{0}, {1}, {2}};
27150
std::span<Foo> const span(array, 3);
28-
{
29-
auto it = span.end();
30-
TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
31-
}
32-
{
33-
auto it = span.end();
34-
TEST_LIBCPP_ASSERT_FAILURE(it->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
35-
}
36-
{
37-
auto it = span.begin();
38-
TEST_LIBCPP_ASSERT_FAILURE(it[3], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
39-
}
151+
test_iterator(span.begin(), span.end(), /*reverse=*/false);
40152
}
41153

42154
// span<T, N>::iterator
43155
{
44156
Foo array[] = {{0}, {1}, {2}};
45157
std::span<Foo, 3> const span(array, 3);
46-
{
47-
auto it = span.end();
48-
TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
49-
}
50-
{
51-
auto it = span.end();
52-
TEST_LIBCPP_ASSERT_FAILURE(it->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
53-
}
54-
{
55-
auto it = span.begin();
56-
TEST_LIBCPP_ASSERT_FAILURE(it[3], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
57-
}
158+
test_iterator(span.begin(), span.end(), /*reverse=*/false);
58159
}
59160

60161
// span<T>::reverse_iterator
61162
{
62163
Foo array[] = {{0}, {1}, {2}};
63164
std::span<Foo> const span(array, 3);
64-
{
65-
auto it = span.rend();
66-
TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
67-
}
68-
{
69-
auto it = span.rend();
70-
TEST_LIBCPP_ASSERT_FAILURE(it->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
71-
}
72-
{
73-
auto it = span.rbegin();
74-
TEST_LIBCPP_ASSERT_FAILURE(it[3], "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
75-
}
165+
test_iterator(span.rbegin(), span.rend(), /*reverse=*/true);
76166
}
77167

78168
// span<T, N>::reverse_iterator
79169
{
80170
Foo array[] = {{0}, {1}, {2}};
81171
std::span<Foo, 3> const span(array, 3);
82-
{
83-
auto it = span.rend();
84-
TEST_LIBCPP_ASSERT_FAILURE(*it, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
85-
}
86-
{
87-
auto it = span.rend();
88-
TEST_LIBCPP_ASSERT_FAILURE(it->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
89-
}
90-
{
91-
auto it = span.rbegin();
92-
TEST_LIBCPP_ASSERT_FAILURE(it[3], "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
93-
}
172+
test_iterator(span.rbegin(), span.rend(), /*reverse=*/true);
94173
}
95174

96175
return 0;

‎libcxx/test/libcxx/iterators/bounded_iter/dereference.pass.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -58,15 +58,15 @@ void test_death() {
5858
std::__bounded_iter<Iter> const oob = std::__make_bounded_iter(Iter(e), Iter(b), Iter(e));
5959

6060
// operator*
61-
TEST_LIBCPP_ASSERT_FAILURE(*oob, "__bounded_iter::operator*: Attempt to dereference an out-of-range iterator");
61+
TEST_LIBCPP_ASSERT_FAILURE(*oob, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
6262
// operator->
63-
TEST_LIBCPP_ASSERT_FAILURE(oob->x, "__bounded_iter::operator->: Attempt to dereference an out-of-range iterator");
63+
TEST_LIBCPP_ASSERT_FAILURE(oob->x, "__bounded_iter::operator->: Attempt to dereference an iterator at the end");
6464
// operator[]
65-
TEST_LIBCPP_ASSERT_FAILURE(iter[-1], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
66-
TEST_LIBCPP_ASSERT_FAILURE(iter[5], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
67-
TEST_LIBCPP_ASSERT_FAILURE(oob[0], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
68-
TEST_LIBCPP_ASSERT_FAILURE(oob[1], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
69-
TEST_LIBCPP_ASSERT_FAILURE(oob[-6], "__bounded_iter::operator[]: Attempt to index an iterator out-of-range");
65+
TEST_LIBCPP_ASSERT_FAILURE(iter[-1], "__bounded_iter::operator[]: Attempt to index an iterator past the start");
66+
TEST_LIBCPP_ASSERT_FAILURE(iter[5], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
67+
TEST_LIBCPP_ASSERT_FAILURE(oob[0], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
68+
TEST_LIBCPP_ASSERT_FAILURE(oob[1], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
69+
TEST_LIBCPP_ASSERT_FAILURE(oob[-6], "__bounded_iter::operator[]: Attempt to index an iterator past the start");
7070
}
7171

7272
int main(int, char**) {

0 commit comments

Comments
 (0)