reSIProcate

Mailing Lists

Source Code

Google Search:



< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

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@xxxxxxxxxxxx]
Sent: Thursday, August 31, 2006 11:54 AM
To: Derek MacDonald
Cc: 'Kath, Heiner'; resiprocate-devel@xxxxxxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxxxxxx [mailto:resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Byron Campen
Sent: Thursday, August 31, 2006 10:34 AM
To: Kath, Heiner
Cc: resiprocate-devel@xxxxxxxxxxxxxxxxxxx
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

Attachment: smime.p7s
Description: S/MIME cryptographic signature