C++: is push_back(new Object()) a memory leak?

Question

Is the following C++ code a memory leak?

list.push_back(new String("hi"));

As I understand it, push_back from any std collection/container always makes a copy. So if the new string is copied, nothing can ever delete the new'd string right? since there is no reference to it after the push_back...

Am I correct or wrong here?

Thanks.

Jbu

edit: I think I am wrong, since new will return a pointer...we'll always have the pointer to be able to delete the new String

Solution 1

No, the vector stores pointers and the copy is made of the pointer. You can delete the object any time later.

(You may get a leak, if the statement happens to throw an exception and you don't catch and handle it properly. That's why you might consider using smart pointers.)

Solution 2

Yes, but not for the reason you think.

Depending on how list is defined and initialized, push_back might throw an exception. If it does, the pointer returned from new is lost, and can never be freed.

But assuming push_back returns successfully, it stores a copy of the pointer returned by new, and so we can free the memory later by calling delete on that copy, so no memory is leaked as long as you do call delete properly.

Solution 3

If I saw this code I would be very suspicious a memory leak was possible. On the surface it appears to be adding an allocated String* into a list<String*>. In my experience this is often followed by bad error handling code which does not properly free the allocated memory.

While this dangerous in many circumstances it is not necessarily a memory leak. Consider the following example:

class Container {
  ~Container() {
    std::list<String*>::iterator it = list.begin();
    while (it != list.end()) {
      delete *it;
      it++;
    }
  }

  void SomeMethod() {
    ...
    list.push_back(new String("hi"));
  }

  std::list<String*> list;
}

In this code there is no leak because the containing class is responsible for the memory allocated and will free it in the destructor.

EDIT

As aschepler pointed out there is still a leak if the push_back method throws an exception.

Solution 4

Yes, this is a memory leak unles you somehow take steps to delete the contained pointers.

And the best way to accomplish that is to use a smart pointer. For example, Boost's shared_ptr or C++0x's shared_ptr.

Solution 5

You are correct, provided nothing deletes the string when it is removed from the list.

View more solutions

Comments

[aschepler]

Depends what list is. Assuming it's a std::list<String*>, @UncleBens is correct: it may still be possible to clean up correctly most of the time. But you must do that cleanup manually; std::list will not do it for you.

[Drew Delano]

scaled_ptr?? Do you mean shared_ptr?

[aschepler]

If the call to push_back throws an exception, this is still a leak.

[James McNellis]

There is also a risk of double deletion unless you declare a copy constructor and copy assignment operator.

[Steve Jessop]

"if the statement happens to throw an exception and you don't catch and handle it properly" - in the case of this statement there is no way to handle it properly, really. If push_back throws, then it hasn't stored the pointer, and there's no way to free it because this calling code doesn't have the pointer either. I suppose in theory you could maybe do something with String::operator new, but that doesn't sound like fun.

[Drew Delano]

The only other thing I could think you meant was scoped_ptr, but that won't work in a container since it's noncopyable.

[default]

@Fred: It's probably a future version of Boost :)

[Admin]

I am sorry but i couldn't figure out whther the pointer is just copied or the pointed to data is also copied? For Ex :- String *str = new String("HI"); list.push_back(str); Now can i call delete str and still be able to access "HI" from the list because valgrind shows a leak at list.push_back without any delete calls.