[reSIProcate] Bug in BaseConnection (or TcpBaseTransport?)

[Derek MacDonald]

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