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

[Byron Campen]

  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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20080710/7b2170b4/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2423 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20080710/7b2170b4/attachment.bin>