[ASan][libcxx] A way to turn off annotations for containers with a specific allocator

[AdvenamTacet]

Summary

This revision is part of our efforts to support container annotations with (almost) every allocator.
That patch is necessary to enable support for most annotations (D136765). Without a way to turn off annotations, it's hard to use ASan with area allocators (no calls to destructors).

This is an answer to a request about it. This patch provides a solution to the aforementioned issue by introducing a new template structure __asan_annotate_container_with_allocator, which allows the disabling of container annotations for a specific allocator.

This patch also introduces _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS FTM.

To turn off annotations, it is sufficient to create a template specialization with a false value using a Unary Type Trait.

The proposed structure is being used in the code enabling annotations for all allocators in std::vector, std::basic_string, and std::deque. (D136765 D146214 D146815)

Possibility to do it was added to ASan API in rGdd1b7b797a116eed588fd752fbe61d34deeb24e4 commit.

For context on not calling a destructor, look at https://eel.is/c++draft/basic.life#5 and notes there, you may also read a discussion in D136765.

[hans]

Thanks for adding this!

I think for https://crbug.com/1420967 we will need this. Unpoisoning is not an option there (I think?) since other code will access the memory buffer while the vector is still "active".

libcxx/docs/UsingLibcxx.rst

527

Could you please include a code example which shows how to do the specialization?

Also, will it be possible to write this in a way that works both with libc++ versions that have and don't have this change?

[AdvenamTacet]

This update adds examples how to turn off annotations in a simple way.

I was also thinking about creating a macro _LIBCPP_ASAN_ANNOTATE_CONTAINER_WITH_ALLOCATOR, but I don't fully like that idea.
Let me know if you think it's a good change.

#define _LIBCPP_ASAN_ANNOTATE_CONTAINER_WITH_ALLOCATOR(ALLOC, VALUE) template <class T> \
    struct __asan_annotate_container_with_allocator< ALLOC<T> > { \
        static bool const value = VALUE; \
    };

Then description would be:

For simple allocators with only one template parameter, it's possible to use
``_LIBCPP_ASAN_ANNOTATE_CONTAINER_WITH_ALLOCATOR`` macro.

.. code-block:: cpp

  _LIBCPP_ASAN_ANNOTATE_CONTAINER_WITH_ALLOCATOR(user_allocator, false);

[AdvenamTacet]

libcxx/docs/UsingLibcxx.rst

527

I added examples. I hope those are helpful. If you believe that another example is necessary as well, let me know.

Also, will it be possible to write this in a way that works both with libc++ versions that have and don't have this change?

Possibly a macro would be answer to that, I suggested one in an update comment. But it should be possible to solve easily with libc++ version number as well. I don't really like the idea of creating a macro here.

[hans]

libcxx/docs/UsingLibcxx.rst

521

When this lands, it should probably have a link from the release notes.

524

Perhaps the "ASan annotations for containers" could be a link to https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow for those who are not familiar with the feature.

527

This is great, thanks!

I'd suggest writing the example in a way that's portable, i.e. that will work both with libc++ versions which do and do not have this feature, as well as other standard libraries.

In order to do that, I guess we'll need some kind of feature test macro. I don't think checking the libc++ version number is a good solution as it's not fine grained enough. I'm guessing libc++ has some kind of mechanism for this already similar to the standard feature test macros.

530

It would be good to have an example (not necessarily in code) of why an allocator would not work with this, and either code or a link to how to unpoison the memory.

[AdvenamTacet]

This update extends description with:

• Why one may want to turn off annotations,
• and what else one may do (unpoisoning, turning off instrumentation).

[AdvenamTacet]

libcxx/docs/UsingLibcxx.rst

527

I don't really see a good way to do it and believe that relaying on libc++ version seems the best option. If @philnik or @ldionne think that some macro should be created, I will do it. Just I have to know what macro exactly.

[AdvenamTacet]

Rebase to apply to the newest commit from main.

[hans]

libcxx/docs/UsingLibcxx.rst

527

A version check won't work for consumers of libc++ trunk, since there are versions both with and without this template that have the same _LIBCPP_VERSION.

I think we need a macro or other feature flag, or some mechanism that works both before and after this change (instead of a class template, could we do something based on function overload resolution with the allocator type work?)

[AdvenamTacet]

This update moves a check for all allocators support to libcxx/include/__memory/allocator_traits.h.
Similar changes has to be made in paches turning annotations for all allocators in vector/string and andded in deque patch.

This idea was suggested by @EricWF in D146214, I think it's a good idea. String/vector/deque code will be simplified.

[philnik]

libcxx/docs/UsingLibcxx.rst

524–530

532–552

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.

554–568

This is (mostly) incorporated into my description change above, so nothing should be lost by removing this.

564–566

From what I can tell [[clang::no_sanitize("address")]] is quite fiddly, so I wouldn't recommend it here.

libcxx/include/__memory/allocator_traits.h

26

You shouldn't include this here.

406–413

IMO we should require this to be a Cpp17UnaryTypeTrait.

[AdvenamTacet]

libcxx/include/__memory/allocator_traits.h

406–413

@phillnik does it work with C++03? It will be used inside std::vector etc. so it should work with every supported standard.

It's nice to suggest it in libcxx/docs/UsingLibcxx.rst, but I don't think we can use it here.

[philnik]

libcxx/include/__memory/allocator_traits.h

406–413

