< Previous by Date | Date Index | Next by Date > |
< Previous in Thread | Thread Index | Next in Thread > |
Good find Byron, Is there a test to reproduce this which
shows the problem under, say, valgrind? --Derek From:
resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx
[mailto:resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Byron Campen 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 |