[ASan] Unpoisoning vectors memory before deallocation

[AdvenamTacet]

Summary

This revision fixes unpoisoning in std::vector.

A call to __annotate_delete() in operator() (in __destroy_vector class) is moved after a call to clear(), because clear may poison memory (as container overflow - fc).
It also adds unpoisoning memory before passing it to the deallocator in __vdeallocate() and __copy_assign_alloc(const vector& __c, true_type).

It guarantees that __alloc_traits::deallocate may access returned memory.

Change is motivated by false positives after committing D136765 (currently reverted):

• https://bugs.chromium.org/p/chromium/issues/detail?id=1410719#c6)
• https://github.com/llvm/llvm-project/issues/60384 (minimal failing example)

Standard deallocator does not access the memory, so it does not produce an error, but a fix is necessary to turn on support for non-standard allocators.

Additionally, with that change, revision D136765 should not cause false positives, as returned memory will always be unpoisoned.

[MaskRay]

clear() calls __annotate_shrink(__old_size); which eventually calls __sanitizer_annotate_contiguous_container (with new size as 0). The inserted __annotate_delete calls __sanitizer_annotate_contiguous_container (with new size as the capacity) as well. Isn't it wasteful? Notably the second __sanitizer_annotate_contiguous_container call will unpoison the full capacity bytes.

[AdvenamTacet]

The point of __annotate_delete is to return memory in the same state as it was just after allocation. And yes, clear() is poisoning memory of all present objects in a vector (at the end, whole buffer is poisoned as container overflow).

1. Therefore, there is no point to unpoison memory before a call to clear.
2. I don't think clear() is called there because of ASan, as from ASan point of view, container does not have to be empty.
3. The goal is to deallocate unpoisoned memory and there is no easy way to turn off poisoning inside a single call to the clear function (at least I don't know it).

If complexity is the main concern, I can add additional if before every __annotate_delete() call, and don't call them with standard allocator, as in that implementation, it's not necessary. But it depends on memory deallocator implementation.
Also, that memory will be probably poisoned by deallocator soon after, but sometimes deallocator may access memory and then it cannot be poisoned.

[philnik]

I think this should be put in D136765 and not as a separate commit. Regression tests are also missing.

[AdvenamTacet]

Sure, I can add it there.

I created a new revision, because I considered it a separate issue, which should be fixed regardless of the support for more allocators.

Right now, __annotate_delete() in __destroy_vector class makes no sense (it unpoisons whole memory, and just after that, clear() poisons part of it), but the purpose of __annotate_delete(); is to unpoison memory before deallocation.
Missing __annotate_delete() in __vdeallocate() and __copy_assign_alloc(const vector& __c, true_type) makes all existing calls to __annotate_delete() pointless, as with todays implementation of default deallocator, everything should work without calling __annotate_delete() at all.

But I will add it in D136765 and I will add there a simple test with allocator which cleans the memory during allocation and deallocation.

libcxx/include/vector

#endif
   vector(_ForwardIterator __first, _ForwardIterator __last, const allocator_type& __a);
 
 private:
   class __destroy_vector {
     public:
       _LIBCPP_CONSTEXPR __destroy_vector(vector& __vec) : __vec_(__vec) {}
 
       _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void operator()() {
-          __vec_.__annotate_delete();
           std::__debug_db_erase_c(std::addressof(__vec_));
 
           if (__vec_.__begin_ != nullptr) {
             __vec_.__clear();
+            __vec_.__annotate_delete();
             __alloc_traits::deallocate(__vec_.__alloc(), __vec_.__begin_, __vec_.capacity());
           }
       }
 
     private:
       vector& __vec_;
   };
 
#endif
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI
   void __copy_assign_alloc(const vector& __c, true_type)
   {
     if (__alloc() != __c.__alloc())
     {
       __clear();
+      __annotate_delete();
       __alloc_traits::deallocate(__alloc(), this->__begin_, capacity());
       this->__begin_ = this->__end_ = __end_cap() = nullptr;
     }
     __alloc() = __c.__alloc();
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI
   void __copy_assign_alloc(const vector&, false_type)

 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20
 void
 vector<_Tp, _Allocator>::__vdeallocate() _NOEXCEPT
 {
     if (this->__begin_ != nullptr)
     {
         clear();
+        __annotate_delete();
         __alloc_traits::deallocate(this->__alloc(), this->__begin_, capacity());
         this->__begin_ = this->__end_ = this->__end_cap() = nullptr;
     }
 }
 
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20
 typename vector<_Tp, _Allocator>::size_type