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

[Byron Campen <bcampen@xxxxxxxxxxxx>]

> 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

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