-2

I read one answer about using std::vector::emplace_back on std::vector<std::unique_ptr<SomeClass>> and getting memory leaks when passing raw pointers (my_vec.emplace_back(new SomeClass())) in some exceptional scenario. Since that post is old (2016) and we have new standards now, I thought of trying an experiment if this issue is somehow fixed.

#include <iostream>
#include <memory>
#include <vector>
#include <limits>

struct MyClass
{
    ~MyClass() { std::cout << "Destroyed\n"; }
};

int main()
{
    std::vector<std::unique_ptr<MyClass>> store;
    auto limit = std::numeric_limits<uint32_t>::max();
    
    std::cout << "Trying to allocate " << limit << " unqiue ptrs\n";
    for (size_t i = 0; i < limit; i++)
    {
        store.emplace_back(new MyClass());     // THIS LINE
    }
}

I saw that compilers didn't throw any errors or warnings for the line marked with the comment // THIS LINE.

It's been a long time since we know this issue. Of course, the solution to avoid this is to wrap those raw pointers around smart pointers. But still, if somebody attempts to use raw pointers anyway, then compilers should warn them which doesn't happen.

Is there any work ongoing regarding this? If there are already some ways to detect such issues, please share them.

Code link: https://godbolt.org/z/d5ooeEWf5

P.S. I thought of trying this experiment because I like C++ and I found Rust fans pointing out issues with C++ which makes me sad.

