Heap corruption when deleting class instance of another class

Question

(c++ class issue)

This is really getting me down,when ever the following class deletes its member pointer (to class player) in it's destructor I get a 'Heap corruption after normal block' error.

//Team.h
class Team
{
public:
    Team (const char* team_dir_path);
    ~Team(void);

    Team (const Team &team);

    Player* player;
    int player_cnt;
};

//Team.cpp

Team::Team( const char* team_dir_path )
{
    player = 0;
    player_cnt = 0;

    std::string* name;
    int* body;
    int* mind;
    int* speed;
    int* health;
    int* item;

    std::ifstream infile;
    infile.open( team_dir_path );
    if (infile.is_open())
    {
        infile >> player_cnt;

        if (player_cnt)
        {
            name = new std::string [player_cnt];
            body = new int [player_cnt];
            mind = new int [player_cnt];
            speed = new int [player_cnt];
            health = new int [player_cnt];
            item = new int [player_cnt];

            player = new Player[ player_cnt];


            for (int i=0; i<player_cnt; i++)
            {
                infile >> name[i] >> body[i] >> mind[i]
                       >> speed[i] >> health[i] >> item[i];
            }

            infile.close();

            for (int i=0; i<player_cnt; i++)

            {
                player[i].name = name[i];
                player[i].set_stat( BODY, body[i] );
                player[i].set_stat( MIND, mind[i] );
                player[i].set_stat( SPEED, speed[i] );
                player[i].set_stat( HEALTH, health[i] );
                player[i].set_stat( ITEM, item[i] );
            }

            delete [] name;
            delete [] body;
            delete [] mind;
            delete [] speed;
            delete [] health;
            delete [] item;
        }
    }
}

Team::~Team(void)
{
    if (player){ delete [] player;}
}

Team::Team (const Team &team)
{
    this -> player = new Player;
    this -> player_cnt = 0;
}

//Player.h
class Player
{
public:
    Player(void);
    ~Player(void);

    void set_stat (const int which, const int value){ stat[which] = value;}
    void assign_stat (const int which, const int value){ stat[which] += value;}
    int get_stat (const int which){ return stat[which];}

    std::string name;

private:    

    // BODY;MIND;SPEED;HEALTH;ITEM.

    int stat[ MAXSTAT ];
};

//Player.cpp
Player::Player(void)
{
}

Player::~Player(void)
{
}

// Main.cpp
int main()
{
    initscr();
    cbreak();
    noecho();  

    Team* team1 = 0;
    Team* team2 = 0;

    team1 = new Team("c:\\IB\\ib\\teams\\bombers.txt");
    team2 = new Team("c:\\IB\\ib\\teams\\maruaders.txt");

    refresh();
    napms(1000);

    if (team1){ delete team1;}
    if (team2){ delete team2;}

    endwin();
}

So whenever ...

if (player){ delete [] player;}

is reached in class Team destructor I get the heap corruption error.

Please help.

Answer 1

@Martin York

[QUOTE} You don't provide the definition off: BODY, MIND, SPEED, HEALTH, ITEM, MAXSTATI bet there is an error in there [/QUOTE]

You were right,those values are defined in the source file a failed to post...

@Everyone

const int BODY = 1; const int MIND = 2; etc....

they are relevant to each element in the array 'stat [MAXSTAT ]' and so I was over running the stat array since BODY should have been 0, mind 1 etc.

So thanks heaps,and yes the program structure is wanting.

Also,do I need to explicitly define a copy constructor,operator etc since I'm using a raw pointer in the class...EVEN if I will only create 2 instances of class Team which will stay until program close and not need to copy assign them???

p.s.Sorry about that copy constructor error regarding player = new Player, it was a typo.

Really great to get this help.

Answer 2

You have a RAW pointer in your class.
This is bad idea because with a RAW pointer you MUST correctly define the following methods:

Team::~Team()
{
    delete [] Player;
}

Team::Team()
{
    // Other stuff
    player = new Player[ player_cnt ];
    // Other stuff
}

Team::Team(Team const& copy)
{
    // Other stuff
    player = new Player[copy.player_cnt];
    // Other stuff
}

Team& operator=(Team const& copy)
{
    Team   tmp(copy);
    swap(tmp);
    return *this;
}

void swap(Team& s)
{
    std::swap(player,s.player);
    std::swap(player_cnt,s.player_cnt);
}

As you can see it gets complicated. So change your code to use std::vector and all these problems disappear.

Also your constructor dynamically allocates 6 arrays. If any of these requests fail you end up with some nasty memory leaks. You could stick the code in a large try {} catch {} block and make sure the memory is always de-allocated on exit. OR you can use std::vector and everything is handled correctly.

Looking at your Player class. You have a whole host of other problems!
Why do you create an invalid object then go about setting all the values with set_XXX(). This looks obnoxiously like Java. The whole point of a constructor is that you should provide all the parameters required to correctly initialize the object into a VALID state. At the moment it is not valid until 6 set_XXX() methods are called.

Also note there is no need to test for a NULL pointer.

if (player){ delete [] player;}

Change this too:

delete [] player;

Why is the Team class handling the reading of Players?
The Player class should read its own data. The Team should read the count. Create the required number of players, asking each Player to read its own data from the file.

Why not use the initializer list to define the default values.

Team::Team( const char* team_dir_path )
    :player(NULL)
    ,player_cnt(0)
{  /* Other stuff */ }

You don't provide the definition off:

BODY, MIND, SPEED, HEALTH, ITEM, MAXSTAT 

I bet there is an error in there.

Answer 3

This will help in the long run:

vector

Also, consider allocating memory on the stack instead. You have unneccesary heap allocations here.

Answer 4

EDIT: it appears that, Alex Martelli has found the reason for the crash, see his answer for the solution to your actual question.

This doesn't directly answer your question, but vastly simplifies the code. The less code, the less room for bugs:

Team::Team( const char* team_dir_path ) {  
    // NOTE: use RAII, it's just simpler        
    std::ifstream infile(team_dir_path);
    if (infile) {
        infile >> player_cnt;

        if (player_cnt) {
            // NOTE: would be better to use a std::vector!
            player = new Player[player_cnt];

            for (int i=0; i<player_cnt; i++) {
                std::string name;
                int body;
                int mind;
                int speed;
                int health;
                int item;

                // NOTE: would be better if a player knew how to read the stats itself, then you could just do: 
                // player[i].load(infile);
                infile >> name >> body >> mind >> speed >> health >> item;
                player[i].name = name;
                player[i].set_stat( BODY, body );
                player[i].set_stat( MIND, mind );
                player[i].set_stat( SPEED, speed );
                player[i].set_stat( HEALTH, health );
                player[i].set_stat( ITEM, item );
            }
        }
    }
}

Answer 5

Your default constructor does

new Player;

while your file-input constructor does

new Player[num];

But your destructor always does

delete [] Player;

Try fixing that.

Answer 6

You do new Player(); and delete [] Player

Either do new Player [value] or delete Player.

Answer 7

While C++ does not formally distinguish the cases, you need to keep the distinction between a pointer to a single player and one to an array of players, as they must not be deleted in the same way. In your code you have both:

player = new Player[ player_cnt];

where player points to an array, which is appropriate for your destructor doing

delete [] player;

but ALSO, in your ctor:

this -> player = new Player;

which makes player point to a single Player and thus makes the delete [] in the dtor incorrect.