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

Thu Nov 24 06:19:27 PST 2022

ldionne 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:
> > 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138601