Memory Management Confusion C++

Question

I'm having trouble understanding some basic memory management principles in C++. This code is part of a loop that is part of a function that reads in a maze file into a 2D vector.

According to Valgrind, the following code is causing memory leaks...

Note that t is a MazeNode object and verts is a vector within the t class that holds pointers to node objects (not to be confused with MazeNode objects):

node* top = new node(TOP, rowCount, i, t.type);
node* bot = new node(BOTTOM, rowCount, i, t.type);
node* left = new node(LEFT, rowCount, i,  t.type);
node* right = new node(RIGHT, rowCount, i, t.type);

t.verts.push_back(top);
t.verts.push_back(bot);
t.verts.push_back(left);
t.verts.push_back(right);

temp.push_back(t);

top = NULL;
bot = NULL;
left = NULL;
right = NULL;
delete top;
delete bot;
delete left;
delete right;

Initially I did not set each of the pointers to NULL before deleting them, but would get allocation errors. So I just set them to NULL and my code works. I guess I'm just really confused why this would cause memory leaks and why I would need to set the pointers to NULL. There is probably a way easier non-pointer way to do this, but maybe this problem will help me understand memory management better.

Thanks everyone.

EDIT: Here's the MazeNode class (which is what 't' is) (also excuse my lazyness in writing this class, making everythign public like a struct)

class MazeNode 
{
public:
    void setType(char c);
    char getChar();

    NodeType type;
    vector<Direction> visitedFrom;

    vector<node*> verts;
};

And the node class:

class node
{
public:
    node();
    node(Direction d, int r, int c, NodeType t);
    ~node(); //empty definition
    node(const node* n);
    node& operator=(const node& n);

    void addAdj(node* a, int w);
    void printAdj() const;
    string direction() const;

    void print() const;
    bool operator<(const node& n) const;


    int distance; //from start
    bool visited;
    node* prev;
    vector<Edge> adj;
    Direction dir;
    int row, col;
    NodeType type;
};

EDIT2: Thanks everyone. I understand the problem now. I changed my vectors of pointer objects so that I wasn't using pointers anymore.

Answer 1

Prior to adding the null assignment, your code had a problem different (worse) than a memory leak: storing and probably also using a stray pointer, that is, a pointer pointing into de-allocated memory.

Making it a memory leak by adding the null assignment makes it better, but not much.

The real solution is not to keep any pointers anywhere after you have called delete on them. That is, do not push_back or do not delete here.

Answer 2

You are placing the pointers into a container, then deleting the pointers. When your code later tries to use those pointers they are invalid and cause a crash.

By setting the pointers to NULL before you delete them, you end up not deleting them at all - deleting a NULL pointer doesn't do anything. But now there's nothing to delete the objects later, and you get a memory leak.

You need to find some spot in the code where you're not using the pointers anymore, and delete them there.

Edit: Of course I should have mentioned that a smart pointer such as std::shared_ptr eliminates this hassle altogether, because it deletes the object automatically.

Answer 3

You are setting the values to NULL before deleting them, so you are trying to delete NULL and nothing is being deleted. Try moving the delete calls above the NULL calls.

Answer 4

This confusion is exactly why I create a macro for these kind of things:

#define delobj(obj) (delete obj, obj = NULL)

And you would use it like this:

delobj(top);
delobj(bot);
delobj(left);
delobj(right);

Answer 5

The error is using the vector of pointers. According to you, verts is this:

vector<node*> verts;

But what it should be is this:

vector<node> verts;

In the first case, when you push_back() the pointer, that's OK, but when you pop_back or otherwise re-size the vector, the pointer is the "contents" of the vector, and is de-allocated, but not what the pointer points to, which is the node. Hence the node leaks. But in the second case, the node is "part" of the vector, and is allocated/deallocated as part of re-sizing the vector.

Your pattern here probably indicates a Java/C# background, as "new-ing" into a container is very very common in those languages, but to do that in C++, you need a container of smart pointers (like vector<shared_ptr<node>> or something), which is probably beyond the scope of the question. But in those languages, every reference to a reference type is a "smart pointer" (more or less) and so this is done automatically. C++ isn't like that.

You either need to change your code to use a vector<node> (and change how you're pushing back on to it) or you need to explicitly de-allocate your nodes when the vector shrinks.

Answer 6

Change to:

delete top;
delete bot;
delete left;
delete right;

top = NULL;
bot = NULL;
left = NULL;
right = NULL;

And it should work.