search

lidgren / lidgren-network-gen3

Lidgren Network Library
https://groups.google.com/forum/#!forum/lidgren-network-gen3
MIT License
1.19k stars 331 forks source link

Different fixes caught by coverity #35

Closed Therzok closed 9 years ago

Therzok commented 9 years ago

Best reviewed commit by commit, as they're pretty atomic.

There are a few issues remaining that coverity unveiled, but I didn't touch since I don't have that much expertise in the repository's code.

Specifically, NetQueue's TryDrain and TryDequeue are passible to having data races caused by the m_size checks.

NetPeer.Internal.cs seems to be deadlock-able. m_handshakes -> m_connections starting on line 655 conflicts with NetPeer.Internal.cs line 375 and line 249 which have m_connections -> m_handshakes.

Therzok commented 9 years ago

Any comments on the issues I have raised in the first comment? There are a few things I'd rather not touch myself. Especially the deadlock one seems really really dangerous.

lidgren commented 9 years ago

The first one (TryDrain and TryDequeue) isn't an issue. The size is checked a second time inside the lock so state will always remain consistent. If the first check return zero altho size really is larger, that's perfectly valid - if the check returns non-zero altho size is really zero, it will be caught by the second check.

The second issue I'm not sure about. There are no obvious potential deadlocks I can see... the code at line 655 doesn't take any lock on m_connections. However, the code in ExecutePeerShutdown doesn't need to take both locks simultaneously, altho shouldn't be a problem; I fixed it in c42b885e1f4788c46df0841ced1e24b607fe5826.

Therzok commented 9 years ago

:+1:

lidgren commented 9 years ago

Thanks for fixes!