cinder / Cinder

Cinder is a community-developed, free and open source library for professional-quality creative coding in C++.
http://libcinder.org
Other
5.27k stars 939 forks source link

osc: fix crash when closing TCP receiver connections #2186

Closed totalgee closed 3 years ago

totalgee commented 3 years ago
totalgee commented 3 years ago

I came across this bug when trying to write a server using osc::ReceiverTcp, with multiple client connections. It took me a while to figure out what was going on; I would get crashes in the server when disconnecting a client. However, the issue here is that std::remove_if not only moves any matching elements (which in this case is a unique_ptr<Connection>) to the end of the vector, but also "moves from" it -- which for a unique_ptr means it's set to nullptr. So later, when it is dereferenced to call shutdown(), we get undefined behaviour (a crash, generally).

The rest of the code is fine -- the later call to mConnections.erase(rem) to actually eliminate the item from the vector of connections works as expected (and the Connection is now destroyed at that point, rather than during the remove_if).

The only potential issue with calling find_if instead of remove_if would be if we expected multiple connections in the vector with the same identifier, but there can't...it always increments by one for each new connection. So "find" is the appropriate idiom anyhow!

totalgee commented 3 years ago

Here is an example illustrating the bug in a standalone "dummy" example.

totalgee commented 3 years ago

This is to fix issue https://github.com/cinder/Cinder/issues/2192, which I just opened.

andrewfb commented 3 years ago

Wow - that's a subtle bug. For tracking it down and providing a helpful explanation as well as the fix.