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

[AdvenamTacet]

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: #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 #72677.

Additionally it removes __begin != nullptr because data() should never return a nullptr.

Support in ASan API exists since 1c5ad6d. 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 2fa1bec.

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

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Tacet (AdvenamTacet)

Changes

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 #72677.

Support in ASan API exests since 1c5ad6d. 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 2fa1bec.

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:

• advenam.tacet@trailofbits.com
• disconnect3d@trailofbits.com

---

Full diff: https://github.com/llvm/llvm-project/pull/75845.diff

4 Files Affected:

• (modified) libcxx/include/string (+1-2)
• (added) libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp (+61)
• (added) libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp (+77)
• (modified) libcxx/test/support/asan_testing.h (+1-1)

diff --git a/libcxx/include/string b/libcxx/include/string
index fdffca5aed18be..ce2df8da8a91eb 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<allocator_type, __default_allocator_type>::value)
+    if (!__libcpp_is_constant_evaluated() && __asan_annotate_container_with_allocator<allocator_type>::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 00000000000000..4c3018c2b33cc5
--- /dev/null
+++ b/libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp
@@ -0,0 +1,61 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+
+// <string>
+
+// Basic test if ASan annotations work.
+
+#include <string>
+#include <cassert>
+#include <cstdlib>
+
+#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<char*> MyInputIter;
+    // Should not trigger ASan.
+    std::basic_string<char, std::char_traits<char>, safe_allocator<char>> 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<T, std::char_traits<T>, safe_allocator<T>>;
+    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);
+  }
+}
\ No newline at end of file
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..875b3904fc6b29
--- /dev/null
+++ b/libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp
@@ -0,0 +1,77 @@
+//===----------------------------------------------------------------------===//
+//
+// 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;
+}
\ No newline at end of file
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<ChrT, TraitsT
     return true;
 
   if (!is_string_short(c) || _LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED) {
-    if (std::is_same<Alloc, std::allocator<ChrT>>::value)
+    if (std::__asan_annotate_container_with_allocator<Alloc>::value)
       return __sanitizer_verify_contiguous_container(c.data(), c.data() + c.size() + 1, c.data() + c.capacity() + 1) !=
              0;
     else

[AdvenamTacet]

I think we are ready to turn on annotations for all allocators as no bugs are reported for #72677. @vitalybuka @ldionne let me know if you agree.

[EricWF]

I'm pretty opposed to a couple of aspects of this patch.

1. I think the customization point std::__asan_annotate_container_with_allocator encourages users to (A) use internals, (B) specialize STL types, and (C) configure the customization point for types they don't own.

2. The added tests contain a bunch of UB.

Could you please provide an example that is well-formed? Then we can figure out a more appropriate way to provide the customization needed.

[EricWF]

OK, I have a few requested changes. Then I say we land this, and then take a short break to ensure everything that's landed so far is going to stick, and to do some cleanup.

One cleanup we need to undertake is one that removes our dependence on the optimizer removing dead calls for us (even if it does, that's likely at the cost of some other code it can't optimize because it's hit its inliner limits).

I'm going to hold off actually approving this patch until we figure out if the performance fixes for the earlier patches are sufficient.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[EricWF]

I think this LGTM, but it needs updating since we're not going to land the if constexpr magic it's using. But I think we have a good handle on ensuring this is no overhead by other means.

If you wouldn't mind giving me a few days to test this internally before landing, I would appreciate it. But if there's time pressure to land this, that's OK too.

[AdvenamTacet]

I will remove the constexpr magic Tomorrow.

No big time pressure, I believe we can wait a few days. Probably it's good to test before landing to avoid reverting. Also, @ldionne didn't stamp this PR yet.

Would be good, however, to work on short string annotations in parallel. I will update/finish #75882 Tomorrow, could you look at it as well then?

[AdvenamTacet]

@ldionne Thx for looking at it! I will merge when CI ends running.

[AdvenamTacet]

Buildbots are failing. https://lab.llvm.org/buildbot/#/builders/37/builds/29906

Error is not clear for me, but I think problem is not in that PR, therefore I'm not reverting.

/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/fuzzer/fork.test:15:8: error: CRASH: expected string not found in input
CRASH: {{SEGV|access-violation}} on unknown address 0x00000000
       ^

Could it be that another error was detected?