<optional>: Add Address Sanitizer annotations
#6010
Conversation
|
@microsoft-github-policy-service agree |
StephanTLavavej
changed the title
<optional>: Add Address Sanitizer annotations
StephanTLavavej
added
enhancement
|
Thank you! I need to focus on landing |
| #endif | ||
|
|
||
| #ifdef _INSERT_OPTIONAL_ANNOTATION | ||
| extern const bool _Asan_optional_should_annotate; |
There was a problem hiding this comment.
This variable isn't used. Is it really necessary?
There was a problem hiding this comment.
Added a check for this variable inside the if(!_STD _Is_constant_evaluated()) block.
| #endif | ||
| } // extern "C" | ||
|
|
||
| #if defined(_INSERT_VECTOR_ANNOTATION) || defined(_INSERT_STRING_ANNOTATION) |
There was a problem hiding this comment.
I think there should be a concentrated controlling macro for these.
There was a problem hiding this comment.
I don't think it's necessary since this check is only done here. I also added defined(_INSERT_OPTIONAL_ANNOTATION) to the check.
stl/inc/optional
Outdated
| constexpr _Optional_destruct_base() noexcept : _Dummy{}, _Has_value{false} {} // initialize an empty optional | ||
| constexpr _Optional_destruct_base() noexcept : _Dummy{}, _Has_value{false} { | ||
| #ifdef _INSERT_OPTIONAL_ANNOTATION | ||
| __asan_poison_memory_region(_STD addressof(_Value), sizeof(_Value)); |
There was a problem hiding this comment.
This invocations probably break constant evaluation. We should wrap them in if (!_STD _Is_constant_evaluated()) { }, IMO.
Also, why not consistently write sizeof(_Ty)?
There was a problem hiding this comment.
Done.
Also, why not consistently write sizeof(_Ty)?
That's how I usually write it. Changed it to sizeof(_Ty) to match the existing style.
|
|
||
| void test_emplace_unpoisoning() { | ||
| std::optional<Payload> opt; | ||
| opt.emplace(Payload()); |
There was a problem hiding this comment.
For clarity:
| opt.emplace(Payload()); | |
| opt.emplace(); |
|
|
||
| void test_assignment_unpoisoning() { | ||
| std::optional<Payload> opt = std::nullopt; | ||
| opt = Payload(); |
There was a problem hiding this comment.
For clarity, to avoid confusion with type-id:
| opt = Payload(); | |
| opt = Payload{}; |
Ditto below.
| test_poison_on_empty_access(); | ||
| test_emplace_unpoisoning(); | ||
| test_assignment_unpoisoning(); | ||
| test_repoison_after_reset(); |
There was a problem hiding this comment.
I think we can make these cases constexpr-friendly and test them in static_assert since C++20.
There was a problem hiding this comment.
Added a separate function to test constexpr functionality in static_assert.
3 participants
When
optionalis empty the internal storage is poisoned, it's unpoisoned when a value is assigned._ANNOTATE_OPTIONAL,_DISABLE_OPTIONAL_ANNOTATION, etc. to__msvc_sanitizer_annotate_container.hpp._Optional_destruct_baseon empty construction andreset._Optional_construct_base::_Construct.Resolves #5974