Copying a vector of structs containing unique_ptrs

Question

I'm working on a simple game using C++ and Allegro. I am running into an Access violation runtime error regarding a vector of structs that contain unique_ptrs to ALLEGRO_BITMAPs.

Here is my struct declaration.

struct Skin {
    std::unique_ptr<ALLEGRO_BITMAP> img;
    Skin();
    Skin(ALLEGRO_BITMAP*);
    Skin& operator=(const Skin& s);
    Skin(const Skin& s);
};

And here are the definitions of the constructors in another file.

Skin::Skin() {
    img.reset();
}

Skin::Skin(ALLEGRO_BITMAP* bitmap) {
    img.reset(bitmap);
}

Skin::Skin(const Skin& s) {
    img.reset(s.img.get());
}

Skin& Skin::operator=(const Skin& s) {
    img.reset(s.img.get());
    return *this;
}

Here is the code that gets called before my access violation.

generateBase(world, display.get());

Which calls this function.

void generateBase(World& world, ALLEGRO_DISPLAY* display) {
    int x = TILESIZE - WIDTH;
    int y = HEIGHT - TILESIZE;
    int groundWidth = 3 * WIDTH - 2 * TILESIZE;
    Point min{ x, y };
    Point max{ x + groundWidth, y + (int)TILESIZE };
    ALLEGRO_BITMAP* black = al_create_bitmap(groundWidth, TILESIZE);
    ALLEGRO_BITMAP* white = al_create_bitmap(groundWidth, TILESIZE);
    al_set_target_bitmap(black);
    al_clear_to_color(al_map_rgb(0, 0, 0));
    al_set_target_bitmap(white);
    al_clear_to_color(al_map_rgb(255, 255, 255));
    al_set_target_bitmap(al_get_backbuffer(display));
    std::cout << "Errors incoming!" << endl;
    createPlayer(world, x, y, 0, 0, 5, vector < AABB > { AABB(min, max) }, vector < Skin > { Skin(black), Skin(white) });
    std::cout << "Did we make it?" << endl;
}

Which in turn calls this function.

unsigned int createPlayer(World& world, int x, int y, float dx, float dy, float speed, vector<AABB>& mesh, vector<Skin>& imgs) {
    unsigned int entity = newEntityIndex(world);
    world.masks[entity].set(COMPONENT_TYPE);
    world.masks[entity].set(COMPONENT_POINT);
    world.masks[entity].set(COMPONENT_UNITVECTOR);
    world.masks[entity].set(COMPONENT_SPEED);
    world.masks[entity].set(COMPONENT_COLLISIONMESH);
    world.masks[entity].set(COMPONENT_SKINLIST);
    world.types[entity] = TYPE_PLAYER;
    world.points[entity] = Point(x, y);
    world.unitVectors[entity] = UnitVector(dx, dy);
    world.speeds[entity] = Speed(speed);
    world.collisionMeshes[entity].mesh = mesh;
    cout << "Starting vector copy" << endl;
    for (auto skin : imgs) {
        world.skinLists[entity].imgs.push_back(move(skin));
    }
    cout << "Ending vector copy" << endl;
    return entity;
}

Here is my deleter for unique_ptr.

namespace std {
    template<>
    class default_delete < ALLEGRO_BITMAP > {
    public:
        void operator()(ALLEGRO_BITMAP* ptr) {
            cout << ptr << endl;
            al_destroy_bitmap(ptr);
        }
    };
}

Here is the output.

Errors incoming!
Starting vector copy
00AF9468
00AF9468

When I modified my createPlayer call in generateBase by removing Skin(white), the output changed to.

Errors incoming!
Starting vector copy
00799468
Ending vector copy
00799468

The change in output had me a bit puzzled, but my biggest question is what do I need to change about how I copy my vector of structs of unique_ptrs so that I don't try to delete the same pointer twice.

Thanks in advance!

Answer 1

The first thing to understand is you can only have one std::unique_ptr object containing a pointer to a particular object. Your Skin(const Skin& s) constructor violates this principle, resulting in two copies of a unique_ptr. If you have an object containing unique_ptr members, you'll need to do one of the following:

1. Not have a copy constructor or assignment operator.
2. In the copy constructor and/or assignment operator, allocate a new copy of the underlying resource. This would require calling al_clone_bitmap to duplicate the resource.

Second, when you are holding a resource in a unique_ptr, you want to initialize the unique_ptr at the same location where you create the resource. For example, instead of creating a local variable ALLEGRO_BITMAP* black, use the following:

std::unique_ptr<ALLEGRO_BITMAP> black(al_create_bitmap(groundWidth, TILESIZE));

Since this code is creating the unique_ptr directly from the result of al_create_bitmap, you'll want to remove the Skin constructor that takes an ALLEGRO_BITMAP* and replace it with this:

Skin::Skin(std::unique_ptr<ALLEGRO_BITMAP>&& bitmap)
    : img(bitmap)
{
}

You can then create a Skin by moving your unique_ptr into it:

Skin(std::move(black))

---

Putting the above together, a working copy constructor might look like the following. It's not particularly efficient, but it's safe.

Skin::Skin(const Skin& s)
    : img(al_clone_bitmap(s.img.get()))
{
}

Answer 2

The problem is here:

Skin::Skin(const Skin& s) {
    img.reset(s.img.get());
}

Skin& Skin::operator=(const Skin& s) {
    img.reset(s.img.get());

You are stealing the raw pointer from one unique_ptr and assigning it to another one. Now unique_ptr belongs to the RAII category. They expect to own the lifetime of an object as long as they are alive. When you do this

img.reset(s.img.get());

You took out the pointer from one unique_ptr and handed over to the other unique_ptr. Now unique_ptr1 believes that it own the underlying object unaware that there is another unique_ptr2 that believes the same. So when they die they will happily free the memory allocated for _Ptr. So your code is bound to end up accessing/freeing memory which has already being freed by the first unique_ptr which dies.

You must either move the unique_ptr (thereby yielding ownership) or explicitly calling release on s.img