Skip to content

Commit 8938bc0

Browse files
authored
[libc++][hardening] Categorize assertions related to strict weak ordering (#77405)
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, the algorithm 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 [the related RFC](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: - 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 incorrectly-sorted sequence without actually triggering 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). The check is expensive but the 100 element limit can somewhat compensate for that, especially for large values of `N`. The first set of checks is enabled in the fast hardening mode while the other two are only enabled in the debug mode. This patch also removes the `_LIBCPP_DEBUG_STRICT_WEAK_ORDERING_CHECK` macro that previously was used to selectively enable the 100-element check. Now this check is enabled unconditionally in the debug mode. Also, introduce a new category `_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT`. This category is intended for checking the semantic requirements from the Standard. Typically, these are hard or impossible to completely validate, so these checks are expected to be heuristic in nature and potentially quite expensive. See https://reviews.llvm.org/D150264 for additional background. Fixes #71496
1 parent 9e2c0f0 commit 8938bc0

File tree

11 files changed

+390
-286
lines changed

11 files changed

+390
-286
lines changed

libcxx/include/__algorithm/comp_ref_type.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ struct __debug_less {
4444
_LIBCPP_CONSTEXPR_SINCE_CXX14 inline _LIBCPP_HIDE_FROM_ABI decltype((void)std::declval<_Compare&>()(
4545
std::declval<_LHS&>(), std::declval<_RHS&>()))
4646
__do_compare_assert(int, _LHS& __l, _RHS& __r) {
47-
_LIBCPP_ASSERT_UNCATEGORIZED(!__comp_(__l, __r), "Comparator does not induce a strict weak ordering");
47+
_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(!__comp_(__l, __r), "Comparator does not induce a strict weak ordering");
4848
(void)__l;
4949
(void)__r;
5050
}
@@ -53,8 +53,7 @@ struct __debug_less {
5353
_LIBCPP_CONSTEXPR_SINCE_CXX14 inline _LIBCPP_HIDE_FROM_ABI void __do_compare_assert(long, _LHS&, _RHS&) {}
5454
};
5555

56-
// Pass the comparator by lvalue reference. Or in debug mode, using a
57-
// debugging wrapper that stores a reference.
56+
// Pass the comparator by lvalue reference. Or in the debug mode, using a debugging wrapper that stores a reference.
5857
#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
5958
template <class _Comp>
6059
using __comp_ref_type = __debug_less<_Comp>;

libcxx/include/__algorithm/nth_element.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,12 +114,12 @@ __nth_element(
114114
while (true) {
115115
while (!__comp(*__first, *__i)) {
116116
++__i;
117-
_LIBCPP_ASSERT_UNCATEGORIZED(
117+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
118118
__i != __last,
119119
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
120120
}
121121
do {
122-
_LIBCPP_ASSERT_UNCATEGORIZED(
122+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
123123
__j != __first,
124124
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
125125
--__j;
@@ -150,13 +150,13 @@ __nth_element(
150150
// __m still guards upward moving __i
151151
while (__comp(*__i, *__m)) {
152152
++__i;
153-
_LIBCPP_ASSERT_UNCATEGORIZED(
153+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
154154
__i != __last,
155155
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
156156
}
157157
// It is now known that a guard exists for downward moving __j
158158
do {
159-
_LIBCPP_ASSERT_UNCATEGORIZED(
159+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
160160
__j != __first,
161161
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
162162
--__j;

libcxx/include/__algorithm/sort.h

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ __insertion_sort_unguarded(_RandomAccessIterator const __first, _RandomAccessIte
325325
do {
326326
*__j = _Ops::__iter_move(__k);
327327
__j = __k;
328-
_LIBCPP_ASSERT_UNCATEGORIZED(
328+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
329329
__k != __leftmost,
330330
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
331331
} 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,
544544
// Not guarded since we know the last element is greater than the pivot.
545545
do {
546546
++__first;
547-
_LIBCPP_ASSERT_UNCATEGORIZED(
547+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
548548
__first != __end,
549549
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
550550
} while (!__comp(__pivot, *__first));
@@ -557,7 +557,7 @@ __bitset_partition(_RandomAccessIterator __first, _RandomAccessIterator __last,
557557
// It will be always guarded because __introsort will do the median-of-three
558558
// before calling this.
559559
do {
560-
_LIBCPP_ASSERT_UNCATEGORIZED(
560+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
561561
__last != __begin,
562562
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
563563
--__last;
@@ -635,7 +635,7 @@ __partition_with_equals_on_right(_RandomAccessIterator __first, _RandomAccessIte
635635
// this.
636636
do {
637637
++__first;
638-
_LIBCPP_ASSERT_UNCATEGORIZED(
638+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
639639
__first != __end,
640640
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
641641
} while (__comp(*__first, __pivot));
@@ -647,7 +647,7 @@ __partition_with_equals_on_right(_RandomAccessIterator __first, _RandomAccessIte
647647
} else {
648648
// Guarded.
649649
do {
650-
_LIBCPP_ASSERT_UNCATEGORIZED(
650+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
651651
__last != __begin,
652652
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
653653
--__last;
@@ -665,12 +665,12 @@ __partition_with_equals_on_right(_RandomAccessIterator __first, _RandomAccessIte
665665
_Ops::iter_swap(__first, __last);
666666
do {
667667
++__first;
668-
_LIBCPP_ASSERT_UNCATEGORIZED(
668+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
669669
__first != __end,
670670
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
671671
} while (__comp(*__first, __pivot));
672672
do {
673-
_LIBCPP_ASSERT_UNCATEGORIZED(
673+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
674674
__last != __begin,
675675
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
676676
--__last;
@@ -702,7 +702,7 @@ __partition_with_equals_on_left(_RandomAccessIterator __first, _RandomAccessIter
702702
// Guarded.
703703
do {
704704
++__first;
705-
_LIBCPP_ASSERT_UNCATEGORIZED(
705+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
706706
__first != __end,
707707
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
708708
} while (!__comp(__pivot, *__first));
@@ -715,7 +715,7 @@ __partition_with_equals_on_left(_RandomAccessIterator __first, _RandomAccessIter
715715
// It will be always guarded because __introsort will do the
716716
// median-of-three before calling this.
717717
do {
718-
_LIBCPP_ASSERT_UNCATEGORIZED(
718+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
719719
__last != __begin,
720720
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
721721
--__last;
@@ -725,12 +725,12 @@ __partition_with_equals_on_left(_RandomAccessIterator __first, _RandomAccessIter
725725
_Ops::iter_swap(__first, __last);
726726
do {
727727
++__first;
728-
_LIBCPP_ASSERT_UNCATEGORIZED(
728+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
729729
__first != __end,
730730
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
731731
} while (!__comp(__pivot, *__first));
732732
do {
733-
_LIBCPP_ASSERT_UNCATEGORIZED(
733+
_LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(
734734
__last != __begin,
735735
"Would read out of bounds, does your comparator satisfy the strict-weak ordering requirement?");
736736
--__last;

libcxx/include/__algorithm/three_way_comp_ref_type.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,14 +50,14 @@ struct __debug_three_way_comp {
5050
__expected = _Order::greater;
5151
if (__o == _Order::greater)
5252
__expected = _Order::less;
53-
_LIBCPP_ASSERT_UNCATEGORIZED(__comp_(__l, __r) == __expected, "Comparator does not induce a strict weak ordering");
53+
_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(
54+
__comp_(__l, __r) == __expected, "Comparator does not induce a strict weak ordering");
5455
(void)__l;
5556
(void)__r;
5657
}
5758
};
5859

59-
// Pass the comparator by lvalue reference. Or in debug mode, using a
60-
// debugging wrapper that stores a reference.
60+
// Pass the comparator by lvalue reference. Or in the debug mode, using a debugging wrapper that stores a reference.
6161
# if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
6262
template <class _Comp>
6363
using __three_way_comp_ref_type = __debug_three_way_comp<_Comp>;

libcxx/include/__config

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,10 @@
313313
// - `_LIBCPP_ASSERT_PEDANTIC` -- checks prerequisites which are imposed by the Standard, but violating which happens to
314314
// be benign in our implementation.
315315
//
316+
// - `_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT` -- checks that the given argument satisfies the semantic requirements imposed
317+
// by the Standard. Typically, there is no simple way to completely prove that a semantic requirement is satisfied;
318+
// thus, this would often be a heuristic check and it might be quite expensive.
319+
//
316320
// - `_LIBCPP_ASSERT_INTERNAL` -- checks that internal invariants of the library hold. These assertions don't depend on
317321
// user input.
318322
//
@@ -359,6 +363,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
359363
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
360364
# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSUME(expression)
361365
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
366+
# define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message) _LIBCPP_ASSUME(expression)
362367
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
363368
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)
364369

@@ -378,6 +383,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
378383
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
379384
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
380385
// Disabled checks.
386+
# define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message) _LIBCPP_ASSUME(expression)
381387
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
382388

383389
// Debug hardening mode checks.
@@ -394,6 +400,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
394400
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
395401
# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSERT(expression, message)
396402
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
403+
# define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message) _LIBCPP_ASSERT(expression, message)
397404
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSERT(expression, message)
398405
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
399406

@@ -411,6 +418,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
411418
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
412419
# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSUME(expression)
413420
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
421+
# define _LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(expression, message) _LIBCPP_ASSUME(expression)
414422
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
415423
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)
416424

libcxx/include/__debug_utils/strict_weak_ordering_check.h

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,12 @@ _LIBCPP_BEGIN_NAMESPACE_STD
2626
template <class _RandomAccessIterator, class _Comp>
2727
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
2828
__check_strict_weak_ordering_sorted(_RandomAccessIterator __first, _RandomAccessIterator __last, _Comp& __comp) {
29-
#ifdef _LIBCPP_DEBUG_STRICT_WEAK_ORDERING_CHECK
29+
#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
3030
using __diff_t = __iter_diff_t<_RandomAccessIterator>;
3131
using _Comp_ref = __comp_ref_type<_Comp>;
3232
if (!__libcpp_is_constant_evaluated()) {
3333
// Check if the range is actually sorted.
34-
_LIBCPP_ASSERT_UNCATEGORIZED(
34+
_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(
3535
(std::is_sorted<_RandomAccessIterator, _Comp_ref>(__first, __last, _Comp_ref(__comp))),
3636
"The range is not sorted after the sort, your comparator is not a valid strict-weak ordering");
3737
// Limit the number of elements we need to check.
@@ -46,18 +46,18 @@ __check_strict_weak_ordering_sorted(_RandomAccessIterator __first, _RandomAccess
4646
// Check that the elements from __p to __q are equal between each other.
4747
for (__diff_t __b = __p; __b < __q; ++__b) {
4848
for (__diff_t __a = __p; __a <= __b; ++__a) {
49-
_LIBCPP_ASSERT_UNCATEGORIZED(
49+
_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(
5050
!__comp(*(__first + __a), *(__first + __b)), "Your comparator is not a valid strict-weak ordering");
51-
_LIBCPP_ASSERT_UNCATEGORIZED(
51+
_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(
5252
!__comp(*(__first + __b), *(__first + __a)), "Your comparator is not a valid strict-weak ordering");
5353
}
5454
}
5555
// Check that elements between __p and __q are less than between __q and __size.
5656
for (__diff_t __a = __p; __a < __q; ++__a) {
5757
for (__diff_t __b = __q; __b < __size; ++__b) {
58-
_LIBCPP_ASSERT_UNCATEGORIZED(
58+
_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(
5959
__comp(*(__first + __a), *(__first + __b)), "Your comparator is not a valid strict-weak ordering");
60-
_LIBCPP_ASSERT_UNCATEGORIZED(
60+
_LIBCPP_ASSERT_SEMANTIC_REQUIREMENT(
6161
!__comp(*(__first + __b), *(__first + __a)), "Your comparator is not a valid strict-weak ordering");
6262
}
6363
}
@@ -69,7 +69,7 @@ __check_strict_weak_ordering_sorted(_RandomAccessIterator __first, _RandomAccess
6969
(void)__first;
7070
(void)__last;
7171
(void)__comp;
72-
#endif
72+
#endif // _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
7373
}
7474

7575
_LIBCPP_END_NAMESPACE_STD

0 commit comments

Comments
 (0)