Skip to content

Commit da5e4a3

Browse files
davidbenyuxuanchen1997
authored andcommitted
[libc++][hardening] Use bounded iterators in std::vector and std::string (#78929)
~~NB: This PR depends on #78876. Ignore the first commit when reviewing, and don't merge it until #78876 is resolved. When/if #78876 lands, I'll clean this up.~~ This partially restores parity with the old, since removed debug build. We now can re-enable a bunch of the disabled tests. Some things of note: - `bounded_iter`'s converting constructor has never worked. It needs a friend declaration to access the other `bound_iter` instantiation's private fields. - The old debug iterators also checked that callers did not try to compare iterators from different objects. `bounded_iter` does not currently do this, so I've left those disabled. However, I think we probably should add those. See #78771 (comment) - The `std::vector` iterators are bounded up to capacity, not size. This makes for a weaker safety check. This is because the STL promises not to invalidate iterators when appending up to the capacity. Since we cannot retroactively update all the iterators on `push_back()`, I've instead sized it to the capacity. This is not as good, but at least will stop the iterator from going off the end of the buffer. There was also no test for this, so I've added one in the `std` directory. - `std::string` has two ambiguities to deal with. First, I opted not to size it against the capacity. https://eel.is/c++draft/string.require#4 says iterators are invalidated on an non-const operation. Second, whether the iterator can reach the NUL terminator. The previous debug tests and the special-case in https://eel.is/c++draft/string.access#2 suggest no. If either of these causes widespread problems, I figure we can revisit. - `resize_and_overwrite.pass.cpp` assumed `std::string`'s iterator supported `s.begin().base()`, but I see no promise of this in the standard. GCC also doesn't support this. I fixed the test to use `std::to_address`. - `alignof.compile.pass.cpp`'s pointer isn't enough of a real pointer. (It needs to satisfy `NullablePointer`, `LegacyRandomAccessIterator`, and `LegacyContiguousIterator`.) `__bounded_iter` seems to instantiate enough to notice. I've added a few more bits to satisfy it. Fixes #78805
1 parent f5f2c58 commit da5e4a3

24 files changed

+294
-47
lines changed
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,4 @@
11
set(LIBCXX_HARDENING_MODE "fast" CACHE STRING "")
22
set(LIBCXX_ABI_DEFINES "_LIBCPP_ABI_BOUNDED_ITERATORS" CACHE STRING "")
3+
set(LIBCXX_ABI_DEFINES "_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING" CACHE STRING "")
4+
set(LIBCXX_ABI_DEFINES "_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR" CACHE STRING "")

libcxx/docs/Hardening.rst

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,22 @@ Vendors can use the following ABI options to enable additional hardening checks:
325325
- ``span``;
326326
- ``string_view``.
327327

328+
- ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING`` -- changes the iterator type of
329+
``basic_string`` to a bounded iterator that keeps track of whether it's within
330+
the bounds of the original container and asserts it on every dereference and
331+
when performing iterator arithmetics.
332+
333+
ABI impact: changes the iterator type of ``basic_string`` and its
334+
specializations, such as ``string`` and ``wstring``.
335+
336+
- ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR`` -- changes the iterator type of
337+
``vector`` to a bounded iterator that keeps track of whether it's within the
338+
bounds of the original container and asserts it on every dereference and when
339+
performing iterator arithmetics. Note: this doesn't yet affect
340+
``vector<bool>``.
341+
342+
ABI impact: changes the iterator type of ``vector`` (except ``vector<bool>``).
343+
328344
ABI tags
329345
--------
330346

@@ -367,10 +383,10 @@ Hardened containers status
367383
- ❌
368384
* - ``vector``
369385
- ✅
370-
-
386+
- ✅ (see note)
371387
* - ``string``
372388
- ✅
373-
-
389+
- ✅ (see note)
374390
* - ``list``
375391
- ✅
376392
- ❌
@@ -429,6 +445,12 @@ Hardened containers status
429445
- ❌
430446
- N/A
431447

448+
Note: for ``vector`` and ``string``, the iterator does not check for
449+
invalidation (accesses made via an invalidated iterator still lead to undefined
450+
behavior)
451+
452+
Note: ``vector<bool>`` iterator is not currently hardened.
453+
432454
Testing
433455
=======
434456

libcxx/docs/ReleaseNotes/19.rst

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,20 @@ Improvements and New Features
8787
- ``std::ignore``\s ``const __ignore_t& operator=(_Tp&&) const`` was changed to
8888
``const __ignore_type& operator=(const _Tp&) const noexcept`` for all language versions.
8989

90+
- Vendors can now configure the ABI so that ``string`` and ``vector`` will use bounded iterators when hardening is
91+
enabled. Note that checks for iterator invalidation are currently not supported -- any accesses made through an
92+
invalidated bounded iterator will still result in undefined behavior (bounded iterators follow the normal invalidation
93+
rules of the associated container). ``string`` bounded iterators use the logical size of the container (``index
94+
< str.size()``) whereas ``vector`` bounded iterators use the "physical" size of the container (``index
95+
< vec.capacity()``) which is a less strict check; refer to the implementation for further details.
96+
97+
Bounded iterators can be enabled via the ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING`` ABI macro for ``string`` and via
98+
the ``_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR`` ABI macro for ``vector``; note that checks will only be performed if
99+
the hardening mode is set to ``fast`` or above (i.e., no checking is performed in the unchecked mode, even if bounded
100+
iterators are enabled in the ABI configuration).
101+
102+
Note: bounded iterators currently are not supported for ``vector<bool>``.
103+
90104
Deprecations and Removals
91105
-------------------------
92106

libcxx/include/__configuration/abi.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,19 @@
141141
// - `string_view`.
142142
// #define _LIBCPP_ABI_BOUNDED_ITERATORS
143143

144+
// Changes the iterator type of `basic_string` to a bounded iterator that keeps track of whether it's within the bounds
145+
// of the original container and asserts it on every dereference and when performing iterator arithmetics.
146+
//
147+
// ABI impact: changes the iterator type of `basic_string` and its specializations, such as `string` and `wstring`.
148+
// #define _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
149+
150+
// Changes the iterator type of `vector` to a bounded iterator that keeps track of whether it's within the bounds of the
151+
// original container and asserts it on every dereference and when performing iterator arithmetics. Note: this doesn't
152+
// yet affect `vector<bool>`.
153+
//
154+
// ABI impact: changes the iterator type of `vector` (except `vector<bool>`).
155+
// #define _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
156+
144157
#if defined(_LIBCPP_COMPILER_CLANG_BASED)
145158
# if defined(__APPLE__)
146159
# if defined(__i386__) || defined(__x86_64__)

libcxx/include/__iterator/bounded_iter.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,8 @@ struct __bounded_iter {
225225
private:
226226
template <class>
227227
friend struct pointer_traits;
228+
template <class, class>
229+
friend struct __bounded_iter;
228230
_Iterator __current_; // current iterator
229231
_Iterator __begin_, __end_; // valid range represented as [begin, end]
230232
};

libcxx/include/string

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,7 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
598598
#include <__functional/unary_function.h>
599599
#include <__fwd/string.h>
600600
#include <__ios/fpos.h>
601+
#include <__iterator/bounded_iter.h>
601602
#include <__iterator/distance.h>
602603
#include <__iterator/iterator_traits.h>
603604
#include <__iterator/reverse_iterator.h>
@@ -822,9 +823,16 @@ public:
822823
"Allocator::value_type must be same type as value_type");
823824
static_assert(__check_valid_allocator<allocator_type>::value, "");
824825

