[reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

[Byron Campen]

	No, don't have something that will work with the code on sipfoundry  
right now. But I imagine just blowing gibberish into an open tcp  
socket in tfm would be sufficient. Just need to make the  
MsgHeaderScanner unhappy.

Best regards,
Byron Campen


On May 3, 2006, at 4:52 PM, Derek MacDonald wrote:

> Good find Byron,
>
>
>
> Is there a test to reproduce this which shows the problem under,  
> say, valgrind?
>
>
>
> --Derek
>
>
>
> 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 11:38 AM
> 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/20060503/ba052021/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/ba052021/attachment.bin>