kiner_shah
  • 3,939
  • 7
  • 23
  • 37
  • "*then compilers should warn them*" - there is no such requirement anywhere, C++ programmers are free to use raw pointers in any way they like. – user7860670 Jul 14 '23 at 06:49
  • I would be way too hard for a compiler to detect it to issue a warning. – HolyBlackCat Jul 14 '23 at 06:51
  • The only possible [`std::unique_ptr` constructor](https://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr) for the `emplace_back` call to use is number 2, which is the normal used to take over ownership of the object. This is really nothing different from doing e.g. `std::unique_ptr ptr(new MyClass());`. – Some programmer dude Jul 14 '23 at 06:51
  • Also note that the example program you link to crashes because you try to create a too large container. If you limit it to say `10` then it works as expected, without any leaks. – Some programmer dude Jul 14 '23 at 06:52
  • Oh, and read the warnings! Your program will not work as expected! – Some programmer dude Jul 14 '23 at 06:53
  • @Someprogrammerdude I think the point of this program is to show a situation when memory leak happens during `emplace_back` when container fails to reallocate larger buffer. – user7860670 Jul 14 '23 at 06:53
  • @user7860670, I know C++ provides the freedom to use raw pointers whenever needed. But in this case, there can be an issue as you stated when memory isn't avaliable and container fails to allocate. As per the answer in that link, the memory is leaked. – kiner_shah Jul 14 '23 at 06:55
  • @Someprogrammerdude, yes constructor number 2 should be used. But since that's marked as `explicit`, I wonder why it works when raw pointer is passed to emplace_back. And if it works, then is it really that can cause memory leak? I am confused. – kiner_shah Jul 14 '23 at 06:56
  • @user7860670 Then it's still not a good example, because the OS kills the process so there's no leak actually happening. Not to mention the signed/unsigned problem with the loop variables. – Some programmer dude Jul 14 '23 at 06:57
  • 1
    @kiner_shah Basically `emplace_back` does a placement-new once the container memory have been allocated. So the right constructor will be explicitly called. – Some programmer dude Jul 14 '23 at 06:58
  • I apologize if the example isn't proper. I was trying to check what happens when vector cannot allocate memory for new items. – kiner_shah Jul 14 '23 at 06:58
  • @kiner_shah Nothing have really changed the last few years. If the containers allocator fails to [allocate](https://en.cppreference.com/w/cpp/memory/allocator/allocate) memory then `std::bad_alloc` will be thrown, [`emplace_back`](https://en.cppreference.com/w/cpp/container/vector/emplace_back) will do nothing, and the memory allocated by `new MyClass` will be lost. – Some programmer dude Jul 14 '23 at 07:02
  • 1
    @kiner_shah if it can't allocate, it will throw an exception. Which will leak the `new`'ed object that was created before the exception is thrown. The *correct* solution is to wrap the `new`' ed object in a `unique_ptr` before calling `emplace_back()`, then `std::move` it into the `vector`. – Remy Lebeau Jul 14 '23 at 07:02
  • 1
    @Someprogrammerdude I agree that this example is not really good, but let's assume that OOM kill won't happen. *Not to mention the signed/unsigned problem with the loop variables.* - but there are no signed types on sight anywhere. – user7860670 Jul 14 '23 at 07:06
  • *"Is there any work ongoing regarding this?"* -- Questions that have one-word answers (e.g. "no") are not particularly interesting. I'm somewhat inclined to signify that the situation is unchanged by marking this question as a duplicate of the one you linked to, but you did as a different question than that one. I don't know what should be done with this question. – JaMiT Jul 14 '23 at 07:20
  • @user7860670 Doh! That signed/unsigned problem was one I introduced while experimenting. Please forget I ever said that! :) – Some programmer dude Jul 14 '23 at 07:20
  • 1
    @kiner_shah - I'm tempted to say "This is not the leak you are looking for!". The problem here only occurs when you are already out of memory for the container. So there must be *other* leaks that are more important to look for. Otherwise, this is never going to happen, and you probably worry for no reason. – BoP Jul 14 '23 at 07:55

1 Answers1

3

I don't get the problem - emplace_back can fail by throwing in strong-exception manner which is documented. In that case the passed arguments are not "consumed", i.e. they still belong to the caller.

The fact that you forget to check whether the ownership was really passed to the function or remained with you is your problem, it's not the fault std::vector, std::unique_ptr or C++ design, it can happen to any resource, not just memory.

I think you can get the same thing to happen in Rust if you use unsafe which using raw new is no different from really.

But still, if somebody attempts to use raw pointers anyway, then compilers should warn them which doesn't happen. Is there any work ongoing regarding this?

In modern C++, you should use raw pointers exactly because you want to do what you want to do, you are in your own. new MyClass() mean I will manage the lifetime of this object myself.

Sure, the compiler could issue a warning "passing a pointer to which there is no local reference to a function which can throw", feel free to contribute that to gcc, clang, they are open source after all. But doing such analysis without many false-positives might be very tricky.

Quimby
  • 17,735
  • 4
  • 35
  • 55
  • Yes, I understand now clearly. When we use raw pointers, we definitely are taking control over their management. I understand it is difficult to include such a warning and wanted to know if the attempts are already in progress. Having such a warning can really help new programmers to write safe code (instead of them learning it the hard way). – kiner_shah Jul 14 '23 at 07:17
  • Maybe this kind of check can be included in address sanitizer in the future. – kiner_shah Jul 14 '23 at 07:19
  • 1
    @kiner_shah Of course, I agree the more warnings the better, I think you should raise the feature request for the individual compilers. Well, the safe way is to teach them `new` is unsafe ;) But I get it, many are learning the old C with classes where `malloc` is replaced with `new`. Still, there are sanitizers now which are really really powerful and I think they can detect this, at least valgrind can. – Quimby Jul 14 '23 at 07:20
  • @kiner_shah Yea, I think that the clang's leak/memory sanitizer might also already do this, not sure though, could be tested with a `void emplace_back(void* ptr){throw 42;}` – Quimby Jul 14 '23 at 07:22
  • 2
    *"But doing such analysis without many false-positives might be very tricky."* -- How about (a flag to enable) checking for the use of `new` (outside library headers) and issuing "warning: You used `new`. Don't do that. The person who taught you that probably speaks C++ with a strong C accent." :) – JaMiT Jul 14 '23 at 07:24