From 60ac394dc9ed617f802b33c3b9ac8881ca6a940c Mon Sep 17 00:00:00 2001 From: Tacet Date: Sat, 13 Jan 2024 18:11:53 +0100 Subject: [PATCH] [ASan][libc++] Annotating `std::basic_string` with all allocators (#75845) This commit turns on ASan annotations in `std::basic_string` for all allocators by default. Originally suggested here: https://reviews.llvm.org/D146214 String annotations added here: https://github.com/llvm/llvm-project/pull/72677 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. Additionally it removes `__begin != nullptr` because `data()` should never return a nullptr. Support in ASan API exists since https://github.com/llvm/llvm-project/commit/1c5ad6d2c01294a0decde43a88e9c27d7437d157. 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. The motivation for a research and those changes was a bug, found by Trail of Bits, in a real code where an out-of-bounds read could happen as two strings were compared via a call to `std::equal` that took `iter1_begin`, `iter1_end`, `iter2_begin` iterators (with a custom comparison function). When object `iter1` was longer than `iter2`, read out-of-bounds on `iter2` could happen. Container sanitization would detect it. If you have any questions, please email: - advenam.tacet@trailofbits.com - disconnect3d@trailofbits.com --- libcxx/include/string | 3 +- .../strings/basic.string/asan.pass.cpp | 56 ++++++++++ .../basic.string/asan_turning_off.pass.cpp | 102 ++++++++++++++++++ libcxx/test/support/asan_testing.h | 2 +- 4 files changed, 160 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 bdc9a6e5616fd1..e97139206d4fa7 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 (__asan_annotate_container_with_allocator::value && !__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 new file mode 100644 index 00000000000000..c7e0924be3df6f --- /dev/null +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp @@ -0,0 +1,56 @@ +//===----------------------------------------------------------------------===// +// +// 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); + 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 new file mode 100644 index 00000000000000..c8ee17c580a4c4 --- /dev/null +++ b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp @@ -0,0 +1,102 @@ +//===----------------------------------------------------------------------===// +// +// 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. +// +// 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 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 +// +// 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 +#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() { + 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 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; +}; + +int main(int, char**) { + using S = std::basic_string, user_allocator>; + + { + // Create a string with a buffer from reuse allocator object: + S* s = new (reuse_buffers.alloc()) S(); + // 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; + // 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; +} diff --git a/libcxx/test/support/asan_testing.h b/libcxx/test/support/asan_testing.h index 2dfec5c42b00b2..6bfc8280a4ead3 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