[reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

[Derek MacDonald]

I removed the offending log statement; I will re-work the code so that the
delete this is removed and ConnectionBase lifetime is more explicit.  Joszef
wrote some testes in tfm for this; they currently are expected to fail; will
look into changing this.

 

--Derek

 

  _____  

From: resiprocate-devel-bounces at list.sipfoundry.org
[mailto:resiprocate-devel-bounces at list.sipfoundry.org] On Behalf Of Byron
Campen
Sent: Thursday, May 04, 2006 9:07 AM
To: Scott Godin
Cc: resiprocate-devel
Subject: Re: [reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

 

          Huh, this is a behavior I was not aware of. Makes more sense than
the alternative though. Sorry for the false alarm.

 

Best regards,

Byron Campen





I'm not sure I understand the virtual destructor case you are making: 

 

In your example the Connection destructor should get properly called right?

 

BTW:  I found the following text:

Note: if your base class has a virtual destructor, then your destructor is
automatically virtual. You might need an explicit destructor for other
reasons, but there's no need to redeclare a destructor simply to make sure
it is virtual. No matter whether you declare it with the virtual keyword,
declare it without the virtual keyword, or don't declare it at all, it's
still virtual.

At:
<http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7>
http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7

 

Scott

 

  _____  

From: Byron Campen [mailto:bcampen at estacado.net] 
Sent: Thursday, May 04, 2006 10:23 AM
To: Scott Godin
Cc: resiprocate-devel
Subject: Re: [reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

 

Scott:

          Yes, but all destructors in a class tree need to be virtual, or
you run into problems. For instance, if you have a TcpConnection* cast as a
Connection*, and you delete it, ~TcpConnection() never gets called, since
the vptr doesn't exist.

 

list:

          I would argue that this self-deletion behavior is incorrect, and
needs to be removed. Having "delete this" as a side-effect of a call is
usually insane, and makes a class unsuitable for stack allocation. (I am
planning on making a sweep through the codebase for other instances of
"delete this", and bringing poorly-chosen instances to the attention to the
list) Also, this sort of decision should not be made way down at the
base-class; this should be up to the Transport objects.

 

Best regards,

Byron Campen






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>
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/20060508/d1843c0d/attachment.htm>