Problems deleting a 2D dynamic array in C++ (which is eventually store in a vector)

Question

So I have this 2d dynamic array which content I want to free when I am done with it. However I keep running into a heap corruption after the destructor. The code works fine (of course with memory leaks) if I comment out the destructor. (Visual Studio 2005)

FrameData::FrameData(int width, int height)
{
    width_ = width;
    height_ = height;

    linesize[0] = linesize[1] = linesize[2] = linesize[3] = 0;

    // Initialise the 2d array
    // Note: uint8_t is used by FFMPEG (typedef unsigned char uint8_t)
    red = new uint8_t* [height];
    green = new uint8_t* [height];
    blue = new uint8_t* [height];

    for (int i=0; i < height; i++)
    {
     red[i] = new uint8_t [width];
     green[i] = new uint8_t [width];
     blue[i] = new uint8_t [width];
    }  
}

FrameData::~FrameData()
{

    // Delete each column
    for (int i=0; i < height_; i++)
    {      
     delete[] ((uint8_t*) red[i]);
     delete[] ((uint8_t*)green[i]);
     delete[] ((uint8_t*)blue[i]);  
    }

    // Final cleanup
    delete[] red;
    red = NULL;

    delete[] green;
    green = NULL;

    delete[] blue;
    blue = NULL; 
}

I have no idea what is wrong with the code. The only another thing is somewhere else, I did this in a loop where the crash occurred

FrameData myFrame;
std::vector<FrameData> frames;
...snipped...
frames.push_back(myFrame);

This shouldn't be causing any problem, right? If I remember correct, push_back makes a copy instead of storing a pointer or a reference.

PS. Yes, I should use vectors. But I am not allowed to.

Additional Info:

The operator= and copy constructor are not defined. I guess that's a reason for the problem.

Answer 1

You are correct about push_back making a copy, but does FrameData have a suitable copy constructor and assignment operator?

Also, why the cast here:

delete[] ((uint8_t*) red[i]);

In C++, if you have to use a C-style (or reinterpret) cast, the code is almost certainly wrong.

Answer 2

Your problem is as you guessed in here:

FrameData myFrame;
std::vector<FrameData> frames;
...snipped...
frames.push_back(myFrame);

The vector makes copies of the elements that you push in. What do you have for your copy constructor and/or operator= for your class? If you have none defined, the default version that the compiler creates for you simply makes copies of the members of your class. This will copy the pointer members red, green and blue to the new instance. Then the old instance that you copied will be destroyed when it goes out of scope, causing the pointers to be deleted. The one you copied into the vector will then have invalid pointers since the target of the pointer is thus deleted.

A good rule of thumb is that if you have any raw pointer members, then you need to make a copy constructor and operator= that will handle this situation correctly, by making sure that the pointers are given new values and not shared, or that ownership is transferred between the instances.

For example, the std::auto_ptr class has a raw pointer - the semantics of the copy constructor is to transfer ownership of the pointer to the target.

The boost::shared_ptr class has a raw pointer - the semantics is to share ownership by means of reference counting. This is a nice way to handle std::vectors containing pointers to your class - the shared pointers will control the ownership for you.

Another way might be to use vectors to take the place of your member pointers - the member pointers are simply aliases for your arrays anyway, so the vector is a good substitute.

Answer 3

Unless you have a deep copy constructor and assignment operator for the FrameData class my gut feeling is that the compiler generates a copy constructor to use with push_back. Automatically generated copy constructors and assignment operators will do a memberwise copy, which will result in a shallow copy in this instance. Unfortunately your destructor doesn't know about the copy so during the copying, there is a good chance that a temporary copy of FrameData gets destroyed and that will take all your data with it.

Calling the destructor again later in the process will result in a double free, plus other allocations might have used part of the "free" memory. That looks like a good reason for heap corruption from here.

Best way to find problems like these is usually to use a tool like ValGrind or Purify to pinpoint the problem.

Answer 4

This is not an answer to your question, just an observation.

Since your frame data could be large, to avoid excessive copying, may be it's better for you to use

std::vector<FrameData *> frames;

EDIT: As others have pointed out, this will also solve your crashing problem.