Re-enable ASAN string annotations #3164

strega-nil-ms · 2022-10-19T22:30:44Z

tested with the bugs with the previous implementation!

Fixes #3205
internal PR for testing purposes: ~~MSVC-PR-433542~~ ~~MSVC-PR-440101~~ MSVC-PR-440279
Fixes the following internal bugs: VSO-1664238, VSO-1586016, VSO-1543191

stl/inc/xstring

stl/inc/xmemory

stl/inc/xstring

stl/inc/xmemory

moving code back and forth between msvc and stl is Fun

stl/inc/vector

stl/inc/xmemory

stl/inc/xstring

stl/inc/vector

stl/inc/xstring

getting stuck on: AddressSanitizer CHECK failed: D:\a\_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\asan\asan_poisoning.cpp:405 "((*(u8*)MemToShadow(a))) == ((0))" (0xfc, 0x0)

comment explaining what's going on at the top of _Get_asan_aligned_first_last.

miscco

I am a bit rusty on the invariants that we provide (last years code, urghh)

_Construct_from_iter gives me some strange feelings, as I am unsure where the repeated calls to _ASAN_STRING_REMOVE come from

stl/inc/vector

stl/inc/xmemory

stl/inc/xstring

additionally, in tests, assume std::allocator is Good

tested all of them since I'm now on a powerful enough computer that that doesn't take ages

StephanTLavavej · 2022-11-11T02:24:52Z

MSVC-PR-433542 has failed checks; it may be an innocuous option difference between prod/fe's Clang 15 and prod/be's Clang 14, but I'm going to remove this PR from the current merge batch until this is investigated.

strega-nil-ms · 2022-11-14T23:06:23Z

Error report from MSVC-PR-433542:

TL;DR

The only concerning failures (to me) are:

The await tests, which show that co_await'd variables are misaligned in experimental coroutines - seems best to turn those tests off entirely when /fsanitize=address is on.
Fuzzer_x86chk_*: an actual bug, which is fixed in the latest commit (f6984d8)
AsanWindows_*_MT(d): looks like a bug in the build system, where libconcrt is not being built with /fsanitize=address; not sure what to do here since this is unrelated to my changes.
websocketpp: there is an issue where _Apply_annotation reading _Aligned is treated as out of bounds. This is extremely concerning and I don't know what to do with this.

Full Report

Runall-x86:
- std x86chk: *: this is a clang version mismatch issue - prod/be uses clang 14, we use clang 15, so this is -Wno-unqualified-std-cast-call biting us. Not a problem
- compiler x86chk: tests\cxx\modules\regress\ADO1217191: this is an issue with an outdated version of the code, that declared constexpr auto _Mask in a function; I can't say I understand the error, but it is solved by removing _Mask in favor of _Asan_granularity_mask
Runall-amd64: same as Runall-x86
MSVC-ASAN-Private:
- ASanStress_amd64_amd64: await (vso275614 ( -Od ) exe): this is a weird error.
  This seems to be a bug in co_await such that string is unaligned:
```
6>==7512==ERROR: AddressSanitizer: bad parameters to __sanitizer_annotate_contiguous_container:
 6>      beg     : 0x1240058202a1
 6>      end     : 0x1240058202b8
 6>      old_mid : 0x1240058202b1
 6>      new_mid : 0x1240058202a
```
  coming from _Create_annotation in something that's only called in SSO mode; this clearly is assuming
  that _Small_string_always_aligned, so beg = _Bx._Buf, end = aligned_after(_Bx._Buf + 16).
  I don't know what to do here, given that it's a very fundamental bug in experimental generator.
MSVC-ASAN-UnitTests-Private:
- Fuzzer_x86chk_*: this found a bug! in _Take_contents, if _Right is large,
  we need to remove the asan annotations on the SSO buffer before copying from it.
  We should just not do the memcpy when _INSERT_STRING_ANNOTATION is turned on,
  like we already do in _Swap_data.
- AsanWindows_*_MT(d): looks like libconcrt is being built without /fsanitize=address
  and without /D_ANNOTATE_VECTOR; not sure what's going on here.
