This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by AdvenamTacet on Mar 16 2023, 4:07 AM.

Details

Reviewers
philnik
Group Reviewers
Restricted Project
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:

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

Diff Detail

Event Timeline

AdvenamTacet created this revision.Mar 16 2023, 4:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 4:07 AM
AdvenamTacet requested review of this revision.Mar 16 2023, 4:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 4:07 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF added a subscriber: EricWF.Mar 20 2023, 12:11 PM

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
1845

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
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))

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 added inline comments.Mar 22 2023, 4:20 AM
libcxx/include/string
1845

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

AdvenamTacet marked 3 inline comments as done.

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.

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.

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