C++ Program crashes when accessing object after copy

Question

I'm having a problem which I think is related to creating a copy of an object. I have a map class that contains a std::map with a bunch of classes defining the different tile types indexed by their ID, and a bunch of tiles in a vector each with a pointer to one of those classes. Additionally I have another class, Scenario, that contains the map.

The problem comes when I create a Scenario object and try to do anything with its map. There must be some problem when copying the pointers but I haven't been able to figure it out.

I have tried to reduce the code to what's necessary to reproduce the problem:

#include <string>
#include <map>
#include <vector>
#include <iostream>

using namespace std;

class TileInfo {
    string name;

public:
    int id;
    TileInfo() {};
    TileInfo(int id, string name);

    string getName() const;
};

TileInfo::TileInfo(int id, string name)
{
    this->id = id;
    this->name = name;
}

string TileInfo::getName() const 
{
    return name;
}

class Tile
{
public:
    TileInfo* info;

    Tile() {};
    Tile(const Tile & other, map<int,TileInfo> & info);
    Tile (TileInfo* info);

    string getName() const;
};

Tile::Tile (TileInfo* tileInfo)
{
    this->info = tileInfo;
}

Tile::Tile( const Tile & other, map<int,TileInfo> & tileInfo )
{
    map<int,TileInfo>::iterator it = tileInfo.find(other.info->id);

    if(it == tileInfo.end())
        this->info = NULL;
    else
        this->info = &(it->second);
}

string Tile::getName() const 
{
    return info->getName();
}

class Map
{
    vector <vector <Tile>> tiles;
    map <int, TileInfo> tileInfo;

public:
    void print() const;

    Map() {};
    Map(map<int,TileInfo> tileInfo);
    Map(const Map & other);

    string getName() const;
    void loadTiles();
};

Map::Map(map<int,TileInfo> tileInfo)
{
    this->tileInfo = tileInfo;
}

Map::Map(const Map & other)
{
    this->tileInfo = other.tileInfo;

    for(unsigned int i = 0; i < other.tiles.size(); i++)
    {
        vector<Tile> line;

        for(unsigned int j = 0; j < other.tiles[i].size(); j++)
        {
            Tile tmpTile = Tile(other.tiles[i][j],tileInfo);
            line.push_back(tmpTile);
        }

        this->tiles.push_back(line);
    }
}

void Map::print() const
{
    for(unsigned int i = 0; i < tiles.size(); i++)
    {
        for(unsigned int j = 0; j < tiles[i].size(); j++)
        {
            cout << "Tile at " << i << ", " << j << " is a "; cout << tiles[i][j].getName() << endl;
        }
    }
}

void Map::loadTiles()
{
    for(unsigned int i = 0; i < 3; i++)
    {
        vector <Tile> line;
        for(unsigned int j = 0; j < 3; j++)
        {
            map<int,TileInfo>::iterator ite = tileInfo.find(i);
            if( ite!=tileInfo.end())
                line.push_back(Tile(&(ite->second)));
            else
            {
                line.push_back(Tile(NULL));
            }
        }

        tiles.push_back(line);
    }
}

class Scenario
{
public:
    Map map;
    Scenario() {};
    Scenario(Map map);
};

Scenario::Scenario(Map map)
{
    this->map = map;
    this->map.print();
}

Map createMap()
{
    map<int,TileInfo> tmpInfo;

    for(unsigned int i = 0; i < 3; i++)
    {
        tmpInfo.insert(pair<int,TileInfo>(i,TileInfo(i,"testname")));
    }

    Map rtrnVal = Map(tmpInfo);

    rtrnVal.loadTiles();

    return rtrnVal;
}

int main()
{
    Map newMap = createMap();

    Scenario newScenario = Scenario(newMap);
    newScenario.map.print();
    int input;
    cin >> input;
}

The print() at the end of the Scenario constructor prints the map's contents correctly, but the one after the scenario is assigned causes a crash. Any help would be appreciated.

Answer 1

The original std::map where your original TileInfo objects are stored is here:

map<int,TileInfo> tmpInfo;

for(unsigned int i = 0; i < 3; i++)
{
    tmpInfo.insert(pair<int,TileInfo>(i,TileInfo(i,"testname")));
}

That is the map the houses the pairs. you then make a copy of said map. It creates copies of all your TileInfo objects, which is ok.

Map rtrnVal = Map(tmpInfo);

But this is where the wheels begin to come off. You fire loadTiles here, which sets up your map of vectors that addres the relative TileInfo objects in rtrnVal. This code:

rtrnVal.loadTiles();

leads to this code. Note that you're pushing the addresses of your mapped TileInfo objects into your vectors. The map where these reside is this objects map.

void Map::loadTiles()
{
    for(unsigned int i = 0; i < 3; i++)
    {
        vector <Tile> line;
        for(unsigned int j = 0; j < 3; j++)
        {
            map<int,TileInfo>::iterator ite = tileInfo.find(i);
            if( ite!=tileInfo.end())
                line.push_back(Tile(&(ite->second)));
            else
            {
                line.push_back(Tile(NULL));
            }
        }

        tiles.push_back(line);
    }
}

Finally, back where we started you do this:

return rtrnVal;

This makes a copy of the Map, which makes a copy of the map of vectors, which contain pointers to the TileInfo in the map within rtrnVal that is about to be destroyed on function exit. The result back on the caller side.

Map newMap = createMap(); 

newMap now holds dangling pointers to TileInfo objects that were in rtrnVal in createMap.

Possible Solution

Rather than having a map of vectors containing pointers to TileInfos, consider a map of vectors of indexes, where each index keys to a 0-based slot in a TileInfo std::vector that is contained along side the map. I can see what you were trying to do (singular TileInfos shared across multiple cells) so you still reap that benefit. The vector will come along for the ride wherever the map does (they're owned by the same Map object) and copying won't harm anything because both the map (of indexes) and the vector (of TileInfo) will copy safely. In the end you still get what you want (singular TileInfos) but now-indexed by number rather than pointer. As a bonus, you don't have to deal with rebasing pointers to TileInfo from one map to another in a custom copy-ctor. Its all relative (0.. n-1)

Best of luck.

Answer 2

You're storing pointers to TileInfo:

class Tile
{
public:
    TileInfo* info;
    ...

But once the object is copied, the pointers are copied across, but the TileInfo that they pointed to have been destructed, so they are referencing invalid memory = crash.

Consider implementing the copy constructor / copy operator to handle it.