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

Re-enable ASAN string annotations #3164

Merged
merged 33 commits into from
Dec 15, 2022

Conversation

strega-nil-ms
Copy link
Contributor

@strega-nil-ms strega-nil-ms commented Oct 19, 2022

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

@CaseyCarter

This comment was marked as resolved.

stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
@strega-nil-ms strega-nil-ms marked this pull request as ready for review October 24, 2022 18:53
@strega-nil-ms strega-nil-ms requested a review from a team as a code owner October 24, 2022 18:53
@strega-nil-ms

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

moving code back and forth between msvc and stl is Fun
stl/inc/vector Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Oct 25, 2022
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
@StephanTLavavej StephanTLavavej self-assigned this Oct 26, 2022
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.
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xstring Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
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
Copy link
Member

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
Copy link
Contributor Author

strega-nil-ms commented Nov 14, 2022

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

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

@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! 🧊 🥶 🏂

@strega-nil-ms
Copy link
Contributor Author

strega-nil-ms commented Dec 8, 2022

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

Copy link
Member

@amyw-msft amyw-msft left a comment

Choose a reason for hiding this comment

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

LGTM after Open3d_amd64 passes :)

#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) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

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

stl/inc/xstring Outdated Show resolved Hide resolved
#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) {
Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
stl/inc/__msvc_sanitizer_annotate_container.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_sanitizer_annotate_container.hpp Outdated Show resolved Hide resolved
stl/inc/xstring Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
@zacklj89
Copy link
Member

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 StephanTLavavej self-assigned this Dec 15, 2022
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit faaf094 into microsoft:main Dec 15, 2022
@StephanTLavavej
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<vector>: x86 ASAN failure with pmr::monotonic_buffer_resource
7 participants
  翻译: