GUI class leaves dangling pointers in fZoneMap when it's moved or copied and destroyed

[oddfacade]

Here. Either these clist* should be turned into smart pointers, the GUI class should implement a copy constructor that deep copies the map, or the default copy constructor should be deleted to force move semantics (and the destructor should be adjusted accordingly). I can make a PR if you'd like. Do you have a preferred solution?

[sletz]

What is the problem in the first place ? Can you provide a example of code that demonstrate it ?

[oddfacade]

I have a custom GUI class that works roughly like this:

class MyGUI : public GUI
{
    void addNumEntry(const char* label, FAUSTFLOAT* zone, FAUSTFLOAT init, FAUSTFLOAT min, FAUSTFLOAT max, FAUSTFLOAT step)
    {
        registerZone(zone, new MyItem(this, label, init, min, max));
    }
    // etc. ...
};

It's used in context where it needs to be moveable. It's hard to succinctly summarize my exact usage without explaining a bunch of context about my codebase, but suffice to say even something as simple as this would cause a double free bug:

{
    MyGUI gui_a;
    my_dsp.buildUserInterface(&gui_a);
    {
        MyGUI tmp(gui_a); // create a copy
    } // copy freed
} // segfault

[sletz]

Why not allocating a pointer with MyGUI* gui_a = new MyGUI(); and using it instead of the object ?

[oddfacade]

Yes, I did something very similar to your suggestion to work around the issue. I appreciate you trying to help me get unstuck, but to be clear, I'm not really asking for a solution for my project. I'm just trying to report this bug I encountered and offering to fix it. I would have just opened a PR, but I wanted to get clarification on how this class is supposed to work first. Presumably this isn't intended behavior, right? If you're not meant to be able to move/copy this object then it shouldn't have those constructors defined, and if you are meant to be able to move/copy it, it shouldn't segfault when you do so.

It seems like a straightforward fix. If the clists are actually meant to be shared between all copies of the GUI then changing fZoneMap to be a std::shared_ptr<std::map<FAUSTFLOAT*, clist>> instead of a std::map<FAUSTFLOAT*, clist*> would fix it. The map is a private member so we know there isn't any code depending on the fact that it's a map to pointers rather than a pointer to a map.

[sletz]

We can try that, can you test and prepare a PR then ?