Double Free Error

Question

I made a function for an object called copy() that should just return an instance of the object with all the same values -

Grid Grid::copy() {

Grid result;

result.setFilename(f_name);
result.setNumOfRows(num_rows);
result.setNumOfCols(num_cols);
result.setMap(map);


return result;
}

My destructor looks like this -

Grid::~Grid() {
for(int r=0;r<num_rows;r++)
    delete [] map[r];
}  

Now whenever my code is running and the copy function gets called, I get an error

*** glibc detected *** ./go: double free or corruption (!prev): 0x0982c6a8 ***

with a lot of other information (big wall of text) after that. That just means the memory is being deleted twice correct? If so, how can this be? Why does the destructor get called twice?

The code where it gets called looks like this -

for(;;) {
    Grid g;

    if(which_display == 1) {

       .....
       .....
        g = myServer->getAgent()->getGrid()->copy(); //HERE


    }
    //print
    std::cout<<g.toString();
}

I feel like I'm missing something obvious. Can someone point out to me how the destructor is being called twice?

Answer 1

Your copy function is not creating a deep copy of map; it is just copying the pointers map contains. When the destructor is called on the original object and the copy, those pointers are being deleted twice.

Answer 2

You're missing a copy constructor and assignment operator. It's the Law of the Big Three.

The Big Three are:

1. copy constructor
2. assignment operator
3. destructor

The law of the big three is that if you need one of them there's a good chance that you need all three. They usually involve handling resources in a nontrivial way.

In your example you explicitly free memory in the destructor. This probably means that you need to handle memory specially in the copy constructor and the assignment operator: either correctly allocate new memory and copy the values, or block copying and assignment (by declaring them private and not implementing them).

Answer 3

You are returning a temporary object from your copy function. What you probably want is to allocate Grid at heap and then pass around a pointer (or better, a smart pointer):

Grid *Grid::copy() {

Grid *result = new Grid();

result->setFilename(f_name);
result->setNumOfRows(num_rows);
result->setNumOfCols(num_cols);
result->setMap(map);


return result;
}

Smart pointer version (you can also use std::shared_ptr with C++11):

boost::shared_ptr<Grid> Grid::copy() {

boost::shared_ptr<Grid> result(new Grid());

result->setFilename(f_name);
result->setNumOfRows(num_rows);
result->setNumOfCols(num_cols);
result->setMap(map);


return result;
}

In the code you posted, result gets destroyed when the function exits and you get undefined behavior.

EDIT: Also make sure to deep copy the map as mentioned in comments by Chad. Alternatively, you can use a shared_ptr on it as well to save the copy costs.

Answer 4

You don't actually want a copy method at all. You simply want a copy constructor and assignment operator. I'm guessing your line originally looked like this:

g = myServer->getAgent()->getGrid();

And since that didn't work, you added the copy method. But the way it is now, fixing your copy method alone will not solve the problem, as you need the copy constructor and assignment operator too, or your hard work in fixing the copy method could be destroyed.

First, a quick explanation as to what is happening, and why the program fails:

1. You call copy
2. This enters your copy method, which creates a Grid on the stack.
3. It sets the members of the Grid, but we suspect it does a shallow copy.
4. The copy method returns, which invokes the copy constructor of Grid. *
5. The default copy constructor does a shallow copy.
6. The stack-based Grid's destructor fires, deleting the contents of map.
7. The copy method has now returned, providing a temporary Grid, but one that points to deleted memory.
8. Now the temporary Grid object is assigned into g. This invokes the assignment operator of Grid. *
9. The default assignment operator does a shallow copy, just like the default copy constructor did.
10. At the end of the line, the temporary object is destroyed, where it tries to delete the contents of map -- which were already deleted. boom.
11. When g goes out of scope, its destructor will try to delete the contents of map yet again.

As you can see, there are 3 places where a shallow copy occurs -- all must be fixed or this will still fail.

How to fix this

1. Get rid of the copy method -- it doesn't provide any value anyway.
2. Fix your setMap and setFilename to do a deep copy.
3. Create an assignment operator. This should deep-copy the contents of the other Grid.
4. Create a copy constructor, just like the assignment operator.

Here's what the assignment operator could look like (assuming all set methods do deep copy):

Grid& operator= (const Grid& g) {
  setFilename(f_name);
  setNumOfRows(num_rows);
  setNumOfCols(num_cols);
  setMap(map);

  return *this;
}

There are techniques to write the copy constructor, and then make the assignment operator use the copy constructor. This is a good technique (less duplicated code), but I don't have the link handy. When I find it, I'll link it here.

Lastly, I marked a few lines in my explanation(*). Compilers can do Return Value Optimization (RVO), and Named RVO. With these optimizations, it would not actually create the Grid object on the stack within copy, and then copy-construct for the return value -- it would simply create the temporary object for the result of copy, and then the copy method would use that instead of its own internal stack-based Grid object. So with enough compiler optimizations, your code might make it past this point and crash later. Obviously that's not helpful, so this is more of an fyi.

Answer 5

There are important parts missing in the code you posted, like the class definition and the implementation of setMap. Nonetheles, I can infer from what I saw that probably the problem happens when the compiler invokes the default copy constructor for the temporary object (the return value). You end up having two Grid objects whose map member points to the same memory that was allocated only once, and obviously their respective destructor calls will conflict. If you have any dynamically allocated member in your class (like map seems to be) you have to do one of these things:

a) Fully support value semantics, implementing the default and copy constructor and the assignment operator b) Prohibit it, by declaring (and not defining) the copy constructor and the assignment operator private.

Option b) is the preferred choice if your object is costly (in memory and execution time) to create.

If you go for (a), you don't need a copy() method, just do an assignment. If you go for (b), your copy() method should return a pointer (preferably a smart pointer), as exemplified by dark_charlie. In that case, I'd also suggest to rename copy() to clone(), which is the most popular convention for the name of such a method.