[reSIProcate] bug in SipFrag.cxx

Tue Sep 12 17:46:10 CDT 2006

	Yeah, that did it. Check testSipFrag under valgrind. So, the fix you  
have proposed sounds like it would work. Lets have that integrated/ 
tested sometime tomorrow if we can.

Best regards,
Byron Campen

> 	Ahh, that isn't a full parse at MultipartMixedContents.cxx:228.  
> So, I guess if you called MultipartMixedContents::parts(), without  
> causing the SipFrag to parse, and THEN copied the SipMessage, we  
> would get badness. Let me try this...
>
> Best regards,
> Byron Campen
>> Not sure it can’t occur; It looks like Contents::createContents  
>> doesn’t add padding bytes(which is fine).  The issue is that
>>
>> MultipartMixedContents::MultipartMixedContents(const  
>> MultipartMixedContents& rhs) will call SipFrag->clone() and if the  
>> SipFrag isn’t parsed the passing bytes are lost.  Where do you see  
>> the individual parts being non-lazily parsed?
>>
>>
>>
>> From: Byron Campen [mailto:bcampen at estacado.net]
>> Sent: Tuesday, September 12, 2006 2:51 PM
>> To: Derek MacDonald
>> Cc: 'resiprocate-devel'
>> Subject: Re: [reSIProcate] bug in SipFrag.cxx
>>
>>
>>
>>           I thought about that possibility; but right now the  
>> individual parts never get their own HeaderFieldValues. Only one  
>> is ever assigned, to the MultipartMixedContents. When we call parts 
>> (), checkParsed() is called, and the individual *Contents are then  
>> created and immediately parsed (HeaderFieldValues are never  
>> assigned). However, it occurs to me that if we had a single  
>> unparsed SipFrag in a SipMessage's body, and then copied that  
>> SipMessage, we could be in loads of trouble.
>>
>>
>>
>> Best regards,
>>
>> Byron Campen
>>
>>
>>
>>
>> Looking at this further, we don’t have an issue with  
>> MultipartMixed.  What the sipfrag parser does it save the extra 4  
>> bytes it needs, parse the sipfrag, and restores the four bytes.   
>> As a result, we don’t actually stomp on the boundary token, we  
>> just temporarily overwrite.  This is completely safe until the  
>> unlikely day where we parallelize the MultipartMixed parser.
>>
>>
>>
>> However, there might be an even more subtle problem.  If the  
>> MultiPartMixed is cloned after parse but before the parts are  
>> parsed, HeaderFieldValue(const HeaderFieldValue& hfv,  
>> CopyPaddingEnum) is not invoked which would mean that  
>> SipFrag::parse will write beyond the memory of its hfv.
>>
>>
>>
>> This didn’t show up in valgrind, but I believe it occurs(may not  
>> have hit the test, it might be valgrind behaviour). SipMessage  
>> invokes HeaderFieldValue(const HeaderFieldValue& hfv,  
>> CopyPaddingEnum) to avoid an issue we found w/ SipFrag parsing  
>> after a copy.  The fix would be to have SipFrag have the same  
>> effective behaviour, adding the CopyPaddingEnum cons. Parameter to  
>> a new constructor for Contents and LazyParser.
>>
>>
>>
>> Thoughts?
>>
>>
>>
>> -Derek
>>
>>
>>
>> From: Byron Campen [mailto:bcampen at estacado.net]
>> Sent: Tuesday, September 12, 2006 8:55 AM
>> To: Derek MacDonald
>> Cc: resiprocate-devel
>> Subject: Re: [reSIProcate] bug in SipFrag.cxx
>>
>>
>>
>>           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:
>>
>>
>>
>>
>>
>>    msgHeaderScanner.prepareForFrag(mMessage, hasStartLine(buffer,  
>> size));
>>
>>    saveTermCharArray[0] = termCharArray[0];
>>
>> ;
>>
>>                                      &scanTermCharPtr);
>>
>>
>
> _______________________________________________
> 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/5e61adda/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/5e61adda/attachment.bin>