[reSIProcate] bug in SipFrag.cxx

[Byron Campen]

	Well, I wrote a test-case for this, and it appears to function  
without the fix. It looks like we stomp on the boundary token while  
parsing the sipfrag, but at that point we have already done  
everything we need with the boundary token. When we encode, we use  
the boundary token we stored at the very beginning, and so the bits  
that we stomped on never make it to the wire. (Although it looks like  
we are encoding the extra CRLFCRLF at the end of the sipfrag; is this  
a problem?) This seems fragile to me; any change in the way we are  
parsing or encoding could cause the whole thing to fall apart.

Best regards,
Byron Campen

> Does this look sane? Is it important to check whether the primary  
> mimetype is message?
>
> Index: resip/stack/Contents.cxx
> ===================================================================
> --- resip/stack/Contents.cxx    (revision 6561)
> +++ resip/stack/Contents.cxx    (working copy)
> @@ -148,6 +148,14 @@
>     assert(!contents.mMine);
>     HeaderFieldValue *hfv = new HeaderFieldValue(contents.data(),  
> contents.size());
>
> +   if(contentType.subType()=="sipfrag"||contentType.subType() 
> =="external-body")
> +   {
> +      // !bwc! The parser for sipfrag requires padding at the end  
> of the hfv.
> +      HeaderFieldValue* temp = hfv;
> +      hfv = new HeaderFieldValue 
> (*temp,HeaderFieldValue::CopyPadding);
> +      delete temp;
> +   }
> +
>     Contents* c;
>     if (ContentsFactoryBase::getFactoryMap().find(contentType) !=  
> ContentsFactoryBase::getFactoryMap().end())
>     {
>
> Best regards,
> Byron Campen
>
>> Hmm, probably not; looking at MultipartMixedContents we probably  
>> need a fairly expensive special case copy when the content-type is  
>> message/sipfrag or message/external.  It should be easy to  
>> reproduce&fix.
>>
>>
>>
>> -Derek
>>
>>
>>
>> From: Byron Campen [mailto:bcampen at estacado.net]
>> Sent: Thursday, August 31, 2006 11:54 AM
>> To: Derek MacDonald
>> Cc: 'Kath, Heiner'; resiprocate-devel at list.sipfoundry.org
>> Subject: Re: [reSIProcate] bug in SipFrag.cxx
>>
>>
>>
>>           Yes, but is this code safe when you have a sipfrag in a  
>> multipart-mixed payload?
>>
>>
>>
>> Best regards,
>>
>> Byron Campen
>>
>>
>>
>>
>> Yes; the transport classes use MsgHeaderScanner::allocateBuffer 
>> (int size) to guarantee that there are 5 extra bytes at the end of  
>> each buffer. I do not know of any memory problems as a result of  
>> this with the reciprocate transports.
>>
>>
>>
>> -Derek
>>
>>
>>
>> From: resiprocate-devel-bounces at list.sipfoundry.org  
>> [mailto:resiprocate-devel-bounces at list.sipfoundry.org] On Behalf  
>> Of Byron Campen
>> Sent: Thursday, August 31, 2006 10:34 AM
>> To: Kath, Heiner
>> Cc: resiprocate-devel at list.sipfoundry.org
>> Subject: Re: [reSIProcate] bug in SipFrag.cxx
>>
>>
>>
>>           Good eye. Your solution sounds about as optimal as can  
>> be managed.
>>
>>
>>
>> Best regards,
>>
>> Byron Campen
>>
>>
>>
>> Hi,
>>
>>
>>
>> I found this in SipFrag.cxx:
>>
>>
>>
>> {
>>
>>
>>
>>
>>
>>    const char *constBuffer = pb.position();
>>
>>
>>
>>
>>
>>    // than size bytes of the message.
>>
>>
>>
>>    msgHeaderScanner.prepareForFrag(mMessage, hasStartLine(buffer,  
>> size));
>>
>>    char saveTermCharArray[sentinelLength];
>>
>>    saveTermCharArray[0] = termCharArray[0];
>>
>>    saveTermCharArray[2] = termCharArray[2];
>>
>>    termCharArray[0] = '\r';
>>
>>    termCharArray[2] = '\r';
>>
>>    char *scanTermCharPtr;
>>
>>        msgHeaderScanner.scanChunk(buffer,
>>
>>                                   &scanTermCharPtr);
>>
>>    termCharArray[1] = saveTermCharArray[1];
>>
>>    termCharArray[3] = saveTermCharArray[3];
>>
>>
>>
>> The problem with this code is that the sentinel is wrote *behind*  
>> the ParseBuffer. If the ParseBuffer stands at the very end of an  
>> allocated buffer, this code writes 4 bytes behind it. So it  
>> happened in our application. As a consequence a further allocation  
>> of memory inside of scanChunk() failed – probably because some  
>> administration information of the memory heap was overwritten.
>>
>> I can easily reproduce this using the gflags tool of the Mircosoft  
>> debugging tools.
>>
>> My solution consists in allocating a new buffer that has the  
>> required size (+4), copying the content of the ParseBuffer to it.  
>> The new buffer is referenced by the
>>
>>
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at list.sipfoundry.org
> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060912/06a22443/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2369 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060912/06a22443/attachment.bin>