[reSIProcate] resiprocate crash due to no free memory when receiving sip message with large header
Byron Campen
bcampen at estacado.net
Thu Jul 10 15:42:08 CDT 2008
>
> On Jul 10, 2008, at 10:39 , Byron Campen wrote:
>>
>>
>> Actually, this patch will cause heap corruption, since mMessage
>> has taken ownership of mBuffer. A better one would be:
>>
>> Index: resip/stack/ConnectionBase.cxx
>> ===================================================================
>> --- resip/stack/ConnectionBase.cxx (revision 8120)
>> +++ resip/stack/ConnectionBase.cxx (working copy)
>> @@ -176,16 +176,25 @@
>> 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.
>
>
> I don't think you can reject this -- what if the header is very
> large? Legitimately?
> I think that in general we should detect that we have an unprocessed
> number of bytes > chunkSize and grow chunkSize to accommodate this
> case (doubling or 3/2s at a time) as was the intent of the original
> code...
>
> // ...but some of the chunk must be shifted into the
> next one.
> size_t size = numUnprocessedChars*3/2;
> if (size < ConnectionBase::ChunkSize)
> {
> size = ConnectionBase::ChunkSize;
> }
> char* newBuffer =
> MsgHeaderScanner::allocateBuffer(size);
> memcpy(newBuffer, unprocessedCharPtr,
> numUnprocessedChars);
> mBuffer = newBuffer;
> mBufferPos = numUnprocessedChars;
> mBufferSize = size;
> }
>
> The above is my 'main' branch ConnectionBase code....
>
> Note that the size of the buffer will grow by 3/2s the number of
> unprocessed chars. In the case where the header has a value that
> continues beyond 2 buffers in size we SHOULD see the following
> behavior:
>
> [ a 60 char value for the 3 char header HDR ]
>
> HDR : valuevalu1valuevalu2valuevalu3valuevalu4valuevalu5valuevalu6
>
> With a ChunkSize of 16 (for simplicity's sake)
>
> 0 1 2 3 4 5 6 7 8 9 A B C D E F
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0000| H | D | R | | : | | v | a | l | u | e | v | a | l | u | 1 |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0010| v | a | l | u | e | v | a | l | u | 2 | v | a | l | u | e | v |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0020| a | l | u | 3 | v | a | l | u | e | v | a | l | u | 4 | v | a |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0030| l | u | e | v | a | l | u | 5 | v | a | l | u | e | v | a | l |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0040| u | 6 |
> +---+---+
>
>
> chunk0:
> 0 1 2 3 4 5 6 7 8 9
> +---+---+---+---+---+---+---+---+---+---+
> 0000| H | D | R | | : | | v | a | l | u |
> +---+---+---+---+---+---+---+---+---+---+
> ^ ^
> `-- Unknown(l=3) `-- unprocessedCharPtr
>
>
> After the first scanner pass, the SIP message has a header pointer
> (Unknown Header in this case), that points to chunk0+0x0 and has a
> length of 3. Therefore, some of chunk0's contents are being used by
> the message The unprocessedCharPtr has value chunk0 + 0x6 in this
> case.
>
> The code will then allocate a new buffer, necessarily copying chunk0
> into the new buffer, of size unproc*3/2 (6 in this case) -- this is
> a bit odd since we would expect better allocation related to
> chunksize, not # unprocessed chars. The latter is random based on
> the framing of the headers within the chunk.
>
> chunk1 ends up being:
> 0 1 2 3 4 5
> +---+---+---+---+---+---+
> 0000| v | a | l | u | e | v |
> +---+---+---+---+---+---+
> ^
> `- unprocessedCharPtr
>
> Another round (which requires that we save chunk1 gets a 9 char
> buffer:
>
> 0 1 2 3 4 5 6 7 8
> +---+---+---+---+---+---+---+---+---+
> 0000| v | a | l | u | e | v | a | l | u |
> +---+---+---+---+---+---+---+---+---+
>
> Another few rounds and we have: chunk(size)
> chunk2 (13), chunk3(19),
> chunk4 (19), chunk5(28),
> chunk6 (42), chunk7(63)
>
> Chunk7 is finally large enough to hold the entire header:
>
> 0 1 2 3 4 5 6 7 8 9 A B C D E F
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0000| v | a | l | u | e | v | a | l | u | 1 | v | a | l | u | e | v |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0010| a | l | u | 2 | v | a | l | u | e | v | a | l | u | 3 | v | a |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0020| l | u | e | v | a | l | u | 4 | v | a | l | u | e | v | a | l |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0030| u | 5 | v | a | l | u | e | v | a | l | u | 6 |
> +---+---+---+---+---+---+---+---+---+---+---+---+
>
>
> This was a really terrible growth mechanism since it too 7 or so
> copies to get the data for the header in a contiguous buffer. Worse,
> as you point out, chunks 1 through 6 are wasted memory.
>
> Here is what we have floating around...
>
> chunk0:
> 0 1 2 3 4 5 6 7 8 9
> +---+---+---+---+---+---+---+---+---+---+
> 0000| H | D | R | | : | | v | a | l | u |
> +---+---+---+---+---+---+---+---+---+---+
> ^ ^
> `-- Unknown(l=3) `-- unprocessedCharPtr
>
>
> chunk1:
> 0 1 2 3 4 5
> +---+---+---+---+---+---+
> 0000| v | a | l | u | e | v |
> +---+---+---+---+---+---+
> 2
> 0 1 2 3 4 5 6 7 8
> +---+---+---+---+---+---+---+---+---+
> 0000| v | a | l | u | e | v | a | l | u |
> +---+---+---+---+---+---+---+---+---+
> 3
> 0 1 2 3 4 5 6 7 8 9 A B C
> +---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0000| v | a | l | u | e | v | a | l | u | 1 | v | a | l |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+
> 4
> 0 1 2 3 4 5 6 7 8 9 A B C D E F
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0000| v | a | l | u | e | v | a | l | u | 1 | v | a | l | u | e | v |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0010| a | l | u |
> +---+---+---+
> 5
> 0 1 2 3 4 5 6 7 8 9 A B C D E F
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0000| v | a | l | u | e | v | a | l | u | 1 | v | a | l | u | e | v |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0010| a | l | u | 2 | v | a | l | u | e | v | a | l |
> +---+---+---+---+---+---+---+---+---+---+---+---+
> 6
> 0 1 2 3 4 5 6 7 8 9 A B C D E F
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0000| v | a | l | u | e | v | a | l | u | 1 | v | a | l | u | e | v |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0010| a | l | u | 2 | v | a | l | u | e | v | a | l | u | 3 | v | a |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0020| l | u | e | v | a | l | u | 4 | v | a |
> +---+---+---+---+---+---+---+---+---+---+
>
>
> If you look in the code ...
>
>
> //.jacob. I've discarded the "assigned" concept.
>
> Once upon a time there was a concept of 'assigned' on a Preparse
> buffer. If this was set when you had a fragmentation issue, you could
> safely release the buffer since NOTHING in the SIP header is using the
> buffer. NOTE WELL: THe first buffer is being used for the header field
> name. The second through n-1 buffers are useless duplication.
>
> I think this is a bug that was introduced with the MHS implementation
> when the assigned state was taken out of the FSM. Not a big deal and
> we can fix it.
>
> Additionally, it would be a great idea for us to change the test
> cases for connected transports to use a rediculously small chunkSize
> to validate that this all works in all cases ( I haven't checked
> that we do this yet ).
>
> Thanks,
>
> Alan Hawrylyshen
> Early reSIProcate Developer and Lurker
>
>
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...
Best regards,
Byron Campen
-------------- 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/4200205c/attachment.bin>
More information about the resiprocate-devel
mailing list