Resource leak during object creation

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_leve开发者_运维百科l = 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.

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.

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;.

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;

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)