Socket write sequencing memory leak bug in free version of socket server framework

I’ve been tracking a bug for a client recently and yesterday I got a remarkably similar bug report from one of the users of my free socket server framework. The client’s code is pretty old and was originally based on a server framework that was based on the free version of the code and it hasn’t been upgraded to the latest version yet. The bug in my client code is also present in the latest (Feb 14 2006) version of the free socket server framework - found here. The bug does NOT affect the licensed version of The Server Framework unless you’re using a pre-refactored version (i.e. one that is VERY old), if you’re unsure, contact me.

The bug manifests in servers that are using write sequencing as data loss when sending and a memory leak. Once a connection has got into this state then it is unable to send ever again and every attempt to send will result in the data buffer that contains the data to be sent being placed in the sequencing list and left there forever. Closing the connection frees the leaked memory.

The bug is something that is so glaringly obvious and SO likely to cause problems that I’m amazed that it hasn’t been caught before. I think the reason for this is that it is most likely to manifest on a system which has a small number of very busy connections that are written to from multiple threads and the black box server stress tests that I have for the free version of the code all focus on many busy connections. The bug will cause problems if two threads attempt to write to a connection at the same time, sequence numbers can become non-sequential (i.e. some are missed and some duplication might occur) and this will cause the code that ensures that writes go to the wire in the correct sequence to queue data indefinitely (whilst it waits for the missing buffers).

In SocketServer.cpp (in the Win32Tools library) this code:

long CSocketServer::Socket::GetSequenceNumber(
   SequenceType type) const 
{
   SequenceData *pSequenceData = GetSequenceData();
  
   if (pSequenceData)
   {
      return pSequenceData->m_numbers[type]++;
   }
  
   return 0;
}

Should be changed to this:

long CSocketServer::Socket::GetSequenceNumber(
   SequenceType type) const 
{
   SequenceData *pSequenceData = GetSequenceData();
  
   if (pSequenceData)
   {
      return ::InterlockedIncrement(&pSequenceData->m_numbers[type]);
   }
  
   return 0;
}

And then, due to the way that InterlockedIncrement() returns the result of the increment and the previous code was returning the value before increment, we need to change the InOrderBufferList class in IOBuffer.cpp, first in the constructor:

From:

CIOBuffer::InOrderBufferList::InOrderBufferList(
   CCriticalSection &criticalSection)
   :  m_next(0),
      m_criticalSection(criticalSection)
{
}

To:

CIOBuffer::InOrderBufferList::InOrderBufferList(
   CCriticalSection &criticalSection)
   :  m_next(1),
      m_criticalSection(criticalSection)
{
}

And next in the Reset() function, from:

void CIOBuffer::InOrderBufferList::Reset()
{
   m_next = 0;
  
   if (!m_list.empty())

To:

void CIOBuffer::InOrderBufferList::Reset()
{
   m_next = 1;
  
   if (!m_list.empty())

I will, most likely, release an updated version of the code in the near future that incorporates this fix.