[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.