- AsanWindows_amd64chk_MD: this was an issue with replacing AddVectoredExceptionHandler;
  not sure what went wrong here, but unrelated to my changes.
MSVC-OSS-x86-All-Private-ASan:
- assimp: looks like we found a bug in the project
- azure-sdk-c: looks like a bug in project (also this is C, so almost certainly unrelated to my changes)
- cpprestsdk: cpprestsdk incorrectly, intentionally, reads out of bounds in win32_encryption::win32_encryption.
- glslang: dunno how this is happening, but it's unrelated to my changes.
- libzmq: failure to allocate enough memory - unrelated
- onnx: unrelated to my changes; access on stack
- qt6: failed to build; misaligned global in moc - unrelated.
- websocketpp: this is extremely weird; we're getting access failures when trying to read _Aligned, the local variable, in _Apply_annotation. I don't know how this could possibly be happening.

StephanTLavavej · 2022-11-15T00:33:45Z

Thanks for analyzing the MSVC-PR results!

On GitHub, GH_002030_asan_annotate_string is failing for x64. As a reminder, please run updated tests locally before pushing changes. (Usually, running for x64 is sufficient, but given ASAN's alignment sensitivity, running for both x64 and x86 would be a good idea.)

This repros locally and running python tests\utils\stl-lit\stl-lit.py C:\GitHub\STL\tests\std\tests\GH_002030_asan_annotate_string C:\GitHub\STL\tests\std\tests\GH_002030_asan_annotate_vector --order=random -o testing_x64.log takes 3 minutes on my 8C/16T i7-10700 at work. (--max-failures 1 stops after 40 seconds after finding the first failure.) python C:\GitHub\STL\tools\scripts\print_failures.py testing_x64.log can then be used to inspect the log.

StephanTLavavej · 2022-11-30T22:12:01Z

@strega-nil-ms is investigating an LTCG ICE found in MSVC-internal (prod/be) testing. Moving back to WIP until that's resolved. Thanks Nicole! 🧊 🥶 🏂

we've had issues with them and I do not trust them.

strega-nil-ms · 2022-12-08T18:53:07Z

I consider this ready for merging based on the MSVC testing!

regress_amd64

regr-O2b0-amd64-amd64 asan (vectorCall ( -O2b0 ) exe): timeout

OSS_x86

build failures:
- libunifex: unrelated build error
- openssl: unrelated ICE
- qt6: ODR violation issue, should be fixed by turning that off (Zack)
- torque2d: unrelated build issue (could not find directory)
test failures:
- azure_iot_sdk_c: unrelated stack overflow
- cpprestsdk: unrelated heap buffer overflow (they're attempting to memcpy out of a wstring without caring about the size)

OSS_amd64

build failures:
- bond: unrelated compiler bug where private is not respected in SFINAE constraints
- libunifex: unrelated build error
- ~~open3d: looks like an issue with the antivirus~~ passed in subsequent run
- openssl: unrelated ICE
- qt6: unrelated issue with cmake -E make_directory
- qtcreator: could not find version 6 of qt5 (I think this may be to do with qt6 failing to build? unsure)
test failures:
- aleth: bug in version 1.8.0 of jsoncpp where they read from [s.data(), s.data() + s.capacity()) instead of [s.data(), s.data() + s.size())
- cpprestsdk: still an actual bug in cpprestsdk

amyw-msft

LGTM after Open3d_amd64 passes :)

amyw-msft · 2022-12-09T00:49:00Z

stl/inc/xstring

-#define _ASAN_STRING_REMOVE(_Str) (_Str)._Remove_annotation()
-#define _ASAN_STRING_CREATE(_Str) (_Str)._Create_annotation()
+        _Asan_aligned_pointers _Aligned;
+        if constexpr (_Large_string_always_asan_aligned) {


This is essentially the same as what's in vector with the SSO stuff gone, but it's written differently (clamp is avoided in vector since everything happens under the if constexpr instead of setting _Aligned). Or am I missing something?

yeah, I could rewrite the vector stuff to more closely follow the string stuff, if you like?

yeah, I could rewrite the vector stuff to more closely follow the string stuff, if you like?

I would like the two to be as consistent as we can make them. I would also like any such changes to vector to happen in a different PR - let's put this baby to bed.

If the vector implementation is currently correct, I would prefer to see that changed in a followup PR, to avoid too much happening at once. (If the question were changing string to align more closely with vector, then that would be reasonable to do in this PR which is already changing string significantly.)

stl/inc/xstring

CaseyCarter

None of these comments should block merging; we can address any of them in a followup (if at all).

stl/inc/xstring

CaseyCarter · 2022-12-10T00:56:20Z

stl/inc/xstring

-#define _ASAN_STRING_REMOVE(_Str) (_Str)._Remove_annotation()
-#define _ASAN_STRING_CREATE(_Str) (_Str)._Create_annotation()
+        _Asan_aligned_pointers _Aligned;
+        if constexpr (_Large_string_always_asan_aligned) {


yeah, I could rewrite the vector stuff to more closely follow the string stuff, if you like?

I would like the two to be as consistent as we can make them. I would also like any such changes to vector to happen in a different PR - let's put this baby to bed.

stl/inc/__msvc_sanitizer_annotate_container.hpp

stl/inc/xstring

tests/std/tests/GH_002030_asan_annotate_string/env.lst

zacklj89 · 2022-12-12T16:08:10Z

We discussed offline a bit the possibility of versioning annotations because of the SBO annotations being removed. Removing them now, and following the same ABI guarantees as the STL, might cause some headaches in the future if we try to support them.

StephanTLavavej · 2022-12-15T06:28:55Z

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

StephanTLavavej · 2022-12-15T21:43:16Z

Thanks for overhauling this machinery and helping users find bugs in their code! 😻 🛠️ ✨

strega-nil added 2 commits October 19, 2022 12:10

initial attempt

b1cfcde

moar tests

34bacc1

This comment was marked as resolved.

Sign in to view

CaseyCarter reviewed Oct 19, 2022

View reviewed changes

strega-nil added 3 commits October 20, 2022 11:53

CRs, plus some more minor work

d51e580

minor code quality

c62e749

fix vector

997f9b7

strega-nil-ms marked this pull request as ready for review October 24, 2022 18:53

strega-nil-ms requested a review from a team as a code owner October 24, 2022 18:53

This comment was marked as resolved.

Sign in to view

strega-nil added 2 commits October 24, 2022 11:54

fix stuff with testing

f108c24

fixey fixey format

86642d0

moving code back and forth between msvc and stl is Fun

CaseyCarter reviewed Oct 24, 2022

View reviewed changes

StephanTLavavej added the enhancement Something can be improved label Oct 25, 2022

strega-nil added 2 commits October 25, 2022 15:35

casey crs

277e68b

fix comment

b76170a

CaseyCarter reviewed Oct 26, 2022

View reviewed changes

stl/inc/xstring Outdated Show resolved Hide resolved

stl/inc/vector Outdated Show resolved Hide resolved

stl/inc/vector Outdated Show resolved Hide resolved

stl/inc/xstring Outdated Show resolved Hide resolved

stl/inc/xstring Outdated Show resolved Hide resolved

moar fixin

99ea61a

StephanTLavavej self-assigned this Oct 26, 2022

[ci skip] [wip] re-enable tests

65dbe64

getting stuck on: AddressSanitizer CHECK failed: D:\a\_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\asan\asan_poisoning.cpp:405 "((*(u8*)MemToShadow(a))) == ((0))" (0xfc, 0x0)

strega-nil added 2 commits October 31, 2022 18:24

ooh, tests are passing!

d869441

oh okay, so you need to align both first and end

90ed742

comment explaining what's going on at the top of _Get_asan_aligned_first_last.

miscco reviewed Nov 1, 2022

View reviewed changes

strega-nil added 3 commits November 1, 2022 07:19

CRs, plus fix vector test

7f52caa

additionally, in tests, assume std::allocator is Good

add test for DevCom-10109507

8afead5

please tell me i actually fixed the tests this time

888cc8b

tested all of them since I'm now on a powerful enough computer that that doesn't take ages

Caball009 mentioned this pull request Nov 11, 2022

<vector>: x86 ASAN failure with pmr::monotonic_buffer_resource #3205

Closed

xstring: remove memcpies in _INSERT_STRING_ANNOTATION mode

f6984d8

strega-nil added 2 commits November 21, 2022 10:26

Merge remote-tracking branch 'origin/main' into strega-nil/anno-asan01

13b122f

ughghghughuhughg

ace0d8b

strega-nil added 2 commits December 6, 2022 16:42

Merge remote-tracking branch 'origin/main' into strega-nil/anno-asan01

b413060

remove SBO annotations

1f55a10

we've had issues with them and I do not trust them.

amyw-msft approved these changes Dec 9, 2022

View reviewed changes

StephanTLavavej approved these changes Dec 10, 2022

View reviewed changes

stl/inc/xstring Outdated Show resolved Hide resolved

CaseyCarter approved these changes Dec 10, 2022

View reviewed changes

minor CRs

e38dd35

CaseyCarter approved these changes Dec 13, 2022

View reviewed changes

StephanTLavavej approved these changes Dec 14, 2022

View reviewed changes

StephanTLavavej self-assigned this Dec 15, 2022

StephanTLavavej merged commit faaf094 into microsoft:main Dec 15, 2022

strega-nil-ms deleted the anno-asan01 branch December 19, 2022 23:41

zlsun mentioned this pull request Aug 12, 2023

<xstring>: ASAN report container-overflow in a legal case #3955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable ASAN string annotations #3164

Re-enable ASAN string annotations #3164

strega-nil-ms commented Oct 19, 2022 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

miscco left a comment

StephanTLavavej commented Nov 11, 2022

strega-nil-ms commented Nov 14, 2022 •

edited

Loading

StephanTLavavej commented Nov 15, 2022

StephanTLavavej commented Nov 30, 2022

strega-nil-ms commented Dec 8, 2022 •

edited

Loading

amyw-msft left a comment

amyw-msft Dec 9, 2022

strega-nil-ms Dec 9, 2022

CaseyCarter Dec 10, 2022

StephanTLavavej Dec 10, 2022

CaseyCarter left a comment

CaseyCarter Dec 10, 2022

zacklj89 commented Dec 12, 2022

StephanTLavavej commented Dec 15, 2022

StephanTLavavej commented Dec 15, 2022

Re-enable ASAN string annotations #3164

Re-enable ASAN string annotations #3164

Conversation

strega-nil-ms commented Oct 19, 2022 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

miscco left a comment

Choose a reason for hiding this comment

StephanTLavavej commented Nov 11, 2022

strega-nil-ms commented Nov 14, 2022 • edited Loading

TL;DR

Full Report

StephanTLavavej commented Nov 15, 2022

StephanTLavavej commented Nov 30, 2022

strega-nil-ms commented Dec 8, 2022 • edited Loading

regress_amd64

OSS_x86

OSS_amd64

amyw-msft left a comment

Choose a reason for hiding this comment

amyw-msft Dec 9, 2022

Choose a reason for hiding this comment

strega-nil-ms Dec 9, 2022

Choose a reason for hiding this comment

CaseyCarter Dec 10, 2022

Choose a reason for hiding this comment

StephanTLavavej Dec 10, 2022

Choose a reason for hiding this comment

CaseyCarter left a comment

Choose a reason for hiding this comment

CaseyCarter Dec 10, 2022

Choose a reason for hiding this comment

zacklj89 commented Dec 12, 2022

StephanTLavavej commented Dec 15, 2022

StephanTLavavej commented Dec 15, 2022

strega-nil-ms commented Oct 19, 2022 •

edited

Loading

strega-nil-ms commented Nov 14, 2022 •

edited

Loading

strega-nil-ms commented Dec 8, 2022 •

edited

Loading