Lifetime issue in ReleaseWallet(CWallet* wallet) #16668

While checking #16661 I stumbled upon what seems to be a lifetime issue in ReleaseWallet(CWallet* wallet) (wallet.cpp).

https://github.com/bitcoin/bitcoin/blob/fa6431055abfacea4f72ef11d061c7c755de327a/src/wallet/wallet.cpp#L107-L111

After delete wallet; the pointer is invalid and shouldn't be used like in the call g_unloading_wallet_set.erase(wallet).

The code was introduced in #14941 which was merged into master on January 15, 2019.

In addition to fixing this specific instance it might be worth thinking about how we can use static analysis to automatically catch this class of issues going forward :-)

It's not dereferenced so it should be fine?

@promag Isn't any use of a pointer that has been invalidated due to deallocation prohibited?

@promag From the C++11 standard:

If the argument given to a deallocation function in the standard library is a pointer that is not the null pointer value (4.10), the deallocation function shall deallocate the storage referenced by the pointer, rendering invalid all pointers referring to any part of the deallocated storage.

The effect of using an invalid pointer value (including passing it to a deallocation function) is undefined.

On some implementations, it causes a system-generated runtime fault.

Ok, it's easily fixed by grabbing the set iterator before deleting.

@promag Would you mind fixing it since this is your code originally? Let me know otherwise and I'll jump in of course :-)

Sure will do

I'm kind of surprised an implemenation would change the pointer value, but apparently it's a thing:

https://stackoverflow.com/questions/57286818/pointer-to-deallocated-variable-changes-address https://timsong-cpp.github.io/cppwp/n4659/basic.stc.dynamic.safety#4