Observer removal can leave pointer dangling

[lucijan]

If at least two connections with the same DelegateKey are added to an Observer the Observer will remove the first connection that matches the DelegateKey. The removed connection under some circumstances doesn't have to be the connection that is intended to remove. Once the Observer is destructed the dangling pointer will be accessed in Observer::removeAll(). See the enclosed proof of concept. For our project it was enough to change the Observer::remove method to take an observer as a second argument and match the DelegateKey and observer.

#include <nano_signal_slot.hpp>
#include <iostream>

using namespace Nano;

class SlotClass : public Observer
{
public:
    SlotClass()
    {
    }

    void foo()
    {
    }
};

class SignalClass
{
public:
    SignalClass()
    {
    }

    Signal<void()> fired;
};

int main()
{
    SlotClass *dest = new SlotClass();
    char *block = new char[sizeof(SignalClass)];
    SignalClass *source1 = new(block) SignalClass();
    SignalClass *source2 = new SignalClass();

    source1->fired.connect<SlotClass, &SlotClass::foo>(dest);
    source2->fired.connect<SlotClass, &SlotClass::foo>(dest);

    // This will actually remove the connection to source2 since the delegate keys match
    // and source2 is the head in dest's observer list
    source1->~SignalClass();

    memset(block, 0xff, sizeof(SignalClass));

    // Here the danging pointer is accessed in Observer::removeAll
    // (which tries to call remove on source1)
    delete dest;

    delete source2;
    delete[] block;
}

Edit: replaced delete of soruce1 with a call to the deconstructor so that the subsequent call to memset doesn't access free'd memory

[NoAvailableAlias]

This is a very serious issue. I am looking to redesign Nano::Observer, but it looks like some gauze will need to be applied first. I have verified that the Band-Aid fix you provided has resolved this particular testcase and passes the nano test. Will try to get this up to master soon.

Thankyou for opening this issue.

Matt M.

[MarkoTurunen]

I stumbled to this issue, too. Used few hours debugging my own code until I found out what @lucijan described above. I implemented similar fix as in band-aid fix and now my application quit crashes are finally gone. Really annoying and serious bug.