Ok, I have committed the fix, plus some other fixes. Please take the code on head for a spin and see if it handles your testing.
Best regards, Byron Campen Yeah, I know that you cant guarantee anything there, and I also later realized it would be better to do the ErrLog after the other things have been deleted in case ErrLog also tries to allocate memory. Since the logging might be different on different systems you don't really know what it will do.
But my take on it is just that catching the exception is the only reasonable thing you can do there. And most likely you will want it logged somehow. If something like this happens and the system is no longer stable I'd like to have the reason in the logs.
Obviously, the best solution is to have checks in place so that you will not even try the allocation, but catching the exception could/should be an additional safe guard to prevent a complete crash.
/Markus
-----Original Message-----
From: Alan Hawrylyshen on behalf of Alan Hawrylyshen
Sent: Fri 7/11/2008 3:43 PM
To: Markus Eriksson
Cc: Byron Campen; resiprocate-devel@xxxxxxxxxxxxxxx
Subject: Re: [reSIProcate] resiprocate crash due to no free memory when receiving sip message with large header
Markus;
What platform are you running on? Your patch isn't really reliable..
once you have a new allocation failure, certain assurances from the
environment are no longer preset and even running ErrLog() is a
problem...
Alan
On Jul 11, 2008, at 03:00 , Markus Eriksson wrote:
> That patch works great. 1+ MB headers are no longer a problem (on my
> system).
>
> There still need to be some kind of handling for extremely large
> headers though. The problem that a tcp connection can use up all
> memory is still there, but this fixed the memory "leak".
>
> Maybe the failed allocation should be detected and then just drop
> the connection? That would be good even if there is a limit on the
> header size.
>
> Here is a patch that I believe will fix that issue also. It includes
> your patch from below and adds exception handling for when the
> memory allocation fails:
>
> 160a161
> >
> 161d161
> < mMessage->addBuffer(mBuffer);
> 163a164,197
> >
> > if(numUnprocessedChars==chunkLength)
> > {
> > // .bwc. MsgHeaderScanner wasn't able to parse
> anything useful;
> > // don't bother mMessage yet, but make more room in
> mBuffer.
> > size_t size = numUnprocessedChars*3/2;
> > if (size < ConnectionBase::ChunkSize)
> > {
> > size = ConnectionBase::ChunkSize;
> > }
> > char* newBuffer;
> > try{
> > newBuffer = MsgHeaderScanner::allocateBuffer(size);
> > }
> > catch( ... )
> > {
> > ErrLog(<< "Out of memory during preparse!
> Discarding connection");
> > delete [] mBuffer;
> > delete mMessage;
> > mMessage = 0;
> > mBuffer = 0;
> > delete this;
> > return;
> >
> > }
> > memcpy(newBuffer, unprocessedCharPtr,
> numUnprocessedChars);
> > delete [] mBuffer;
> > mBuffer = newBuffer;
> > mBufferPos = numUnprocessedChars;
> > mBufferSize = size;
> > mConnState = ReadingHeaders;
> > return;
> > }
> >
> 163a198
> > mMessage->addBuffer(mBuffer);
> 184c219,234
> < char* newBuffer =
> MsgHeaderScanner::allocateBuffer(size);
> ---
> > char* newBuffer;
> > try{
> > newBuffer =
> MsgHeaderScanner::allocateBuffer(size);
> > }
> > catch( ... )
> > {
> > ErrLog(<< "Out of memory during preparse!
> Discarding connection");
> > delete mMessage;
> > mMessage = 0;
> > mBuffer = 0;
> > // !bwc! mMessage just took ownership of
> mBuffer, so we don't
> > // delete it here. We do zero it though, for
> completeness.
> > delete this;
> > return;
> >
> > }
>
>
> /Markus
>
>
>
> -----Original Message-----
> From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx]
> Sent: Thu 7/10/2008 10:42 PM
> To: Alan Hawrylyshen
> Cc: Markus Eriksson; resiprocate-devel@xxxxxxxxxxxxxx
> Subject: Re: [reSIProcate] resiprocate crash due to no free memory
> when receiving sip message with large header
>
>
> ...
>
>
>
> > I guess this would be pretty easy to implement; if scanChunk
> returns
> > the whole buffer as unprocessed (ie; unprocessedCharPtr points at
> the
> > beginning of mBuffer), we won't call addBuffer(). (We will make more
> > room if necessary though) How's this for a patch:
>
> > Index: ConnectionBase.cxx
> > ===================================================================
> > --- ConnectionBase.cxx (revision 8120)
> > +++ ConnectionBase.cxx (working copy)
> > @@ -158,9 +158,30 @@
> > delete this;
> > return;
> > }
> > - mMessage->addBuffer(mBuffer);
> > +
> > unsigned int numUnprocessedChars =
> > (mBuffer + chunkLength) - unprocessedCharPtr;
> > +
> > + if(numUnprocessedChars==chunkLength)
> > + {
> > + // .bwc. MsgHeaderScanner wasn't able to parse
> anything useful;
> > + // don't bother mMessage yet, but make more room in
> mBuffer.
> > + size_t size = numUnprocessedChars*3/2;
> > + if (size < ConnectionBase::ChunkSize)
> > + {
> > + size = ConnectionBase::ChunkSize;
> > + }
> > + char* newBuffer =
> MsgHeaderScanner::allocateBuffer(size);
> > + memcpy(newBuffer, unprocessedCharPtr,
> numUnprocessedChars);
> > + delete [] mBuffer;
> > + mBuffer = newBuffer;
> > + mBufferPos = numUnprocessedChars;
> > + mBufferSize = size;
> > + mConnState = ReadingHeaders;
> > + return;
> > + }
> > +
> > + mMessage->addBuffer(mBuffer);
> > if (scanChunkResult == MsgHeaderScanner::scrNextChunk)
> > {
> > // Message header is incomplete...
>
>
>
|