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.

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

Add the ability to opt-out of ASan container annotations on a per-allocator basis #5241

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

davidmrdavid
Copy link
Member

@davidmrdavid davidmrdavid commented Jan 16, 2025

Context:

The STL "annotates" the vector and basic_string containers so that ASan will report container-overflow whenever allocated but un-initialized data is accessed.

A simple repro is the container overflow error fired in this sample program (assuming it's /fsanitize=address'ed):

// Compile with: cl /EHsc /fsanitize=address /Zi
#include <vector>

int main() {   
    std::vector<int> v(10);
    v.reserve(20); // we've allocated 20 entries, but only initialized only 10

    // Accessing the 10th entry (0-indexed, naturally) triggers an AV 
    v[10] = 1;

}

This is sensible behavior in most cases.

One case where it does not bode well is when an arena allocator is used as the custom allocator of the container. Arena allocators often tamper with the entire allocated memory at once (e.g. they commonly deallocate their entire 'arena' at once) which would trigger ASan AVs when the capacity of the container exceeds it's size.

We encountered one such bug in the msvc front end.

This PR:

This PR introduces the ability for custom allocators to opt-out of vector and basic_string's ASan annotations. This is controlled by the newly introduced template variable: _Disable_ASan_container_annotations_for_allocator<...some allocator type...>.

Testing:

  • For the new annotation opt-out feature: a simple test case was added for basic_string and vector respectively
  • For the pre-existing annotation feature: a simple 'death test' test case was added for the aforementioned containers respectively as well. In the case of basic_string, this replaces the recently added test_gh_5251 test (from ASan should detect writing to a basic_string's reserved but uninitialized memory #5252) to avoid repetition.

stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/vector Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added enhancement Something can be improved ASan Address Sanitizer labels Jan 16, 2025
@davidmrdavid davidmrdavid changed the title [WIP] Add naive first implementation of per-allocator disablement of ASan Add per-allocator disablement of ASan Jan 23, 2025
@davidmrdavid davidmrdavid marked this pull request as ready for review January 23, 2025 02:58
@davidmrdavid davidmrdavid requested a review from a team as a code owner January 23, 2025 02:58
@davidmrdavid

This comment was marked as resolved.

@davidmrdavid davidmrdavid marked this pull request as draft January 23, 2025 16:24
stl/inc/xmemory Outdated Show resolved Hide resolved
@davidmrdavid davidmrdavid marked this pull request as ready for review January 30, 2025 01:20
@davidmrdavid

This comment was marked as resolved.

This comment was marked as resolved.

@davidmrdavid davidmrdavid changed the title Add per-allocator disablement of ASan Add the ability to opt-out of ASan container annotations on a per-allocator basis Jan 30, 2025
Copy link
Member

@zacklj89 zacklj89 left a comment

Choose a reason for hiding this comment

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

Cool 👍

Is the plan to hold on merge while testing the FE?

@davidmrdavid
Copy link
Member Author

davidmrdavid commented Jan 30, 2025

Thanks, @zacklj89!

Is the plan to hold on merge while testing the FE?

My current plan is to work on getting the FE to test this by Friday one way or another: either by manually porting this internally on a private branch, or by consuming this 'as normal' by merging this PR and mirror'ing it on ADO's default branch.

So I think it depends on how quickly we can STL's final approval here. One way or another, I'll be working towards enabling this to be validated by FE starting tomorrow :) . Put another way: I don't think it's strictly necessary to block the PR merge on it - the feature and tests seems up to spec to me. But I'll defer to more experienced owners on that decision.

stl/inc/xmemory Show resolved Hide resolved
stl/inc/vector Outdated Show resolved Hide resolved
tests/std/tests/GH_002030_asan_annotate_string/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_002030_asan_annotate_string/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_002030_asan_annotate_string/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_002030_asan_annotate_vector/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_002030_asan_annotate_vector/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_002030_asan_annotate_vector/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

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

@davidmrdavid
Copy link
Member Author

davidmrdavid commented Jan 30, 2025

Thanks for the feedback, I've taken notes on the common feedback items (don't shadow, don't add std::, remember the null terminator is initialized, etc.). Hoping to retain that for next time around.

New diff seems sound to me, thanks. Good to merge afaict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASan Address Sanitizer enhancement Something can be improved
Projects
Status: Merging
Development

Successfully merging this pull request may close these issues.

5 participants