Skip to content

Commit

Permalink
[libc++][hardening] Use bounded iterators in std::vector and std::string
Browse files Browse the repository at this point in the history
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 capacaity. 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 invalided 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
  • Loading branch information
davidben committed Jan 22, 2024
1 parent 49d9168 commit 9290806
Show file tree
Hide file tree
Showing 17 changed files with 214 additions and 44 deletions.
2 changes: 2 additions & 0 deletions libcxx/include/__iterator/bounded_iter.h
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,8 @@ struct __bounded_iter {
private:
template <class>
friend struct pointer_traits;
template <class, class>
friend struct __bounded_iter;
_Iterator __current_; // current iterator
_Iterator __begin_, __end_; // valid range represented as [begin, end]
};
Expand Down
26 changes: 25 additions & 1 deletion libcxx/include/string
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,7 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
#include <__functional/unary_function.h>
#include <__fwd/string.h>
#include <__ios/fpos.h>
#include <__iterator/bounded_iter.h>
#include <__iterator/distance.h>
#include <__iterator/iterator_traits.h>
#include <__iterator/reverse_iterator.h>
Expand Down Expand Up @@ -736,9 +737,16 @@ public:
"[allocator.requirements] states that rebinding an allocator to the same type should result in the "
"original allocator");

// TODO: Implement iterator bounds checking without requiring the global database.
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
// The pointer must be passed through __wrap_iter because
// __alloc_traits::pointer may not be detected as a continguous iterator on
// its own.
typedef __bounded_iter<__wrap_iter<pointer>> iterator;
typedef __bounded_iter<__wrap_iter<const_pointer>> const_iterator;
#else
typedef __wrap_iter<pointer> iterator;
typedef __wrap_iter<const_pointer> const_iterator;
#endif
typedef std::reverse_iterator<iterator> reverse_iterator;
typedef std::reverse_iterator<const_iterator> const_reverse_iterator;

Expand Down Expand Up @@ -868,11 +876,27 @@ private:
__init_with_sentinel(std::move(__first), std::move(__last));
}

#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 iterator __make_iterator(pointer __p) {
return __make_bounded_iter(
__wrap_iter<pointer>(__p),
__wrap_iter<pointer>(__get_pointer()),
__wrap_iter<pointer>(__get_pointer() + size()));
}

_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_iterator __make_const_iterator(const_pointer __p) const {
return __make_bounded_iter(
__wrap_iter<const_pointer>(__p),
__wrap_iter<const_pointer>(__get_pointer()),
__wrap_iter<const_pointer>(__get_pointer() + size()));
}
#else
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 iterator __make_iterator(pointer __p) { return iterator(__p); }

_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_iterator __make_const_iterator(const_pointer __p) const {
return const_iterator(__p);
}
#endif

public:
_LIBCPP_TEMPLATE_DATA_VIS static const size_type npos = -1;
Expand Down
30 changes: 29 additions & 1 deletion libcxx/include/vector
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ template<class T, class charT> requires is-vector-bool-reference<T> // Since C++
#include <__functional/hash.h>
#include <__functional/unary_function.h>
#include <__iterator/advance.h>
#include <__iterator/bounded_iter.h>
#include <__iterator/distance.h>
#include <__iterator/iterator_traits.h>
#include <__iterator/reverse_iterator.h>
Expand Down Expand Up @@ -399,9 +400,16 @@ public:
typedef typename __alloc_traits::difference_type difference_type;
typedef typename __alloc_traits::pointer pointer;
typedef typename __alloc_traits::const_pointer const_pointer;
// TODO: Implement iterator bounds checking without requiring the global database.
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
// The pointer must be passed through __wrap_iter because
// __alloc_traits::pointer may not be detected as a continguous iterator on
// its own.
typedef __bounded_iter<__wrap_iter<pointer>> iterator;
typedef __bounded_iter<__wrap_iter<const_pointer>> const_iterator;
#else
typedef __wrap_iter<pointer> iterator;
typedef __wrap_iter<const_pointer> const_iterator;
#endif
typedef std::reverse_iterator<iterator> reverse_iterator;
typedef std::reverse_iterator<const_iterator> const_reverse_iterator;

