Adds method to detach Observers from Observables

[jprjr]

This should fix a use-after-free bug.

The player object calls DetachObserver in its destructor, which may already be freed based on destructor order.

Example:

xgm::NSFPlayer player;
xgm::NSFPlayerConfig config;

player.SetConfig(&config);

This will result in a crash (tested on arch linux), since config will be destroyed before player, but player will attempt to call DetachObserver on config during destruction.

This proposed fix is to have Observables request that their Observers detach when destroyed (just replace the pointer with NULL). This allows the destructors to occur in any order.

[bbbradsmith]

Would it make sense for ~Player to just call Detach, and then have Detach do the DetachObserver too, i.e.:

if(config != NULL) config->DetachObserver(this);
config = NULL;

[jprjr]

Not 100% sure, but I'd lean towards no.

If NSFPlayerConfig is destroyed before Player, then Player doing anything with the config object should be avoided (since that memory has been freed now).

So, if you have ~Player call Player::Detach() and have Player::Detach() call config->DetachObserver(), I think you'd still be facing the fundamental issue of trying to use that just-freed config object.

[eatnumber1]

Do these objects need to support destruction in any order? In #48 I just swap the destruction ordering here and it seems to work correctly.

Speaking abstractly, player.SetConfig(&config); causes player to hold a non-owning reference to config, meaning that it would make sense that player must be destroyed before config.

[jprjr]

As long as that order is documented it'd be fine. I'm not really a great c++ programmer so this threw me for a loop.

If this is something that is fairly obvious to people more familiar with c++ idioms then it could just be closed.

[eatnumber1]

Note that in C++, local variables are guaranteed to be destroyed in reverse order of declaration (this is a common C++'ism).

In the case of player.SetConfig(&config);, since it's taking a pointer and the method is called "Set...", it can be inferred by just looking at the call site that player is keeping that pointer, and also because it's not documented to take ownership it implicitly isn't. That said, it wouldn't be out of place to see a warning on the docs for SetConfig, e.g. "ownership is not transferred, and must outlive this instance".

[bbbradsmith]

Was this addressed in #48 ? Should I close this?

[jprjr]

Yeah, go ahead and close it