[ASan][libc++] Annotating std::basic_string with all allocators

[AdvenamTacet]

Summary

This patch is part of our efforts to support container annotations with (almost) every allocator.
Annotating std::basic_string with default allocator is implemented in D132769.

In revision D132522, support for non-aligned memory buffers (sharing first/last granule with other objects) was added, therefore the check for standard allocator is not necessary.
This patch removes the check in std::basic_string annotation member function (__annotate_contiguous_container) to support different allocators and also includes changes from D145628, creating an easy way to turn off annotations for a specific allocator.

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 std::equals function 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:

• [e-mail]
• [e-mail]

[EricWF]

Has this patch been tested against Chromium? Has it been tested elsewhere?
The more testing it's undergone, the more confident I can be.

libcxx/include/string

1851

conditional compilation blocks are the source of a lot of bugs.

Instead of an #if and #else block here. Why not make it so that __asan_annonate_container_with_allocator only returns true when the other conditions _LIBCPP_CLANG_VER > 1600 etc.

Then you could simply write the condition

if (!__libcpp_is_constant_evaluated() && __begin != nullptr &&  __asan_annotate_container_with_allocator<allocator_type, __default_allocator_type>)

Where __asan_annotate_container_allocator_type can return true in the case that the allocator is the default.

libcxx/test/libcxx/containers/strings/no_asan.pass.cpp

2

It's not clear to me what this test is supposed to do.

Does the name mean it should only run without address sanitizer?

libcxx/test/support/asan_testing.h

53–54

Again, I would much rather see zero code inside this conditional compilation block.

If need-be, please write something sorta like this:

#if TEST_CLANG_VER < 16000 || defined(_LIBCPP_ASAN_ANNOTATE_ONLY_LONG)
# define SHORT_STRING_ALLOWED 0
#else
# define SHORT_STRING_ALLOWED 1
#endif

if (SHORT_STRING_ALLOWED || !is_short_string(s))

[AdvenamTacet]

This update adds a description to the test case.

libcxx/test/libcxx/containers/strings/no_asan.pass.cpp

2

Maybe I should change the name, but I added a description. If you have any suggestion, please let me know. This test turns off annotations for a specific allocator.

[AdvenamTacet]

libcxx/include/string

1851

Moving those checks into __asan_annonate_container_with_allocator is a nice idea. I will try to do it Tomorrow. Thanks!

[AdvenamTacet]

This update removes conditional code as much as possible, as suggested in inline comments.
One #if was moved to __asan_annotate_container_with_allocator, check D145628 for details.

[AdvenamTacet]

Previous update removed 4 lines too much, this reverted this part of the previous one.
It also adds __asan_annotate_container_with_allocator check there.

[AdvenamTacet]

This update adds a simple asan libcxx test, similar to one in vector.

[AdvenamTacet]

Moved to GH: https://github.com/llvm/llvm-project/pull/75845

libcxx/include/string

 #  define _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS __attribute__((__no_sanitize__("address")))
 // Sometimes string access own memory, which should not be accessed by different parts of program
 // (eg. not-in-use bytes of buffer in short string) and are poisoned by ASan.
 // This macro turns off instrumentation in a function, so memory accesses which normally would crash
 // work as expected.
 #else
 #  define _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS
 #endif
+#if (_LIBCPP_CLANG_VER >= 1600)
+   // TODO LLVM18: Remove special casing or macro
+#  define _LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED true
+#else
+#  define _LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED false
+#endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // basic_string
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
const_pointer __get_pointer() const _NOEXCEPT
         {return __is_long() ? __get_long_pointer() : __get_short_pointer();}
 
     // The following functions are no-ops outside of AddressSanitizer mode.
 #ifndef _LIBCPP_HAS_NO_ASAN
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_contiguous_container(
         const void* __old_mid, const void* __new_mid) const {
         const void* __begin = data();
         const void* __end = data() + capacity() + 1;
-        if (!__libcpp_is_constant_evaluated() && __begin != nullptr && is_same<allocator_type, __default_allocator_type>::value)
+        if (!__libcpp_is_constant_evaluated() && __begin != nullptr && __asan_annotate_container_with_allocator<_Allocator>::value)
           __sanitizer_annotate_contiguous_container(__begin, __end, __old_mid, __new_mid);
     }
 #else
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void
     __annotate_contiguous_container(const void*, const void*) const {}
 #endif
 
     // ASan: short string is poisoned if and only if this function returns true.
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 bool __annotate_short_string_check() const _NOEXCEPT {
-#if !defined(_LIBCPP_HAS_NO_ASAN) && (_LIBCPP_CLANG_VER >= 1600)
-      // TODO LLVM18: remove special case value
-      return !__libcpp_is_constant_evaluated();
-#else
-      return false;
+      return _LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED && !__libcpp_is_constant_evaluated();
 #endif
     }
 
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_new(size_type __current_size) const _NOEXCEPT {
       if (!__libcpp_is_constant_evaluated() && (__annotate_short_string_check() || __is_long()))
         __annotate_contiguous_container(data() + capacity() + 1, data() + __current_size + 1);
     }
 

libcxx/test/libcxx/containers/strings/no_asan.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+
+// <string>
+
+// 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 <assert.h>
+#include <stdlib.h>
+#include <string>
+#include <new>
+
+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 <typename T>
+struct user_allocator {
+  using value_type = T;
+  user_allocator() = default;
+  template <class U>
+  user_allocator(user_allocator<U>) {}
+  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 <class T>
+struct std::__asan_annotate_container_with_allocator<user_allocator<T>> {
+  static bool const value = false;
+};
+
+int main() {
+  using S = std::basic_string<char, std::char_traits<char>, user_allocator<char>>;
+
+  {
+    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;
+}

libcxx/test/support/asan_testing.h

bool is_string_short(S const& s) {
   return (void*)std::addressof(s) <= (void*)std::addressof(s[0]) &&
     (void*)std::addressof(s[0]) < (void*)(std::addressof(s) + 1);
 }
 
 template <typename ChrT, typename TraitsT, typename Alloc>
 TEST_CONSTEXPR bool is_string_asan_correct(const std::basic_string<ChrT, TraitsT, Alloc>& c) {
   if (TEST_IS_CONSTANT_EVALUATED)
     return true;
   if (c.data() != NULL) {
-#if TEST_CLANG_VER < 16000
-    // TODO LLVM18: remove special case
-  if(!is_string_short(c)) {
-#  endif
-      if (std::is_same<Alloc, std::allocator<ChrT>>::value)
-        return __sanitizer_verify_contiguous_container(
-                   c.data(), c.data() + c.size() + 1, c.data() + c.capacity() + 1) != 0;
-      else
-        return __sanitizer_verify_contiguous_container(
-                   c.data(), c.data() + c.capacity() + 1, c.data() + c.capacity() + 1) != 0;
-#  if TEST_CLANG_VER < 16000
-      // TODO LLVM18: remove special case
-    }
-#endif
+    if(_LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED || !is_string_short(c))
+      return __sanitizer_verify_contiguous_container(c.data(), c.data() + c.size() + 1, c.data() + c.capacity() + 1) != 0;
   }
   return true;
 }
 #else
 #  include <string>
 template <typename ChrT, typename TraitsT, typename Alloc>
 TEST_CONSTEXPR bool is_string_asan_correct(const std::basic_string<ChrT, TraitsT, Alloc>&) {
   return true;
 }
 #endif // TEST_HAS_FEATURE(address_sanitizer)
 #endif // ASAN_TESTING_H