[libc++] Fix vector sanitization annotations on destruction

[ddcc]

In D136765/D144155, the asan annotations for std::vector were modified to unpoison freed backing memory on destruction, instead of leaving it poisoned. However, calling __clear() instead of clear() skips informing the asan runtime of this decrease in the accessible container size, which breaks the invariant that the value of old_mid should match the value of new_mid from the previous call to __sanitizer_annotate_contiguous_container, which can trip the sanity checks for the partial poison between [d1, d2) and the container redzone between [d2, c), if enabled. To fix this, ensure that clear() is called instead, as is already done by __vdeallocate(). Also remove __clear(), since it is no longer called.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Dominic Chen (ddcc)

Changes

In D144155/D136765, the asan annotations for std::vector were modified to unpoison freed backing memory on destruction, instead of leaving it poisoned. However, calling __clear() instead of clear() skips informing the asan runtime of this decrease in the accessible container size, which breaks the invariant that the value of old_mid should match the value of new_mid from the previous call to _sanitizer_annotate_contiguous_container(), which can trip the sanity checks for the partial poison between [d1, d2) and the container redzone between [d2, c), if enabled. To fix this, ensure that clear() is called instead, as is already done by __vdeallocate().

---

Full diff: https://github.com/llvm/llvm-project/pull/121031.diff

1 Files Affected:

• (modified) libcxx/include/__vector/vector.h (+2-2)

diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 6ba7ba7bcf724b..fe87e825ce6d04 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -241,7 +241,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
 
     _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void operator()() {
       if (__vec_.__begin_ != nullptr) {
-        __vec_.__clear();
+        __vec_.clear();
         __vec_.__annotate_delete();
         __alloc_traits::deallocate(__vec_.__alloc_, __vec_.__begin_, __vec_.capacity());
       }
@@ -759,7 +759,7 @@ class _LIBCPP_TEMPLATE_VIS vector {
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __copy_assign_alloc(const vector& __c, true_type) {
     if (this->__alloc_ != __c.__alloc_) {
-      __clear();
+      clear();
       __annotate_delete();
       __alloc_traits::deallocate(this->__alloc_, this->__begin_, capacity());
       this->__begin_ = this->__end_ = this->__cap_ = nullptr;

[vitalybuka]

LGTM, but libc++ maintainers need to check

[philnik777]

Since this is fixing a bug, this needs a test.

[ddcc]

I'm not too familiar with the libcxx testing infrastructure, and I'm not sure what the best way to test this would be. Part of the problem is that the two codepaths being changed call __annotate_delete() immediately after the clear(), so by the time control flow returns to the caller, the intermediate asan container state has been lost and we can't assert on it. The other factor is that the two sanity checks in the compiler-rt asan runtime for the asan shadow state are currently disabled: https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/asan/asan_poisoning.cpp#L448 (lines 448, 455), and I'm hesitant to touch that because it would have broader scope and seems deliberately not fixed.

[AdvenamTacet]

Thank you for noticing it!

Let's make sure that the commit message mentions removal of void __clear().

Please wait for a green light from @philnik777 before merging.

breaks the invariant that the value of old_mid should match the value of new_mid from the previous call to __sanitizer_annotate_contiguous_container()

It shouldn't be observed by a user as we actually do not want to enforce that "invariant" (we want to keep it only to easier ensure correctness of the implementation while testing).

Reasoning is that there are situations where users may want to (un)poison containers memory. We worked on making sure that annotation functions work with modified annotations (including over-unpoisoned memory), so it should not result in any error.

I support that change as keeping that "invariant" helps to avoid annotation errors in future, but unless I missed something, it's not necessary change. Users using public vectors API won't notice that.
Unpoisoning memory from D144155 is necessary, but poisoning here is not.

Part of the problem is that the two codepaths being changed call __annotate_delete() immediately after the clear()

I don't think we can test it in a reasonable way as it shouldn't be observable by using public vector functions. But if someone has an idea, I'm happy to learn how to do it (without cluttering vectors implementation or recompiling compiler-rt).

[philnik777]

@AdvenamTacet

which can trip the sanity checks for the partial poison between [d1, d2) and the container redzone between [d2, c), if enabled

sounds to me a lot like this is oberservable somehow. I don't know exactly what they are referring to, but that doesn't make it not observable (even if it's not observable to the average user). If the observable behaviour is only that there is some ASan mode which trips, we should enable that.

[AdvenamTacet]

I could misunderstood and thought that commented sanity checks are not satisfied. If there is an example which hits sanity check error, I would love to see it and we should definitely add it.

@ddcc do you have an example tripping a sanity check?

[ddcc]

Yeah, I should clarify that the relevant sanity checks are currently commented out in the upstream asan runtime. However, we are using an internal asan runtime that does have these sanity checks enabled, and that is where we see the failures.

[ddcc]

@ldionne @philnik777 Is this PR ok now?