SmallVector: fix use-after-poison MSAN error in destructor

[elsteveogrande]

Summary

Addresses issue: https://bugs.llvm.org/show_bug.cgi?id=34595

The topmost class, SmallVector, has internal storage for some elements; N - 1 elements' bytes worth of space. Meanwhile a base class SmallVectorTemplateCommon has room for one element as well, totaling N elements' worth of space.

The space for the N elements is contiguous and straddles SmallVectorTemplateCommon and SmallVector.

A class "between" those two owning the storage, SmallVectorImpl, in its destructor, calls the destructor for elements contained in the vector, if any. It uses destroy_range(begin, end) and deletes all items in sequence, starting from the end.

By the time the destructor for SmallVectorImpl is running, though, the memory for elements [1, N) is already poisoned, due to SmallVector's destructor having done its thing already.

So if the element type T has a nontrivial destructor that accesses any members of the T instance being destroyed, we'll run into a user-after-poison bug.

This patch moves the destruction loop into SmallVector's destructor, so any memory being accessed while dtors are running is not yet poisoned.

Confirmed this broke before (and now works with this patch) with these compiler flags:

-fsanitize=memory
-fsanitize-memory-use-after-dtor
-fsanitize-memory-track-origins

and with the cmake flag -DLLVM_USE_SANITIZER='MemoryWithOrigins;Undefined' as well as MSAN_OPTIONS=poison_in_dtor=1.

[morehouse]

LGTM for fixing the use-after-dtor. I'll let @dblaikie comment since he brought up template bloat issues on https://reviews.llvm.org/D12970.

[dblaikie]

Thanks for CCing me Matt - but yeah, this seems reasonable/makes sense to me!

[elsteveogrande]

Thanks all! Seems I'm unable to land this.

I was working off the github clone (which I think is readonly). I then changed the remote URL to https://git.llvm.org/git/llvm.git/ and tried git svn dcommit, but it hangs there seemingly forever.

Any tricks to pushing this, or would someone else (with privileges?) be able to merge this?

Thanks! :)

[morehouse]

Steve, you need commit access. See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access. I've pushed this patch for you.

llvm/trunk/include/llvm/ADT/SmallVector.h

protected:
   explicit SmallVectorImpl(unsigned N)
     : SmallVectorTemplateBase<T, isPodLike<T>::value>(N*sizeof(T)) {
   }
 
 public:
   SmallVectorImpl(const SmallVectorImpl &) = delete;
 
   ~SmallVectorImpl() {
-    // Destroy the constructed elements in the vector.
-    this->destroy_range(this->begin(), this->end());
-
+    // Subclass has already destructed this vector's elements.
     // If this wasn't grown from the inline copy, deallocate the old space.
     if (!this->isSmall())
       free(this->begin());
   }
 
   void clear() {
     this->destroy_range(this->begin(), this->end());
     this->EndX = this->BeginX;

 template <typename T, unsigned N>
 class SmallVector : public SmallVectorImpl<T> {
   /// Inline space for elements which aren't stored in the base class.
   SmallVectorStorage<T, N> Storage;
 
 public:
   SmallVector() : SmallVectorImpl<T>(N) {}
 
+  ~SmallVector() {
+    // Destroy the constructed elements in the vector.
+    this->destroy_range(this->begin(), this->end());
+  }
+
   explicit SmallVector(size_t Size, const T &Value = T())
     : SmallVectorImpl<T>(N) {
     this->assign(Size, Value);
   }
 
   template <typename ItTy,
             typename = typename std::enable_if<std::is_convertible<
                 typename std::iterator_traits<ItTy>::iterator_category,