[reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

[Scott Godin]

I suspect what is causing the trap is the DebugLog of *currConnection
after it has been deleted - we could either remove this log, or to get
rid of the self deletion...  Perhaps we should change preparseNewBytes
to return an error (or set to a newly created error state) - then in
Connection::read(), if preparseNewBytes returns an error - we should
return bytesRead < 0 - which will cause TcpBaseTransport to delete the
connection.

 

As for the destructor problem - you are correct - default destructors
are not virtual.  But TlsConnection is not inherited from TcpConnection
- it is derived from Connection - which does have a virtual destructor.


 

Scott

 

________________________________

From: resiprocate-devel-bounces at list.sipfoundry.org
[mailto:resiprocate-devel-bounces at list.sipfoundry.org] On Behalf Of
Byron Campen
Sent: Wednesday, May 03, 2006 2:38 PM
To: resiprocate-devel
Subject: [reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

 

          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/20060504/726107cd/attachment.htm>