< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

Re: [reSIProcate] resiprocate crash due to no free memory when receiving sip message with large header


Excellent. Thanks for bringing this to our attention! I will be cutting a point-release with this fix later today (after some testing of my own).

Best regards,
Byron Campen

It seems to be working fine.

/Markus


-----Original Message-----
From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx]
Sent: Sat 7/12/2008 12:25 AM
To: Markus Eriksson
Cc: Alan Hawrylyshen; resiprocate-devel@xxxxxxxxxxxxxxx
Subject: Re: [reSIProcate] resiprocate crash due to no free memory when receiving sip message with large header

        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...
> >
> >
> >
>
>
>



Attachment: smime.p7s
Description: S/MIME cryptographic signature