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

<vector> : Fix ASan annotations when clearing #2464

Merged
merged 4 commits into from
Jan 14, 2022

Conversation

cbezault
Copy link
Contributor

@cbezault cbezault commented Jan 10, 2022

  • Make _Modify_annotation(0) a no-op and add a comment explaining why: this avoids passing nullptr into the ASan runtime when dealing with zero-capacity vectors.
  • Comment _ASAN_VECTOR_MODIFY with // negative when destroying elements when subtracting pointers, making it clear that the order of operands is intentional.
  • For debug mode, optimize clear() for empty vectors, and add a comment explaining why. This is permitted by the Standard, also explained in the comment.
  • Fix the pointer subtraction in clear()'s use of _ASAN_VECTOR_MODIFY.
  • Add test coverage for zero-capacity and empty vectors.

@cbezault cbezault added the bug Something isn't working label Jan 10, 2022
@cbezault cbezault requested a review from a team as a code owner January 10, 2022 23:12
stl/inc/vector Outdated Show resolved Hide resolved
stl/inc/vector Outdated Show resolved Hide resolved
@cbezault cbezault added the high priority Important! label Jan 12, 2022
@cbezault cbezault changed the title <vector> : Correctly handle empty vectors with ASan annotations <vector> : Fix ASan annotations when clearing Jan 12, 2022
cbezault and others added 2 commits January 12, 2022 14:43
* Make `_Modify_annotation(0)` a safe no-op.
  + Add a comment explaining why.

* Undo logic changes in `assign(n, val)` now that `_ASAN_VECTOR_MODIFY`
  is always safe.
  + There are no potential gains from calling `clear()` as this function
    begins by unconditionally calling `_My_data._Orphan_all();` and is
    required to do so by the Standard.

* Comment `_ASAN_VECTOR_MODIFY` with `// negative when destroying elements`
  when subtracting pointers, making it clear that the order of operands
  is intentional.

* Similarly, undo logic changes in `assign(i, j)` and `assign(il)`.
  They are also required to invalidate all iterators, so they can't
  benefit from `clear()`'s optimization here.

* Ditto for `operator=(il)`. (The Standard isn't absolutely clear here,
  but behaving identically to `assign(il)` seems obviously good.)

* Extend the `clear()` optimization to handle all empty vectors
  (zero size), not just zero capacity.
  + Add a comment explaining why, and citing the Standard for why this
    is allowed.

* Similarly undo logic changes in `_Move_assign_unequal_alloc()`.

* Add test coverage for calling `assign()` with an empty range of
  input iterators.
@cbezault
Copy link
Contributor Author

I approve of Stephan's changes.

@StephanTLavavej StephanTLavavej removed their assignment Jan 13, 2022
stl/inc/vector Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I have mirrored this to MSVC-PR-373503 (prod/fe) and MSVC-PR-373504 (prod/be). Please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 86e22c3 into microsoft:main Jan 14, 2022
@StephanTLavavej
Copy link
Member

Thanks for clearing out this bug! 🐞 🪄 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority Important!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants