Re: [reSIProcate-users] Possible issue on WebSockets transport.

[Jorge Bustamante <jbustamante@xxxxxxxxxxxxx>]

Hi,

Any comments on this?

Regards,

Jorge Bustamante

On Tue, Oct 21, 2014 at 7:48 AM, Jorge Bustamante <jbustamante@xxxxxxxxxxxxx> wrote:

Hi,

Using resiprocate 1.9.7 (http://www.resiprocate.org/files/pub/reSIProcate/releases/resiprocate-1.9.7.tar.gz), I am getting the following memory corruption issue (got with valgrind's help) inside resiprocate code - this leads to random issues later on:

==10469== Invalid write of size 1
==10469==    at 0x78889CE: resip::WsFrameExtractor::processBytes(unsigned char*, unsigned int, bool&) (WsFrameExtractor.cxx:105)
==10469==    by 0x7768134: resip::ConnectionBase::wsProcessData(int) (ConnectionBase.cxx:775)
==10469==    by 0x7762BD8: resip::Connection::read() (Connection.cxx:363)
==10469==    by 0x7762CF1: resip::Connection::performReads(unsigned int) (Connection.cxx:385)
==10469==    by 0x7763282: resip::Connection::processPollEvent(unsigned short) (Connection.cxx:476)
==10469==    by 0x7C30A4A: resip::FdPollGrp::processItem(resip::FdPollItemIf*, unsigned short) (FdPoll.cxx:71)
==10469==    by 0x7C32A26: resip::FdPollImplEpoll::epollWait(int) (FdPoll.cxx:845)
==10469==    by 0x7C3275B: resip::FdPollImplEpoll::processFdSet(resip::FdSet&) (FdPoll.cxx:796)
==10469==    by 0x7C32582: resip::FdPollImplEpoll::waitAndProcess(int) (FdPoll.cxx:755)
==10469==    by 0x782422A: resip::SipStack::process(unsigned int) (SipStack.cxx:863)
==10469==    by 0x82EADF: sip::SipStack::processSipData(sip::SipSocketEvent&) (sipstack.cpp:1864)
==10469==    by 0x8B6C67: boost::_mfi::mf1<void, sip::SipStack, sip::SipSocketEvent&>::operator()(sip::SipStack*, sip::SipSocketEvent&) const (mem_fn_template.hpp:165)
==10469==  Address 0x959cecd is 0 bytes after a block of size 4,077 alloc'd
==10469==    at 0x4C28152: operator new[](unsigned long) (vg_replace_malloc.c:363)
==10469==    by 0x788892F: resip::WsFrameExtractor::processBytes(unsigned char*, unsigned int, bool&) (WsFrameExtractor.cxx:96)
==10469==    by 0x7768134: resip::ConnectionBase::wsProcessData(int) (ConnectionBase.cxx:775)
==10469==    by 0x7762BD8: resip::Connection::read() (Connection.cxx:363)
==10469==    by 0x7762CF1: resip::Connection::performReads(unsigned int) (Connection.cxx:385)
==10469==    by 0x7763282: resip::Connection::processPollEvent(unsigned short) (Connection.cxx:476)
==10469==    by 0x7C30A4A: resip::FdPollGrp::processItem(resip::FdPollItemIf*, unsigned short) (FdPoll.cxx:71)
==10469==    by 0x7C32A26: resip::FdPollImplEpoll::epollWait(int) (FdPoll.cxx:845)
==10469==    by 0x7C3275B: resip::FdPollImplEpoll::processFdSet(resip::FdSet&) (FdPoll.cxx:796)
==10469==    by 0x7C32582: resip::FdPollImplEpoll::waitAndProcess(int) (FdPoll.cxx:755)
==10469==    by 0x782422A: resip::SipStack::process(unsigned int) (SipStack.cxx:863)
==10469==    by 0x82EADF: sip::SipStack::processSipData(sip::SipSocketEvent&) (sipstack.cpp:1864)

Taking a closer look on this code (WsFrameExtractor.cxx), if I am not missing anything obvious (highly possible), I found out that if a read on the corresponding socket returns more bytes than one message (possible if there is network congestion and suddenly a lot of packets are received all at once?), mPayloadPos will end up being higher than mPayloadLength+1, causing the aforementioned memory corruption (since mPayload buffer was created with mPayloadLength+1 bytes). I made some changes on this processBytes(...) function and it is working fine for me after that, please don't trust me so much as it is highly possible I am totally wrong :), but at least it can help you see what I am talking about. Here is the diff of my changes:

diff -ENwbur resiprocate-1.9.7/resip/stack/WsFrameExtractor.cxx resiprocate-1.9.7_mod/resip/stack/WsFrameExtractor.cxx
--- resiprocate-1.9.7/resip/stack/WsFrameExtractor.cxx    2014-05-31 05:46:12.000000000 -0500
+++ resiprocate-1.9.7_mod/resip/stack/WsFrameExtractor.cxx    2014-10-21 07:39:47.000000000 -0500
@@ -84,9 +84,9 @@
          }
 
          Data::size_type takeBytes = len - pos;
-         if(takeBytes > mPayloadLength)
+         if(takeBytes > mPayloadLength - mPayloadPos)
          {
-            takeBytes = mPayloadLength;
+            takeBytes = mPayloadLength - mPayloadPos;
          }
 
          if(mPayload == 0)
@@ -121,6 +121,7 @@
             mHaveHeader = false;
             mHeaderLen = 0;
             mPayload = 0;
+            mPayloadPos = 0;
             if(mFinalFrame)
             {
                joinFrames();

Please let me know your comments and if you think this is a real issue or I might be doing something wrong.

Regards,

Jorge Bustamante

This email is intended solely for the person or entity to which it is addressed and may contain confidential and/or privileged information. If you are not the intended recipient and have received this email in error, please notify BroadSoft, Inc. immediately by replying to this message, and destroy all copies of this message, along with any attachment, prior to reading, distributing or copying it.