[reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

[Byron Campen]

	Here is the relevant code in TcpBaseTransport...

*snip*

    Connection* currConnection = mConnectionManager.getNextRead(fdset);
    if (currConnection)
    {
       if ( fdset.readyToRead(currConnection->getSocket()) ||
            currConnection->hasDataToRead() )
       {
          DebugLog (<< "TcpBaseTransport::processSomeReads() " <<  
*currConnection);
          fdset.clear(currConnection->getSocket());

          int bytesRead = currConnection->read(mStateMachineFifo);
          DebugLog (<< "TcpBaseTransport::processSomeReads() "
                    << *currConnection << " read=" << bytesRead);
          if (bytesRead < 0)
          {
             DebugLog (<< "Closing connection bytesRead=" << bytesRead);
             delete currConnection;
          }
       }

*snip*

The problem here is that Connection::read() can result in deletion of  
currConnection (see the following)

from Connection::read()
*snip*
int
Connection::read(Fifo<TransactionMessage>& fifo)
{
    std::pair<char*, size_t> writePair = getWriteBuffer();
    size_t bytesToRead = resipMin(writePair.second,
                                  static_cast<size_t> 
(Connection::ChunkSize));

    assert(bytesToRead > 0);
    int bytesRead = read(writePair.first, bytesToRead);
    if (bytesRead <= 0)
    {
       delete [] writePair.first;
       return bytesRead;
    }
    getConnectionManager().touch(this);
    preparseNewBytes(bytesRead, fifo); //.dcm. may delete this
    return bytesRead;
}

*snip*

from ConnectionBase::preparseNewBytes()
*snip*
       case ReadingHeaders:
       {
          unsigned int chunkLength = mBufferPos + bytesRead;
          char *unprocessedCharPtr;
          MsgHeaderScanner::ScanChunkResult scanChunkResult =
             mMsgHeaderScanner.scanChunk(mBuffer,
                                         chunkLength,
                                         &unprocessedCharPtr);
          if (scanChunkResult == MsgHeaderScanner::scrError)
          {
             //.jacob. Not a terribly informative warning.
             WarningLog(<< "Discarding preparse!");
             delete [] mBuffer;
             mBuffer = 0;
             delete mMessage;
             mMessage = 0;
             //.jacob. Shouldn't the state also be set here?
             delete this;
             return;
          }

*snip*

	Long story short, someone throws garbage at us over a Tcp socket,  
and we corrupt our heap. So, where should the responsibility for  
deletion of connections lie? If it belongs in ConnectionBase, we need  
to document this in the header file, in upper case, preferably about  
50 times. (Although it looks like the function return cannot be used  
to discover whether or not deletion occurred, and it appears that  
this would be difficult to change, so I would argue we need to stop  
this behavior.) And, as a general principle, I get very angry when my  
objects delete themselves on a whim. Imagine the fun if I had a stack- 
allocated Connection that decided to delete itself?

	Also, it is worth noting that a virtual destructor has not been  
declared for TcpConnection, and an explicitly non-virtual destructor  
has been declared for TlsConnection. I am not sure whether auto- 
generated destructors are virtual, so there are potentially two more  
guns aimed at our foot here.

Best regards,
Byron Campen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060503/ba08a2e1/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2369 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060503/ba08a2e1/attachment.bin>