Yes, it works in our C++03. We have lots of extensions there (it's basically anything that can be implemented in C++03 with clang extensions that was added in C++11).

[AdvenamTacet]

This update introduces changes suggested in code review:

• use of Cpp17UnaryTypeTrait,
• description update.

libcxx/docs/UsingLibcxx.rst

564–566

I think it's a nice option, but I am also strongly biased. And that's true that turning off instrumentation may be really tricky.
I removed it, but if someone thinks it would be good to describe every option, I think it may be rediscussed. I'm fine with it and without it.

What else can I do?
-------------------
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.

[hans]

libcxx/docs/UsingLibcxx.rst

535

Did this lose the template line above, or am I missing something?

I also still think the example needs to be made portable.

[AdvenamTacet]

This update adds missing template <class T> in the example.

[AdvenamTacet]

*Adds missing space.

[AdvenamTacet]

libcxx/docs/UsingLibcxx.rst

535

Yes, thank you! I fixed it.

I also still think the example needs to be made portable.

As there is a separate thread about it, I marked this comment as done.

[AdvenamTacet]

This update:

• resolves a problem with C++03,
• tries to sole a problem with g++ compilation.

[ldionne]

I'm not super happy with this patch, I wish it weren't necessary. But it seems like the arena case where destructors aren't actually called indeed requires it and I can't think of an easy way to support this otherwise, so LGTM. I will let @philnik give the final approval, but don't consider it blocked on me.

[hans]

I still think we need to address how code which builds against different c++ standard libraries are going to realistically use this. As written, the instructions only work with libc++ versions containing this patch -- how is a developer going to detect that?

[AdvenamTacet]

This update:

• adds _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS FTM,
• updates the description,
• updates the example.

As discussed on Discord, #libcxx.

@hans I hope this also answers your concerns.

[hans]

In D145628#4238358, @AdvenamTacet wrote:

This update:

• adds _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS FTM,
• updates the description,
• updates the example.

As discussed on Discord, #libcxx.

@hans I hope this also answers your concerns.

Yes it does. Thanks!

libcxx/docs/UsingLibcxx.rst

538

Should it be std::false_type?

[philnik]

LGTM with nits addressed.

libcxx/docs/UsingLibcxx.rst

538

yes

libcxx/include/__memory/allocator_traits.h

420

[AdvenamTacet]

This update addresses nits.

[AdvenamTacet]

This update solves the merge conflict with the newest commit.

libcxx/docs/UsingLibcxx.rst

 Extensions to ``<format>``
 --------------------------
 
 The exposition only type ``basic-format-string`` and its typedefs
 ``format-string`` and ``wformat-string`` became ``basic_format_string``,
 ``format_string``, and ``wformat_string`` in C++23. Libc++ makes these types
 available in C++20 as an extension.
 
 For padding Unicode strings the ``format`` library relies on the Unicode
 Standard. Libc++ retroactively updates the Unicode Standard in older C++
 versions. This allows the library to have better estimates for newly introduced
 Unicode code points, without requiring the user to use the latest C++ version
 in their code base.
+=======
+Turning off ASan annotation in containers
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+``__asan_annotate_container_with_allocator`` is a customization point to allow users to disable
+`Address Sanitizer annotations for containers <https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow>`_ for specific allocators. This may be necessary for allocators that access allocated memory.
+This customization point exists only when ``_LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS`` Feature Test Macro is defined.
+
+For allocators not running destructors, it is also possible to `bulk-unpoison memory <https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning>`_ instead of disabling annotations altogether.
+
+The struct may be specialized for user-defined allocators. It is a `Cpp17UnaryTypeTrait <http://eel.is/c++draft/type.traits#meta.rqmts>`_ with a base characteristic of ``true_type`` if the container is allowed to use annotations and ``false_type`` otherwise.
+
+The annotations for a ``user_allocator`` can be disabled like this:
+
+.. code-block:: cpp
+
+  #ifdef _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS
+  template <class T>
+  struct std::__asan_annotate_container_with_allocator<user_allocator<T>> : std::false_type {};
+  #endif
+
+Why may I want to turn it off?
+------------------------------
+
+There are a few reasons why you may want to turn off annotations for an allocator.
+Unpoisoning may not be an option, if (for example) you are not maintaining the 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.

libcxx/include/__memory/allocator_traits.h

 #include <__type_traits/is_empty.h>
 #include <__type_traits/is_move_constructible.h>
 #include <__type_traits/make_unsigned.h>
 #include <__type_traits/remove_reference.h>
 #include <__type_traits/void_t.h>
 #include <__utility/declval.h>
 #include <__utility/forward.h>
 #include <limits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_STD

 template <class _Alloc>
 struct __is_cpp17_copy_insertable<_Alloc, __enable_if_t<
     !__is_default_allocator<_Alloc>::value &&
     __has_construct<_Alloc, typename _Alloc::value_type*, const typename _Alloc::value_type&>::value
 > >
     : __is_cpp17_move_insertable<_Alloc>
 { };
 
+// ASan choices
+#ifndef _LIBCPP_HAS_NO_ASAN
+#   define _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS 1
+#endif
+
+#ifdef _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS
+template <class _Alloc>
+struct __asan_annotate_container_with_allocator
+#   if defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER >= 1600
+      : true_type {};
+#   else
+      // TODO LLVM18: Remove the special-casing
+      : false_type {};
+#   endif
+
+template <class _Tp>
+struct __asan_annotate_container_with_allocator<allocator<_Tp> > : true_type {};
+#endif
+
 #undef _LIBCPP_ALLOCATOR_TRAITS_HAS_XXX
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP___MEMORY_ALLOCATOR_TRAITS_H