Skip to content

Commit f6a3ba6

Browse files
committed
[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.
1 parent 4f215fd commit f6a3ba6

File tree

6 files changed

+53
-29
lines changed

6 files changed

+53
-29
lines changed

libcxx/include/__algorithm/comp_ref_type.h

Lines changed: 4 additions & 5 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_ARGUMENT_WITHIN_DOMAIN(!__comp_(__l, __r), "Comparator does not induce a strict weak ordering");
4848
(void)__l;
4949
(void)__r;
5050
}
@@ -53,10 +53,9 @@ 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.
58-
#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
59-
template <class _Comp>
56+
// Pass the comparator by lvalue reference. Or in the extensive hardening mode and above, using a debugging wrapper that
57+
// stores a reference.
58+
#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_EXTENSIVE || _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
6059
using __comp_ref_type = __debug_less<_Comp>;
6160
#else
6261
template <class _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: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,17 @@ 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_ARGUMENT_WITHIN_DOMAIN(
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.
61-
# if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
60+
// Pass the comparator by lvalue reference. Or in the extensive hardening mode and above, using a debugging wrapper that
61+
// stores a reference.
62+
# if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_EXTENSIVE || \
63+
_LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
6264
template <class _Comp>
6365
using __three_way_comp_ref_type = __debug_three_way_comp<_Comp>;
6466
# else

libcxx/include/__config

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,9 +283,20 @@
283283
// - `_LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR` -- checks any operations that exchange nodes between containers to make sure
284284
// the containers have compatible allocators.
285285
//
286+
// - `_LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN` -- checks that the given argument is within the domain of valid arguments
287+
// for the function. Violating this typically produces an incorrect result (e.g. the clamp algorithm returns the
288+
// original value without clamping it due to incorrect functors) or puts an object into an invalid state (e.g.
289+
// a string view where only a subset of elements is possible to access). This doesn't cause an immediate issue within
290+
// the library but is always a logic bug and is likely to cause problems within user code.
291+
// This is somewhat of a catch-all (or fallback) category -- it covers errors triggered by user input that don't have
292+
// a more specific category defined (which is always preferable when available).
293+
//
286294
// - `_LIBCPP_ASSERT_PEDANTIC` -- checks prerequisites which are imposed by the Standard, but violating which happens to
287295
// be benign in our implementation.
288296
//
297+
// - `_LIBCPP_ASSERT_INTRUSIVE` -- for assertions that perform intrusive and typically very expensive validations of
298+
// user input or function results.
299+
//
289300
// - `_LIBCPP_ASSERT_INTERNAL` -- checks that internal invariants of the library hold. These assertions don't depend on
290301
// user input.
291302
//
@@ -327,8 +338,10 @@ _LIBCPP_HARDENING_MODE_DEBUG
327338
// Overlapping ranges will make algorithms produce incorrect results but don't directly lead to a security
328339
// vulnerability.
329340
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
341+
# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSUME(expression)
330342
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
331343
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
344+
# define _LIBCPP_ASSERT_INTRUSIVE(expression, message) _LIBCPP_ASSUME(expression)
332345
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
333346
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)
334347

@@ -341,23 +354,31 @@ _LIBCPP_HARDENING_MODE_DEBUG
341354
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSERT(expression, message)
342355
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSERT(expression, message)
343356
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
357+
# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSERT(expression, message)
344358
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
345359
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
346360
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
347361
// Disabled checks.
362+
# define _LIBCPP_ASSERT_INTRUSIVE(expression, message) _LIBCPP_ASSUME(expression)
348363
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
349364

350365
// Debug hardening mode checks.
351366

352367
# elif _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
353368

369+
#ifndef _LIBCPP_DEBUG_STRICT_WEAK_ORDERING_CHECK
370+
#define _LIBCPP_DEBUG_STRICT_WEAK_ORDERING_CHECK
371+
#endif
372+
354373
// All checks enabled.
355374
# define _LIBCPP_ASSERT_VALID_INPUT_RANGE(expression, message) _LIBCPP_ASSERT(expression, message)
356375
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSERT(expression, message)
357376
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSERT(expression, message)
358377
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSERT(expression, message)
378+
# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSERT(expression, message)
359379
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSERT(expression, message)
360380
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSERT(expression, message)
381+
# define _LIBCPP_ASSERT_INTRUSIVE(expression, message) _LIBCPP_ASSERT(expression, message)
361382
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSERT(expression, message)
362383
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
363384

@@ -370,8 +391,10 @@ _LIBCPP_HARDENING_MODE_DEBUG
370391
# define _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(expression, message) _LIBCPP_ASSUME(expression)
371392
# define _LIBCPP_ASSERT_NON_NULL(expression, message) _LIBCPP_ASSUME(expression)
372393
# define _LIBCPP_ASSERT_NON_OVERLAPPING_RANGES(expression, message) _LIBCPP_ASSUME(expression)
394+
# define _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(expression, message) _LIBCPP_ASSUME(expression)
373395
# define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message) _LIBCPP_ASSUME(expression)
374396
# define _LIBCPP_ASSERT_PEDANTIC(expression, message) _LIBCPP_ASSUME(expression)
397+
# define _LIBCPP_ASSERT_INTRUSIVE(expression, message) _LIBCPP_ASSUME(expression)
375398
# define _LIBCPP_ASSERT_INTERNAL(expression, message) _LIBCPP_ASSUME(expression)
376399
# define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSUME(expression)
377400

libcxx/include/__debug_utils/strict_weak_ordering_check.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ __check_strict_weak_ordering_sorted(_RandomAccessIterator __first, _RandomAccess
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_INTRUSIVE(
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_INTRUSIVE(
5050
!__comp(*(__first + __a), *(__first + __b)), "Your comparator is not a valid strict-weak ordering");
51-
_LIBCPP_ASSERT_UNCATEGORIZED(
51+
_LIBCPP_ASSERT_INTRUSIVE(
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_INTRUSIVE(
5959
__comp(*(__first + __a), *(__first + __b)), "Your comparator is not a valid strict-weak ordering");
60-
_LIBCPP_ASSERT_UNCATEGORIZED(
60+
_LIBCPP_ASSERT_INTRUSIVE(
6161
!__comp(*(__first + __b), *(__first + __a)), "Your comparator is not a valid strict-weak ordering");
6262
}
6363
}

0 commit comments

Comments
 (0)