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 address sanitizer annotations to UninitializedMemoryHacks.h when building with msvc 2022 #1935

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

Conversation

pps83
Copy link

@pps83 pps83 commented Feb 17, 2023

I use UninitializedMemoryHacks.h, but it adds lots of false positives when running with address sanitizer. I grepped MS includes and based on the version that comes with VS2022 I added these annotations. In my case, this fixed all the issues. I did check vs 2019 headers and didn't see the same annotation code used there.

@pps83 pps83 changed the title Add address sanitizer annotations when building with MS compiler Add address sanitizer annotations when building with msvc 2022 Feb 17, 2023
@pps83
Copy link
Author

pps83 commented Feb 17, 2023

@pps83 pps83 changed the title Add address sanitizer annotations when building with msvc 2022 Add address sanitizer annotations to UninitializedMemoryHacks.h when building with msvc 2022 Feb 17, 2023
@pps83
Copy link
Author

pps83 commented Feb 17, 2023

based on git history, seems that these are available only in vs 2022:
image

@pps83 pps83 force-pushed the master-umemhacks-asan-msvc branch 2 times, most recently from ef6e288 to f6acdc4 Compare February 17, 2023 20:06
@facebook-github-bot
Copy link
Contributor

@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@pps83
Copy link
Author

pps83 commented Feb 21, 2023

FYI, it looks like in current version that ships with latest VS2022 asan annotations are commented out in std::string. I had some false positives because of that with asan reporting and had to comment out the string part in UninitializedMemoryHacks.h

@pps83 pps83 force-pushed the master-umemhacks-asan-msvc branch from f6acdc4 to 9dae19f Compare November 21, 2023 01:06
@pps83
Copy link
Author

pps83 commented Nov 21, 2023

More fixes added (support for std::string annotations was added starting from _MSC_VER >= 1938)

@BillyONeal
Copy link

Has folly considered asking for an _Ugly member that does what they need rather than trying to incapability declare standard library parts?

@facebook-github-bot
Copy link
Contributor

@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

folly/memory/UninitializedMemoryHacks.h Outdated Show resolved Hide resolved
folly/memory/UninitializedMemoryHacks.h Outdated Show resolved Hide resolved
folly/memory/UninitializedMemoryHacks.h Outdated Show resolved Hide resolved
folly/memory/UninitializedMemoryHacks.h Outdated Show resolved Hide resolved
@pps83
Copy link
Author

pps83 commented Dec 2, 2023

@yfeldblum thanks for the review. I'm traveling in remote locations and rarely connect, so, will take some time before I can address it. There was also a suggestion by @BillyONeal and I think that would be a better approach.

@BillyONeal
Copy link

There was also a suggestion by @BillyONeal and I think that would be a better approach.

To be clear, while in the long term doing this with some mechanism that isn't a hack would have the benefit that it doesn't break every 3 seconds.

That doesn't change that VS2022 17.8 is already out there which means Windows customers who want to use Folly are broken

@pps83 pps83 force-pushed the master-umemhacks-asan-msvc branch from 0c5be06 to d6738c9 Compare December 6, 2023 14:23
@pps83
Copy link
Author

pps83 commented Dec 6, 2023

@yfeldblum all issues should be addressed

@pps83
Copy link
Author

pps83 commented Dec 6, 2023

_INSERT_STRING_ANNOTATION / _INSERT_VECTOR_ANNOTATION are defined by https://github.com/microsoft/STL/blob/0403d19f5461fd15983737c3f01ec34800ea9275/stl/inc/__msvc_sanitizer_annotate_container.hpp (this file is included by string/vector). string/vector undef these defines at the end. This PR replicates what the MS headers do.

_Asan_string_should_annotate also comes from __msvc_sanitizer_annotate_container.hpp

@pps83
Copy link
Author

pps83 commented Apr 1, 2024

@yfeldblum ping

@facebook-github-bot
Copy link
Contributor

@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Orvid
Copy link
Contributor

Orvid commented Apr 1, 2024

There are merge conflicts in this that need to be resolved before the tooling will let me import this PR. Would you be able to rebase this PR?

@pps83
Copy link
Author

pps83 commented Apr 1, 2024

There are merge conflicts in this that need to be resolved before the tooling will let me import this PR. Would you be able to rebase this PR?

give me a few mins

@pps83 pps83 force-pushed the master-umemhacks-asan-msvc branch from d6738c9 to fe54c3f Compare April 1, 2024 22:27
@pps83
Copy link
Author

pps83 commented Apr 1, 2024

I rebased, but I didn't have any merge conflicts. @Orvid @yfeldblum

@facebook-github-bot
Copy link
Contributor

@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Orvid
Copy link
Contributor

Orvid commented Apr 2, 2024

Thanks, the import worked that time. Not sure why it was having issues before if there weren't merge conflicts.

@pps83 pps83 force-pushed the master-umemhacks-asan-msvc branch from fe54c3f to 2673b49 Compare June 10, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants