Segmentation fault when using a shared_ptr

Question

I am making a particle system and I'm struggling with how to structure my code. The idea is that a user can create one or several ParticleEmitter objects that are passed to a ParticleManager object via the ofxCurlNoise object.

Now, I want that when the user updates the ParticleEmitters objects, the ParticleManager object sees the changes made. So I used shared pointers but I have segmentation faults at different times, whether I use one ParticleEmitter (segmentation fault when the program starts) or a vector<ParticleEmitter> (segmentation fault when the program exits).

What is wrong with this? Is there a design pattern for doing what I'm trying to do?

ofApp.h

#include "ofxCurlNoise.h"

class ofApp : public ofBaseApp{

    // ParticleEmitter particleEmitter;
    vector<ParticleEmitter> particleEmitters;
    ofxCurlNoise curlNoise;

    public:
        void setup();

};

ofApp.cpp

#include "ofApp.h"

void ofApp::setup(){
    // This produces a segfault as soon as the program starts
    // particleEmitter.setup();
    // curlNoise.setup(particleEmitter, 1024*256);

    // This produces a segfault when the program exits
    ParticleEmitter emitter;
    emitter.setup();
    particleEmitters.push_back(emitter);
    curlNoise.setup(particleEmitters, 1024*256);    

}

ofxCurlNoise.h

#include "ParticleManager.h"

class ofxCurlNoise {    

    ParticleManager particleManager;

    public:
        void setup(ParticleEmitter& emitter, int n);
        void setup(vector<ParticleEmitter>& emitters, int n);

    private:
        void setup(int n);    

};

ofxCurlNoise.cpp

#include "ofxCurlNoise.h"

void ofxCurlNoise::setup(ParticleEmitter& emitter, int n){
    particleManager.addEmitter(shared_ptr<ParticleEmitter>(&emitter));
    setup(n);
}

void ofxCurlNoise::setup(vector<ParticleEmitter>& emitters, int n){
    for(auto& e : emitters){
        particleManager.addEmitter(shared_ptr<ParticleEmitter>(&e));
    }
    setup(n);
}

void ofxCurlNoise::setup(int n){
    particleManager.setup(n);
}

ParticleManager.h

#include "ParticleEmitter.h"

class ParticleManager{    

    vector<shared_ptr<ParticleEmitter>> emitters;

    public:
        void addEmitter(const shared_ptr<ParticleEmitter>& emitter);
        void setup(int n);
};

ParticleManager.cpp

#include "ParticleManager.h"

void ParticleManager::setup(int n){
    //...
}

void ParticleManager::addEmitter(const shared_ptr<ParticleEmitter>& emitter){
    emitters.push_back(emitter);
}

Solution 1

This is not how std::shared_ptr works. You are creating your instances of ParticleEmitter on the stack, but std::shared_ptr is used to manage instances which are created on the heap. In your code, when you add a new emitter to the ParticleManager, and wrap it into a shared pointer, the emitter is destroyed when the particleEmitters vector is destroyed (when, in turn, your ofApp instance is destroyed) and is thus destroyed regardless.

When the instance of ofApp is destroyed, both the instance of ofxCurlNoise and particleEmitters are destroyed (in that order). So ofxCurlNoise will in turn destroy the particleManager, which manages your shared pointers, which will then delete your particle emitters (which were originally created on the stack). After all that is done, the particleEmitters vector is getting destroyed, and the runtime system will try to destroy your particle emitters again, leading to the error you are seeing.

Furthermore, shared pointers are used to model shared ownership semantics, which I don't see in your use case. I think you'd be better off to either use std::unique_ptr to manage instances created on the heap, or to not use smart pointers at all and create everything on the stack (which you are almost doing already).

Solution 2

You should never create shared_ptr from an ordinary pointer as you do here:

 shared_ptr<ParticleEmitter>(&e)

This tries to free the ParticleEmitter twice. Once as the vector holding the ParticleEmitter objects goes out of scope, and once as the shared_ptr goes out of scope.

Comments

[Ulrich Eckhardt]

Quoting SO guidelines: "Questions seeking debugging help must include the desired behavior, a specific problem or error and the shortest code necessary to reproduce it in the question itself. Questions without a clear problem statement are not useful to other readers."

[Angew is no longer proud of SO]

@UlrichEckhardt This one does actually contain both the code necessary to identify the error, and a reasonably clear problem statement. It's not a stellar question, but I've seen worse.

[PiotrNycz]

In std::shared_ptr you can kept only dynamically created objects (by new)... From what I see - you are passing to std::shared_ptr whatever is in vector<ParticleEmitter>...

[Ulrich Eckhardt]

It doesn't have a main(), it has a bunch of different classes and functions, so there are both parts missing and superfluous. This stands in contrast to the quoted rule, @Angew.

[Elie Génard]

Yes sorry if I wasn't clear enough. It's code for openFrameworks, ofApp::setup() is called when the program starts.

[Ulrich Eckhardt]

I guessed that already, but there are reasons for those guidelines. If you had taken the time to first reduce the problem, you would have found that the use of openFrameworks is irrelevant. You would then have been able to reduce the problem to something like 20 lines of code, which in turn would have helped you focus on the issue. That's a basic problem solving strategy which you should learn. It also makes this post clearer and more useful for others.

[Angew is no longer proud of SO]

+1, with a minor nitpick: the OP is pointing the shared_ptrs at elements in a vector, not at local variables.

[Elie Génard]

Ok, I understand now. So shared_ptr are not the right tool to do it. But the thing is that I'd like not to use pointers to ParticleEmitters objects in the ofApp class. However, the changes made to a ParticleEmitter in ofApp have to be seen in ParticleManager. How can I achieve this?

[galinette]

e is not a stack object, it is a reference to an object managed by a std::vector

[Manuel Barbe]

A built-in pointer e.g. ParticleEmitter*, not a smart-pointer.

[Luka Rahne]

It is not on stack, but looks like it is. On top of destruction controlled by vector it is also moved on another memory location, when size of vector changes.

[Kristian Duske]

@ElieGnrd First you need to clarify the ownership semantics. Who owns the particle emitters, i.e., who should be responsible for managing their lifecycle?

[Elie Génard]

ofApp owns the ParticleEmitters

[Kristian Duske]

You can create your emitters on the heap and store them in a vector of std::unique_ptrs in ofApp. Then you can either pass the instances as raw pointers, or you can pass a reference to the vector. Or you can create them on the stack and have them managed by the vector, and pass a reference to the vector.

[Elie Génard]

The purpose of this code is to be used by other users (it's an addon for a framework). The users will only write code in the ofApp class, and I want it to be as simple as possible (i.e. not using pointers in ofApp). So is there a way to do this and keeping the code in ofApp as it is?

[Kristian Duske]

Yes, pass your vector of emitters by reference to the constructor of ParticleManager and store the reference there. But you have to make damn sure that ParticleManager is destroyed before ofApp is.

[Elie Génard]

Let us continue this discussion in chat.

[galinette]

Then you need an ordinary pointer when creating a shared_ptr, there is no other way to use it

[Manuel Barbe]

Best practise is to use make_shared() to create the shared_ptr. But you are right, I could have been more precise about it.