[reSIProcate] bug in SipFrag.cxx

Tue Sep 12 17:31:18 CDT 2006

	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);
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060912/56505b76/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/56505b76/attachment.bin>