[libc++][hardening] Add hardening assertions to std::deque #79397
Conversation
This file contains hidden or 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
This aligns std::deque with std::vector w.r.t. hardening checks. There's probably more that can be done with iterators, but start with this. This caught a bug with one of libc++'s tests. One of the erase calls in asan_caterpillar.pass.cpp was a no-op because the iterators were in the other order. (deque::erase happened to cleanly do nothing when the distance is negative.) Fixes llvm#63809
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: David Benjamin (davidben) ChangesThis aligns std::deque with std::vector w.r.t. hardening checks. There's probably more that can be done with iterators, but start with this. This caught a bug with one of libc++'s tests. One of the erase calls in asan_caterpillar.pass.cpp was a no-op because the iterators were in the other order. (deque::erase happened to cleanly do nothing when the distance is negative.) Fixes #63809 Full diff: https://github.com/llvm/llvm-project/pull/79397.diff 3 Files Affected:
diff --git a/libcxx/include/deque b/libcxx/include/deque
index fca8b3d6e2c737..68a8dd51cc8925 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -1487,6 +1487,7 @@ void deque<_Tp, _Allocator>::shrink_to_fit() _NOEXCEPT {
template <class _Tp, class _Allocator>
inline typename deque<_Tp, _Allocator>::reference deque<_Tp, _Allocator>::operator[](size_type __i) _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__i < size(), "deque::operator[] index out of bounds");
size_type __p = __start_ + __i;
return *(*(__map_.begin() + __p / __block_size) + __p % __block_size);
}
@@ -1494,6 +1495,7 @@ inline typename deque<_Tp, _Allocator>::reference deque<_Tp, _Allocator>::operat
template <class _Tp, class _Allocator>
inline typename deque<_Tp, _Allocator>::const_reference
deque<_Tp, _Allocator>::operator[](size_type __i) const _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__i < size(), "deque::operator[] index out of bounds");
size_type __p = __start_ + __i;
return *(*(__map_.begin() + __p / __block_size) + __p % __block_size);
}
@@ -1516,22 +1518,26 @@ inline typename deque<_Tp, _Allocator>::const_reference deque<_Tp, _Allocator>::
template <class _Tp, class _Allocator>
inline typename deque<_Tp, _Allocator>::reference deque<_Tp, _Allocator>::front() _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "deque::front called on an empty deque");
return *(*(__map_.begin() + __start_ / __block_size) + __start_ % __block_size);
}
template <class _Tp, class _Allocator>
inline typename deque<_Tp, _Allocator>::const_reference deque<_Tp, _Allocator>::front() const _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "deque::front called on an empty deque");
return *(*(__map_.begin() + __start_ / __block_size) + __start_ % __block_size);
}
template <class _Tp, class _Allocator>
inline typename deque<_Tp, _Allocator>::reference deque<_Tp, _Allocator>::back() _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "deque::back called on an empty deque");
size_type __p = size() + __start_ - 1;
return *(*(__map_.begin() + __p / __block_size) + __p % __block_size);
}
template <class _Tp, class _Allocator>
inline typename deque<_Tp, _Allocator>::const_reference deque<_Tp, _Allocator>::back() const _NOEXCEPT {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "deque::back called on an empty deque");
size_type __p = size() + __start_ - 1;
return *(*(__map_.begin() + __p / __block_size) + __p % __block_size);
}
@@ -2255,6 +2261,7 @@ void deque<_Tp, _Allocator>::__add_back_capacity(size_type __n) {
template <class _Tp, class _Allocator>
void deque<_Tp, _Allocator>::pop_front() {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "deque::pop_front called on an empty deque");
size_type __old_sz = size();
size_type __old_start = __start_;
allocator_type& __a = __alloc();
@@ -2395,6 +2402,8 @@ void deque<_Tp, _Allocator>::__move_construct_backward_and_check(
template <class _Tp, class _Allocator>
typename deque<_Tp, _Allocator>::iterator deque<_Tp, _Allocator>::erase(const_iterator __f) {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
+ __f != end(), "deque::erase(iterator) called with a non-dereferenceable iterator");
size_type __old_sz = size();
size_type __old_start = __start_;
iterator __b = begin();
@@ -2420,6 +2429,7 @@ typename deque<_Tp, _Allocator>::iterator deque<_Tp, _Allocator>::erase(const_it
template <class _Tp, class _Allocator>
typename deque<_Tp, _Allocator>::iterator deque<_Tp, _Allocator>::erase(const_iterator __f, const_iterator __l) {
+ _LIBCPP_ASSERT_VALID_INPUT_RANGE(__f <= __l, "deque::erase(first, last) called with an invalid range");
size_type __old_sz = size();
size_type __old_start = __start_;
difference_type __n = __l - __f;
diff --git a/libcxx/test/libcxx/containers/sequences/deque/asan_caterpillar.pass.cpp b/libcxx/test/libcxx/containers/sequences/deque/asan_caterpillar.pass.cpp
index d3c6e0493ee28f..4c7b2317d04d3a 100644
--- a/libcxx/test/libcxx/containers/sequences/deque/asan_caterpillar.pass.cpp
+++ b/libcxx/test/libcxx/containers/sequences/deque/asan_caterpillar.pass.cpp
@@ -25,7 +25,7 @@ void test1() {
for (int i = 0; i < 1100; i += 1) {
test.insert(test.begin(), buff, buff + 320);
- test.erase(test.end(), test.end() - 320);
+ test.erase(test.end() - 320, test.end());
}
test.insert(test.begin(), buff, buff + 32000);
diff --git a/libcxx/test/libcxx/containers/sequences/deque/assert.pass.cpp b/libcxx/test/libcxx/containers/sequences/deque/assert.pass.cpp
new file mode 100644
index 00000000000000..a0c80ce30ec78d
--- /dev/null
+++ b/libcxx/test/libcxx/containers/sequences/deque/assert.pass.cpp
@@ -0,0 +1,54 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <deque>
+
+// Test hardening assertions for std::deque.
+
+// REQUIRES: has-unix-headers
+// UNSUPPORTED: libcpp-hardening-mode=none
+
+#include <deque>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+ std::deque<int> c;
+ TEST_LIBCPP_ASSERT_FAILURE(c.front(), "deque::front called on an empty deque");
+ TEST_LIBCPP_ASSERT_FAILURE(c.back(), "deque::back called on an empty deque");
+ TEST_LIBCPP_ASSERT_FAILURE(c[0], "deque::operator[] index out of bounds");
+ TEST_LIBCPP_ASSERT_FAILURE(c.pop_front(), "deque::pop_front called on an empty deque");
+ TEST_LIBCPP_ASSERT_FAILURE(c.pop_back(), "deque::pop_back called on an empty deque");
+
+ // Repeat the test with a const reference to test the const overloads.
+ {
+ const std::deque<int>& cc = c;
+ TEST_LIBCPP_ASSERT_FAILURE(cc.front(), "deque::front called on an empty deque");
+ TEST_LIBCPP_ASSERT_FAILURE(cc.back(), "deque::back called on an empty deque");
+ TEST_LIBCPP_ASSERT_FAILURE(cc[0], "deque::operator[] index out of bounds");
+ }
+
+ c.push_back(1);
+ c.push_back(2);
+ c.push_back(3);
+ TEST_LIBCPP_ASSERT_FAILURE(c[3], "deque::operator[] index out of bounds");
+ TEST_LIBCPP_ASSERT_FAILURE(c[100], "deque::operator[] index out of bounds");
+
+ // Repeat the test with a const reference to test the const overloads.
+ {
+ const std::deque<int>& cc = c;
+ TEST_LIBCPP_ASSERT_FAILURE(cc[3], "deque::operator[] index out of bounds");
+ TEST_LIBCPP_ASSERT_FAILURE(cc[100], "deque::operator[] index out of bounds");
+ }
+
+ TEST_LIBCPP_ASSERT_FAILURE(c.erase(c.end()), "deque::erase(iterator) called with a non-dereferenceable iterator");
+ TEST_LIBCPP_ASSERT_FAILURE(
+ c.erase(c.begin() + 1, c.begin()), "deque::erase(first, last) called with an invalid range");
+
+ return 0;
+}
|
var-const
approved these changes
Jan 27, 2024
danakj
reviewed
Jan 31, 2024
| TEST_LIBCPP_ASSERT_FAILURE(c[3], "deque::operator[] index out of bounds"); | ||
| TEST_LIBCPP_ASSERT_FAILURE(c[100], "deque::operator[] index out of bounds"); | ||
|
|
||
| // Repeat the test with a const reference to test the const overloads. |
There was a problem hiding this comment.
The test cases above are also on a const reference, aren't they? Are we missing mutable overloads then?
There was a problem hiding this comment.
Oh nevermind, sorry, I was looking at the front() ones but this is [].
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This aligns std::deque with std::vector w.r.t. hardening checks. There's probably more that can be done with iterators, but start with this.
This caught a bug with one of libc++'s tests. One of the erase calls in asan_caterpillar.pass.cpp was a no-op because the iterators were in the other order. (deque::erase happened to cleanly do nothing when the distance is negative.)
Fixes #63809