vector of unique_ptr deleting?

Question

I have an issue with a segfault that I can not figure out. It's from an EntityManager for a small game engine I'm working on. I can add the Ship Entity, and the Ship can add 1 Bullet Entity, but it segfaults if I try to add more than 1 Bullet. I've been trying to figgure this out for the past day now. Below is a small excerpt from the actual code.

#include <vector>
#include <memory>

struct EntityManager;
struct Entity {
    Entity(EntityManager* manager) : manager(manager) { }
    virtual ~Entity() { }
    virtual void update() = 0;

    EntityManager* manager;
};
struct EntityManager {
    void update() {
        for (auto& entity : entities) {
            entity->update();
        }
    }
    void add(Entity* e) {
        entities.emplace_back(e);
    }
    std::vector<std::unique_ptr<Entity>> entities;
};
struct Bullet : public Entity {
    Bullet(EntityManager* manager) : Entity(manager) { printf("Bullet ctor\n"); }

    virtual void update() override { }
};
struct Ship : public Entity {
    Ship(EntityManager* manager) : Entity(manager) { }

    virtual void update() override {
        printf("Adding Bullet\n");
        manager->add(new Bullet(manager));
    }
};
int main() {
    EntityManager manager;
    manager.add(new Ship(&manager));

    int loops{0};
    while (loops < 100) {
        manager.update();
        loops++;
        printf("Completed Loop #%d\n", loops);
    }
    return 0;
}

In the actual code, everything is in their own .h/.cpp files, and classes instead of structs, but the issue is the same. The output is `Adding Bullet // Bullet ctor // Completed Loop #1 // Adding Bullet // Bullet ctor // Signal: SIGSEGV (Segmentation fault)

The segfault happens in the EntityManager::update() on the entity->update(); line.

`EntityManager` needs custom move operations to update the entities' `manager` pointer. — dyp, Sep 25 '15 at 10:03
When you update the entities, that loop adds more entities which invalidates your iterators. You can't add to your vector while you are in the middle of looping through it. — Galik, Sep 25 '15 at 10:07
[Write games not engines](http://geometrian.com/programming/tutorials/write-games-not-engines/) is a good thing to read. That said, instead of adding things immediately or destroying things immediately, you can delay those operations to after the update loop, through events or whatever you find appropriate, so that you don't invalidate iterators. — aslg, Sep 25 '15 at 10:08
Prefer `void add(std::unique_ptr e)` over `void add(Entity* e)`. — Jarod42, Sep 25 '15 at 10:15

Jonathan Wakely · Accepted Answer · 2015-09-25T10:09:38.720

The problem is that this loop modifies the vector:

    for (auto& entity : entities) {
        entity->update();
    }

You are busy iterating through it when you modify the vector to add a new element, which invalidates the iterators being used to traverse the container.

The range-based for loop is expanded by the compiler to:

auto begin = entities.begin(), end = entities.end();
for (; begin != end; ++begin)
  begin->update();

The call to begin->update() adds a new element to the vector, which invalidates all iterators into the container, so the ++begin is undefined behaviour. In practical terms, begin no longer points into the vector (because it has reallocated and freed the old memory that begin pointed to) so the next begin->update() call dereferences an invalid iterator, accessing freed memory and seg-faulting.

To do it safely you probably want to use indices not iterators:

for (size_t i = 0, size = entities.size(); i != size; ++i)
  entities[i].update();

This captures the size at the start of the loop and so only iterates up to the last element that exists when the loop starts, so new elements added to the end will not get visited.

This still works when the vector is modified because you don't store iterators or pointers to elements, only an index. As long as you don't remove elements from the vector the index is still valid even after inserting new elements.

I changed my loop to be a fixed for loop that does access through an iterator for(auto index{0u}; index < vec.size(); ++index). It seems like, since the vector is only expanded, not reduced, during this loop, it will work the way I need it to (not updating new entities till the next game loop). — BFritz, Sep 25 '15 at 10:15
No, that is not safe. It's not a "fixed for loop" because `vec.size()` changes, and it could loop forever visiting new entities as they are added. It happens to work for your example above because `Bullet` doesn't modify the vector, but you do visit both entities during the first update loop. There's a good reason I wrote the corrected loop the way I did. As I said: _"This captures the size at the start of the loop and so only iterates up to the last element that exists when the loop starts, so new elements added to the end will not get visited."_ — Jonathan Wakely, Sep 25 '15 at 10:20
You are correct, after running some tests I found out that it doesn't work the way I mentioned above. I'll use the pre-captured variables, as you suggested in your answer. I honestly feel like I should have known what the issue was. Thanks for your help! — BFritz, Sep 25 '15 at 10:30

vector of unique_ptr deleting?

1 Answers1