State pattern implementation in C++ run into segmentation fault

Question

I'm implementing state patter in C++, generally all works fine and according to needs, but after state is changed multiple times i get segmentation fault, stack allocation is rising continuously until reach maximum value and then crash hire is code: Interface:

class ControllerT1; // Forward declaration

class IControllerStateT1 {
public:
    virtual ~IControllerStateT1() {}
    virtual void handle(ControllerT1* controller) = 0;
};



class ControllerT1 {
    public:
        ControllerT1(std::vector<int> &input, std::vector<int> &output);
        ~ControllerT1();

        void stop();
        void initialize();
        void run();

        void setState(IControllerStateT1* newState);

    private:
        IControllerStateT1* currentState;
};


#include "ControllerT1.h"
#include "StopT1.h"

ControllerT1::ControllerT1(std::vector<int> &input, std::vector<int> &output) : currentState(new StopT1()) {
//  ControllerInputs = input;
//  ControllerOutputs = output;
}

ControllerT1::~ControllerT1() {
    delete currentState;
}

void ControllerT1::run() {
    currentState->handle(this);
}

void ControllerT1::stop() {
    currentState->handle(this);
}

void ControllerT1::initialize() {
    //std::cout << "Reading Confihuration files" << std::endl;
    currentState->handle(this);
}

void ControllerT1::setState(IControllerStateT1* newState) {
    delete currentState;
    currentState = newState;
    currentState->handle(this);
}

example of state:

class RunningT1 : public IControllerStateT1 {
public:
    RunningT1();
    void handle(ControllerT1* controller) override;
};

RunningT1::RunningT1() {

}

void RunningT1::handle(ControllerT1* controller) {

    std::cout<<"Bettery api 1 runing"<<std::endl;
try {
    std::lock_guard<std::mutex> lock(DataMutex);
    for (auto& element: ControllerOutputs) {
        element=element+1;
    }

} catch (const std::exception&  e) {
    std::cerr<<e.what()<<std::endl;
}
    std::this_thread::sleep_for(std::chrono::milliseconds(30));



    controller->setState(new Intermediate());
}

I suspect this part of code to create problems

void ControllerT1::setState(IControllerStateT1* newState) {
    delete currentState;
    currentState = newState;
    currentState->handle(this);
}

but i don't knew how to change it, i was trying to use pointer there like:

std::unique_ptr<IControllerStateT1> newState

to avoid using new keyword but problem was the same i will be grateful for any advice Program is compiled for QNX x86_64

Answer 1

You have a basic design flaw. In your

void ControllerT1::setState(IControllerStateT1* newState) {
    delete currentState;
    currentState = newState;
    currentState->handle(this);
}

you are first deleting the currentState, then assigning a new one, then invoking the handling. Now invocation of currentState->handle(this); can reach controller->setState(new Intermediate()); inside of your handle routine, and that in turn can delete the object whose method handle still didn't return possibly causing Undefined Behaviour (might be a crash in your case).

It would be better if you deal with references instead of pointers and keep your State objects inside of a State Machine.

Or you can return a new state from handle method, explained quite clearly in this article.

Another thing, mentioned by Alan in comments, is the possibly infinite recursion, if you always plan to handle inside your setState. You should instead "post transition", or similar, in the State Machine.