[reSIProcate] MHS Problems.
Byron Campen
bcampen at estacado.net
Fri Jul 11 10:07:44 CDT 2008
I think we should allow the max header length to be configurable.
Should we restrict by default?
I believe my latest patch addresses the "efficient use of memory"
problem without adding an 'assignment detection' concept. If I am
missing something, please let me know.
Also, we could allow other limitations to be configured, like:
- Maximum number of header field values per message (maybe even on a
per header type basis)
- Maximum number of parameters per header field value
- Maximum number of bytes in body
Best regards,
Byron Campen
>
> As my message points out, there is potentially a DOS / buffer
> handling error in the MHS class due to the lack of 'assignment
> detection' at present on long header field values.
>
> I'm calling this out from the earlier thread since this (like the
> body problem Byron points out) is a real issue that we should
> jointly agree on how to solve.
>
> My suggestions:
>
> There COULD be a max header length setting, should be settable in
> the API.
> There SHOULD be efficient use of memory to prevent lots of wasted
> space in busy systems (this favors smaller buffers).
> Given that we favor smaller buffers, we need to ensure that any
> unused buffers are freed (unlike now).
>
> Thoughts?
>
> Alan
>
> Here's the original posting that I had on this problem...
> Thanks to Markus for setting the wheels in motion.
>
> Message-Id: <A0720461-B4A5-4051-8379-F4C69A1A9C56 at polyphase.ca>
>
>
>
> 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
> On Jul 10, 2008, at 12:24 , Alan Hawrylyshen wrote:
>
>>
>> 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
>>
>>
>
-------------- 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/20080711/63f4d13f/attachment.bin>
More information about the resiprocate-devel
mailing list