Expand Down Expand Up @@ -796,10 +804,30 @@ private:
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __append(size_type __n);
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __append(size_type __n, const_reference __x);
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI iterator __make_iter(pointer __p) _NOEXCEPT {
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
// Bound the iterator according to the capacity, rather than the size.
// Resizing a vector up to the capacity will not invalidate iterators, so,
// Without a way to update all live iterators on resize, we must
// conservatively bound the iterator by the capacity rather than the size.
return __make_bounded_iter(
__wrap_iter<pointer>(__p), __wrap_iter<pointer>(this->__begin_), __wrap_iter<pointer>(this->__end_cap()));
#else
return iterator(__p);
#endif
}
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_iterator __make_iter(const_pointer __p) const _NOEXCEPT {
#ifdef _LIBCPP_ABI_BOUNDED_ITERATORS
// Bound the iterator according to the capacity, rather than the size.
// Resizing a vector up to the capacity will not invalidate iterators, so,
// Without a way to update all live iterators on resize, we must
// conservatively bound the iterator by the capacity rather than the size.
return __make_bounded_iter(
__wrap_iter<const_pointer>(__p),
__wrap_iter<const_pointer>(this->__begin_),
__wrap_iter<const_pointer>(this->__end_cap()));
#else
return const_iterator(__p);
#endif
}
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
__swap_out_circular_buffer(__split_buffer<value_type, allocator_type&>& __v);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,36 +10,48 @@

// Add to iterator out of bounds.

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

#include <vector>
#include <cassert>

#include "check_assertion.h"
#include "min_allocator.h"

template <typename T, typename A>
void fill_to_capacity(std::vector<T, A>& vec) {
// Fill vec up to its capacity. Our bounded iterators currently unable to
// catch accesses between size and capacity due to iterator stability
// guarantees. This function clears those effects.
while (vec.size() < vec.capacity()) {
vec.push_back(T());
}
}

