-
Notifications
You must be signed in to change notification settings - Fork 11.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[libc++][hardening] Check bounds on arithmetic in __bounded_iter (#78876
) 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 common iterator patterns, which use `!=` instead of `<`, so it's easier to delete checks with local reasoning. 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. (CC @danakj)
- Loading branch information
Showing
6 changed files
with
385 additions
and
221 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
174 changes: 174 additions & 0 deletions
174
libcxx/test/libcxx/containers/views/views.span/assert.iterator-indexing.pass.cpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
// UNSUPPORTED: c++03, c++11, c++14, c++17 | ||
|
||
// Make sure that std::span's iterators check for OOB accesses when the debug mode is enabled. | ||
|
||
// REQUIRES: has-unix-headers, libcpp-has-abi-bounded-iterators | ||
// UNSUPPORTED: libcpp-hardening-mode=none | ||
|
||
#include <span> | ||
|
||
#include "check_assertion.h" | ||
|
||
struct Foo { | ||
int x; | ||
}; | ||
|
||
template <typename Iter> | ||
void test_iterator(Iter begin, Iter end, bool reverse) { | ||
std::ptrdiff_t distance = std::distance(begin, end); | ||
|
||
// Dereferencing an iterator at the end. | ||
{ | ||
TEST_LIBCPP_ASSERT_FAILURE( | ||
*end, | ||
reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start" | ||
: "__bounded_iter::operator*: Attempt to dereference an iterator at the end"); | ||
#if _LIBCPP_STD_VER >= 20 | ||
// In C++20 mode, std::reverse_iterator implements operator->, but not operator*, with | ||
// std::prev instead of operator--. std::prev ultimately calls operator+ | ||
TEST_LIBCPP_ASSERT_FAILURE( | ||
end->x, | ||
reverse ? "__bounded_iter::operator+=: Attempt to rewind an iterator past the start" | ||
: "__bounded_iter::operator->: Attempt to dereference an iterator at the end"); | ||
#else | ||
TEST_LIBCPP_ASSERT_FAILURE( | ||
end->x, | ||
reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start" | ||
: "__bounded_iter::operator->: Attempt to dereference an iterator at the end"); | ||
#endif | ||
} | ||
|
||
// Incrementing an iterator past the end. | ||
{ | ||
[[maybe_unused]] const char* msg = | ||
reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start" | ||
: "__bounded_iter::operator++: Attempt to advance an iterator past the end"; | ||
auto it = end; | ||
TEST_LIBCPP_ASSERT_FAILURE(it++, msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(++it, msg); | ||
} | ||
|
||
// Decrementing an iterator past the start. | ||
{ | ||
[[maybe_unused]] const char* msg = | ||
reverse ? "__bounded_iter::operator++: Attempt to advance an iterator past the end" | ||
: "__bounded_iter::operator--: Attempt to rewind an iterator past the start"; | ||
auto it = begin; | ||
TEST_LIBCPP_ASSERT_FAILURE(it--, msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(--it, msg); | ||
} | ||
|
||
// Advancing past the end with operator+= and operator+. | ||
{ | ||
[[maybe_unused]] const char* msg = | ||
reverse ? "__bounded_iter::operator-=: Attempt to rewind an iterator past the start" | ||
: "__bounded_iter::operator+=: Attempt to advance an iterator past the end"; | ||
auto it = end; | ||
TEST_LIBCPP_ASSERT_FAILURE(it += 1, msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(end + 1, msg); | ||
it = begin; | ||
TEST_LIBCPP_ASSERT_FAILURE(it += (distance + 1), msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(begin + (distance + 1), msg); | ||
} | ||
|
||
// Advancing past the end with operator-= and operator-. | ||
{ | ||
[[maybe_unused]] const char* msg = | ||
reverse ? "__bounded_iter::operator+=: Attempt to rewind an iterator past the start" | ||
: "__bounded_iter::operator-=: Attempt to advance an iterator past the end"; | ||
auto it = end; | ||
TEST_LIBCPP_ASSERT_FAILURE(it -= (-1), msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(end - (-1), msg); | ||
it = begin; | ||
TEST_LIBCPP_ASSERT_FAILURE(it -= (-distance - 1), msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(begin - (-distance - 1), msg); | ||
} | ||
|
||
// Rewinding past the start with operator+= and operator+. | ||
{ | ||
[[maybe_unused]] const char* msg = | ||
reverse ? "__bounded_iter::operator-=: Attempt to advance an iterator past the end" | ||
: "__bounded_iter::operator+=: Attempt to rewind an iterator past the start"; | ||
auto it = begin; | ||
TEST_LIBCPP_ASSERT_FAILURE(it += (-1), msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(begin + (-1), msg); | ||
it = end; | ||
TEST_LIBCPP_ASSERT_FAILURE(it += (-distance - 1), msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(end + (-distance - 1), msg); | ||
} | ||
|
||
// Rewinding past the start with operator-= and operator-. | ||
{ | ||
[[maybe_unused]] const char* msg = | ||
reverse ? "__bounded_iter::operator+=: Attempt to advance an iterator past the end" | ||
: "__bounded_iter::operator-=: Attempt to rewind an iterator past the start"; | ||
auto it = begin; | ||
TEST_LIBCPP_ASSERT_FAILURE(it -= 1, msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(begin - 1, msg); | ||
it = end; | ||
TEST_LIBCPP_ASSERT_FAILURE(it -= (distance + 1), msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(end - (distance + 1), msg); | ||
} | ||
|
||
// Out-of-bounds operator[]. | ||
{ | ||
[[maybe_unused]] const char* end_msg = | ||
reverse ? "__bounded_iter::operator--: Attempt to rewind an iterator past the start" | ||
: "__bounded_iter::operator[]: Attempt to index an iterator at or past the end"; | ||
[[maybe_unused]] const char* past_end_msg = | ||
reverse ? "__bounded_iter::operator-=: Attempt to rewind an iterator past the start" | ||
: "__bounded_iter::operator[]: Attempt to index an iterator at or past the end"; | ||
[[maybe_unused]] const char* past_start_msg = | ||
reverse ? "__bounded_iter::operator-=: Attempt to advance an iterator past the end" | ||
: "__bounded_iter::operator[]: Attempt to index an iterator past the start"; | ||
TEST_LIBCPP_ASSERT_FAILURE(begin[distance], end_msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(begin[distance + 1], past_end_msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(begin[-1], past_start_msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(begin[-99], past_start_msg); | ||
|
||
auto it = begin + 1; | ||
TEST_LIBCPP_ASSERT_FAILURE(it[distance - 1], end_msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(it[distance], past_end_msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(it[-2], past_start_msg); | ||
TEST_LIBCPP_ASSERT_FAILURE(it[-99], past_start_msg); | ||
} | ||
} | ||
|
||
int main(int, char**) { | ||
// span<T>::iterator | ||
{ | ||
Foo array[] = {{0}, {1}, {2}}; | ||
std::span<Foo> const span(array, 3); | ||
test_iterator(span.begin(), span.end(), /*reverse=*/false); | ||
} | ||
|
||
// span<T, N>::iterator | ||
{ | ||
Foo array[] = {{0}, {1}, {2}}; | ||
std::span<Foo, 3> const span(array, 3); | ||
test_iterator(span.begin(), span.end(), /*reverse=*/false); | ||
} | ||
|
||
// span<T>::reverse_iterator | ||
{ | ||
Foo array[] = {{0}, {1}, {2}}; | ||
std::span<Foo> const span(array, 3); | ||
test_iterator(span.rbegin(), span.rend(), /*reverse=*/true); | ||
} | ||
|
||
// span<T, N>::reverse_iterator | ||
{ | ||
Foo array[] = {{0}, {1}, {2}}; | ||
std::span<Foo, 3> const span(array, 3); | ||
test_iterator(span.rbegin(), span.rend(), /*reverse=*/true); | ||
} | ||
|
||
return 0; | ||
} |
Oops, something went wrong.