From 7af271d66472e81cedfe077df5a3dbb50b9ab302 Mon Sep 17 00:00:00 2001 From: Advenam Tacet Date: Mon, 18 Dec 2023 20:34:49 +0100 Subject: [PATCH 1/7] [ASan][libc++] Annotating `std::basic_string` with all allocators This commit turns on ASan annotations in `std::basic_string` for all allocators by default. Originally suggested here: https://reviews.llvm.org/D146214 This commit is part of our efforts to support container annotations with (almost) every allocator. Annotating `std::basic_string` with default allocator is implemented in https://github.com/llvm/llvm-project/pull/72677. Support in ASan API exests since https://github.com/llvm/llvm-project/commit/dd1b7b797a116eed588fd752fbe61d34deeb24e4. This patch removes the check in std::basic_string annotation member function (__annotate_contiguous_container) to support different allocators. You can turn off annotations for a specific allocator based on changes from https://github.com/llvm/llvm-project/commit/2fa1bec7a20bb23f2e6620085adb257dafaa3be0. This PR is a part of a series of patches extending AddressSanitizer C++ container overflow detection capabilities by adding annotations, similar to those existing in `std::vector` and `std::deque` collections. These enhancements empower ASan to effectively detect instances where the instrumented program attempts to access memory within a collection's internal allocation that remains unused. This includes cases where access occurs before or after the stored elements in `std::deque`, or between the `std::basic_string`'s size (including the null terminator) and capacity bounds. The introduction of these annotations was spurred by a real-world software bug discovered by Trail of Bits, involving an out-of-bounds memory access during the comparison of two strings using the `std::equals` function. This function was taking iterators (`iter1_begin`, `iter1_end`, `iter2_begin`) to perform the comparison, using a custom comparison function. When the `iter1` object exceeded the length of `iter2`, an out-of-bounds read could occur on the `iter2` object. Container sanitization, upon enabling these annotations, would effectively identify and flag this potential vulnerability. If you have any questions, please email: - advenam.tacet@trailofbits.com - disconnect3d@trailofbits.com --- libcxx/include/string | 3 +- .../strings/basic.string/asan.pass.cpp | 54 +++++++++++++ .../basic.string/asan_turning_off.pass.cpp | 78 +++++++++++++++++++ libcxx/test/support/asan_testing.h | 2 +- 4 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp create mode 100644 libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp diff --git a/libcxx/include/string b/libcxx/include/string index c676182fba8ba..aca396d06c062 100644 --- a/libcxx/include/string +++ b/libcxx/include/string @@ -1891,8 +1891,7 @@ private: #if !defined(_LIBCPP_HAS_NO_ASAN) && defined(_LIBCPP_INSTRUMENTED_WITH_ASAN) const void* __begin = data(); const void* __end = data() + capacity() + 1; - if (!__libcpp_is_constant_evaluated() && __begin != nullptr && - is_same::value) + if (!__libcpp_is_constant_evaluated() && __asan_annotate_container_with_allocator::value) __sanitizer_annotate_contiguous_container(__begin, __end, __old_mid, __new_mid); #endif } diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp new file mode 100644 index 0000000000000..b578b0fadffcd --- /dev/null +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp @@ -0,0 +1,54 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// REQUIRES: asan +// UNSUPPORTED: c++03 + +// Basic test if ASan annotations work for basic_string. + +#include +#include +#include + +#include "asan_testing.h" +#include "min_allocator.h" +#include "test_iterators.h" +#include "test_macros.h" + +extern "C" void __sanitizer_set_death_callback(void (*callback)(void)); + +void do_exit() { exit(0); } + +int main(int, char**) { + { + typedef cpp17_input_iterator MyInputIter; + // Should not trigger ASan. + std::basic_string, safe_allocator> v; + char i[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'a', 'b', 'c', 'd', 'e', + 'f', 'g', 'h', 'i', 'j', 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'}; + + v.insert(v.begin(), MyInputIter(i), MyInputIter(i + 29)); + assert(v[0] == 'a'); + assert(is_string_asan_correct(v)); + } + + __sanitizer_set_death_callback(do_exit); + { + using T = char; + using C = std::basic_string, safe_allocator>; + const T t[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'a', 'b', 'c', 'd', 'e', + 'f', 'g', 'h', 'i', 'j', 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'}; + C c(std::begin(t), std::end(t)); + assert(is_string_asan_correct(c)); + assert(__sanitizer_verify_contiguous_container(c.data(), c.data() + c.size() + 1, c.data() + c.capacity() + 1) != + 0); + volatile T foo = c[c.size() + 1]; // should trigger ASAN. Use volatile to prevent being optimized away. + assert(false); // if we got here, ASAN didn't trigger + ((void)foo); + } +} diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp new file mode 100644 index 0000000000000..4e12e3c86248d --- /dev/null +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp @@ -0,0 +1,78 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// REQUIRES: asan +// UNSUPPORTED: c++03 + +// + +// Test based on: https://bugs.chromium.org/p/chromium/issues/detail?id=1419798#c5 +// Some allocators during deallocation may not call destructors and just reuse memory. +// In those situations, one may want to deactivate annotations for a specific allocator. +// It's possible with __asan_annotate_container_with_allocator template class. +// This test confirms that those allocators work after turning off annotations. + +#include +#include +#include +#include + +struct reuse_allocator { + static size_t const N = 100; + reuse_allocator() { + for (size_t i = 0; i < N; ++i) + __buffers[i] = malloc(8 * 1024); + } + ~reuse_allocator() { + for (size_t i = 0; i < N; ++i) + free(__buffers[i]); + } + void* alloc() { + assert(__next_id < N); + return __buffers[__next_id++]; + } + void reset() { __next_id = 0; } + void* __buffers[N]; + size_t __next_id = 0; +} reuse_buffers; + +template +struct user_allocator { + using value_type = T; + user_allocator() = default; + template + user_allocator(user_allocator) {} + friend bool operator==(user_allocator, user_allocator) { return true; } + friend bool operator!=(user_allocator x, user_allocator y) { return !(x == y); } + + T* allocate(size_t) { return (T*)reuse_buffers.alloc(); } + void deallocate(T*, size_t) noexcept {} +}; + +template +struct std::__asan_annotate_container_with_allocator> { + static bool const value = false; +}; + +int main() { + using S = std::basic_string, user_allocator>; + + { + S* s = new (reuse_buffers.alloc()) S(); + for (int i = 0; i < 100; i++) + s->push_back('a'); + } + reuse_buffers.reset(); + { + S s; + for (int i = 0; i < 1000; i++) + s.push_back('b'); + } + + return 0; +} diff --git a/libcxx/test/support/asan_testing.h b/libcxx/test/support/asan_testing.h index 2dfec5c42b00b..6bfc8280a4ead 100644 --- a/libcxx/test/support/asan_testing.h +++ b/libcxx/test/support/asan_testing.h @@ -75,7 +75,7 @@ TEST_CONSTEXPR bool is_string_asan_correct(const std::basic_string>::value) + if (std::__asan_annotate_container_with_allocator::value) return __sanitizer_verify_contiguous_container(c.data(), c.data() + c.size() + 1, c.data() + c.capacity() + 1) != 0; else From 1eea92e6a18bcd633d130ce942186fa58aae37d7 Mon Sep 17 00:00:00 2001 From: Advenam Tacet Date: Wed, 27 Dec 2023 10:54:36 +0100 Subject: [PATCH 2/7] Code review from EricWF This commit addresses some comments from a code review: - add size check, - add coments with context to a test, - add comments explaining what happens in the test, - remove volatile, - split an if to constexpr if and normal if. --- libcxx/include/string | 5 +-- .../strings/basic.string/asan.pass.cpp | 4 +-- .../basic.string/asan_turning_off.pass.cpp | 32 ++++++++++++++++--- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/libcxx/include/string b/libcxx/include/string index aca396d06c062..204acab97b1f4 100644 --- a/libcxx/include/string +++ b/libcxx/include/string @@ -1891,8 +1891,9 @@ private: #if !defined(_LIBCPP_HAS_NO_ASAN) && defined(_LIBCPP_INSTRUMENTED_WITH_ASAN) const void* __begin = data(); const void* __end = data() + capacity() + 1; - if (!__libcpp_is_constant_evaluated() && __asan_annotate_container_with_allocator::value) - __sanitizer_annotate_contiguous_container(__begin, __end, __old_mid, __new_mid); + if _LIBCPP_CONSTEXPR_SINCE_CXX17 (__asan_annotate_container_with_allocator::value) + if(!__libcpp_is_constant_evaluated()) + __sanitizer_annotate_contiguous_container(__begin, __end, __old_mid, __new_mid); #endif } diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp index b578b0fadffcd..88570a0e7daa7 100644 --- a/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp @@ -47,8 +47,8 @@ int main(int, char**) { assert(is_string_asan_correct(c)); assert(__sanitizer_verify_contiguous_container(c.data(), c.data() + c.size() + 1, c.data() + c.capacity() + 1) != 0); - volatile T foo = c[c.size() + 1]; // should trigger ASAN. Use volatile to prevent being optimized away. - assert(false); // if we got here, ASAN didn't trigger + T foo = c[c.size() + 1]; // should trigger ASAN and call do_exit(). + assert(false); // if we got here, ASAN didn't trigger ((void)foo); } } diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp index 4e12e3c86248d..fd1785b3a4d58 100644 --- a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp @@ -16,12 +16,26 @@ // In those situations, one may want to deactivate annotations for a specific allocator. // It's possible with __asan_annotate_container_with_allocator template class. // This test confirms that those allocators work after turning off annotations. +// +// A context to this test is a situations when memory is repurposed and destructors are not called. +// Related issue: https://github.com/llvm/llvm-project/issues/60384 +// +// That issue appeared in the past and was addressed here: https://reviews.llvm.org/D145628 +// +// There is also a question, if it's UB. And it's not clear. +// Related discussion: https://reviews.llvm.org/D136765#4155262 +// Related notes: https://eel.is/c++draft/basic.life#6 +// +// Even if it is UB, we want to make sure that it works that way, because people rely on this behavior. +// In similar situations. a user explicitly turns off annotations for a specific allocator. #include #include #include #include +// Allocator with pre-allocated (with malloc in constructor) buffers. +// Memory may be freed without calling destructors. struct reuse_allocator { static size_t const N = 100; reuse_allocator() { @@ -50,10 +64,15 @@ struct user_allocator { friend bool operator==(user_allocator, user_allocator) { return true; } friend bool operator!=(user_allocator x, user_allocator y) { return !(x == y); } - T* allocate(size_t) { return (T*)reuse_buffers.alloc(); } + T* allocate(size_t n) { + if (n * sizeof(T) > 8 * 1024) + throw std::bad_array_new_length(); + return (T*)reuse_buffers.alloc(); + } void deallocate(T*, size_t) noexcept {} }; +// Turn off annotations for user_allocator: template struct std::__asan_annotate_container_with_allocator> { static bool const value = false; @@ -63,15 +82,20 @@ int main() { using S = std::basic_string, user_allocator>; { + // Create a string with a buffer from reuse allocator object: S* s = new (reuse_buffers.alloc()) S(); - for (int i = 0; i < 100; i++) + // Use string, so it's poisoned, if container annotations for that allocator are not turned off: + for (int i = 0; i < 40; i++) s->push_back('a'); } + // Reset the state of the allocator, don't call destructors, allow memory to be reused: reuse_buffers.reset(); { + // Create a next string with the same allocator, so the same buffer due to the reset: S s; - for (int i = 0; i < 1000; i++) - s.push_back('b'); + // Use memory inside the string again, if it's poisoned, an error will be raised: + for (int i = 0; i < 60; i++) + s.push_back('a'); } return 0; From 428a15f3bfb7e27abbb29f8c9dd100ed04ff84b1 Mon Sep 17 00:00:00 2001 From: Advenam Tacet Date: Wed, 10 Jan 2024 01:55:21 +0100 Subject: [PATCH 3/7] Remove constexpr magic --- libcxx/include/string | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libcxx/include/string b/libcxx/include/string index 204acab97b1f4..e62a9268190f6 100644 --- a/libcxx/include/string +++ b/libcxx/include/string @@ -1891,9 +1891,8 @@ private: #if !defined(_LIBCPP_HAS_NO_ASAN) && defined(_LIBCPP_INSTRUMENTED_WITH_ASAN) const void* __begin = data(); const void* __end = data() + capacity() + 1; - if _LIBCPP_CONSTEXPR_SINCE_CXX17 (__asan_annotate_container_with_allocator::value) - if(!__libcpp_is_constant_evaluated()) - __sanitizer_annotate_contiguous_container(__begin, __end, __old_mid, __new_mid); + if (__asan_annotate_container_with_allocator::value && !__libcpp_is_constant_evaluated()) + __sanitizer_annotate_contiguous_container(__begin, __end, __old_mid, __new_mid); #endif } From a73df63c89efbe266226213172d3f6cd6220b791 Mon Sep 17 00:00:00 2001 From: Advenam Tacet Date: Fri, 12 Jan 2024 02:15:33 +0100 Subject: [PATCH 4/7] Mention CWG2523 --- .../strings/basic.string/asan_turning_off.pass.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp index fd1785b3a4d58..55ad6b1abfa9d 100644 --- a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp @@ -22,12 +22,14 @@ // // That issue appeared in the past and was addressed here: https://reviews.llvm.org/D145628 // -// There is also a question, if it's UB. And it's not clear. +// There was also a discussion, if it's UB. // Related discussion: https://reviews.llvm.org/D136765#4155262 // Related notes: https://eel.is/c++draft/basic.life#6 +// Probably it's no longer UB due a change in CWG2523. +// https://cplusplus.github.io/CWG/issues/2523.html // -// Even if it is UB, we want to make sure that it works that way, because people rely on this behavior. -// In similar situations. a user explicitly turns off annotations for a specific allocator. +// Therefore we make sure that it works that way, also because people rely on this behavior. +// Annotations are turned off only, if a user explicitly turns off annotations for a specific allocator. #include #include From d6a2fcc4728f4da8f99d622b38a7fc5157bcf1dc Mon Sep 17 00:00:00 2001 From: Tacet <4922191+AdvenamTacet@users.noreply.github.com> Date: Sat, 13 Jan 2024 00:24:59 +0100 Subject: [PATCH 5/7] Remove a comment Co-authored-by: Louis Dionne --- .../containers/strings/basic.string/asan_turning_off.pass.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp index 55ad6b1abfa9d..f448d2be0fd73 100644 --- a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp @@ -9,7 +9,6 @@ // REQUIRES: asan // UNSUPPORTED: c++03 -// // Test based on: https://bugs.chromium.org/p/chromium/issues/detail?id=1419798#c5 // Some allocators during deallocation may not call destructors and just reuse memory. From 55980f59b28b393dae16e0d21413bf2e56ba6605 Mon Sep 17 00:00:00 2001 From: Tacet <4922191+AdvenamTacet@users.noreply.github.com> Date: Sat, 13 Jan 2024 00:25:23 +0100 Subject: [PATCH 6/7] Add arguments to main Co-authored-by: Louis Dionne --- .../containers/strings/basic.string/asan_turning_off.pass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp index f448d2be0fd73..c2635051f9d49 100644 --- a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp @@ -79,7 +79,7 @@ struct std::__asan_annotate_container_with_allocator> { static bool const value = false; }; -int main() { +int main(int, char**) { using S = std::basic_string, user_allocator>; { From 200aaf26c571ab1eaa6a873cf499ee88d11ce2f1 Mon Sep 17 00:00:00 2001 From: Advenam Tacet Date: Sat, 13 Jan 2024 00:30:27 +0100 Subject: [PATCH 7/7] Add return to main Also refactor one line. --- .../test/libcxx/containers/strings/basic.string/asan.pass.cpp | 2 ++ .../containers/strings/basic.string/asan_turning_off.pass.cpp | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp index 88570a0e7daa7..c7e0924be3df6 100644 --- a/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp @@ -50,5 +50,7 @@ int main(int, char**) { T foo = c[c.size() + 1]; // should trigger ASAN and call do_exit(). assert(false); // if we got here, ASAN didn't trigger ((void)foo); + + return 0; } } diff --git a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp index c2635051f9d49..c8ee17c580a4c 100644 --- a/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp @@ -9,7 +9,6 @@ // REQUIRES: asan // UNSUPPORTED: c++03 - // Test based on: https://bugs.chromium.org/p/chromium/issues/detail?id=1419798#c5 // Some allocators during deallocation may not call destructors and just reuse memory. // In those situations, one may want to deactivate annotations for a specific allocator.