[reSIProcate] bug in SipFrag.cxx
Byron Campen
bcampen at estacado.net
Tue Sep 12 16:56:40 CDT 2006
Jeez, I need to learn to finish reading mail before I hit reply. So
the scenario I just mentioned has already been corrected.
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:
>
>
>
>
>
> *constBuffer = pb.position();
>
>
>
> // than size bytes of the message.
>
> msgHeaderScanner.prepareForFrag(mMessage, hasStartLine(buffer,
> size));
>
> char saveTermCharArray[0] = termCharArray[0];
>
> termCharArray[0] = ;
>
> '\r' *scanTermCharPtr;
>
> &scanTermCharPtr);
>
> 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/9eecaf00/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/9eecaf00/attachment.bin>
More information about the resiprocate-devel
mailing list