[reSIProcate] bug in SipFrag.cxx

[Derek MacDonald]

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>
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:  <mailto:resiprocate-devel at list.sipfoundry.org>
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/a3058a59/attachment.htm>