825-
// TODO: Implement iterator bounds checking without requiring the global database.
826+
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
827+
// Users might provide custom allocators, and prior to C++20 we have no existing way to detect whether the allocator's
828+
// pointer type is contiguous (though it has to be by the Standard). Using the wrapper type ensures the iterator is
829+
// considered contiguous.
830+
typedef __bounded_iter<__wrap_iter<pointer>> iterator;
831+
typedef __bounded_iter<__wrap_iter<const_pointer>> const_iterator;
832+
#else
826833
typedef __wrap_iter<pointer> iterator;
827834
typedef __wrap_iter<const_pointer> const_iterator;
835+
#endif
828836
typedef std::reverse_iterator<iterator> reverse_iterator;
829837
typedef std::reverse_iterator<const_iterator> const_reverse_iterator;
830838

@@ -940,10 +948,33 @@ private:
940948
__init_with_sentinel(std::move(__first), std::move(__last));
941949
}
942950

943-
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 iterator __make_iterator(pointer __p) { return iterator(__p); }
951+
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 iterator __make_iterator(pointer __p) {
952+
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
953+
// Bound the iterator according to the size (and not the capacity, unlike vector).
954+
//
955+
// By the Standard, string iterators are generally not guaranteed to stay valid when the container is modified,
956+
// regardless of whether reallocation occurs. This allows us to check for out-of-bounds accesses using logical size,
957+
// a stricter check, since correct code can never rely on being able to access newly-added elements via an existing
958+
// iterator.
959+
return std::__make_bounded_iter(
960+
std::__wrap_iter<pointer>(__p),
961+
std::__wrap_iter<pointer>(__get_pointer()),
962+
std::__wrap_iter<pointer>(__get_pointer() + size()));
963+
#else
964+
return iterator(__p);
965+
#endif // _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
966+
}
944967

