Strange segmentation faults - pushing back an object pointed by pointer to a vector causes crash

Question

I'm trying to write a game using SDL, but i'm getting a lot of strange segfaults.

I have created class Monster and Player which get public variables from class Creature. Class Creature gets variables from class Object. Just like that:

class Object {
public:
    Area* area_pointer; // Pointer to an Area in which object is present.
    Sprite sprite; // Class that has SDL_Surface* and some other things.
    Animation animation; // nothing usefull
    int ID;
    short int position_x; // nothing usefull
    short int position_y; // nothing usefull
    short int effect_x; // nothing usefull
    short int effect_y; // nothing usefull
    .... //Some functions that are not important right now.
};

Class Sprite:

class Sprite {
public:
    SDL_Surface* image;
    short int x;
    short int y;
    short int w;
    short int h;
    Sprite ()
    {
        image = NULL;
        x = 0;
        y = 0;
        w = 0;
        h = 0;
    }   
};

Class Creature, Player, Monster:

class Creature : public Object {
        public:
            char move_direction;
            char speed; 
            ..// Some not important functions.
};
class Player : public Creature {
        public:
            char select_type;
            int select_ID;
            std::vector <Item> equip;
            std::vector <Item> inventory;
            std::vector <Effect> effects;
            Player (Area* c_area_pointer, int data [])
            {
                area_pointer = c_area_pointer;
                sprite.image = SurfaceLoad ("Images/Players/" + IntToString (data [0]) + ".png");
                sprite.x = 0;
                sprite.y = 0;
                sprite.w = 0;
                sprite.h = 0;
                ID = data [0];
                position_x = data [1];
                position_y = data [2];
                effect_x = 0;
                effect_y = 0;
            }
            ~Player ()
            {
                SDL_FreeSurface (sprite.image);
                .....
            }
};
class Monster : public Creature {
        public:
            char type;
            std::vector <Item> loot;
            std::vector <Effect> effects;
            Monster (Area* c_area_pointer, int data [])
            {
                area_pointer = c_area_pointer;
                sprite.image = SurfaceLoad ("Images/Monsters/" + IntToString (data [3]) + ".png");
                sprite.x = 0;
                sprite.y = 0;
                sprite.w = 0;
                sprite.h = 0;
                ID = data [0];
                position_x = data [1];
                position_y = data [2];
                effect_x = 0;
                effect_y = 0;
                type = data [3];
            }
            ~Monster ()
            {
                SDL_FreeSurface (sprite.image);
                .....
            }
};

SurfaceLoad function:

SDL_Surface* SurfaceLoad (std::string file_name)
{
SDL_Surface* surface_1 = NULL;
SDL_Surface* surface_2 = NULL;
surface_1 = IMG_Load (file_name.c_str ());
surface_2 = SDL_DisplayFormat (surface_1);
if (surface_1 != surface_2) SDL_FreeSurface (surface_1); // This line may be strange to some of you, but it is related to other error i had in past.
SDL_SetColorKey (surface_2, SDL_SRCCOLORKEY, 0xFFFFFF);
return surface_2;
}

I try to load data from txt files and make objects basing on it. I use my own Load function to that. It creates a pointer to the class object for example:

        Player* player;
        std::string players_name = save_name + "Areas/" + IntToString (ID) + "/players.txt"; //Path to the file cointaining players data.
        std::ifstream players_file;
        players_file.open (players_name.c_str ());
        Monster* monster;
        std::string monsters_name = save_name + "Areas/" + IntToString (ID) + "/monsters.txt"; //Path to the file cointaining monsters data.
        std::ifstream monsters_file;
        monsters_file.open (monsters_name.c_str ());

Then it loads data from text file and puts it into array of int named file_data and creates new class object based on it.

        while (!players_file.eof ())
        {
            getline (players_file, file_text);
            while (file_text [data_position_2 + 1] != ';')
            {
                data_position_2 = file_text.find (",", data_position);
                data.assign (file_text, data_position, data_position_2 - data_position);
                file_data [data_index] = atoi (data.c_str ());
                data_position = data_position_2 + 1;
                data_index++;
            }
            player = new Player (this, file_data);
            this->area_map.players.push_back (*player); //Vector players inside object area_map which contain also monsters vector. "this" is a pointer to object that contain Load function and area_map object.
            delete player;
            data_index = 0;
            data_position = 0;
            data_position_2 = 0;
        }

This part of the code works, but doing exactly the same thing with monsters_file and monsters vector caused a lot of strange errors. First error that i got was segfault when deleting a pointer after pushing data pointer by it to the vector. I checked it and found out that program crashes (segfault) in deconstructor when it calls SDL_FreeSurface (). So i checked if my constructor loads surface properly. I found out that everything is ok with constructing an object, but then it suddenly started to crash (segfault) when calling SurfaceLoad (). Checked this function too and everything was ok with that too: pointers to surfaces were ok, pointer returned by it was ok, but for some reason it crashed at:

sprite.image = SurfaceLoad (...);

Some time after it stopped to crash here, without any reason (I just added that line

if (surface_1 != surface_2) SDL_FreeSurface (surface_1);

, because i noticed that SDL_DisplayFormat () sometimes returns same pointer as pointer to the unformatted surface.) and started to crash (segfault too) when i push_back object pointed by pointer to the vector:

this->area_map.monsters.push_back (*monster);

Monster and Player classes are pretty much the same on this stage of creating the game, so i have no idea why it creates players without any problem and it has so many problems with creating monsters. Has anyone any idea how to fix that?

Answer 1

Well loads of code which looks more than a little disorganized. For instance why is the Player destructor freeing the image which is declared in Sprite? Shouldn't that be the job of the Sprite destructor? And why is everything public? But anyway from the symptoms you describe it sounds like a classic case of failing to follow the rule of three.

When you write classes that allocate memory or other resources internally (like the image pointer in Sprite) then you must write copy constructors and assignment operators that handle the allocated memory or resource correctly. If you don't you get errors like these.

See here for some essential information on the rule of three. If you don't understand this stuff then you'll always be writing buggy C++ code. The section on managing resources is the most relevant to you, but read it all.