[libcxx-commits] [PATCH] D138601: [libc++] Fix memory leaks when throwing inside std::vector constructors

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In addition to this memory leak, it seems like we have a pre-existing bug that has been there for a long long time. In `__construct_at_end` (which is used in like 40 places), we fail to destroy the elements (in reverse order) in case one of the constructors throws. While it is not 100% clear to me that it's required in the constructors (I couldn't find the part of the spec that says so), I am pretty sure we're expected to do that. Let's fix this leak and then make a pass at our containers to see which ones suffer from this issue. I'm extremely surprised that we've been going for so long without this being noticed.



================
Comment at: libcxx/include/vector:450
 
-    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI
-    ~vector()
-    {
-      __annotate_delete();
-      std::__debug_db_erase_c(this);
-
-      if (this->__begin_ != nullptr)
-      {
-        __clear();
-        __alloc_traits::deallocate(__alloc(), this->__begin_, capacity());
-      }
-    }
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI ~vector() { __vector_delete<vector>(*this)(); }
 
----------------
I would move the definition of the class here:

```
private:
  // add comment: Helper class to make it easier to rollback when we fail during one of vector's constructors
  struct __destroy_vector {
    operator() etc..
    vector& __vec;
  };

public:
  ~vector() { __vector_destroy()(*this); }
```


================
Comment at: libcxx/include/vector:2488
     }
     std::copy(__first, __last, __make_iter(__old_size));
 }
----------------
We have the same problem here if the iterators throw inside `std::copy` -- we will end up leaking the memory allocated before the call to `__construct_at_end`.


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/throwing.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Maybe rename to `/exceptions.pass.cpp`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138601/new/

https://reviews.llvm.org/D138601