945968
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_iterator __make_const_iterator(const_pointer __p) const {
969+
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
970+
// Bound the iterator according to the size (and not the capacity, unlike vector).
971+
return std::__make_bounded_iter(
972+
std::__wrap_iter<const_pointer>(__p),
973+
std::__wrap_iter<const_pointer>(__get_pointer()),
974+
std::__wrap_iter<const_pointer>(__get_pointer() + size()));
975+
#else
946976
return const_iterator(__p);
977+
#endif // _LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING
947978
}
948979

949980
public:

libcxx/include/vector

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,7 @@ template<class T, class charT> requires is-vector-bool-reference<T> // Since C++
327327
#include <__functional/unary_function.h>
328328
#include <__fwd/vector.h>
329329
#include <__iterator/advance.h>
330+
#include <__iterator/bounded_iter.h>
330331
#include <__iterator/distance.h>
331332
#include <__iterator/iterator_traits.h>
332333
#include <__iterator/reverse_iterator.h>
@@ -400,9 +401,16 @@ public:
400401
typedef typename __alloc_traits::difference_type difference_type;
401402
typedef typename __alloc_traits::pointer pointer;
402403
typedef typename __alloc_traits::const_pointer const_pointer;
403-
// TODO: Implement iterator bounds checking without requiring the global database.
404+
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
405+
// Users might provide custom allocators, and prior to C++20 we have no existing way to detect whether the allocator's
406+
// pointer type is contiguous (though it has to be by the Standard). Using the wrapper type ensures the iterator is
407+
// considered contiguous.
408+
typedef __bounded_iter<__wrap_iter<pointer>> iterator;
409+
typedef __bounded_iter<__wrap_iter<const_pointer>> const_iterator;
410+
#else
404411
typedef __wrap_iter<pointer> iterator;
405412
typedef __wrap_iter<const_pointer> const_iterator;
413+
#endif
406414
typedef std::reverse_iterator<iterator> reverse_iterator;
407415
typedef std::reverse_iterator<const_iterator> const_reverse_iterator;
408416

@@ -827,12 +835,39 @@ private:
827835

828836
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __append(size_type __n);
829837
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __append(size_type __n, const_reference __x);
838+
830839
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator __make_iter(pointer __p) _NOEXCEPT {
840+
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
841+
// Bound the iterator according to the capacity, rather than the size.
842+
//
843+
// Vector guarantees that iterators stay valid as long as no reallocation occurs even if new elements are inserted
844+
// into the container; for these cases, we need to make sure that the newly-inserted elements can be accessed
845+
// through the bounded iterator without failing checks. The downside is that the bounded iterator won't catch
846+
// access that is logically out-of-bounds, i.e., goes beyond the size, but is still within the capacity. With the
847+
// current implementation, there is no connection between a bounded iterator and its associated container, so we
848+
// don't have a way to update existing valid iterators when the container is resized and thus have to go with
849+
// a laxer approach.
850+
return std::__make_bounded_iter(
851+
std::__wrap_iter<pointer>(__p),
852+
std::__wrap_iter<pointer>(this->__begin_),
853+
std::__wrap_iter<pointer>(this->__end_cap()));
854+
#else
831855
return iterator(__p);
856+
#endif // _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
832857
}
858+
833859
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator __make_iter(const_pointer __p) const _NOEXCEPT {
860+
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
861+
// Bound the iterator according to the capacity, rather than the size.
862+
return std::__make_bounded_iter(
863+
std::__wrap_iter<const_pointer>(__p),
864+
std::__wrap_iter<const_pointer>(this->__begin_),
865+
std::__wrap_iter<const_pointer>(this->__end_cap()));
866+
#else
834867
return const_iterator(__p);
868+
#endif // _LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR
835869
}
870+
836871
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
837872
__swap_out_circular_buffer(__split_buffer<value_type, allocator_type&>& __v);
838873
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI pointer

libcxx/test/libcxx/containers/sequences/vector/abi.compile.pass.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,14 @@
1414

1515
template <class T>
1616
class small_pointer {
17+
public:
18+
using value_type = T;
19+
using difference_type = std::int16_t;
20+
using pointer = small_pointer;
21+
using reference = T&;
22+
using iterator_category = std::random_access_iterator_tag;
23+
24+
private:
1725
std::uint16_t offset;
1826
};
1927

libcxx/test/libcxx/containers/sequences/vector/debug.iterator.add.pass.cpp renamed to libcxx/test/libcxx/containers/sequences/vector/assert.iterator.add.pass.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,36 +10,39 @@
1010

1111
// Add to iterator out of bounds.
1212

13-
// REQUIRES: has-unix-headers
14-
// UNSUPPORTED: !libcpp-has-legacy-debug-mode, c++03
13+
// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators-in-vector
14+
// UNSUPPORTED: libcpp-hardening-mode=none, c++03
1515

