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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/vector:297
 #include <__memory/temp_value.h>
 #include <__memory/uninitialized_algorithms.h>
 #include <__memory_resource/polymorphic_allocator.h>
----------------
ldionne wrote:
> ldionne wrote:
> > ldionne wrote:
> > > In the bug report, you mention this was introduced when we removed the base class. Are you referring to b82da8b555603b172f826fdd5ea902eff3aeb7ad?
> > > 
> > > I looked at the commit and I still don't see how that introduced this issue (but I do acknowledge that this is a real issue, and a somewhat embarrassing one at that).
> > Oh crap, I see it now. It's from 12b55821a578929a7b03448a22c3a678aa649bd5.
> > 
> > And now I finally understand what the motivation for introducing `__vector_base` probably was when Howard wrote it. What a great lesson. I still think it was too subtle to not even have a comment, but there's definitely something to learn from this one.
> > 
> > Since we made similar changes to `string` and perhaps other types, we'll have to also look at those with this bug in mind.
> I looked at the changes we made in `string` and `deque`, and we didn't do anything similar.
> 
> Independently of any changes we might have made, I looked at the `string` implementation and I don't think it suffers from the same issue. Indeed, we sometimes allocate and then copy characters into the string, but `char_traits` is required not to throw anything, so I don't think the copy step can fail.
> 
> `deque` however seems to have the same problem (but not introduced by us). In `deque::__append` (which is called from various constructors), we sometimes seem to allocate and then copy over elements, the constructors of which may throw.
Either way, I'll create additional patches to test the constructors of `deque` and `string`, since we also removed the base classes there. (BTW libstdc++ has a comment explaining the rationale for their base classes)


================
Comment at: libcxx/include/vector:1070
 {
     std::__debug_db_insert_c(this);
     if (__n > 0)
----------------
ldionne wrote:
> This comment applies to all the constructors. If `__vallocate()` fails, we won't call the destructor, so we won't remove `*this` from the debug database.
> 
> That being said, the legacy debug mode is already broken pretty bad so I'm not sure we can or want to fix it.
We could just make the transaction cover the whole constructor, that should fix the problem. Since the fix should be easy enough, I think we should do it.


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/throwing.pass.cpp:19
+template <class T>
+struct Allocator {
+  using value_type      = T;
----------------
ldionne wrote:
> I don't understand how this test is testing anything -- we're not asserting anything. Are we relying on the msan bot?
> 
> Instead, I would suggest that we have a counting allocator that ensures that everything that was allocated also was deallocated. That would make the checking explicit.
ASan, but yes. An allocator doesn't really work, since we can't give it a proper state in all cases. I opted for checking new + delete calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138601