int main(int, char**) {
{
typedef int T;
typedef std::vector<T> C;
C c(1);
fill_to_capacity(c);
C::iterator i = c.begin();
i += 1;
i += c.size();
assert(i == c.end());
i = c.begin();
TEST_LIBCPP_ASSERT_FAILURE(i + 2, "Attempted to add/subtract an iterator outside its valid range");
TEST_LIBCPP_ASSERT_FAILURE(i + 2, "__bounded_iter::operator+=: Attempt to advance an iterator past the end");
}

{
typedef int T;
typedef std::vector<T, min_allocator<T> > C;
C c(1);
fill_to_capacity(c);
C::iterator i = c.begin();
i += 1;
i += c.size();
assert(i == c.end());
i = c.begin();
TEST_LIBCPP_ASSERT_FAILURE(i + 2, "Attempted to add/subtract an iterator outside its valid range");
TEST_LIBCPP_ASSERT_FAILURE(i + 2, "__bounded_iter::operator+=: Attempt to advance an iterator past the end");
}

return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

// Decrement iterator prior to begin.

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

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

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

return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,29 +10,41 @@

// Dereference non-dereferenceable iterator.

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

#include <vector>

#include "check_assertion.h"
#include "min_allocator.h"

template <typename T, typename A>
void fill_to_capacity(std::vector<T, A>& vec) {
// Fill vec up to its capacity. Our bounded iterators currently unable to
// catch accesses between size and capacity due to iterator stability
// guarantees. This function clears those effects.
while (vec.size() < vec.capacity()) {
vec.push_back(T());
}
}

int main(int, char**) {
{
typedef int T;
typedef std::vector<T> C;
C c(1);
fill_to_capacity(c);
C::iterator i = c.end();
TEST_LIBCPP_ASSERT_FAILURE(*i, "Attempted to dereference a non-dereferenceable iterator");
TEST_LIBCPP_ASSERT_FAILURE(*i, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
}

{
typedef int T;
typedef std::vector<T, min_allocator<T> > C;
C c(1);
fill_to_capacity(c);
C::iterator i = c.end();
TEST_LIBCPP_ASSERT_FAILURE(*i, "Attempted to dereference a non-dereferenceable iterator");
TEST_LIBCPP_ASSERT_FAILURE(*i, "__bounded_iter::operator*: Attempt to dereference an iterator at the end");
}

return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,34 +10,46 @@

// Increment iterator past end.

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

#include <vector>
#include <cassert>

#include "check_assertion.h"
#include "min_allocator.h"

template <typename T, typename A>
void fill_to_capacity(std::vector<T, A>& vec) {
// Fill vec up to its capacity. Our bounded iterators currently unable to
// catch accesses between size and capacity due to iterator stability
// guarantees. This function clears those effects.
while (vec.size() < vec.capacity()) {
vec.push_back(T());
}
}

int main(int, char**) {
{
typedef int T;
typedef std::vector<T> C;
C c(1);
fill_to_capacity(c);
C::iterator i = c.begin();
++i;
i += c.size();
assert(i == c.end());
TEST_LIBCPP_ASSERT_FAILURE(++i, "Attempted to increment a non-incrementable iterator");
TEST_LIBCPP_ASSERT_FAILURE(++i, "__bounded_iter::operator++: Attempt to advance an iterator past the end");
}

{
typedef int T;
typedef std::vector<T, min_allocator<T> > C;
C c(1);
fill_to_capacity(c);
C::iterator i = c.begin();
++i;
i += c.size();
assert(i == c.end());
TEST_LIBCPP_ASSERT_FAILURE(++i, "Attempted to increment a non-incrementable iterator");
TEST_LIBCPP_ASSERT_FAILURE(++i, "__bounded_iter::operator++: Attempt to advance an iterator past the end");
}

return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,32 +10,50 @@

// Index iterator out of bounds.

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

#include <vector>
#include <cassert>

#include "check_assertion.h"
#include "min_allocator.h"

template <typename T, typename A>
void fill_to_capacity(std::vector<T, A>& vec) {
// Fill vec up to its capacity. Our bounded iterators currently unable to
// catch accesses between size and capacity due to iterator stability
// guarantees. This function clears those effects.
while (vec.size() < vec.capacity()) {
vec.push_back(T());
}
}

int main(int, char**) {
{
typedef int T;
typedef std::vector<T> C;
C c(1);
fill_to_capacity(c);
C::iterator i = c.begin();
assert(i[0] == 0);
TEST_LIBCPP_ASSERT_FAILURE(i[1], "Attempted to subscript an iterator outside its valid range");
TEST_LIBCPP_ASSERT_FAILURE(
i[c.size()], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
TEST_LIBCPP_ASSERT_FAILURE(
i[c.size() + 1], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
}

{
typedef int T;
typedef std::vector<T, min_allocator<T> > C;
C c(1);
fill_to_capacity(c);
C::iterator i = c.begin();
assert(i[0] == 0);
TEST_LIBCPP_ASSERT_FAILURE(i[1], "Attempted to subscript an iterator outside its valid range");
TEST_LIBCPP_ASSERT_FAILURE(
i[c.size()], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
TEST_LIBCPP_ASSERT_FAILURE(
i[c.size() + 1], "__bounded_iter::operator[]: Attempt to index an iterator at or past the end");
}

return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

// UNSUPPORTED: c++03

#include <iterator>
#include <string>

#include "test_macros.h"
Expand All @@ -18,6 +19,14 @@

template <class T>
class small_pointer {
public:
using value_type = T;
using difference_type = std::int16_t;
using pointer = small_pointer;
using reference = T&;
using iterator_category = std::random_access_iterator_tag;

private:
std::uint16_t offset;
};

Expand Down
Loading

0 comments on commit 9290806

Please sign in to comment.