Deleting objects on the heap which store data on the heap

Question

My program has been written using classes from the SDL library.

I have the following class:

class s_group
{
    private:
        SDL_Surface* image;
        unsigned int* F_total;
        float* F_length;
        SDL_Rect** F;
        float* F_current;
        unsigned int S_total;
        unsigned int S_current;

    public:
        s_group(void);
        virtual ~s_group(void);

        bool setup( const char* filename, unsigned int s );
        //other member functions
};

Private member pointers each store the location of memory declared on the heap, as allocated by the member function setup.

bool s_group::setup( const char* filename, unsigned int s )
{
    s_group::~s_group();//delete already allocated heap memory
    if(!load_file(image, filename))
    {
        image = NULL;
        return false; 
    }

    S_total = s;
    F = new SDL_Rect*[S_total];
    F_total = new unsigned int[S_total];
    F_length = new float[S_total];
    F_current = new float[S_total];

    for(unsigned int index = 0; index < S_total; ++index)
    {
        F[index] = NULL;
        F_total[index] = 0;
        F_length[index] = 0.f;
        F_current[index] = 0.f;
    }
    //loop for each array slot and set values of data
    return true;
}

Within a large function I create an object of this class on the heap, storing its address in an s_group pointer named sparkle.

s_group* sparkle = new s_group;
sparkle->setup("sparkle_final.png", 1 );

On completion of the function I call delete to reallocate the heap memory. Removing this line solves the problem, however there would then be a memory leak.

delete sparkle; 
sparkle = NULL;

This will call the destructor of the class which is where I believe the error occurs due to an internal use of the delete operator.

s_group::~s_group(void)
{
    SDL_FreeSurface(image);
    image = NULL;

    for(unsigned int s = 0; s < S_total; ++s)
    {
        for(unsigned int f = 0; f < F_total[s]; ++f)
        {
            F[s][f].x = 0; 
            F[s][f].y = 0;
            F[s][f].w = 0; 
            F[s][f].h = 0;
        }
        delete[] F[s]; 
        F[s] = NULL;
    }
    delete[] F; 
    F = NULL;

    delete[] F_total; 
    F_total = NULL;

    delete[] F_length; 
    F_length = NULL;

    delete[] F_current; 
    F_current = NULL;

    S_total = 0;
    S_current = 0;
}

On reaching the delete operator, a dialog box appears stating:

Windows has triggered a breakpoint in Program.exe. This may be due to a corruption of the heap, which indicates a bug in Program.exe or any of the DLLs it has loaded.

How do I delete this object without causing the heap corruption?

Answer 1

Since posting this question I have located the source of the error and solved the issue. In a separate section of code which set the data values for the dynamic 2D array the loop validation was incorrect.

for( unsigned int index = 0; index <= F_total[ S_current ]; ++index ) {  
    //set data values for each slot in the array
    F[ S_current ][ index ].x = 0; etc...
}

As can be seen the loop will clearly attempt to modify a location equal to the size of the created array. Noting of course that arrays begin at index 0, so the final slot will be at size - 1. Something very silly that I missed when writing the code. Actual loop:

for( unsigned int index = 0; index < F_total[ S_current ]; ++index ) {  
    //set data values for each slot in the array
    F[ S_current ][ index ].x = 0; etc...
}

A message for anyone attempting their own memory management:

• Finding the source of heap corruption is difficult as the compiler will locate the error in sections of code which do not necessarily cause the problem.
• The cause of the problem will only ever be in the section of your code which is affecting the memory. Ensure that you do not attempt to access or worse modify any memory that you have not been given.
• I still believe that memory management is a great way to learn and would rather complete any projects in this way than using containers or smart pointers as recommended. This is my personal preference despite custom memory management often offering very few advantages, only complexities.
• When asking for assistance provide all related code on the problem. Although the compiler may direct you to the problem in one section, as I said before, with heap corruption it's not necessarily there.

Answer 2

From effective C++ Scott Meyers

Item 9: Never call virtual functions during construction or destruction.

You shouldn't call virtual functions during construction or destruction, because the calls won't do what you think, and if they did, you'd still be unhappy. If you're a recovering Java or C# programmer, pay close attention to this Item, because this is a place where those languages zig, while C++ zags.

Actually, even though you should define your destructor, calling it forcibly should be out of the question

Answer 3

I'm unable to compile your code but here goes..

The first thing I noticed was that you called your destructor.. You don't want to do that! Instead, create a release function and call that.

The next thing I noticed is that there is no FRAME variable within the class itself.. so this line:

FRAME = new SDL_Rect*[S_total];

is going to cause a compilation error and your destructor uses FRAME but no such variable exists. I think you meant to change it to F because if not, then this line:

F[index] = NULL;

is undefined behaviour since F is uninitialized..

Also, you never initialized each index of FRAME and so accessing it in the destructor like:

FRAME[s][f].x = 0;

is a no-no.

Again, you call

delete[] F; 
F = NULL;

but F has no memory allocated and is uninitialized.

Thus with all the patches I think:

class s_group
{
    private:
        SDL_Surface* image;
        unsigned int* F_total;
        float* F_length;
        SDL_Rect** FRAME;
        float* F_current;
        unsigned int S_total;
        unsigned int S_current;

        void Release();

    public:
        s_group(void);
        virtual ~s_group(void);

        bool setup(const char* filename, unsigned int s);
        //other member functions
};


bool s_group::setup(const char* filename, unsigned int s)
{
    Release();//delete already allocated heap memory

    if(!load_file(image, filename))
    {
        image = NULL;
        return false; 
    }

    S_total = s;
    FRAME = new SDL_Rect*[S_total];
    F_total = new unsigned int[S_total];
    F_length = new float[S_total];
    F_current = new float[S_total];

    for(unsigned int index = 0; index < S_total; ++index)
    {
        FRAME[index] = NULL;
        F_total[index] = 0;
        F_length[index] = 0.f;
        F_current[index] = 0.f;
    }
    //loop for each array slot and set values of data
    return true;
}

void s_group::Release()
{
    SDL_FreeSurface(image);
    image = NULL;

    for(unsigned int s = 0; s < S_total; ++s)
    {
        for(unsigned int f = 0; f < F_total[s]; ++f)
        {
            if (FRAME[s])
            {
                FRAME[s][f].x = 0; 
                FRAME[s][f].y = 0;
                FRAME[s][f].w = 0; 
                FRAME[s][f].h = 0;
            }
        }
        delete[] FRAME[s]; 
        FRAME[s] = NULL;
    }
    delete[] FRAME; 
    FRAME = NULL;

    delete[] F_total; 
    F_total = NULL;

    delete[] F_length; 
    F_length = NULL;

    delete[] F_current; 
    F_current = NULL;

    S_total = 0;
    S_current = 0;
}

s_group::~s_group(void)
{
    Release();
}

should do it.. Just don't forget to allocate memory for FRAME[index] I wasn't sure how much or what you wanted to allocate so I changed the Release function to check if FRAME[index] is valid with an if-statement

I would strongly advise that you use some SmartPointers and forget about handling every single memory allocation yourself..