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.

I will just admit it. There wasn't a cast in the first place, and I came upon a page (dreamincode.net/code/snippet2835.htm) where the author did a cast, and I tried it. — Extrakun
– Extrakun, Commented Aug 6, 2009 at 8:33
To FireAphis: uint8_t** red; uint8_t** green; uint8_t** blue; Sorry that I've missed it out. — Extrakun
– Extrakun, Commented Aug 6, 2009 at 8:33

1800 INFORMATION · Accepted Answer · 2009-08-06 08:30:09Z

5

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.

answered Aug 6, 2009 at 8:30

1800 INFORMATION

137k30 gold badges164 silver badges242 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Kaz Dragon Over a year ago

+1 - I was going to answer the same way. It looks like the original poster hasn't followed the Rule of 3, and is deleting pointers that have been copied then deleted.

Extrakun Over a year ago

Thanks, adding the copy constructor does solve the problem. I guess this is a good lesson for me too.

1800 INFORMATION Over a year ago

Note that if you create a copy constructor, you probably also need to write an operator= to do the same thing.

Timo Geusch · Accepted Answer · 2009-08-06 08:35:28Z

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.

Indy9000 · Accepted Answer · 2009-08-06 08:53:24Z

1

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

edited Aug 6, 2009 at 8:53

answered Aug 6, 2009 at 8:37

Indy9000

8,8712 gold badges35 silver badges38 bronze badges

Comments

anon · Accepted Answer · 2009-08-06 08:27:39Z

0

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.

answered Aug 6, 2009 at 8:27

anon

Collectives™ on Stack Overflow

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

4 Answers 4

3 Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

3 Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related