< Previous by Date | Date Index | Next by Date > |
< Previous in Thread | Thread Index | Next in Thread > |
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@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 |