Skip to content

<string> Add asan annotations - #2196

Merged
Stephan T. Lavavej (StephanTLavavej) merged 28 commits into
microsoft:mainfrom
miscco:asan_string
May 17, 2022
Merged

<string> Add asan annotations#2196
Stephan T. Lavavej (StephanTLavavej) merged 28 commits into
microsoft:mainfrom
miscco:asan_string

Conversation

@miscco

@miscco Michael Schellenberger Costa (miscco) commented Sep 9, 2021

Copy link
Copy Markdown
Contributor

This is a mirror of #2071

@miscco

This comment was marked as resolved.

Comment thread stl/inc/xstring
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added the enhancement Something can be improved label Sep 9, 2021
@CaseyCarter

This comment was marked as resolved.

@miscco

This comment was marked as resolved.

@miscco

This comment was marked as resolved.

Comment thread stl/inc/xstring Outdated
Comment thread stl/inc/xstring Outdated
Comment thread stl/inc/xstring Outdated
Comment thread stl/inc/xstring Outdated
Comment thread stl/inc/xstring
@miscco

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as outdated.

* Missing capacity for null terminator
* Leaking memor in case of unequal allocators
* Not swapping the proxy during swap
* swapping data in case of self swap
@miscco

This comment was marked as resolved.

Comment thread tests/std/tests/GH_002030_asan_annotate_string/test.cpp Outdated
Comment thread stl/inc/xstring
}

_NODISCARD static const void* _Get_aligned_first(const void* _First, const size_type _Capacity) noexcept {
const char* _CFirst = reinterpret_cast<const char*>(_First);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be a static_cast I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer to use reinterpret_cast here, we need to deal with different character types

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular cast does not though

Comment thread stl/inc/xstring
Comment thread tests/std/tests/GH_002030_asan_annotate_string/test.cpp
Comment thread tests/std/tests/GH_002030_asan_annotate_string/test.cpp
Comment thread stl/inc/xstring Outdated
Comment thread tests/std/tests/GH_002030_asan_annotate_string/test.cpp Outdated
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented May 5, 2022

Copy link
Copy Markdown
Member

I've pushed a conflict-free merge with main, followed by a preprocessor fix and a comment cleanup.

@cbezault

Copy link
Copy Markdown
Contributor

Educational comment: /D_ANNOTATE_VECTOR is needed in src/vctools/crt/asan/mt/libasan.nativeproj because the ASan runtime internally uses vector. As long as the ASan runtime doesn't use string internally /D_ANNOTATE_STRING is not strictly needed but won't hurt.

@StephanTLavavej

Copy link
Copy Markdown
Member

Ah, excellent! IIRC, ASAN is removing (or has removed) the vector dependency.

@StephanTLavavej

Copy link
Copy Markdown
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

Stephan T. Lavavej (StephanTLavavej) added a commit to StephanTLavavej/STL that referenced this pull request May 16, 2022
@StephanTLavavej

Copy link
Copy Markdown
Member

I've pushed a commit to fix an internal test run failure ("unresolved external symbol") by copying this pattern:

#ifdef __SANITIZE_ADDRESS__
extern "C" {
void* __sanitizer_contiguous_container_find_bad_address(const void* beg, const void* mid, const void* end) noexcept;
void __asan_describe_address(void*) noexcept;
}
#endif // ASan instrumentation enabled

template <class T, class Alloc>
bool verify_vector(vector<T, Alloc>& vec) {
#ifdef __SANITIZE_ADDRESS__
size_t buffer_bytes = vec.capacity() * sizeof(T);
void* buffer = vec.data();
void* aligned_start = align(8, 1, buffer, buffer_bytes);
if (!aligned_start) {
return true;
}
void* mid = vec.data() + vec.size();
mid = mid > aligned_start ? mid : aligned_start;
void* bad_address =
__sanitizer_contiguous_container_find_bad_address(aligned_start, mid, vec.data() + vec.capacity());
if (bad_address == nullptr) {
return true;
}
if (bad_address < mid) {
cout << bad_address << " was marked as poisoned when it should not be." << endl;
} else {
cout << bad_address << " was not marked as poisoned when it should be." << endl;
}
cout << "Vector State:" << endl;
cout << " begin: " << buffer << endl;
cout << " aligned begin: " << aligned_start << endl;
cout << " last: " << reinterpret_cast<void*>(vec.data() + vec.size()) << endl;
cout << " end: " << reinterpret_cast<void*>(vec.data() + vec.capacity()) << endl;
__asan_describe_address(bad_address);
return false;
#else // ^^^ ASan instrumentation enabled ^^^ // vvv ASan instrumentation disabled vvv
(void) vec;
return true;
#endif // Asan instrumentation disabled
}

@StephanTLavavej

Copy link
Copy Markdown
Member

I've pushed an additional commit to fix ARM64EC and CHPE build breaks. I applied the fix to <vector> too.

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 46c0bcb into microsoft:main May 17, 2022
@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks for implementing this major feature and completing this part of the ASAN story!

😻 🚀 🎉

Igor Zhukov (fsb4000) pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>
@doomlaur

Laurențiu Leahu-Vlăducu (doomlaur) commented Aug 17, 2022

Copy link
Copy Markdown

Thanks for this amazing feature! 😄 Maybe you are aware of this already, but this feature unfortunately causes issues, as explained here (but also in 64-bit builds): https://developercommunity.visualstudio.com/t/VS2022---Address-sanitizer-on-x86-Debug-/10116361

I'm looking forward to using it as soon as strings work with ASAN again! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved high priority Important!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants