Data race in ProtocolGame

[djarek]

A data race occurs in ProtocolGame::parsePacket() between the ASI/O thread and the Dispatcher. Variables involved: the player pointer, player health and isRemoved flag, game state enum. I know, I'm being annoyingly pedantic about this, but a data race is a data race, it will bite someone in the ass sooner or later. For the most part this race isn't a problem, however, it is possible (though very unlikely) for a null pointer dereference(or use-after-delete) to occur if Protocol:release() and ProtocolGame::parsePacket() are executed concurrently by 2 threads.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[djarek]

On further examination I've also noticed that all parse* methods that access Game-related state also cause data races on them.

[marksamman]

Which parse* method (other than parsePacket itself) is accessing Game-related state in an unsafe manner?

[djarek]

All of them. They dereference the player pointer member variable to obtain the player's ID:

player->getID();

[marksamman]

I see. The race conditions in this case are fine though, I don't see any serious consequences from any of them, but if there's a chance for a null pointer dereference/use-after-free to happen, that must be fixed.

I'm not sure how ProtocolGame::parsePacket can be called simultaneously/after ProtocolGame::release has been called though. In Connection::close, we set connectionState to CLOSED before dispatching the release call. In Connection::parsePacket we only proceed if connectionState is set to OPEN. connectionState is protected with a mutex.

[djarek]

I guess I didn't notice back then that parsePacket is always called when holding the Connection mutex. So a use-after-free or null deref is not possible in this case, but there are some other weird bugs that might occur on architectures other than x86 (properly aligned reads are guaranteed to be atomic on x86, so unless someone does something really stupid with memory allocation it's not going to be a problem in most cases). It's possible for the player to get kicked if a torn read occurs on the health value https://github.com/otland/forgottenserver/blob/master/src/protocolgame.cpp#L405 . I'd say this issue has a low priority because not many people run this application on ARM or other architecture and the bug has a really low probability of occuring.

[Zbizu]

Does doing things like this solve that problem?

https://github.com/opentibiabr/otservbr-global/commit/b57962184d4b9ef980f0370b52fa86bf1aba3bce

[djarek]

Yeah, that approach would solve this issue. There's still an issue with ID reuse, but that is extremely unlikely.

[Zbizu]

Then we should use GUID

[yamaken93]

That is a messed up approach, the code which access the player variable could have been moved to dispatcher thread so whole ProtocolGame::parsePacket would be called by dispatcher thread and you would not need two addTask to dispatcher(one for module, another to parse packets) + copying the NetworkMessage twice for both tasks. The cherry on top: Why you need to use addGameTask with playerId when you are already calling from dispatcher thread?

[Zbizu]

well, threads and data races are beyond my knowledge, but I'm glad you know the solution already