[reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

[Byron Campen]

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] 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/93a84457/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/20060504/93a84457/attachment.bin>