Fix poisoning leftover for vector and string ASan annotations#6285
Fix poisoning leftover for vector and string ASan annotations#6285amyw-msft wants to merge 4 commits into
Conversation
…'end' point in situations where the end has been extended to the end of the allocation block.
There was a problem hiding this comment.
Pull request overview
Fixes ASan container annotation cleanup for std::vector and std::basic_string when the allocator is at least 8-byte aligned. In that path, the implementation over-poisons up to the next 8-byte shadow boundary past _End. When removing or shrinking the annotation, the "old/new last valid" pointers passed to __sanitizer_annotate_contiguous_container must also be bumped to that same aligned boundary whenever they equal _End; otherwise the trailing shadow bytes past _End remain poisoned after the container goes out of scope.
Changes:
- In
stl/inc/vector's aligned-allocator branch of_Apply_annotation, advance_Old_last/_New_lastto the aligned-after-end pointer when they equal_End. - Apply the analogous fix to
stl/inc/xstring's_Apply_annotation, and restructure the function so the aligned branchreturns early. - Add a new regression test (
GH_006276_annotation_poison_cleanup) exercising both the under-poison and over-poison paths forstringandvectorwith an ASan-unaware arena allocator.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| stl/inc/vector | Compute aligned old/new last pointers and pass them to the sanitizer call in the aligned-allocator branch. |
| stl/inc/xstring | Same fix for basic_string; restructure so the aligned branch returns early before the non-aligned code. |
| tests/std/tests/GH_006276_annotation_poison_cleanup/test.cpp | New regression test verifying shadow bytes are fully cleared after container destruction. |
| tests/std/tests/GH_006276_annotation_poison_cleanup/env.lst | ASan test matrix for the new test. |
| tests/std/test.lst | Register the new test directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
davidmrdavid
left a comment
davidmrdavid
left a comment
There was a problem hiding this comment.
LGTM, but will defer to STL. Just a small observation
|
ARM64 crash in the new test; is this something concerning, or pre-existing rare flakiness? |
zacklj89
left a comment
zacklj89
left a comment
There was a problem hiding this comment.
I don't immediately know what could be causing the ARM64 failure, but let me know if I can help investigate.
| const void* const _Old_last = _STD _Unfancy(_Old_last_); | ||
| const void* const _New_last = _STD _Unfancy(_New_last_); | ||
| if constexpr ((_Container_allocation_minimum_asan_alignment<vector>) >= _Asan_granularity) { | ||
| // If we realign the _End forward to maximize coverage, we need to keep other significant points |
There was a problem hiding this comment.
By "significant points" here, we mean _Old_last and _New_last right? Maybe we could clarify if there's another push:
// If we realign _End forward to maximize coverage, also realign _Old_last and _New_last when they
// equal _End. Otherwise, the ASan annotation may leave stale shadow bytes behind when cleaning up.|
|
||
| // Allocation is potentially unaligned, so we cannot annotate whole buffer since we might be | ||
| // entering memory owned by someone else. Therefore, pull back where the annotations end on the buffer, | ||
| // but may miss some coverage near end of buffer. |
There was a problem hiding this comment.
Another comment comment, but feel free to not integrate it. I haven't worked as closely here as you, so maybe this is redundant, but:
// The allocation may not end on an ASan granularity boundary. In that case, annotating up to _End
// could affect shadow bytes for memory beyond this allocation, so restrict annotations to the largest
// aligned subrange inside the buffer. This may leave the tail of the buffer unannotated.|
|
||
| const void* const _New_end_aligned = _STD _Get_asan_aligned_after(_End); | ||
| const void* const _Old_last_aligned = (_Old_last == _End) ? _New_end_aligned : _Old_last; | ||
| const void* const _New_last_aligned = (_New_last == _End) ? _New_end_aligned : _New_last; |
| test_arena.print_shadow(); | ||
| } | ||
|
|
||
| int main() { |
There was a problem hiding this comment.
Do you think it would be worthwhile to add a test here that does like shrink from full and tests the shadow, like testing _Old_last == _End and doing pop_back() on a full vector/string?
Fixes #6276
AddressSanitizer poisons memory in 8 byte chunks. It can mark a memory region as 'partially addressable', but only by marking the first
xbytes as valid to access and remaining as poisoned. There is no way to say "only the firstxbytes are poisoned". The result is that the ends of buffers that are not 8-byte aligned either need to over-poison past the end of the buffer, or under-poison and allow potential buffer overflow to go unnoticed under ASan.For ASan container annotations, we choose which strategy to use based on the allocator. If we can be sure that the allocator only allocations in 8-byte aligned chunks, then we can safely over-poison past the end of the buffer. However, when we do this (increase the pointer that points to the 'end'), we also need to adjust the actual 'last valid' pointer as well if they are equal, because the way we clean up the container poisoning is by calling that annotations API, saying that the last valid and the end pointer are the same.
This change checks for this condition and advances the 'last valid' pointer if it is equal to the 'end' pointer. Without this fix, shadow bytes will remain poisoned after an annotated container leaves scope.