[reSIProcate] resiprocate crash due to no free memory when receiving sip message with large header

[Byron Campen]

>
> Index: resip/stack/ConnectionBase.cxx
> ===================================================================
> --- resip/stack/ConnectionBase.cxx	(revision 8120)
> +++ resip/stack/ConnectionBase.cxx	(working copy)
> @@ -176,16 +176,26 @@
>              else
>              {
>                 // ...but some of the chunk must be shifted into the  
> next one.
> -               size_t size = numUnprocessedChars*3/2;
> -               if (size < ConnectionBase::ChunkSize)
> +               if (numUnprocessedChars >= ConnectionBase::ChunkSize)
>                 {
> -                  size = ConnectionBase::ChunkSize;
> +                  // .bwc. We have lots of unprocessed chars here;  
> this can be
> +                  // caused if we get a really, really large header  
> field value.
> +                  // We can safely reject this, I think.
> +                  delete [] mBuffer;
> +                  mBuffer = 0;
> +                  delete mMessage;
> +                  mMessage = 0;
> +                  //.jacob. Shouldn't the state also be set here?
> +                  delete this;
> +                  return;
>                 }
> -               char* newBuffer =  
> MsgHeaderScanner::allocateBuffer(size);
> +
> +               // ?bwc? Maybe do a slightly larger buffer here?
> +               char* newBuffer =  
> MsgHeaderScanner::allocateBuffer(ConnectionBase::ChunkSize);
>                 memcpy(newBuffer, unprocessedCharPtr,  
> numUnprocessedChars);
>                 mBuffer = newBuffer;
>                 mBufferPos = numUnprocessedChars;
> -               mBufferSize = size;
> +               mBufferSize = ConnectionBase::ChunkSize;
>              }
>              mConnState = ReadingHeaders;
>           }
>
>

	Actually, this patch will cause heap corruption, since mMessage has  
taken ownership of mBuffer. A better one would be:

Index: resip/stack/ConnectionBase.cxx
===================================================================
--- resip/stack/ConnectionBase.cxx	(revision 8120)
+++ resip/stack/ConnectionBase.cxx	(working copy)
@@ -176,16 +176,25 @@
              else
              {
                 // ...but some of the chunk must be shifted into the  
next one.
-               size_t size = numUnprocessedChars*3/2;
-               if (size < ConnectionBase::ChunkSize)
+               if (numUnprocessedChars >= ConnectionBase::ChunkSize)
                 {
-                  size = ConnectionBase::ChunkSize;
+                  // .bwc. We have lots of unprocessed chars here;  
this can be
+                  // caused if we get a really, really large header  
field value.
+                  // We can safely reject this, I think.
+                  delete mMessage;
+                  mBuffer = 0;
+                  mMessage = 0;
+                  //.jacob. Shouldn't the state also be set here?
+                  delete this;
+                  return;
                 }
-               char* newBuffer =  
MsgHeaderScanner::allocateBuffer(size);
+
+               // ?bwc? Maybe do a slightly larger buffer here?
+               char* newBuffer =  
MsgHeaderScanner::allocateBuffer(ConnectionBase::ChunkSize);
                 memcpy(newBuffer, unprocessedCharPtr,  
numUnprocessedChars);
                 mBuffer = newBuffer;
                 mBufferPos = numUnprocessedChars;
-               mBufferSize = size;
+               mBufferSize = ConnectionBase::ChunkSize;
              }
              mConnState = ReadingHeaders;
           }

Best regards,
Byron Campen

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2423 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20080710/bd5aedfc/attachment.bin>