[libc++] Add unsafe-buffer-usage attributes to span, vector, string and string_view #119603
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
…nd string_view
This patch is a first step towards improving the integration of
-Wunsafe-buffer-usage into the standard library. The patch basically
flags common methods of span, vector & friends as unsafe when hardening
is disabled.
This allows producing a warning diagnostic when the user explicitly
opts-into those with -Wunsafe-buffer-usage AND hardening is disabled.
For example, indexing a span with hardening disabled and the warning
enabled will now produce:
<source>:644:11: warning: function introduces unsafe buffer manipulation [-Wunsafe-buffer-usage]
644 | (void)s[3];
| ^~~~
There are certainly more places in the library that could use this
attribute and we can expand on it in the future, but this aims to
cover the most important places and provide a direction for how to
apply the same thing elsewhere.
Note that an explicit anti-goal of this patch is to set a precedent
for marking arbitrary library APIs with this attribute. There's a
large number of APIs in the library that can potentially access
pointers in unsafe way, and we should be careful to consider where
it actually makes sense to apply the attribute to make sure that these
diagnostics stay relevant (for users) and maintainable (for libc++).
Fixes llvm#107904
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesThis patch is a first step towards improving the integration of -Wunsafe-buffer-usage into the standard library. The patch basically flags common methods of span, vector & friends as unsafe when hardening is disabled. This allows producing a warning diagnostic when the user explicitly opts-into those with -Wunsafe-buffer-usage AND hardening is disabled. For example, indexing a span with hardening disabled and the warning enabled will now produce: There are certainly more places in the library that could use this attribute and we can expand on it in the future, but this aims to cover the most important places and provide a direction for how to apply the same thing elsewhere. Note that an explicit anti-goal of this patch is to set a precedent for marking arbitrary library APIs with this attribute. There's a large number of APIs in the library that can potentially access pointers in unsafe way, and we should be careful to consider where it actually makes sense to apply the attribute to make sure that these diagnostics stay relevant (for users) and maintainable (for libc++). Fixes #107904 Patch is 29.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/119603.diff 10 Files Affected:
diff --git a/libcxx/include/__assert b/libcxx/include/__assert
index 90eaa6023587b9..18245c03bf4751 100644
--- a/libcxx/include/__assert
+++ b/libcxx/include/__assert
@@ -115,4 +115,10 @@
#endif // _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_FAST
// clang-format on
+#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_NONE
+# define _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION [[_Clang::__unsafe_buffer_usage__]]
+#else
+# define _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION // those are checked
+#endif
+
#endif // _LIBCPP___ASSERT
diff --git a/libcxx/include/__iterator/wrap_iter.h b/libcxx/include/__iterator/wrap_iter.h
index 966c4675b7049a..cbac5054277bbe 100644
--- a/libcxx/include/__iterator/wrap_iter.h
+++ b/libcxx/include/__iterator/wrap_iter.h
@@ -10,6 +10,7 @@
#ifndef _LIBCPP___ITERATOR_WRAP_ITER_H
#define _LIBCPP___ITERATOR_WRAP_ITER_H
+#include <__assert>
#include <__compare/ordering.h>
#include <__compare/three_way_comparable.h>
#include <__config>
@@ -57,7 +58,10 @@ class __wrap_iter {
int> = 0>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __wrap_iter(const __wrap_iter<_OtherIter>& __u) _NOEXCEPT
: __i_(__u.__i_) {}
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference operator*() const _NOEXCEPT { return *__i_; }
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference
+ operator*() const _NOEXCEPT {
+ return *__i_;
+ }
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pointer operator->() const _NOEXCEPT {
return std::__to_address(__i_);
}
@@ -96,7 +100,8 @@ class __wrap_iter {
*this += -__n;
return *this;
}
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference operator[](difference_type __n) const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference
+ operator[](difference_type __n) const _NOEXCEPT {
return __i_[__n];
}
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 6ba7ba7bcf724b..96207c3c48173a 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -391,11 +391,13 @@ class _LIBCPP_TEMPLATE_VIS vector {
//
// element access
//
- _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference operator[](size_type __n) _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference
+ operator[](size_type __n) _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__n < size(), "vector[] index out of bounds");
return this->__begin_[__n];
}
- _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reference operator[](size_type __n) const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reference
+ operator[](size_type __n) const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__n < size(), "vector[] index out of bounds");
return this->__begin_[__n];
}
@@ -410,19 +412,23 @@ class _LIBCPP_TEMPLATE_VIS vector {
return this->__begin_[__n];
}
- _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference front() _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference
+ front() _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "front() called on an empty vector");
return *this->__begin_;
}
- _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reference front() const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reference
+ front() const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "front() called on an empty vector");
return *this->__begin_;
}
- _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference back() _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference
+ back() _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "back() called on an empty vector");
return *(this->__end_ - 1);
}
- _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reference back() const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reference
+ back() const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "back() called on an empty vector");
return *(this->__end_ - 1);
}
@@ -462,7 +468,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
}
#endif
- _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void pop_back() {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void pop_back() {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "vector::pop_back called on an empty vector");
this->__destruct_at_end(this->__end_ - 1);
}
@@ -1115,7 +1121,8 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
}
template <class _Tp, class _Allocator>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 inline _LIBCPP_HIDE_FROM_ABI typename vector<_Tp, _Allocator>::iterator
+_LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 inline _LIBCPP_HIDE_FROM_ABI
+typename vector<_Tp, _Allocator>::iterator
vector<_Tp, _Allocator>::erase(const_iterator __position) {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__position != end(), "vector::erase(iterator) called with a non-dereferenceable iterator");
@@ -1126,7 +1133,7 @@ vector<_Tp, _Allocator>::erase(const_iterator __position) {
}
template <class _Tp, class _Allocator>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::iterator
+_LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::iterator
vector<_Tp, _Allocator>::erase(const_iterator __first, const_iterator __last) {
_LIBCPP_ASSERT_VALID_INPUT_RANGE(__first <= __last, "vector::erase(first, last) called with invalid range");
pointer __p = this->__begin_ + (__first - begin());
diff --git a/libcxx/include/span b/libcxx/include/span
index 2d43d1d1079e44..34d8209f929c60 100644
--- a/libcxx/include/span
+++ b/libcxx/include/span
@@ -320,12 +320,14 @@ public:
return span<element_type, _Count>{data() + size() - _Count, _Count};
}
- _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent> first(size_type __count) const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent>
+ first(size_type __count) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__count <= size(), "span<T, N>::first(count): count out of range");
return {data(), __count};
}
- _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent> last(size_type __count) const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent>
+ last(size_type __count) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__count <= size(), "span<T, N>::last(count): count out of range");
return {data() + size() - __count, __count};
}
@@ -341,7 +343,7 @@ public:
return _ReturnType{data() + _Offset, _Count == dynamic_extent ? size() - _Offset : _Count};
}
- _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent>
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent>
subspan(size_type __offset, size_type __count = dynamic_extent) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__offset <= size(), "span<T, N>::subspan(offset, count): offset out of range");
if (__count == dynamic_extent)
@@ -355,7 +357,8 @@ public:
_LIBCPP_HIDE_FROM_ABI constexpr size_type size_bytes() const noexcept { return _Extent * sizeof(element_type); }
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty() const noexcept { return _Extent == 0; }
- _LIBCPP_HIDE_FROM_ABI constexpr reference operator[](size_type __idx) const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr reference
+ operator[](size_type __idx) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__idx < size(), "span<T, N>::operator[](index): index out of range");
return __data_[__idx];
}
@@ -368,12 +371,12 @@ public:
}
# endif
- _LIBCPP_HIDE_FROM_ABI constexpr reference front() const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr reference front() const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "span<T, N>::front() on empty span");
return __data_[0];
}
- _LIBCPP_HIDE_FROM_ABI constexpr reference back() const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr reference back() const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "span<T, N>::back() on empty span");
return __data_[size() - 1];
}
@@ -477,36 +480,41 @@ public:
: __data_{__other.data()}, __size_{__other.size()} {}
template <size_t _Count>
- _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, _Count> first() const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, _Count>
+ first() const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(_Count <= size(), "span<T>::first<Count>(): Count out of range");
return span<element_type, _Count>{data(), _Count};
}
template <size_t _Count>
- _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, _Count> last() const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, _Count>
+ last() const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(_Count <= size(), "span<T>::last<Count>(): Count out of range");
return span<element_type, _Count>{data() + size() - _Count, _Count};
}
- _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent> first(size_type __count) const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent>
+ first(size_type __count) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__count <= size(), "span<T>::first(count): count out of range");
return {data(), __count};
}
- _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent> last(size_type __count) const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent>
+ last(size_type __count) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__count <= size(), "span<T>::last(count): count out of range");
return {data() + size() - __count, __count};
}
template <size_t _Offset, size_t _Count = dynamic_extent>
- _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, _Count> subspan() const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, _Count>
+ subspan() const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(_Offset <= size(), "span<T>::subspan<Offset, Count>(): Offset out of range");
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(_Count == dynamic_extent || _Count <= size() - _Offset,
"span<T>::subspan<Offset, Count>(): Offset + Count out of range");
return span<element_type, _Count>{data() + _Offset, _Count == dynamic_extent ? size() - _Offset : _Count};
}
- constexpr span<element_type, dynamic_extent> _LIBCPP_HIDE_FROM_ABI
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr span<element_type, dynamic_extent>
subspan(size_type __offset, size_type __count = dynamic_extent) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__offset <= size(), "span<T>::subspan(offset, count): offset out of range");
if (__count == dynamic_extent)
@@ -520,7 +528,8 @@ public:
_LIBCPP_HIDE_FROM_ABI constexpr size_type size_bytes() const noexcept { return __size_ * sizeof(element_type); }
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool empty() const noexcept { return __size_ == 0; }
- _LIBCPP_HIDE_FROM_ABI constexpr reference operator[](size_type __idx) const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr reference
+ operator[](size_type __idx) const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__idx < size(), "span<T>::operator[](index): index out of range");
return __data_[__idx];
}
@@ -533,12 +542,12 @@ public:
}
# endif
- _LIBCPP_HIDE_FROM_ABI constexpr reference front() const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr reference front() const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "span<T>::front() on empty span");
return __data_[0];
}
- _LIBCPP_HIDE_FROM_ABI constexpr reference back() const noexcept {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI constexpr reference back() const noexcept {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "span<T>::back() on empty span");
return __data_[size() - 1];
}
diff --git a/libcxx/include/string b/libcxx/include/string
index 17bf4b3b98bf34..d6a09951ad1a18 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1340,7 +1340,8 @@ public:
return size() == 0;
}
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_reference operator[](size_type __pos) const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_reference
+ operator[](size_type __pos) const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__pos <= size(), "string index out of bounds");
if (__builtin_constant_p(__pos) && !__fits_in_sso(__pos)) {
return *(__get_long_pointer() + __pos);
@@ -1348,7 +1349,8 @@ public:
return *(data() + __pos);
}
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 reference operator[](size_type __pos) _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 reference
+ operator[](size_type __pos) _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__pos <= size(), "string index out of bounds");
if (__builtin_constant_p(__pos) && !__fits_in_sso(__pos)) {
return *(__get_long_pointer() + __pos);
@@ -1446,24 +1448,31 @@ public:
# endif // _LIBCPP_CXX03_LANG
_LIBCPP_CONSTEXPR_SINCE_CXX20 void push_back(value_type __c);
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void pop_back();
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void pop_back() {
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "string::pop_back(): string is already empty");
+ __erase_to_end(size() - 1);
+ }
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 reference front() _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 reference
+ front() _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "string::front(): string is empty");
return *__get_pointer();
}
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_reference front() const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_reference
+ front() const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "string::front(): string is empty");
return *data();
}
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 reference back() _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 reference
+ back() _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "string::back(): string is empty");
return *(__get_pointer() + size() - 1);
}
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_reference back() const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_reference
+ back() const _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "string::back(): string is empty");
return *(data() + size() - 1);
}
@@ -3311,12 +3320,6 @@ basic_string<_CharT, _Traits, _Allocator>::erase(const_iterator __first, const_i
return __b + static_cast<difference_type>(__r);
}
-template <class _CharT, class _Traits, class _Allocator>
-inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::pop_back() {
- _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "string::pop_back(): string is already empty");
- __erase_to_end(size() - 1);
-}
-
template <class _CharT, class _Traits, class _Allocator>
inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::clear() _NOEXCEPT {
size_type __old_size = size();
diff --git a/libcxx/include/string_view b/libcxx/include/string_view
index 27b9f152ea290a..d58ed418fc470e 100644
--- a/libcxx/include/string_view
+++ b/libcxx/include/string_view
@@ -403,7 +403,8 @@ public:
[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool empty() const _NOEXCEPT { return __size_ == 0; }
// [string.view.access], element access
- _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_reference operator[](size_type __pos) const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_reference
+ operator[](size_type __pos) const _NOEXCEPT {
return _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__pos < size(), "string_view[] index out of bounds"), __data_[__pos];
}
@@ -411,24 +412,28 @@ public:
return __pos >= size() ? (__throw_out_of_range("string_view::at"), __data_[0]) : __data_[__pos];
}
- _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_reference front() const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_reference
+ front() const _NOEXCEPT {
return _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "string_view::front(): string is empty"), __data_[0];
}
- _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_reference back() const _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_reference
+ back() const _NOEXCEPT {
return _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "string_view::back(): string is empty"), __data_[__size_ - 1];
}
_LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI const_pointer data() const _NOEXCEPT { return __data_; }
// [string.view.modifiers], modifiers:
- _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI void remove_prefix(size_type __n) _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI void
+ remove_prefix(size_type __n) _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__n <= size(), "remove_prefix() can't remove more than size()");
__data_ += __n;
__size_ -= __n;
}
- _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI void remove_suffix(size_type __n) _NOEXCEPT {
+ _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI void
+ remove_suffix(size_type __n) _NOEXCEPT {
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__n <= size(), "remove_suffix() can't remove more than size()");
__size_ -= __n;
}
diff --git a/libcxx/test/libcxx/containers/sequences/vector/unsafe-buffer-usage.verify.cpp b/libcxx/test/libcxx/containers/sequences/vector/unsafe-buffer-usage.verify.cpp
new file mode 100644
index 00000000000000..b4fb7d8dd1185b
--- /dev/null
+++ b/libcxx/test/libcxx/containers/sequences/vector/unsafe-buffer-usage.verify.cpp
@@ -0,0 +1,44 @@
+//===----------------------------------------------------------------------===//
+//
+// 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: gcc
+
+// Make sure that std::vector's operations produce unsafe buffer access warnings when
+// -Wunsafe-buffer-usage is used, when hardening is disabled.
+//
+// Note: We disable _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER to ensure that the libc++
+// headers are considered system he...
[truncated]
|
ldionne
commented
Dec 11, 2024
| @@ -115,4 +115,10 @@ | |||
| #endif // _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_FAST | |||
| // clang-format on | |||
|
|
|||
| #if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_NONE | |||
There was a problem hiding this comment.
It would be really nice if these attributes could automatically be added via the _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS assertion macro instead, since we pretty much want to have this attribute on each function that has such preconditions.
jkorous-apple
requested review from
malavikasamak,
ziqingluo-90,
t-rasmud and
jkorous-apple
|
@arichardson I'm failing to understand why the Android CI is failing. Do you folks define |
|
Do you plan to land this? I'd really like to see an RFC before making any such changes. This is already a significant change and covers only a very small fraction of the API surface. |
|
Why only when hardening is disabled? For comparison, in Chromium's span, we flag things like the pointer + size constructor of span as unsafe, even though we effectively "harden" it with a runtime check; we want to discourage use of such APIs at all, and move them to patterns where problems are more detectable at compile time or cannot occur at all. |
Comment on lines
+37
to
+40
| #else | ||
| (void)*it; // expected-warning {{function introduces unsafe buffer manipulation}} | ||
| (void)it[n]; // expected-warning {{function introduces unsafe buffer manipulation}} | ||
| #endif |
There was a problem hiding this comment.
The overall approach here looks great to me from the perspective of WebKit programming. This would catch use of unsafe iterators in Darwin user space, and also just a misconfigured build that tried to enable the hardened C++ library but failed (which we've done before!).
I wonder if iterator operator++ and operator-- should also be marked with the unsafe annotation? Normally unsafe buffer usage warns when you adjust the pointer, not necessarily when you dereference it.
(You could argue that iterators, unlike pointers, should also warn when you dereference, since end() is a valid iterator. But still valuable to warn when you adjust an iterator, since you may ultimately pass the result to code that doesn't enforce safety at time of dereference.)
There was a problem hiding this comment.
...and my reading of the spec says that merely adjusting an iterator past the end or prior to the beginning is undef, even if you don't dereference it.
danakj
reviewed
Jan 6, 2025
There was a problem hiding this comment.
I am trying to understand the use case of not wanting hardening enabled in libc++ but wanting unsafe-buffer-usage warnings. This would seem to imply the use of some other containers library with safer APIs than then (unhardened) libc++, but then is libc++ being used still? And why not enable hardening? Did you have some specific use case in mind for this?
| @@ -57,7 +58,10 @@ class __wrap_iter { | |||
| int> = 0> | |||
| _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __wrap_iter(const __wrap_iter<_OtherIter>& __u) _NOEXCEPT | |||
| : __i_(__u.__i_) {} | |||
| _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference operator*() const _NOEXCEPT { return *__i_; } | |||
| _LIBCPP_VALID_ELEMENT_ACCESS_PRECONDITION _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 reference | |||
There was a problem hiding this comment.
This warns when hardening is disabled, but whether the checking is happening is dependent also on the checked iterator ABI flag. In this case, since the checked iterator ABI flag is off (we're using __wrap_iter) the hardening flag has no effect, so it the behaviour would always be what I would expect to be flagged as unsafe_buffer_usage, even if hardening is enabled? Or am I misunderstanding when __wrap_iter is used?
There was a problem hiding this comment.
The use case is when you've enabled hardening in libc++ (so you get bounds checking in span::operator[], array::operator[], etc.), but your vendor configuration -- e.g., Darwin user space -- does not enable hardening for iterators (so you can't safely use span.begin(), array.begin(), etc.), because enabling hardening for iterators would break ABI.
There was a problem hiding this comment.
Shouldn't this generate -Wunsafe-buffer-usage even if hardening is not enabled then too? The warning isn't tied to hardening. These iterators are never safe buffer usage.
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 patch is a first step towards improving the integration of -Wunsafe-buffer-usage into the standard library. The patch basically flags common methods of span, vector & friends as unsafe when hardening is disabled.
This allows producing a warning diagnostic when the user explicitly opts-into those with -Wunsafe-buffer-usage AND hardening is disabled. For example, indexing a span with hardening disabled and the warning enabled will now produce:
There are certainly more places in the library that could use this attribute and we can expand on it in the future, but this aims to cover the most important places and provide a direction for how to apply the same thing elsewhere.
Note that an explicit anti-goal of this patch is to set a precedent for marking arbitrary library APIs with this attribute. There's a large number of APIs in the library that can potentially access pointers in unsafe way, and we should be careful to consider where it actually makes sense to apply the attribute to make sure that these diagnostics stay relevant (for users) and maintainable (for libc++).
Fixes #107904