[libcxx-commits] [PATCH] D145628: [ASan][libcxx] A way to turn off annotations for containers with a specific allocator

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added 1 blocking reviewer(s): ldionne.
philnik added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/UsingLibcxx.rst:524-530
+Struct template ``__asan_annotate_container_with_allocator`` may be used to turn off
+`ASan annotations for containers <https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow>` with a specific allocator.
+If ``__asan_annotate_container_with_allocator<_Alloc>::value == false``, container won't be poisoned at all.
+Value may be changed by template specialization. Variable ``value`` is of type ``bool``.
+
+If you are creating allocator not working correctly with container annotations from libc++,
+a better choice may be unpoisoning memory, if possible. This way, ASan benefits are present in the program.
----------------



================
Comment at: libcxx/docs/UsingLibcxx.rst:532-552
+If one wants to turn off annotations for a simple ``user_allocator`` with one template argument,
+one may do a specialization like below:
+
+.. code-block:: cpp
+
+  template <class T>
+  struct std::__asan_annotate_container_with_allocator<user_allocator<T>> {
----------------
I don't think it makes sense to disable it for specific types, since I don't see a case where it doesn't purely depend on the allocator whether containers should be annotated, so I don't think we want to advertise that here. We also probably don't want people to poke around in asan annotations if they don't know how to specialize a class.


================
Comment at: libcxx/docs/UsingLibcxx.rst:554-568
+Why may I want to turn it off?
+------------------------------
+
+There are a few reasons why you may want to turn off annotations for an allocator.
+
+* You are using allocator, which does not call destructor during deallocation.
+* You are aware that memory allocated with an allocator may be accessed, even when unused by container.
----------------
This is (mostly) incorporated into my description change above, so nothing should be lost by removing this.


================
Comment at: libcxx/docs/UsingLibcxx.rst:564-566
+If you know in which functions poisoned memory is accessed, you can
+`turn off instrumentation inside a function with attribute <https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address>`
+``__attribute__((no_sanitize("address")))``. Notice that those functions should not modify the container.
----------------
>From what I can tell `[[clang::no_sanitize("address")]]` is quite fiddly, so I wouldn't recommend it here.


================
Comment at: libcxx/include/__memory/allocator_traits.h:26
 #include <limits>
+#include <memory>
 
----------------
You shouldn't include this here.


================
Comment at: libcxx/include/__memory/allocator_traits.h:407-414
+struct __asan_annotate_container_with_allocator {
+#   if _LIBCPP_CLANG_VER >= 1600
+      static bool const value = true;
+#   else
+      // TODO LLVM18: Remove the special-casing
+      static bool const value = std::is_same<_Alloc, std::allocator<_Alloc::value_type> >::value;
+#   endif
----------------
IMO we should require this to be a Cpp17UnaryTypeTrait.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145628/new/

https://reviews.llvm.org/D145628