[reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

[Byron Campen]

	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
>
>
>
> 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] 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/84ff2126/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/84ff2126/attachment.bin>