Resource leak during object creation

Question

I have the following code for creation of a node inside a graph. I'm getting resource leak error when I run a static checking tool (coverity). I would appreciate if you can point out how to improve the code:

class node {   
   public :  
     explicit node(std::string& name) : m_name(name) { }  
     void setlevel(int level)  
     { m_level = level; }  
   private :    
     ...  
 }  
class graph {  
   public :  
      void populateGraph()  
      {  
         std::string nodeName = getNodeName();   
         /* I get error saying variable from new not freed or pointed-to in function  
            nc::node::node(const std::string...) */  
         node* NodePtr = new node(nodeName);  
         /* I get error saying variable NodePtr not freed or pointed-to in function  
            nc::node::setLevel(int) */   
         NodePtr->setLevel(-1);  
         if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end())  
             m_name2NodeMap[nodeName] = NodePtr;  
         NodePtr = NULL;  
      }  
....  
private :  
  std::map< std::string, node*> m_name2NodeMap;   
}

I thought I needed to delete NodePtr in populateGraph, but then released it will call node desctructor (~node) and delete the node from the graph. So, I set NodePtr=NULL to see if it helps, but it is not.

Pulsate

Every new requires a delete and you have no deletes anywhere. If the compiler allocated memory for a Node object, compiler will clean it up. But if you explicitly use "new", you will need to clean up with "delete". Also it seems to me, you should check if nodeName exists in your map before you create. Your graph class should have a deconstructor which will call delete on each element in m_name2NodeMap. – Moberg

@Apprentice Queue: no, every new does not require a delete. Every new result shall be stashed in a resource manager (smart pointer or whatever) that would dispose of the memory when it is called for. Trying to do it manually is akin to juggling with fire torches in a gun powder barrel. – Kinson

@Matthieu M., I think using a "resource manager" here is overkill and blackboxes the basic idea that what is allocated should be deallocated. – Moberg

@Apprentice Queue: It's not. A unique_ptr, a scoped_ptr or a auto_ptr are far from being overkill. They are simple, they work well, and unlike hand-written code, they don't screw up. delete is a code smell (or "code omen", a warning of impeding doom). RAII is the only reliable solution, and anyone choosing an unreliable solution is either malign or ignorant. I hope the latter can be fought off. – Kinson

Answer 1

I am not familiar with coverity or the exact rules that it uses, but it seems that you will have a memory leak if the name of the node is already in the map. That is, if the body of your if statement is not executed, then you loose the pointer to the memory that you just allocated. Perhaps you wanted something like:

if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end())  
    m_name2NodeMap[nodeName] = NodePtr;  
else
    delete NodePtr;
NodePtr = NULL; 

Edit: since I responded almost at the same time as Daemin, let me add more details:

As ildjarn mentioned, you also need to deallocate those objects that do end up in the map by adding a destructor:

~graph()
{
    for( std::map< std::string, node*>::iterator i = m_name2NodeMap.begin(); 
         i != m_name2NodeMap.end(); ++i )
    {
        delete i->second;
    }
}

For completeness, I should note that:

1. The map will be deleted automatically after the destructor finishes because it's a member variable.
2. The entries in the node map will be deleted automatically when the map is deleted.
3. The string keys will be deleted when the entries are deleted.

---

The preferred way to deal with complex object lifetimes is to use a smart pointer. For example, the boost::shared_ptr or the tr1::shared_ptr would work like this. Note: I may not have the syntax exact.

class node {   
    ...
}

class graph {  
    public :  
    void populateGraph()  
    {  
        std::string nodeName = getNodeName();   
        boost::shared_ptr< node > NodePtr( new node(nodeName) );
        NodePtr->setLevel(-1);  
        if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end())  
            m_name2NodeMap[nodeName] = NodePtr;
    }  
    ....  
    private :  
        std::map< std::string, boost::shared_ptr<node> > m_name2NodeMap;   
    }
};

See how we've eliminated the destructor and explicit calls to delete? Now the node objects will be destroyed automatically just like the node names.

On another node, you should look into the std::map::insert function which should eliminate that if statement all together.

Bodkin

Thanks for the answer. I have a question about "2. The entries in the node map will be deleted automatically when the map is deleted". By entry do you mean each element is deleted..If so, why is the node* in each element not deleted. E.g. Map["A"] = AddrofObjA. Let's say each node Obj takes 50 bytes, wouldn't deletion of entry "A" in map, delete the 50 bytes starting from AddrofObjA? Thanks – Pulsate

shared pointers are for expert only. Shared ownership is extremely difficult to manage properly (both semantically and technically) and should only be envisaged as a last resort. Oh, and if one needs a destructor, one also needs a copy constructor and assignment operator. – Kinson

@srikrish: what you are storing in the map are pointers to nodes. The map is only storing the key "A" and a memory address. When you delete the element from the map, it frees the memory associated with the key and the address. However, it does not automatically delete whatever is stored at the address. Note: I am glossing over the issue that the key is stored in an std::string object which does some funny stuff with memory management, but it gives the illusion that the key is just stored right there in the element and deleted with it. – Bodkin

@Matthieu: I would disagree that shared pointers are a 'last resort'. While it is true that they have caveats such as circular reference problems, I would say that they are a lot safer to use and easier to prove correctness for than writing your own destructor. In particular, if what you want to do is to store pointers, then I think shared pointers are better. – Bodkin

I didn't suggested to write your own destructor (which as I said would be inconsistent, at best, unless you also write or disable the copy and assignment). I wanted to point out that many people hold the shared_ptr as a silver bullet against memory leak. They are not. There are many simpler alternatives. If you want a map who own polymorphic values, the idiomatic solution is boost::ptr_map<Key,Value>. You can emulate it with std::map<Key, std::unique_ptr<Value>> in C++0x but it does not offer deep copying and some iterator sugar coating like ptr_map does. – Kinson

Answer 2

What you need to do is give graph a destructor and inside of it, delete all the node*s in m_name2NodeMap. And of course, because you need a destructor, you also need a copy constructor and a copy assignment operator, otherwise you're guaranteed to get memory corruption.

You'll also need an else clause for if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end()) to delete NodePtr;.

Africa

Answer 3

You're not freeing the NodePtr when you don't add it to the list. The if statement needs an else where you delete NodePtr;

if (m_name2NodeMap.find(nodeName) == m_name2NodeMap.end())
{
    m_name2NodeMap[nodeName] = NodePtr;
}
else
{
    delete NodePtr;
}
NodePtr = NULL;

Kanchenjunga

Answer 4

Others have already covered the issue of the leak. In fact there are leaks aplenty, so I won't even bother commenting on them all... (populateGraph, ~Graph, Graph(Graph const&) and Graph& operator=(Graph const&) at the very least...)

I prefer to offer a simple solution that works:

class Graph
{
public:
  void addNode(std::string name) {
    _nodes.insert(std::make_pair(name, Node(name));
  }

private:
  std::map<std::string, Node> _nodes;
};

What's going on here ?

• The dynamic memory allocation is unnecessary, the map can perfectly contain the Node by value, and you won't have any leak this way.
• std::map::insert will only perform the insertion if no equivalent key is already present, no need to do a find + [] (which is twice as complicated since you compute twice the place where to store the element)

Kinson