Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

<string> Add asan annotations #2196

Merged
merged 28 commits into from
May 17, 2022
Merged

<string> Add asan annotations #2196

merged 28 commits into from
May 17, 2022

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Sep 9, 2021 •
edited by StephanTLavavej
Loading

This is a mirror of #2071

@miscco

This comment was marked as resolved.

Sign in to view
miscco
miscco commented Sep 9, 2021
View reviewed changes
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Sep 9, 2021
@CaseyCarter

This comment was marked as resolved.

Sign in to view
@miscco

This comment was marked as resolved.

Sign in to view
@miscco

This comment was marked as resolved.

Sign in to view
@miscco miscco force-pushed the asan_string branch 6 times, most recently from 6158c7a to c86baf6 Compare September 13, 2021 13:21
@miscco miscco marked this pull request as ready for review September 13, 2021 13:53
@miscco miscco requested a review from a team as a code owner September 13, 2021 13:53
AlexGuteniev
AlexGuteniev reviewed Sep 13, 2021
View reviewed changes
AlexGuteniev
AlexGuteniev reviewed Sep 13, 2021
View reviewed changes
BillyONeal
BillyONeal reviewed Sep 13, 2021
View reviewed changes
@miscco

This comment was marked as resolved.

Sign in to view
@StephanTLavavej

This comment was marked as outdated.

Sign in to view
@miscco miscco mentioned this pull request Sep 16, 2021
Merged
@miscco miscco force-pushed the asan_string branch 3 times, most recently from d1d9da7 to aa2e01a Compare November 26, 2021 07:14
@miscco
Fix bugs in std::string
98ff44b 
* 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
Allow for SSO for constexpr string
25c2b50
@miscco

This comment was marked as resolved.

Sign in to view
barcharcraz
barcharcraz reviewed Apr 21, 2022
View reviewed changes
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
Member

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

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

StephanTLavavej
StephanTLavavej reviewed May 5, 2022
View reviewed changes
@StephanTLavavej
Copy link
Member

StephanTLavavej commented May 5, 2022 •
edited
Loading

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

@StephanTLavavej StephanTLavavej removed their assignment May 5, 2022
@cbezault
Copy link
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
Member

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

@StephanTLavavej
Copy link
Member

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

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request May 16, 2022
@StephanTLavavej
microsoftGH-2196
f9d1af0
@StephanTLavavej
Copy link
Member

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

STL/tests/std/tests/GH_002030_asan_annotate_vector/test.cpp

Lines 25 to 30 in 313aa59

#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

STL/tests/std/tests/GH_002030_asan_annotate_vector/test.cpp

Lines 148 to 185 in 313aa59

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
Member

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

@StephanTLavavej StephanTLavavej merged commit 46c0bcb into microsoft:main May 17, 2022
@StephanTLavavej
Copy link
Member

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

😻 🚀 🎉

@miscco miscco deleted the asan_string branch May 17, 2022 06:51
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
@miscco @StephanTLavavej @fsb4000
<string> Add asan annotations (microsoft#2196)
3f275a5 
Co-authored-by: Stephan T. Lavavej <[email protected]>
@doomlaur
Copy link

doomlaur commented Aug 17, 2022 •
edited
Loading

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! 😃

@StephanTLavavej StephanTLavavej mentioned this pull request Jan 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

@cbezault cbezault cbezault left review comments

@AlexGuteniev AlexGuteniev AlexGuteniev left review comments

@barcharcraz barcharcraz barcharcraz approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@amyw-msft amyw-msft Awaiting requested review from amyw-msft

@CaseyCarter CaseyCarter Awaiting requested review from CaseyCarter

Assignees

@StephanTLavavej StephanTLavavej

Labels
enhancement Something can be improved high priority Important!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@miscco @CaseyCarter @StephanTLavavej @cbezault @amyw-msft @doomlaur @barcharcraz @BillyONeal @AlexGuteniev @mnatsuhara