[libc++] Honor __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ in libc++#168955
[libc++] Honor __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ in libc++#168955
Conversation
Address sanitizer recently got a new macro __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ which is intended to disable container overflow checks in libraries (either the standard library or user libraries that might provide such checks). This patch makes libc++ honor that macro and, in addition, cleans up how these checks are enabled for string (which is special) by introducing a macro just for string.
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesAddress sanitizer recently got a new macro SANITIZER_DISABLE_CONTAINER_OVERFLOW which is intended to disable container overflow checks in libraries (either the standard library or user libraries that might provide such checks). This patch makes libc++ honor that macro and, in addition, cleans up how these checks are enabled for string (which is special) by introducing a macro just for string. Full diff: https://github.com/llvm/llvm-project/pull/168955.diff 5 Files Affected:
diff --git a/libcxx/include/__debug_utils/sanitizers.h b/libcxx/include/__debug_utils/sanitizers.h
index 058feab0269e1..3dc69fff38ffc 100644
--- a/libcxx/include/__debug_utils/sanitizers.h
+++ b/libcxx/include/__debug_utils/sanitizers.h
@@ -17,7 +17,16 @@
# pragma GCC system_header
#endif
-#if __has_feature(address_sanitizer)
+// _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS determines whether the containers should provide ASAN container
+// overflow checks. Some containers like std::string need stricter requirements in order to enable these
+// checks and also need to check that the library was built with sanitizer support (_LIBCPP_INSTRUMENTED_WITH_ASAN).
+#if __has_feature(address_sanitizer) && !defined(__SANITIZER_DISABLE_CONTAINER_OVERFLOW__)
+# define _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS 1
+#else
+# define _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS 0
+#endif
+
+#if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
extern "C" {
_LIBCPP_EXPORTED_FROM_ABI void
@@ -28,12 +37,12 @@ _LIBCPP_EXPORTED_FROM_ABI int
__sanitizer_verify_double_ended_contiguous_container(const void*, const void*, const void*, const void*);
}
-#endif // __has_feature(address_sanitizer)
+#endif // _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
_LIBCPP_BEGIN_NAMESPACE_STD
// ASan choices
-#if __has_feature(address_sanitizer)
+#if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
# define _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS 1
#endif
@@ -57,7 +66,7 @@ _LIBCPP_HIDE_FROM_ABI void __annotate_double_ended_contiguous_container(
const void* __last_old_contained,
const void* __first_new_contained,
const void* __last_new_contained) {
-#if !__has_feature(address_sanitizer)
+#if !_LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
(void)__first_storage;
(void)__last_storage;
(void)__first_old_contained;
@@ -86,7 +95,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void __annotate_contiguous_c
const void* __last_storage,
const void* __old_last_contained,
const void* __new_last_contained) {
-#if !__has_feature(address_sanitizer)
+#if !_LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
(void)__first_storage;
(void)__last_storage;
(void)__old_last_contained;
diff --git a/libcxx/include/deque b/libcxx/include/deque
index 08bf8141eb782..649b3dc41efa6 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -932,7 +932,7 @@ private:
(void)__end;
(void)__annotation_type;
(void)__place;
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
// __beg - index of the first item to annotate
// __end - index behind the last item to annotate (so last item + 1)
// __annotation_type - __asan_unposion or __asan_poison
@@ -1025,23 +1025,23 @@ private:
std::__annotate_double_ended_contiguous_container<_Allocator>(
__mem_beg, __mem_end, __old_beg, __old_end, __new_beg, __new_end);
}
-# endif // __has_feature(address_sanitizer)
+# endif // _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
}
_LIBCPP_HIDE_FROM_ABI void __annotate_new(size_type __current_size) const _NOEXCEPT {
(void)__current_size;
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
if (__current_size == 0)
__annotate_from_to(0, __map_.size() * __block_size, __asan_poison, __asan_back_moved);
else {
__annotate_from_to(0, __start_, __asan_poison, __asan_front_moved);
__annotate_from_to(__start_ + __current_size, __map_.size() * __block_size, __asan_poison, __asan_back_moved);
}
-# endif // __has_feature(address_sanitizer)
+# endif // _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
}
_LIBCPP_HIDE_FROM_ABI void __annotate_delete() const _NOEXCEPT {
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
if (empty()) {
for (size_t __i = 0; __i < __map_.size(); ++__i) {
__annotate_whole_block(__i, __asan_unposion);
@@ -1050,19 +1050,19 @@ private:
__annotate_from_to(0, __start_, __asan_unposion, __asan_front_moved);
__annotate_from_to(__start_ + size(), __map_.size() * __block_size, __asan_unposion, __asan_back_moved);
}
-# endif // __has_feature(address_sanitizer)
+# endif // _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
}
_LIBCPP_HIDE_FROM_ABI void __annotate_increase_front(size_type __n) const _NOEXCEPT {
(void)__n;
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
__annotate_from_to(__start_ - __n, __start_, __asan_unposion, __asan_front_moved);
# endif
}
_LIBCPP_HIDE_FROM_ABI void __annotate_increase_back(size_type __n) const _NOEXCEPT {
(void)__n;
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
__annotate_from_to(__start_ + size(), __start_ + size() + __n, __asan_unposion, __asan_back_moved);
# endif
}
@@ -1070,7 +1070,7 @@ private:
_LIBCPP_HIDE_FROM_ABI void __annotate_shrink_front(size_type __old_size, size_type __old_start) const _NOEXCEPT {
(void)__old_size;
(void)__old_start;
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
__annotate_from_to(__old_start, __old_start + (__old_size - size()), __asan_poison, __asan_front_moved);
# endif
}
@@ -1078,7 +1078,7 @@ private:
_LIBCPP_HIDE_FROM_ABI void __annotate_shrink_back(size_type __old_size, size_type __old_start) const _NOEXCEPT {
(void)__old_size;
(void)__old_start;
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
__annotate_from_to(__old_start + size(), __old_start + __old_size, __asan_poison, __asan_back_moved);
# endif
}
@@ -1091,7 +1091,7 @@ private:
__annotate_whole_block(size_t __block_index, __asan_annotation_type __annotation_type) const _NOEXCEPT {
(void)__block_index;
(void)__annotation_type;
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
__map_const_iterator __block_it = __map_.begin() + __block_index;
const void* __block_start = std::__to_address(*__block_it);
const void* __block_end = std::__to_address(*__block_it + __block_size);
@@ -1104,7 +1104,7 @@ private:
}
# endif
}
-# if __has_feature(address_sanitizer)
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
public:
_LIBCPP_HIDE_FROM_ABI bool __verify_asan_annotations() const _NOEXCEPT {
@@ -1166,7 +1166,7 @@ public:
}
private:
-# endif // __has_feature(address_sanitizer)
+# endif // _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
_LIBCPP_HIDE_FROM_ABI bool __maybe_remove_front_spare(bool __keep_one = true) {
if (__front_spare_blocks() >= 2 || (!__keep_one && __front_spare_blocks())) {
__annotate_whole_block(0, __asan_unposion);
diff --git a/libcxx/include/string b/libcxx/include/string
index c4806069d0b44..4848cb27dcb95 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -678,14 +678,18 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
_LIBCPP_PUSH_MACROS
# include <__undef_macros>
-# if __has_feature(address_sanitizer) && _LIBCPP_INSTRUMENTED_WITH_ASAN
+// Since std::string is partially instantiated in the built library, we require that the library
+// has been built with ASAN support in order to enable the container checks in std::string.
+//
+// The _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS macro disables AddressSanitizer (ASan) instrumentation
+// for a specific function, allowing memory accesses that would normally trigger ASan errors to proceed
+// without crashing. This is useful for accessing parts of objects memory, which should not be accessed,
+// such as unused bytes in short strings, that should never be accessed by other parts of the program.
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS && _LIBCPP_INSTRUMENTED_WITH_ASAN
+# define _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS_FOR_STRING 1
# define _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS __attribute__((__no_sanitize__("address")))
-// This macro disables AddressSanitizer (ASan) instrumentation for a specific function,
-// allowing memory accesses that would normally trigger ASan errors to proceed without crashing.
-// This is useful for accessing parts of objects memory, which should not be accessed,
-// such as unused bytes in short strings, that should never be accessed
-// by other parts of the program.
# else
+# define _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS_FOR_STRING 0
# define _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS
# endif
@@ -749,7 +753,7 @@ public:
//
// This string implementation doesn't contain any references into itself. It only contains a bit that says whether
// it is in small or large string mode, so the entire structure is trivially relocatable if its members are.
-# if __has_feature(address_sanitizer) && _LIBCPP_INSTRUMENTED_WITH_ASAN
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS_FOR_STRING
// When compiling with AddressSanitizer (ASan), basic_string cannot be trivially
// relocatable. Because the object's memory might be poisoned when its content
// is kept inside objects memory (short string optimization), instead of in allocated
@@ -764,7 +768,7 @@ public:
void>;
# endif
-# if __has_feature(address_sanitizer) && _LIBCPP_INSTRUMENTED_WITH_ASAN
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS_FOR_STRING
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pointer __asan_volatile_wrapper(pointer const& __ptr) const {
if (__libcpp_is_constant_evaluated())
return __ptr;
@@ -2321,7 +2325,7 @@ private:
__annotate_contiguous_container(const void* __old_mid, const void* __new_mid) const {
(void)__old_mid;
(void)__new_mid;
-# if _LIBCPP_INSTRUMENTED_WITH_ASAN
+# if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS_FOR_STRING
# if defined(__APPLE__)
// TODO: remove after addressing issue #96099 (https://llvm.org/PR96099)
if (!__is_long())
diff --git a/libcxx/test/extensions/libcxx/asan/disable_container_overflow_checks.sh.cpp b/libcxx/test/extensions/libcxx/asan/disable_container_overflow_checks.sh.cpp
new file mode 100644
index 0000000000000..231f8c3c86d61
--- /dev/null
+++ b/libcxx/test/extensions/libcxx/asan/disable_container_overflow_checks.sh.cpp
@@ -0,0 +1,26 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// XFAIL: FROZEN-CXX03-HEADERS-FIXME
+
+//
+// Check that libc++ honors when __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ is set.
+//
+
+// RUN: %{cxx} %s %{flags} %{compile_flags} -D__SANITIZER_DISABLE_CONTAINER_OVERFLOW__
+// RUN: %{cxx} %s %{flags} %{compile_flags} -fsanitize=address -D__SANITIZER_DISABLE_CONTAINER_OVERFLOW__
+
+#include <vector>
+#include <string>
+#include <deque>
+
+#if _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
+# error "Container overflow checks should be disabled in libc++"
+#endif
+
+int main(int, char**) {}
diff --git a/libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp b/libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp
index 10889eb50870d..33d405edcb7b7 100644
--- a/libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp
+++ b/libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp
@@ -103,7 +103,7 @@ static_assert(!std::__libcpp_is_trivially_relocatable<std::array<NotTriviallyCop
static_assert(std::__libcpp_is_trivially_relocatable<std::array<std::unique_ptr<int>, 1> >::value, "");
// basic_string
-#if !__has_feature(address_sanitizer) || !_LIBCPP_INSTRUMENTED_WITH_ASAN
+#if !_LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS_FOR_STRING
struct MyChar {
char c;
};
|
libcxx/test/extensions/libcxx/asan/disable_container_overflow_checks.sh.cpp
Show resolved
Hide resolved
|
This is neat, and solves the conflicting definition issue that could occur if someone did this (and we didn't guard out the forward declarations): |
| data[4] = 42; | ||
| } | ||
|
|
||
| // For std::string, we must use a custom char_traits class to reliably test this behavior. Since |
There was a problem hiding this comment.
I realized something rather important while re-writing this test. Basically, __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ doesn't really work for any container that is (partly or fully) compiled in a separate library. That's kind of obvious when you think about it, but it means that e.g. std::string won't be able to honour this setting because the char specialization with default char_traits is partly instantiated in the dylib. That's really unfortunate.
So, basically, if libc++.dylib has been built with ASAN enabled, you'll be unable to turn off the sanitizer checks by defining __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ in your own translation unit. The reverse is not a problem per se: if libc++.dylib has been built with ASAN disabled, std::string will never enable container overflow checks, because it knows it can't actually implement it. Hence, it doesn't matter whether you define __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ or not, you're never getting any checks.
IMO this goes back to my initial comments about ASAN being a fundamentally ABI-affecting property that really needs "a different slice". Mixing TUs built with ASAN and TUs built without ASAN is always going to be fragile.
There was a problem hiding this comment.
Thinking about this some more, we could actually "prevent" the invalid/surprising behaviour in std::string. We could say that attempting to disable container overflow checks when using a libc++.dylib that has been built with ASAN enabled is just not valid, and produce an error. Something like:
#if __has_feature(address_sanitizer) && !defined(__SANITIZER_DISABLE_CONTAINER_OVERFLOW__)
# define _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS 1
#else
# define _LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS 0
#endif
#if _LIBCPP_INSTRUMENTED_WITH_ASAN && !_LIBCPP_ENABLE_ASAN_CONTAINER_CHECKS
# error "We can't disable ASAN container checks when the library has been built with these checks enabled"
#endifThat way, we catch the potential misuse immediately. In practice, this should affect a much smaller number of users since I'm not aware of anyone shipping a sanitized version of libc++ in their toolchain.
WDYT?
There was a problem hiding this comment.
I like this, it seems in line with what I'd expect as a user. My only comment would be that the wording should indicate that it's libcxx (rather than just 'the library'), but I presume that's not the final wording in your comment anyway.
There was a problem hiding this comment.
I like the approach. I agree that it is very unlikely for someone to ship a sanitized libcxx. If they are building that way it is more than likely to allow them to have a fully sanitized stack for testing purposes
|
I think #171689 should fix some of the CI failures. |
DanBlackwell
left a comment
There was a problem hiding this comment.
This looks good to me - I think we should check whether the error message could be better though
|
I think this broke the HWASan bot: https://lab.llvm.org/buildbot/#/builders/55/builds/23244 |
…vm#168955) Address sanitizer recently got a new macro __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ which is intended to disable container overflow checks in libraries (either the standard library or user libraries that might provide such checks). This patch makes libc++ honor that macro and, in addition, cleans up how these checks are enabled for string (which is special) by introducing a macro just for string. rdar://166234942
|
…vm#168955) Address sanitizer recently got a new macro __SANITIZER_DISABLE_CONTAINER_OVERFLOW__ which is intended to disable container overflow checks in libraries (either the standard library or user libraries that might provide such checks). This patch makes libc++ honor that macro and, in addition, cleans up how these checks are enabled for string (which is special) by introducing a macro just for string. rdar://166234942
Address sanitizer recently got a new macro SANITIZER_DISABLE_CONTAINER_OVERFLOW which is intended to disable container overflow checks in libraries (either the standard library or user libraries that might provide such checks). This patch makes libc++ honor that macro and, in addition, cleans up how these checks are enabled for string (which is special) by introducing a macro just for string.
rdar://166234942