Skip to content

[libc++][hardening] Categorize more assertions. #75918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jan 6, 2024
Merged

[libc++][hardening] Categorize more assertions. #75918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b1680c8
[libc++][hardening] Categorize more 'valid-element-access' checks.
var-const Nov 6, 2023
9a8622d
Address feedback (test iterator operations)
var-const Dec 16, 2023
bcc8ef2
Quick fix to new tests
var-const Dec 16, 2023
cd4d8c7
Fix bad merge
var-const Dec 19, 2023
904bb3e
[libc++][hardening] Categorize more assertions.
var-const Dec 19, 2023
24d8122
Classify a few more assertions (mostly in `<regex>``).
var-const Dec 21, 2023
cc436df
Address feedback
var-const Jan 5, 2024
2d66073
clang-format
var-const Jan 5, 2024
8405233
Fix CI
var-const Jan 5, 2024
ea26d9d
Fix CI
var-const Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion libcxx/include/__algorithm/pop_heap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ __pop_heap(_RandomAccessIterator __first,
_RandomAccessIterator __last,
_Compare& __comp,
typename iterator_traits<_RandomAccessIterator>::difference_type __len) {
_LIBCPP_ASSERT_UNCATEGORIZED(__len > 0, "The heap given to pop_heap must be non-empty");
// Calling `pop_heap` on an empty range is undefined behavior, but in practice it will be a no-op.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider taking advantage of this precondition and removing if (__len > 1), but then bumping this assertion to a valid-range (or similar) assertion. Thoughts? If we opt to do this, it would be as a separate change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but would prefer to do this in a separate PR and merge after the next release is branched.

_LIBCPP_ASSERT_PEDANTIC(__len > 0, "The heap given to pop_heap must be non-empty");

__comp_ref_type<_Compare> __comp_ref = __comp;

Expand Down
2 changes: 1 addition & 1 deletion libcxx/include/__algorithm/sift_down.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _RandomAccessIterator __floy
_Compare&& __comp,
typename iterator_traits<_RandomAccessIterator>::difference_type __len) {
using difference_type = typename iterator_traits<_RandomAccessIterator>::difference_type;
_LIBCPP_ASSERT_UNCATEGORIZED(__len >= 2, "shouldn't be called unless __len >= 2");
_LIBCPP_ASSERT_INTERNAL(__len >= 2, "shouldn't be called unless __len >= 2");

_RandomAccessIterator __hole = __first;
_RandomAccessIterator __child_i = __first;
Expand Down
4 changes: 2 additions & 2 deletions libcxx/include/__algorithm/sort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ __bitset_partition(_RandomAccessIterator __first, _RandomAccessIterator __last,
using _Ops = _IterOps<_AlgPolicy>;
typedef typename std::iterator_traits<_RandomAccessIterator>::value_type value_type;
typedef typename std::iterator_traits<_RandomAccessIterator>::difference_type difference_type;
_LIBCPP_ASSERT_UNCATEGORIZED(__last - __first >= difference_type(3), "");
_LIBCPP_ASSERT_INTERNAL(__last - __first >= difference_type(3), "");
const _RandomAccessIterator __begin = __first; // used for bounds checking, those are not moved around
const _RandomAccessIterator __end = __last;
(void)__end; //
Expand Down Expand Up @@ -625,7 +625,7 @@ __partition_with_equals_on_right(_RandomAccessIterator __first, _RandomAccessIte
using _Ops = _IterOps<_AlgPolicy>;
typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
typedef typename std::iterator_traits<_RandomAccessIterator>::value_type value_type;
_LIBCPP_ASSERT_UNCATEGORIZED(__last - __first >= difference_type(3), "");
_LIBCPP_ASSERT_INTERNAL(__last - __first >= difference_type(3), "");
const _RandomAccessIterator __begin = __first; // used for bounds checking, those are not moved around
const _RandomAccessIterator __end = __last;
(void)__end; //
Expand Down
6 changes: 3 additions & 3 deletions libcxx/include/__charconv/to_chars_base_10.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,14 @@ __base_10_u64(char* __buffer, uint64_t __value) noexcept {
/// range that can be used. However the range is sufficient for
/// \ref __base_10_u128.
_LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI inline __uint128_t __pow_10(int __exp) noexcept {
_LIBCPP_ASSERT_UNCATEGORIZED(__exp >= __pow10_128_offset, "Index out of bounds");
_LIBCPP_ASSERT_INTERNAL(__exp >= __pow10_128_offset, "Index out of bounds");
return __pow10_128[__exp - __pow10_128_offset];
}

_LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI inline char*
__base_10_u128(char* __buffer, __uint128_t __value) noexcept {
_LIBCPP_ASSERT_UNCATEGORIZED(
__value > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fail when this isn't true.");
_LIBCPP_ASSERT_INTERNAL(
__value > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fails when this isn't true.");

// Unlike the 64 to 32 bit case the 128 bit case the "upper half" can't be
// stored in the "lower half". Instead we first need to handle the top most
Expand Down
2 changes: 1 addition & 1 deletion libcxx/include/__charconv/to_chars_integral.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ __to_chars_integral(char* __first, char* __last, _Tp __value) {

template <typename _Tp>
_LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI int __to_chars_integral_width(_Tp __value, unsigned __base) {
_LIBCPP_ASSERT_UNCATEGORIZED(__value >= 0, "The function requires a non-negative value.");
_LIBCPP_ASSERT_INTERNAL(__value >= 0, "The function requires a non-negative value.");

unsigned __base_2 = __base * __base;
unsigned __base_3 = __base_2 * __base;
Expand Down
4 changes: 2 additions & 2 deletions libcxx/include/__charconv/traits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,11 @@ struct _LIBCPP_HIDDEN __traits_base<_Tp, __enable_if_t<sizeof(_Tp) == sizeof(__u
/// zero is set to one. This means the first element of the lookup table is
/// zero.
static _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI int __width(_Tp __v) {
_LIBCPP_ASSERT_UNCATEGORIZED(
_LIBCPP_ASSERT_INTERNAL(
__v > numeric_limits<uint64_t>::max(), "The optimizations for this algorithm fail when this isn't true.");
// There's always a bit set in the upper 64-bits.
auto __t = (128 - std::__libcpp_clz(static_cast<uint64_t>(__v >> 64))) * 1233 >> 12;
_LIBCPP_ASSERT_UNCATEGORIZED(__t >= __itoa::__pow10_128_offset, "Index out of bounds");
_LIBCPP_ASSERT_INTERNAL(__t >= __itoa::__pow10_128_offset, "Index out of bounds");
// __t is adjusted since the lookup table misses the lower entries.
return __t - (__v < __itoa::__pow10_128[__t - __itoa::__pow10_128_offset]) + 1;
}
Expand Down
7 changes: 3 additions & 4 deletions libcxx/include/__chrono/parser_std_format_spec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,9 @@ class _LIBCPP_TEMPLATE_VIS __parser_chrono {
private:
_LIBCPP_HIDE_FROM_ABI constexpr _ConstIterator
__parse_chrono_specs(_ConstIterator __begin, _ConstIterator __end, __flags __flags) {
_LIBCPP_ASSERT_UNCATEGORIZED(
__begin != __end,
"When called with an empty input the function will cause "
"undefined behavior by evaluating data not in the input");
_LIBCPP_ASSERT_INTERNAL(__begin != __end,
"When called with an empty input the function will cause "
"undefined behavior by evaluating data not in the input");

if (*__begin != _CharT('%') && *__begin != _CharT('}'))
std::__throw_format_error("The format specifier expects a '%' or a '}'");
Expand Down
7 changes: 7 additions & 0 deletions libcxx/include/__config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,9 @@
// - `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR` -- checks any operations that exchange nodes between containers to make sure
// the containers have compatible allocators.
//
// - `_LIBCPP_ASSERT_PEDANTIC` -- checks prerequisites which are imposed by the Standard, but violating which happens to
// be benign in our implementation.
//
// - `_LIBCPP_ASSERT_INTERNAL` -- checks that internal invariants of the library hold. These assertions don't depend on
// user input.
//
Expand Down Expand Up @@ -325,6 +328,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
// vulnerability.
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)

Expand All @@ -339,6 +343,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
// Disabled checks.
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)

Expand All @@ -352,6 +357,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)

Expand All @@ -365,6 +371,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)

Expand Down
3 changes: 2 additions & 1 deletion libcxx/include/__filesystem/directory_iterator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ class directory_iterator {
_LIBCPP_HIDE_FROM_ABI ~directory_iterator() = default;

_LIBCPP_HIDE_FROM_ABI const directory_entry& operator*() const {
_LIBCPP_ASSERT_UNCATEGORIZED(__imp_, "The end iterator cannot be dereferenced");
// Note: this check duplicates a check in `__dereference()`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our discussion just now, these are the different ways we can think of handling the situation of "redundant" checks:

// Option #1: leave it as-is
void f(std::optional<T> foo) {
    _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(foo.has_value(), "oops");
    use(*foo);
}

// Option #2: remove it, it's implicitly checked, we "know" it
void f(std::optional<T> foo) {
    use(*foo);
}

// Option #3: Use a comment
void f(std::optional<T> foo) {
    // implicit precondition: foo.has_value(), already checked in operator* below
    use(*foo);
}

// Option #4: Macro orthogonal to the assertion category
void f(std::optional<T> foo) {
    _LIBCPP_REDUNDANT_ASSERTION(_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(foo.has_value(), "oops"));
    use(*foo);
}

// Option #5: Add a new category
void f(std::optional<T> foo) {
    // REDUNDANT|EARLY|EXTRA|...
    _LIBCPP_ASSERT_EARLY(foo.has_value(), "oops");
    use(*foo);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion just now, we discussed this by elimination:

  • We think we should not do #1 because the fast mode should be fast, so we should remove duplicate checks
  • #2 is just going to make us scratch our head in the future, it seems that #3 is strictly better in that case
  • #3 is OK, but we lose the property of having early diagnostics with more meaningful error messages in the case where the extensive mode is enabled. So it is worse than #4 and #5.
  • #4 has the benefit over #5 that we can retain the real category of the assertion, so it's kinda cleaner in that way. But either would work.

It seems we have a preference for #4 over #5, although not an extremely strong one.

_LIBCPP_ASSERT_NON_NULL(__imp_, "The end iterator cannot be dereferenced");
return __dereference();
}

Expand Down
4 changes: 2 additions & 2 deletions libcxx/include/__filesystem/path_iterator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class _LIBCPP_EXPORTED_FROM_ABI path::iterator {
_LIBCPP_HIDE_FROM_ABI pointer operator->() const { return &__stashed_elem_; }

_LIBCPP_HIDE_FROM_ABI iterator& operator++() {
_LIBCPP_ASSERT_UNCATEGORIZED(__state_ != _Singular, "attempting to increment a singular iterator");
_LIBCPP_ASSERT_NON_NULL(__state_ != _Singular, "attempting to increment a singular iterator");
_LIBCPP_ASSERT_UNCATEGORIZED(__state_ != _AtEnd, "attempting to increment the end iterator");
return __increment();
}
Expand All @@ -73,7 +73,7 @@ class _LIBCPP_EXPORTED_FROM_ABI path::iterator {
}

_LIBCPP_HIDE_FROM_ABI iterator& operator--() {
_LIBCPP_ASSERT_UNCATEGORIZED(__state_ != _Singular, "attempting to decrement a singular iterator");
_LIBCPP_ASSERT_NON_NULL(__state_ != _Singular, "attempting to decrement a singular iterator");
_LIBCPP_ASSERT_UNCATEGORIZED(
__entry_.data() != __path_ptr_->native().data(), "attempting to decrement the begin iterator");
return __decrement();
Expand Down
12 changes: 6 additions & 6 deletions libcxx/include/__format/buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {

// The output doesn't fit in the internal buffer.
// Copy the data in "__capacity_" sized chunks.
_LIBCPP_ASSERT_UNCATEGORIZED(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
_LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mordante If you have the time, it would be great if you could see if the classification in <format> and <charconv> looks reasonable to you. I classified many of these as "internal" because, if I'm reading the code correctly, these check for internal variants that should always hold unless there's a bug in our implementation. But I'm not very familiar with this part of the code and would really appreciate your feedback. Thanks!

const _InCharT* __first = __str.data();
do {
size_t __chunk = std::min(__n, __capacity_);
Expand All @@ -134,7 +134,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {
class _UnaryOperation,
__fmt_char_type _InCharT = typename iterator_traits<_Iterator>::value_type>
_LIBCPP_HIDE_FROM_ABI void __transform(_Iterator __first, _Iterator __last, _UnaryOperation __operation) {
_LIBCPP_ASSERT_UNCATEGORIZED(__first <= __last, "not a valid range");
_LIBCPP_ASSERT_INTERNAL(__first <= __last, "not a valid range");

size_t __n = static_cast<size_t>(__last - __first);
__flush_on_overflow(__n);
Expand All @@ -146,7 +146,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {

// The output doesn't fit in the internal buffer.
// Transform the data in "__capacity_" sized chunks.
_LIBCPP_ASSERT_UNCATEGORIZED(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
_LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
do {
size_t __chunk = std::min(__n, __capacity_);
std::transform(__first, __first + __chunk, std::addressof(__ptr_[__size_]), __operation);
Expand All @@ -168,7 +168,7 @@ class _LIBCPP_TEMPLATE_VIS __output_buffer {

// The output doesn't fit in the internal buffer.
// Fill the buffer in "__capacity_" sized chunks.
_LIBCPP_ASSERT_UNCATEGORIZED(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
_LIBCPP_ASSERT_INTERNAL(__size_ == 0, "the buffer should be flushed by __flush_on_overflow");
do {
size_t __chunk = std::min(__n, __capacity_);
std::fill_n(std::addressof(__ptr_[__size_]), __chunk, __value);
Expand Down Expand Up @@ -596,7 +596,7 @@ class _LIBCPP_TEMPLATE_VIS __retarget_buffer {
class _UnaryOperation,
__fmt_char_type _InCharT = typename iterator_traits<_Iterator>::value_type>
_LIBCPP_HIDE_FROM_ABI void __transform(_Iterator __first, _Iterator __last, _UnaryOperation __operation) {
_LIBCPP_ASSERT_UNCATEGORIZED(__first <= __last, "not a valid range");
_LIBCPP_ASSERT_INTERNAL(__first <= __last, "not a valid range");

size_t __n = static_cast<size_t>(__last - __first);
if (__size_ + __n >= __capacity_)
Expand All @@ -623,7 +623,7 @@ class _LIBCPP_TEMPLATE_VIS __retarget_buffer {
_LIBCPP_HIDE_FROM_ABI void __grow_buffer() { __grow_buffer(__capacity_ * 1.6); }

_LIBCPP_HIDE_FROM_ABI void __grow_buffer(size_t __capacity) {
_LIBCPP_ASSERT_UNCATEGORIZED(__capacity > __capacity_, "the buffer must grow");
_LIBCPP_ASSERT_INTERNAL(__capacity > __capacity_, "the buffer must grow");
auto __result = std::__allocate_at_least(__alloc_, __capacity);
auto __guard = std::__make_exception_guard([&] {
allocator_traits<_Alloc>::deallocate(__alloc_, __result.ptr, __result.count);
Expand Down
2 changes: 1 addition & 1 deletion libcxx/include/__format/format_arg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool __use_packed_format_arg_store(size_t __size
}

_LIBCPP_HIDE_FROM_ABI constexpr __arg_t __get_packed_type(uint64_t __types, size_t __id) {
_LIBCPP_ASSERT_UNCATEGORIZED(__id <= __packed_types_max, "");
_LIBCPP_ASSERT_INTERNAL(__id <= __packed_types_max, "");

if (__id > 0)
__types >>= __id * __packed_arg_t_bits;
Expand Down
2 changes: 1 addition & 1 deletion libcxx/include/__format/formatter_bool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ struct _LIBCPP_TEMPLATE_VIS formatter<bool, _CharT> {
static_cast<unsigned>(__value), __ctx, __parser_.__get_parsed_std_specifications(__ctx));

default:
_LIBCPP_ASSERT_UNCATEGORIZED(false, "The parse function should have validated the type");
_LIBCPP_ASSERT_INTERNAL(false, "The parse function should have validated the type");
__libcpp_unreachable();
}
}
Expand Down
Loading