From 26210804e5a7cf04b296048357b5636780648318 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Mon, 14 Aug 2023 16:38:41 -0700 Subject: [PATCH 1/2] Avoid `memcmp(x, y, -1)` with ASan ASan considers calls to `memcmp` with size `-1` to indicate bugs. These two test cases are the exception that proves the rule. Drive by: Pull non-dependent test cases out of the `instantiator::call` template magic. Fixes VSO-1854238 / AB#1854238 --- .../test.cpp | 74 ++++++++++++------- 1 file changed, 47 insertions(+), 27 deletions(-) diff --git a/tests/std/tests/P0896R4_ranges_alg_lexicographical_compare/test.cpp b/tests/std/tests/P0896R4_ranges_alg_lexicographical_compare/test.cpp index fa4ec994a72..def97cdfae0 100644 --- a/tests/std/tests/P0896R4_ranges_alg_lexicographical_compare/test.cpp +++ b/tests/std/tests/P0896R4_ranges_alg_lexicographical_compare/test.cpp @@ -194,40 +194,58 @@ struct instantiator { empty1.begin(), empty1.end(), empty2.begin(), empty2.end(), less{}, get_first, get_second); assert(!result); } - { // Validate memcmp case - unsigned char arr1[3]{0, 1, 2}; - unsigned char arr2[3]{0, 1, 3}; - assert(lexicographical_compare(arr1, arr2)); - arr2[2] = 2; - assert(!lexicographical_compare(arr1, arr2)); - arr2[2] = 1; - assert(!lexicographical_compare(arr1, arr2)); - } - { // Validate memcmp + unreachable_sentinel cases - unsigned char arr1[3]{0, 1, 2}; - unsigned char arr2[3]{0, 1, 3}; - - assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr1), unreachable_sentinel)); - assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr1), end(arr1))); - - assert(lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), unreachable_sentinel)); - assert(lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); - assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); - arr2[2] = 2; - assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); - assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); - arr2[2] = 1; - assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), unreachable_sentinel)); - assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); - assert(!lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); - } } }; +constexpr void concrete_tests() { + using ranges::lexicographical_compare, ranges::begin, ranges::end; + + { // Validate memcmp case + unsigned char arr1[3]{0, 1, 2}; + unsigned char arr2[3]{0, 1, 3}; + assert(lexicographical_compare(arr1, arr2)); + arr2[2] = 2; + assert(!lexicographical_compare(arr1, arr2)); + arr2[2] = 1; + assert(!lexicographical_compare(arr1, arr2)); + } + { // Validate memcmp + unreachable_sentinel cases + unsigned char arr1[3]{0, 1, 2}; + unsigned char arr2[3]{0, 1, 3}; + + assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr1), unreachable_sentinel)); + assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr1), end(arr1))); + +#ifndef __SANITIZE_ADDRESS__ + // This test case results in a call to memcmp(arr1, arr2, ~size_t{0}). ASan wisely considers calls + // with size -1 to indicate bugs. While this particular occurrence is a false positive, it's hard + // to argue that this heuristic doesn't find more true positives than false positives, so I think + // we want to leave ASan unchanged. + assert(lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), unreachable_sentinel)); +#endif // __SANITIZE_ADDRESS__ + assert(lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); + assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); + arr2[2] = 2; + assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); + assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); + arr2[2] = 1; +#ifndef __SANITIZE_ADDRESS__ + // This test case results in a call to memcmp(arr1, arr2, ~size_t{0}). ASan wisely considers calls + // with size -1 to indicate bugs. While this particular occurrence is a false positive, it's hard + // to argue that this heuristic doesn't find more true positives than false positives, so I think + // we want to leave ASan unchanged. + assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), unreachable_sentinel)); +#endif // __SANITIZE_ADDRESS__ + assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); + assert(!lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); + } +} + #ifdef TEST_EVERYTHING int main() { // No constexpr tests here: we hit the constexpr step limits too quickly. test_in_in(); + concrete_tests(); } #else // ^^^ test all range combinations / test only interesting combinations vvv template @@ -243,6 +261,8 @@ constexpr void run_tests() { instantiator::call, range_type>(); instantiator::call, range_type>(); instantiator::call, range_type>(); + + concrete_tests(); } int main() { From 6e099dbfe38c0119dbbfbc0f225cbf0048dee68e Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Fri, 25 Aug 2023 09:50:36 -0700 Subject: [PATCH 2/2] Don't call `memcmp(x,y,-1)` with two "infinite" ranges from `lexicographical_compare` --- stl/inc/algorithm | 2 +- .../test.cpp | 12 ------------ 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/stl/inc/algorithm b/stl/inc/algorithm index 29579e712ad..e34578c17fa 100644 --- a/stl/inc/algorithm +++ b/stl/inc/algorithm @@ -10654,7 +10654,7 @@ namespace ranges { constexpr bool _Is_sized2 = sized_sentinel_for<_Se2, _It2>; if constexpr (!is_void_v<_Memcmp_classification_pred> && _Sized_or_unreachable_sentinel_for<_Se1, _It1> && _Sized_or_unreachable_sentinel_for<_Se2, _It2> && same_as<_Pj1, identity> - && same_as<_Pj2, identity>) { + && same_as<_Pj2, identity> && (_Is_sized1 || _Is_sized2)) { if (!_STD is_constant_evaluated()) { size_t _Num1; if constexpr (_Is_sized1) { diff --git a/tests/std/tests/P0896R4_ranges_alg_lexicographical_compare/test.cpp b/tests/std/tests/P0896R4_ranges_alg_lexicographical_compare/test.cpp index def97cdfae0..f3a775346a6 100644 --- a/tests/std/tests/P0896R4_ranges_alg_lexicographical_compare/test.cpp +++ b/tests/std/tests/P0896R4_ranges_alg_lexicographical_compare/test.cpp @@ -216,26 +216,14 @@ constexpr void concrete_tests() { assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr1), unreachable_sentinel)); assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr1), end(arr1))); -#ifndef __SANITIZE_ADDRESS__ - // This test case results in a call to memcmp(arr1, arr2, ~size_t{0}). ASan wisely considers calls - // with size -1 to indicate bugs. While this particular occurrence is a false positive, it's hard - // to argue that this heuristic doesn't find more true positives than false positives, so I think - // we want to leave ASan unchanged. assert(lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), unreachable_sentinel)); -#endif // __SANITIZE_ADDRESS__ assert(lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); arr2[2] = 2; assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); assert(lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); arr2[2] = 1; -#ifndef __SANITIZE_ADDRESS__ - // This test case results in a call to memcmp(arr1, arr2, ~size_t{0}). ASan wisely considers calls - // with size -1 to indicate bugs. While this particular occurrence is a false positive, it's hard - // to argue that this heuristic doesn't find more true positives than false positives, so I think - // we want to leave ASan unchanged. assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), unreachable_sentinel)); -#endif // __SANITIZE_ADDRESS__ assert(!lexicographical_compare(begin(arr1), unreachable_sentinel, begin(arr2), end(arr2))); assert(!lexicographical_compare(begin(arr1), end(arr1), begin(arr2), unreachable_sentinel)); }