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

[Byron Campen <bcampen@xxxxxxxxxxxx>]

 inline

If I understand it correctly it does not grab a huge chunk at a time, it reads only 2048 bytes from the stream at a time.
I will illustrate with a small example. here the chunk size is 5 instead of 2048 and the message has the following header:
Example-Header: aaabbbcccddd

Ah! We had a terminology problem. I was interpreting "header" to mean the set of all header fields in the SIP message. Your problem now makes much more sense to me now.

first round:
bufferSize=5
buffer1="Examp"
scanChunk returns scrNextChunk and that 5 bytes are not parsed
buffer1 is given to SipMessage
buffer2 is allocated at size 1.5*(unparsed bytes) = 8
the unparsed bytes are copied to buffer2
SipMessage now contains buffer1(5 bytes)

second round:
bufferSize=8
buffer2="Example-"
scanChunk returns scrNextChunk and that 8 bytes are not parsed
buffer2 is given to SipMessage
buffer3 is allocated at size 1.5*(unparsed bytes) = 12
the unparsed bytes are copied to buffer3
SipMessage now contains buffer1(5 bytes) + buffer2(8) = 13 in total

third round:
bufferSize=12
buffer3="Example-Head"
scanChunk returns scrNextChunk and that 12 bytes are not parsed
buffer3 is given to SipMessage
buffer4 is allocated at size 1.5*(unparsed bytes) = 18
the unparsed bytes are copied to buffer4
SipMessage now contains buffer1(5 bytes) + buffer2(8) + buffer3(12) = 25 in total


This will continue with larger and larger chunks.
with real world numbers this would of course be buffer1(2000) + buffer2(3000) + buffer3(4500)

When the largest chunk is 1MB it means you also have 600k + 400k + 300k + 200k and so on stored in the SipMessage.

Is this correct or have I misunderstood?
I added debug prints and chunkLength and numUnprocessedChars were both > 1M the last time before it ran out of memory. Both were also increasing for each round.

Ok, now I see what you're talking about. Yes, this is a problem.

> It looks like we are taking things in smallish chunks in order to 
> save room; if we grab a chunk that is 50K long, and it contains 50 SIP 
> messages,

The way I understand it is that it only gets that big because one header is that big. With smaller headers it would never reach that state.

> Since a chunk can contain more than one SIP message, we can't really 
> arbitrarily restrict the chunk-size, although we _can_ put code in 
> that will notice when we have run the pre-parser over an unreasonably 
> large number of bytes.

Isn't that what my code does, saying that 50k is unreasonable and then discard the tcp connection? Or did you mean to handle it in another way? If I'm correct, then 50k means that there is 50 + 30 + 20 + ...  bytes allocated already = 150k or so.

Ok, on closer inspection, I see the code that will limit the size of individual read calls. So, as the code sits right now, it _looks_ like the only thing that causes the chunk size to get unreasonably large is the case you describe. I think we would be better off instead limiting the size of numUnprocessedChars when the MsgHeaderScanner returns scrNextChunk; this more directly addresses the problem I think. A patch might look like this:

Index: resip/stack/ConnectionBase.cxx

===================================================================

--- resip/stack/ConnectionBase.cxx (revision 8120)

+++ resip/stack/ConnectionBase.cxx (working copy)

@@ -176,16 +176,26 @@

             else

             {

                // ...but some of the chunk must be shifted into the next one.

-               size_t size = numUnprocessedChars*3/2;

-               if (size < ConnectionBase::ChunkSize)

+               if (numUnprocessedChars >= ConnectionBase::ChunkSize)

                {

-                  size = ConnectionBase::ChunkSize;

+                  // .bwc. We have lots of unprocessed chars here; this can be

+                  // caused if we get a really, really large header field value.

+                  // We can safely reject this, I think.

+                  delete [] mBuffer;

+                  mBuffer = 0;

+                  delete mMessage;

+                  mMessage = 0;

+                  //.jacob. Shouldn't the state also be set here?

+                  delete this;

+                  return;

                }

-               char* newBuffer = MsgHeaderScanner::allocateBuffer(size);

+               

+               // ?bwc? Maybe do a slightly larger buffer here?

+               char* newBuffer = MsgHeaderScanner::allocateBuffer(ConnectionBase::ChunkSize);

                memcpy(newBuffer, unprocessedCharPtr, numUnprocessedChars);

                mBuffer = newBuffer;

                mBufferPos = numUnprocessedChars;

-               mBufferSize = size;

+               mBufferSize = ConnectionBase::ChunkSize;

             }

             mConnState = ReadingHeaders;

          }

What do people think?

Additionally, as long as we're rooting around in this code, we should take the time to re-examine how we allocate buffers for the contents of a SIP message; we're allocating a buffer based on the assurances of the Content-Length field, but I don't think we should be doing this. I think we should _record_ what the Content-Length claims, but only allocate space as stuff comes in. (It would be easy to open up bunches of connections with SIP messages claiming to have really large (10M) payloads, but never send the bits, causing massive memory waste).

Best regards,

Byron Campen

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