Failing to understand how to fix this memory leak

Question

I'm creating a small solver that takes three arguments: number of hours, current time and true time.

The program itself works fine, but there are memory leaks that I am unable to fix.

Code:

Inside my main:

vector<ClockState> moves = sol.solve(curClock);

solve method:

std::vector<Game> Solver<Game>::solve(Game curCfg){
    std::vector<Game> result;
    std::vector<Game> seen;
    std::queue<Game> q;

    q.push(curCfg);
    seen.push_back(curCfg);

    std::vector<Game> moves;

    Game cfg = q.front();
    Game * newCfg = NULL;

    while (q.size() > 0) {

        if (q.front().isEndState()) {
            break;
        }

        cfg = q.front();
        q.pop();

        cfg.getMoves(moves);
        for (unsigned int i = 0; i < moves.size(); i++) {
            if (find(seen.begin(), seen.end(), moves[i]) == seen.end()) {
                newCfg = new Game(cfg);
                moves[i].setPrev(newCfg);
                q.push(moves[i]);
                seen.push_back(moves[i]);
            }
        }
    }

    delete newCfg;

    if(q.empty()){
        return result;
    }

    Game temp(q.front());

    while (true) {
        result.push_back(temp);
        if (temp == curCfg) {
            break;
        }
        temp = temp.prev();
    }
    reverse(result.begin(), result.end());
    return result;
}

And my setPrev method (inside ClockState)

void ClockState::setPrev(ClockState *prev) {
    previous = prev;
}

previous is a pointer inside the ClockState class.

From my understanding I need to delete newCfg and even when I try to do that it still causes a memory leak.

Here is the output from valgrind --leak-check=full ./clock 12 10 3

==16856==
==16856== HEAP SUMMARY:
==16856==     in use at exit: 64 bytes in 4 blocks
==16856==   total heap usage: 28 allocs, 24 frees, 2,095 bytes allocated
==16856==
==16856== 64 (16 direct, 48 indirect) bytes in 1 blocks are definitely lost in loss record 2 of 2
==16856==    at 0x402569A: operator new(unsigned int) (vg_replace_malloc.c:255)
==16856==    by 0x8049AB1: Solver<ClockState>::solve(ClockState) (Solver.h:102)
==16856==    by 0x804964B: main (clock.cpp:106)
==16856==
==16856== LEAK SUMMARY:
==16856==    definitely lost: 16 bytes in 1 blocks
==16856==    indirectly lost: 48 bytes in 3 blocks
==16856==      possibly lost: 0 bytes in 0 blocks
==16856==    still reachable: 0 bytes in 0 blocks
==16856==         suppressed: 0 bytes in 0 blocks
==16856==
==16856== For counts of detected and suppressed errors, rerun with: -v
==16856== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 17 from 6)

Line 102 is newCfg = new Game(cfg);

When Solver returns that vector, I end up printing the results, and directly after that I delete each previous with the following:

for(int j = 0; j < moves.size(); j++){
    moves[j].deletePrev();
}

deletePrev() just says delete previous;

Appreciate it.

Better use a smart pointer (e.g. shared_ptr) so you don't need to care about pairing new and delete. — kennytm, May 17 '12 at 17:17

score 4 · Accepted Answer · answered May 17 '12 at 17:18

4

You create new objects in aloop, but delete only one, which causes your memory leak.

You need to loop through each move and delete the element pointer to by the previous pointer

Alternatively, just create only one Game object and delete it when you are done with it (as you are re-creating identical objects many times over)

Yet another alternative is to copy the Game object by value (so no new or delete is needed) if you don't want to share the object among many other object

Another approach is to use some sort of smart pointer (e.g. from BOOST or use C++11)

answered May 17 '12 at 17:18

Attila

28,265
3
46
55

Are you suggesting deleting newCfg before the while loop is completed, or after the for loop is finished? – Marcus Recck May 17 '12 at 17:19
`newCfg` has only the last object you created, you need to go through all the others as well. As for when to delete: when you no longer need the object (so no code will try to access the deleted object afterward) – Attila May 17 '12 at 17:25
Well after the solver completes iterating it returns the moves which I then iterate through to print out each step. Suggesting after I print out each step I iterate over the results and delete the previous there? – Marcus Recck May 17 '12 at 17:28
Yes, if you no longer use the moves array after that point (more precisely, their `previous` members) – Attila May 17 '12 at 17:33
Doing that still causes a memory leak inside the Solver class. Only 16 direct and 48 indirect lost this time though. I created a deletePrev() method which deletes previous. – Marcus Recck May 17 '12 at 17:44
I don't see any other dynamic memory allocation on your side and the `std::vector<>` classes empoy RAII, so they should be fine too. Are you doing any more dynamic allocation in the other methods of `Solver`? – Attila May 17 '12 at 17:54
let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/11394/discussion-between-marcus-recck-and-attila) – Marcus Recck May 17 '12 at 18:00

Mike Corcoran · Answer 2 · 2012-05-17T17:45:51.877

2

this code:

Game * newCfg = NULL;
while (q.size() > 0) {

    if (q.front().isEndState()) {
        break;
    }

    cfg = q.front();
    q.pop();
    std::vector<Game> moves;
    cfg.getMoves(moves);
    for (int i = 0; i < moves.size(); i++) {
        if (find(seen.begin(), seen.end(), moves[i]) == seen.end()) {
            newCfg = new Game(cfg);
            moves[i].setPrev(newCfg);
            q.push(moves[i]);
            seen.push_back(moves[i]);
        }
    }
}

delete newCfg;

declares a variable named newCfg that is a pointer to a Game. However, isnide of the loop, you keep new'ing new instances of a Game class and adding them to the moves container. you need to 'delete' each instance of Game* that you've added to the moves container.

your code above essentially does the same thing as this code:

int* myInts = NULL;
myInts = new Int[2];
myInts = new Int[5];
delete myInts; // <-- only deletes memory for Int[5],
               // Int[2] memory is dangling in no-mans-land

edited May 17 '12 at 17:45

answered May 17 '12 at 17:19

Mike Corcoran

14,072
4
37
49

Thanks for the comment. After I print the results of my solver I now delete each previous for each Game inside moves. After that however I still have a leak in the Solver.h class. – Marcus Recck May 17 '12 at 17:48
The leak still occurs at `newCfg = new Game(cfg);` – Marcus Recck May 17 '12 at 17:54
are you still leaking the same amount of memory? or is it significantly less after `delete`'ing the Game*'s in your move vector? – Mike Corcoran May 17 '12 at 18:02
It's now down to 16 direct, 48 indirect lost. – Marcus Recck May 17 '12 at 18:04
could you update your original post with the code as it is now and an updated valgrind report? i'm not sure why you're still leaking memory, someone else might be able to point you in the right direction... – Mike Corcoran May 17 '12 at 18:18
you probably also want to post the code where you ultimately delete the Game*'s from the move vector. – Mike Corcoran May 17 '12 at 18:46

Failing to understand how to fix this memory leak

2 Answers2