Re: [reSIProcate-users] Error in ConnectionBase::getWriteBufferForExtraBytes() ?

[slgodin@xxxxxxxxx]

Looks good to me so far.  I need to find time to cleanup the commit though, since you left some personal comments and commented out code that would make a merge from the pull request not ideal.  If you want to clean those things up, that would help.

Thanks again!. I really appreciate the contribution.

Scott

Sent from my iPhone

On Apr 10, 2018, at 1:00 PM, Michael Trank <Michael.Trank@xxxxxxxxxxxx> wrote:

Hi Scott:

I'm not sure that you've seen my email from last week. I did the pull request thing as you requested.

I'll be interested in any feedback that you have.

So far, this is working out well for me.

---

From: slgodin@xxxxxxxxx <slgodin@xxxxxxxxx> on behalf of Scott Godin <sgodin@xxxxxxxxxxxxxxx>
Sent: Monday, April 2, 2018 7:20:13 PM
To: Michael Trank
Cc: resiprocate-users@xxxxxxxxxxxxxxx
Subject: Re: [reSIProcate-users] Error in ConnectionBase::getWriteBufferForExtraBytes() ?

 

Hi Michael,

I'm still trying to figure out Git myself.  :)  Daniel Pocock may have posted something to mailing list with better instructions, but I think you can also google how to create a pull request on GitHub, and once it's done it will appear here:

https://github.com/resiprocate/resiprocate/pulls

This gives other devs a chance to review the changes, and make comments.  GitHub will verify the pull request builds, and if all is well, we can just merge the pull request into master.  :)

Thanks!

Scott

On Mon, Apr 2, 2018 at 6:45 PM, Michael Trank <Michael.Trank@xxxxxxxxxxxx> wrote:

Hi Scott:

Will I need to provide a URL from which you can pull the changes?

---

From: slgodin@xxxxxxxxx <slgodin@xxxxxxxxx> on behalf of Scott Godin <sgodin@xxxxxxxxxxxxxxx>
Sent: Monday, April 2, 2018 4:19:29 PM
To: Michael Trank
Cc: resiprocate-users@resiprocate.org
Subject: Re: [reSIProcate-users] Error in ConnectionBase::getWriteBufferForExtraBytes() ?

 

Hi Michael,

Thanks so much for posting this.  After a quick read it sounds like you have done a great job analyzing the situation, and what you are saying makes sense to me.  I'd like to spend a little more time looking at the surrounding code and understanding the problem.  Would you be willing to post a Git Pull request with your modifications to the resip GitHub repo?

Thanks again!

Scott

On Fri, Mar 30, 2018 at 2:16 PM, Michael Trank <Michael.Trank@xxxxxxxxxxxx> wrote:

Scott and other resiprocate-users:

I have recently come across problems when using the Repro/Resiprocate code with SIP/WSS UA's that send large SDP payloads such that the entire INVITE request is greater than 8192 bytes. This has become more common of late with WebRTC-based UA's with large "candidate" lists.  Specifically, the first INVITE message arriving over WSS is received and handled okay, but all subsequent large INVITE requests elicit parse errors exceptions.

I have traced the problem to ConnectionBase::getWriteBufferForExtraBytes(), which I believe returns the wrong value for the pointer where the TlsConnection should place the "extra bytes" read in from the TLS pipe.

char*
ConnectionBase::getWriteBufferForExtraBytes(int extraBytes)
{
   if (extraBytes > 0)
   {
      char* buffer = MsgHeaderScanner::allocateBuffer((int)mBufferSize + extraBytes);
      memcpy(buffer, mBuffer, mBufferSize);
      delete [] mBuffer;
      mBuffer = buffer;
      buffer += mBufferSize;
      mBufferSize += extraBytes;
      return buffer;
   }
   else
   {
      resip_assert(0);
      return mBuffer;
   }
}



Note that the size of the buffer before reallocation is not necessarily the place where the extra bytes should be placed. In my case, the SIP/WSS UA used the same connection to send serveral INVITE transactions with large payloads bigger than the hard-coded "chunk size", 8192. On the first request, the first SSL_read() gets 8192 bytes, and SSL_pending() gets back, say, 2000. The buffer is reallocated to 10192 bytes, mBufferSize = 10192, and the pointer returned by getWriteBufferForExtraBytes() returns mBuffer + 8192. So far so good.

But on the next large request, SSL_read() gets 8192 bytes, SSL_pending gets back 2000, and then, without checking is the buffer is already big enough, getWriteBufferForExtraBytes() again increases the size of the buffer to 12192, and returns 10192 as the place when the extra bytes should be written. Meaning that the "extra bytes" don't get written into the buffer concatenated with the first 8192 bytes, but rather from positions 10192 through 12191 of the 12192 byte long buffer. Since the client sends these bytes with "masking" the part of the buffer that got passed over is all goblygook when the SIP message is unmasked and parsed from the buffer.

I changed getWriteBufferForExtraBytes() to be the following code, with a parameter for passing in the number of bytes actually read by TlsConnection::read() in the first SSL_read(), so that reallocation only occurs if necessary and the correct position within the buffer is returned. It seems to have fixed the problem.

I invite comments and advice.

Thanks, as always, for your help with this stuff.

char*
ConnectionBase::getWriteBufferForExtraBytes(int readBytes, int extraBytes)
{
        if ((readBytes > 0) && (extraBytes > 0))
        {
                if ((readBytes + extraBytes) > mBufferSize) {
                        mBufferSize = readBytes + extraBytes;
                        char * buffer = MsgHeaderScanner::allocateBuffer(mBufferSize);
                        memcpy(buffer, mBuffer, readBytes);
                        delete[] mBuffer;
                        mBuffer = buffer;
                }
                return &mBuffer[readBytes];
        }
        else
        {
                resip_assert(0);
                return mBuffer;
        }
}

_______________________________________________
resiprocate-users mailing list
resiprocate-users@resiprocate.org
List Archive: http://list.resiprocate.org/archive/resiprocate-users/