ThreadedCommServer race condition causing segfault

[GingerPrince]

We’ve found a bug that’s can cause a segfault in MOOSDB, I've attached a patch for a fix as .txt files as that's what's supported for upload. There are possible performance implications with our patch, as we're now mutex locking ProcessClient which may have been concurrent before, and now is more sequential. This can only be avoided with C+17 shared_lock or some complicated back-port of that to C++11 ( our compiler standard).

The bug ultimately caused a segfault in MOOSDB at ThreadedCommServer.cpp#388:

bool ThreadedCommServer::ProcessClient(ClientThreadSharedData &SDFromClient,MOOS::ServerAudit & Auditor)
...
for(q=m_ClientThreads.begin();q!=m_ClientThreads.end();++q)
{
ClientThread* pClient = q->second;
if(m_pfnFetchAllMailCallBack!=NULL && pClient->IsAsynchronous())

because pClient is null/uninitialised. This is caused by a race condition with:

bool ThreadedCommServer::AddAndStartClientThread(XPCTcpSocket & NewClientSocket,const std::string & sName)
...
m_ClientThreads[sName] = pNewClientThread;

Adding to m_ClientThreads needs to be mutually exclusive with any other operation on the map and needs guarding with a mutex (only with the Add though, multiple Reads can happen at once).

ThreadedCommServer.cpp.txt
ThreadedCommServer.h.txt

[GingerPrince]

I should add, I don’t think the performance impact will be big, if at all. I’m not even sure how often ProcessClient is called simultaneously and even if it is, we’ve been careful not to hold the mutex longer than needed. Just thought I should point out the fact there is now a mutex in there that could have an impact.

[russkel]

If one were to compile MOOS with ThreadSanitizer (https://clang.llvm.org/docs/ThreadSanitizer.html) from clang enabled it might bring out other issues like this.

Also would help if there was a proper test suite that this could be run on.