[libcxx] [clang-tools-extra] [clang] [llvm] [libc++][hardening] Categorize assertions related to strict weak ordering (PR #77405)
Konstantin Varlamov via cfe-commits
cfe-commits at lists.llvm.org
Sat Jan 20 23:45:04 PST 2024
https://github.com/var-const updated https://github.com/llvm/llvm-project/pull/77405
>From f6a3ba6f2fb00b17182b405312eca4e837fe8977 Mon Sep 17 00:00:00 2001
From: Konstantin Varlamov <varconsteq at gmail.com>
Date: Mon, 8 Jan 2024 19:09:37 -0800
Subject: [PATCH] [libc++][hardening] Categorize assertions related to strict
weak ordering
If a user passes a comparator that doesn't satisfy strict weak ordering
(see https://eel.is/c++draft/algorithms#alg.sorting.general) to
a sorting algorithm, this can produce an incorrect result or even lead
to an out-of-bounds access. Unfortunately, comprehensively validating
that a given comparator indeed satisfies the strict weak ordering
requirement is prohibitively expensive (see https://discourse.llvm.org/t/rfc-strict-weak-ordering-checks-in-the-debug-libc/70217).
As a result, we have three independent sets of checks, in order from
least to most expensive:
- assertions that catch out-of-bounds accesses within the algorithms'
implementation. These are relatively cheap; however, they cannot catch
the underlying cause and cannot prevent the case where an invalid
comparator would result in an uncorrectly-sorted sequence without
producing an OOB access;
- debug comparators that wrap a given comparator and on each comparison
check that if `(a < b)`, then `!(b < a)`, where `<` stands for the
user-provided comparator. This performs up to 2x number of comparisons
but doesn't affect the algorithmic complexity. While this approach can
find more issues, it is still a heuristic;
- a comprehensive check of the comparator that validates up to 100
elements in the resulting sorted sequence (see the RFC above for
details), imposing a significant performance overhead.
Accordingly, the first set of checks is enabled in the fast hardening
mode, the second in the extensive mode, and the third only in the debug
mode.
For the most expensive checks, introduce a new category
`_LIBCPP_ASSERT_INTRUSIVE`. This category is intended for expensive
checks that perform intrusive, extensive validation within the
implementation (either of the user input or of invariants or
postconditions of a function).
See https://reviews.llvm.org/D150264 for additional background.
---
libcxx/include/__algorithm/comp_ref_type.h | 9 ++++----
libcxx/include/__algorithm/nth_element.h | 8 +++----
libcxx/include/__algorithm/sort.h | 22 +++++++++---------
.../__algorithm/three_way_comp_ref_type.h | 10 ++++----
libcxx/include/__config | 23 +++++++++++++++++++
.../strict_weak_ordering_check.h | 10 ++++----
6 files changed, 53 insertions(+), 29 deletions(-)
diff --git a/libcxx/include/__algorithm/comp_ref_type.h b/libcxx/include/__algorithm/comp_ref_type.h
index 15f4a535a30bf0d..2583eec63eef665 100644
--- a/libcxx/include/__algorithm/comp_ref_type.h
+++ b/libcxx/include/__algorithm/comp_ref_type.h
@@ -44,7 +44,7 @@ struct __debug_less {
_LIBCPP_CONSTEXPR_SINCE_CXX14 inline _LIBCPP_HIDE_FROM_ABI decltype((void)std::declval<_Compare&>()(
std::declval<_LHS&>(), std::declval<_RHS&>()))
__do_compare_assert(int, _LHS& __l, _RHS& __r) {
- _LIBCPP_ASSERT_UNCATEGORIZED(!__comp_(__l, __r), "Comparator does not induce a strict weak ordering");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(!__comp_(__l, __r), "Comparator does not induce a strict weak ordering");
(void)__l;
(void)__r;
}
@@ -53,10 +53,9 @@ struct __debug_less {
_LIBCPP_CONSTEXPR_SINCE_CXX14 inline _LIBCPP_HIDE_FROM_ABI void __do_compare_assert(long, _LHS&, _RHS&) {}
};
-// Pass the comparator by lvalue reference. Or in debug mode, using a
-// debugging wrapper that stores a reference.
-#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
-template <class _Comp>
+// Pass the comparator by lvalue reference. Or in the extensive hardening mode and above, using a debugging wrapper that
+// stores a reference.
+#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_EXTENSIVE || _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
using __comp_ref_type = __debug_less<_Comp>;
#else
template <class _Comp>
diff --git a/libcxx/include/__algorithm/nth_element.h b/libcxx/include/__algorithm/nth_element.h
index a05970512595181..37ddfbdacf044d0 100644
--- a/libcxx/include/__algorithm/nth_element.h
+++ b/libcxx/include/__algorithm/nth_element.h
@@ -114,12 +114,12 @@ __nth_element(
while (true) {
while (!__comp(*__first, *__i)) {
++__i;
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__i != __last,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
}
do {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__j != __first,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
--__j;
@@ -150,13 +150,13 @@ __nth_element(
// __m still guards upward moving __i
while (__comp(*__i, *__m)) {
++__i;
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__i != __last,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
}
// It is now known that a guard exists for downward moving __j
do {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__j != __first,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
--__j;
diff --git a/libcxx/include/__algorithm/sort.h b/libcxx/include/__algorithm/sort.h
index ac47489af0aac9e..451133a2d193dd2 100644
--- a/libcxx/include/__algorithm/sort.h
+++ b/libcxx/include/__algorithm/sort.h
@@ -325,7 +325,7 @@ __insertion_sort_unguarded(_RandomAccessIterator const __first, _RandomAccessIte
do {
*__j = _Ops::__iter_move(__k);
__j = __k;
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__k != __leftmost,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
} while (__comp(__t, *--__k)); // No need for bounds check due to the assumption stated above.
@@ -544,7 +544,7 @@ __bitset_partition(_RandomAccessIterator __first, _RandomAccessIterator __last,
// Not guarded since we know the last element is greater than the pivot.
do {
++__first;
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__first != __end,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
} while (!__comp(__pivot, *__first));
@@ -557,7 +557,7 @@ __bitset_partition(_RandomAccessIterator __first, _RandomAccessIterator __last,
// It will be always guarded because __introsort will do the median-of-three
// before calling this.
do {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__last != __begin,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
--__last;
@@ -635,7 +635,7 @@ __partition_with_equals_on_right(_RandomAccessIterator __first, _RandomAccessIte
// this.
do {
++__first;
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__first != __end,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
} while (__comp(*__first, __pivot));
@@ -647,7 +647,7 @@ __partition_with_equals_on_right(_RandomAccessIterator __first, _RandomAccessIte
} else {
// Guarded.
do {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__last != __begin,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
--__last;
@@ -665,12 +665,12 @@ __partition_with_equals_on_right(_RandomAccessIterator __first, _RandomAccessIte
_Ops::iter_swap(__first, __last);
do {
++__first;
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__first != __end,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
} while (__comp(*__first, __pivot));
do {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__last != __begin,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
--__last;
@@ -702,7 +702,7 @@ __partition_with_equals_on_left(_RandomAccessIterator __first, _RandomAccessIter
// Guarded.
do {
++__first;
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__first != __end,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
} while (!__comp(__pivot, *__first));
@@ -715,7 +715,7 @@ __partition_with_equals_on_left(_RandomAccessIterator __first, _RandomAccessIter
// It will be always guarded because __introsort will do the
// median-of-three before calling this.
do {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__last != __begin,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
--__last;
@@ -725,12 +725,12 @@ __partition_with_equals_on_left(_RandomAccessIterator __first, _RandomAccessIter
_Ops::iter_swap(__first, __last);
do {
++__first;
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__first != __end,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
} while (!__comp(__pivot, *__first));
do {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
__last != __begin,
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
--__last;
diff --git a/libcxx/include/__algorithm/three_way_comp_ref_type.h b/libcxx/include/__algorithm/three_way_comp_ref_type.h
index 8fd15c5d66f2fc9..19533270b497d25 100644
--- a/libcxx/include/__algorithm/three_way_comp_ref_type.h
+++ b/libcxx/include/__algorithm/three_way_comp_ref_type.h
@@ -50,15 +50,17 @@ struct __debug_three_way_comp {
__expected = _Order::greater;
if (__o == _Order::greater)
__expected = _Order::less;
- _LIBCPP_ASSERT_UNCATEGORIZED(__comp_(__l, __r) == __expected, "Comparator does not induce a strict weak ordering");
+ _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
+ __comp_(__l, __r) == __expected, "Comparator does not induce a strict weak ordering");
(void)__l;
(void)__r;
}
};
-// Pass the comparator by lvalue reference. Or in debug mode, using a
-// debugging wrapper that stores a reference.
-# if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
+// Pass the comparator by lvalue reference. Or in the extensive hardening mode and above, using a debugging wrapper that
+// stores a reference.
+# if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_EXTENSIVE || \
+ _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
template <class _Comp>
using __three_way_comp_ref_type = __debug_three_way_comp<_Comp>;
# else
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 082c73e672c7492..f2b60f9fad36453 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -283,9 +283,20 @@
// - `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR` -- checks any operations that exchange nodes between containers to make sure
// the containers have compatible allocators.
//
+// - `_LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN` -- checks that the given argument is within the domain of valid arguments
+// for the function. Violating this typically produces an incorrect result (e.g. the clamp algorithm returns the
+// original value without clamping it due to incorrect functors) or puts an object into an invalid state (e.g.
+// a string view where only a subset of elements is possible to access). This doesn't cause an immediate issue within
+// the library but is always a logic bug and is likely to cause problems within user code.
+// This is somewhat of a catch-all (or fallback) category -- it covers errors triggered by user input that don't have
+// a more specific category defined (which is always preferable when available).
+//
// - `_LIBCPP_ASSERT_PEDANTIC` -- checks prerequisites which are imposed by the Standard, but violating which happens to
// be benign in our implementation.
//
+// - `_LIBCPP_ASSERT_INTRUSIVE` -- for assertions that perform intrusive and typically very expensive validations of
+// user input or function results.
+//
// - `_LIBCPP_ASSERT_INTERNAL` -- checks that internal invariants of the library hold. These assertions don't depend on
// user input.
//
@@ -327,8 +338,10 @@ _LIBCPP_HARDENING_MODE_DEBUG
// Overlapping ranges will make algorithms produce incorrect results but don't directly lead to a security
// vulnerability.
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
+# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(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_INTRUSIVE(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)
@@ -341,23 +354,31 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSERT(expression, message)
# 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_ARGUMENT_WITHIN_DOMAIN(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_INTRUSIVE(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
// Debug hardening mode checks.
# elif _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
+#ifndef _LIBCPP_DEBUG_STRICT_WEAK_ORDERING_CHECK
+#define _LIBCPP_DEBUG_STRICT_WEAK_ORDERING_CHECK
+#endif
+
// All checks enabled.
# define _LIBCPP_ASSERT_VALID_INPUT_RANGE(expression, message) _LIBCPP_ASSERT(expression, message)
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSERT(expression, message)
# 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_ARGUMENT_WITHIN_DOMAIN(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_INTRUSIVE(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)
@@ -370,8 +391,10 @@ _LIBCPP_HARDENING_MODE_DEBUG
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
+# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(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_INTRUSIVE(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)
diff --git a/libcxx/include/__debug_utils/strict_weak_ordering_check.h b/libcxx/include/__debug_utils/strict_weak_ordering_check.h
index 99200ad65eac928..7082019d0c61ae4 100644
--- a/libcxx/include/__debug_utils/strict_weak_ordering_check.h
+++ b/libcxx/include/__debug_utils/strict_weak_ordering_check.h
@@ -31,7 +31,7 @@ __check_strict_weak_ordering_sorted(_RandomAccessIterator __first, _RandomAccess
using _Comp_ref = __comp_ref_type<_Comp>;
if (!__libcpp_is_constant_evaluated()) {
// Check if the range is actually sorted.
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_INTRUSIVE(
(std::is_sorted<_RandomAccessIterator, _Comp_ref>(__first, __last, _Comp_ref(__comp))),
"The range is not sorted after the sort, your comparator is not a valid strict-weak ordering");
// Limit the number of elements we need to check.
@@ -46,18 +46,18 @@ __check_strict_weak_ordering_sorted(_RandomAccessIterator __first, _RandomAccess
// Check that the elements from __p to __q are equal between each other.
for (__diff_t __b = __p; __b < __q; ++__b) {
for (__diff_t __a = __p; __a <= __b; ++__a) {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_INTRUSIVE(
!__comp(*(__first + __a), *(__first + __b)), "Your comparator is not a valid strict-weak ordering");
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_INTRUSIVE(
!__comp(*(__first + __b), *(__first + __a)), "Your comparator is not a valid strict-weak ordering");
}
}
// Check that elements between __p and __q are less than between __q and __size.
for (__diff_t __a = __p; __a < __q; ++__a) {
for (__diff_t __b = __q; __b < __size; ++__b) {
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_INTRUSIVE(
__comp(*(__first + __a), *(__first + __b)), "Your comparator is not a valid strict-weak ordering");
- _LIBCPP_ASSERT_UNCATEGORIZED(
+ _LIBCPP_ASSERT_INTRUSIVE(
!__comp(*(__first + __b), *(__first + __a)), "Your comparator is not a valid strict-weak ordering");
}
}
More information about the cfe-commits
mailing list