Shared Ownership double free error

Question

I have an Engine class which contains and owns some Systems. The Engine class has two containers, one map and one vector. Both keep pointers to the Systems.

The addSystem template function should add a pointer of a new System of the wanted type in the map and the addToPipeline should add a pointer of the System passed as a parameter in the vector. I used shared_ptrs for this but I am doing something wrong because I get a double free error if I use the addToPipeline function.

Here is the simplified Engine class:

class Engine
{
public:

        template <class T>
        T& addSystem();

        void addToPipeline(System&);

private:
        std::map<std::type_index, std::shared_ptr<System>> m_systems;
        std::vector<std::shared_ptr<System>> m_pipeline;
};


void Engine::addToPipeline(System& sys)
{
        m_pipeline.push_back(std::shared_ptr<System>(&sys));
}


template <class T>
T& Engine::addSystem()
{
        std::shared_ptr<T> system = std::make_shared<T>();
        auto inserted = m_systems.emplace(typeid(T),system);
        return static_cast<T&>(*(*inserted.first).second);
}

The functions should be used like shown below:

auto& POSITION_SYSTEM = engine.addSystem<PositionSystem>();
engine.addToPipeline(POSITION_SYSTEM);

Any help is appreciated!

Answer 1

In this line:

m_pipeline.push_back(std::shared_ptr<System>(&sys));

You are creating a shared_ptr for an already managed object, since you already wrapped the same object in another smart pointer. So you end up with two reference counts for the same object, thus you get a double free.

This is not how shared_ptr should be used. Instead, you should return a shared_ptr from addSystem and take one as argument for addToPipeline:

void Engine::addToPipeline(std::shared_ptr<System> sys)
{
    m_pipeline.push_back(sys);
}


template <class T>
std::shared_ptr<T> Engine::addSystem()
{
    std::shared_ptr<T> system = std::make_shared<T>();
    m_systems.emplace(typeid(T),system);
    return system; // No need to use the return value of emplace
}

The idea with shared_ptrs is that instead of using bare pointers or references, you always pass a shared_ptr (unless ownership doesn't matter - then you can also pass a reference). You have to do it this way because the reference counter is managed by the smart pointers.

Edit: As rozina pointed out: Of course you can still pass references to the managed object, as long as nobody tries to delete the corresponding address. This might actually be preferable if other code is interested in using a certain object, but not concerned about ownership. For example, you might want to have a public interface which allows obtaining a reference to some object that's managed by a smart pointer internally. For example:

class Foo {
public:
    Bar& getBar() {
        return *m_bar;
    }
private:
    std::shared_ptr<Bar> m_bar;
};

This is perfectly fine as long as nobody does delete &aFoo.getBar() - which happens if you create a new shared_ptr with that reference, as you did in your original code.