1616
#include <vector>
1717
#include <cassert>
1818

1919
#include "check_assertion.h"
20+
#include "fill_to_capacity.h"
2021
#include "min_allocator.h"
2122

2223
int main(int, char**) {
2324
{
2425
typedef int T;
2526
typedef std::vector<T> C;
2627
C c(1);
28+
fill_to_capacity(c);
2729
C::iterator i = c.begin();
28-
i += 1;
30+
i += c.size();
2931
assert(i == c.end());
3032
i = c.begin();
31-
TEST_LIBCPP_ASSERT_FAILURE(i + 2, "Attempted to add/subtract an iterator outside its valid range");
33+
TEST_LIBCPP_ASSERT_FAILURE(i + 2, "__bounded_iter::operator+=: Attempt to advance an iterator past the end");
3234
}
3335

3436
{
3537
typedef int T;
3638
typedef std::vector<T, min_allocator<T> > C;
3739
C c(1);
40+
fill_to_capacity(c);
3841
C::iterator i = c.begin();
39-
i += 1;
42+
i += c.size();
4043
assert(i == c.end());
4144
i = c.begin();
42-
TEST_LIBCPP_ASSERT_FAILURE(i + 2, "Attempted to add/subtract an iterator outside its valid range");
45+
TEST_LIBCPP_ASSERT_FAILURE(i + 2, "__bounded_iter::operator+=: Attempt to advance an iterator past the end");
4346
}
4447

4548
return 0;

libcxx/test/libcxx/containers/sequences/vector/debug.iterator.decrement.pass.cpp renamed to libcxx/test/libcxx/containers/sequences/vector/assert.iterator.decrement.pass.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@
1010

1111
// Decrement iterator prior to begin.
1212

13-
// REQUIRES: has-unix-headers
14-
// UNSUPPORTED: !libcpp-has-legacy-debug-mode, c++03
13+
// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators-in-vector
14+
// UNSUPPORTED: libcpp-hardening-mode=none, c++03
1515

1616
#include <vector>
1717
#include <cassert>
@@ -27,7 +27,7 @@ int main(int, char**) {
2727
C::iterator i = c.end();
2828
--i;
2929
assert(i == c.begin());
30-
TEST_LIBCPP_ASSERT_FAILURE(--i, "Attempted to decrement a non-decrementable iterator");
30+
TEST_LIBCPP_ASSERT_FAILURE(--i, "__bounded_iter::operator--: Attempt to rewind an iterator past the start");
3131
}
3232

3333
{
@@ -37,7 +37,7 @@ int main(int, char**) {
3737
C::iterator i = c.end();
3838
--i;
3939
assert(i == c.begin());
40-
TEST_LIBCPP_ASSERT_FAILURE(--i, "Attempted to decrement a non-decrementable iterator");
40+
TEST_LIBCPP_ASSERT_FAILURE(--i, "__bounded_iter::operator--: Attempt to rewind an iterator past the start");
4141
}
4242

4343
return 0;

libcxx/test/libcxx/containers/sequences/vector/debug.iterator.dereference.pass.cpp renamed to libcxx/test/libcxx/containers/sequences/vector/assert.iterator.dereference.pass.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,29 +10,32 @@
1010

1111
// Dereference non-dereferenceable iterator.
1212

13-
// REQUIRES: has-unix-headers
14-
// UNSUPPORTED: !libcpp-has-legacy-debug-mode, c++03
13+
// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators-in-vector
14+
// UNSUPPORTED: libcpp-hardening-mode=none, c++03
1515

1616
#include <vector>
1717

1818
#include "check_assertion.h"
19+
#include "fill_to_capacity.h"
1920
#include "min_allocator.h"
2021

2122
int main(int, char**) {
2223
{
2324
typedef int T;
2425
typedef std::vector<T> C;
2526
C c(1);
27+
fill_to_capacity(c);
2628
C::iterator i = c.end();
27-
TEST_LIBCPP_ASSERT_FAILURE(*i, "Attempted to dereference a non-dereferenceable iterator");
29+
TEST_LIBCPP_ASSERT_FAILURE(*i, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
2830
}
2931

3032
{
3133
typedef int T;
3234
typedef std::vector<T, min_allocator<T> > C;
3335
C c(1);
36+
fill_to_capacity(c);
3437
C::iterator i = c.end();
35-
TEST_LIBCPP_ASSERT_FAILURE(*i, "Attempted to dereference a non-dereferenceable iterator");
38+
TEST_LIBCPP_ASSERT_FAILURE(*i, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
3639
}
3740

3841
return 0;

0 commit comments

Comments
 (0)