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

Re: [reSIProcate] [resip/stack][patch 1/2] Code cleanup for ConnectionBase.cxx



Hi Anzai,

Thanks for your contributions, they are very welcome

I am reviewing the code tomorrow and will hopefully be able to integrate
it quickly.

Regards,

Daniel


On 10/05/13 18:48, Zhi An Ng wrote:
> Hi all,
> 
> This patch removes an entire portion of wsProcessHandshake() that
> deals with an deprecated protocol. [1]
> There is also a bit of refactoring. Scanning the message header is its
> own function now. Making the handshake response is also its own
> function, which handles instances when a header can not be found.
> 
> [1] http://www.whatwg.org/specs/web-socket-protocol/
> 
> Best,
> Zhi An
> 
> ---diff starts below---
> Index: ConnectionBase.cxx
> ===================================================================
> --- ConnectionBase.cxx (revision 10202)
> +++ ConnectionBase.cxx (working copy)
> @@ -556,104 +556,30 @@
>     mMessage->setSource(mWho);
>     mMessage->setTlsDomain(mWho.transport->tlsDomain());
> 
> -   mMsgHeaderScanner.prepareForMessage(mMessage);
> -   char *unprocessedCharPtr;
> -   MsgHeaderScanner::ScanChunkResult scanResult =
> mMsgHeaderScanner.scanChunk(mBuffer, mBufferPos + bytesRead,
> &unprocessedCharPtr);
> -   if (scanResult != MsgHeaderScanner::scrEnd)
> -   {
> -      if(scanResult != MsgHeaderScanner::scrNextChunk)
> -      {
> -         StackLog(<<"Failed to parse message, more bytes needed");
> -         StackLog(<< Data(mBuffer, bytesRead));
> -      }
> -      delete mMessage;
> -      mMessage=0;
> -      mBufferPos += bytesRead;
> +   if (!scanMsgHeader(bytesRead)) {
>        return false;
>     }
> 
>     try
>     {
> -      Data wsResponse;
> -      wsResponse = "HTTP/1.1 101 WebSocket Protocol Handshake\r\n"
> -            "Upgrade: WebSocket\r\n"
> -            "Connection: Upgrade\r\n"
> -            "Sec-WebSocket-Protocol: sip\r\n";
> -
> -      if(mMessage->exists(h_SecWebSocketKey1) &&
> mMessage->exists(h_SecWebSocketKey2))
> +      Data wsResponse = makeWsHandshakeResponse();
> +      if (!wsResponse.empty())
>        {
> -         Data SecWebSocketKey1 =
> mMessage->const_header(h_SecWebSocketKey1).value();
> -         Data SecWebSocketKey2 =
> mMessage->const_header(h_SecWebSocketKey2).value();
> -         Data Digits1, Digits2;
> -         unsigned int SpacesCount1 = 0, SpacesCount2 = 0;
> -         unsigned int Value1, Value2;
> -         for(unsigned int i = 0; i < SecWebSocketKey1.size(); ++i)
> -         {
> -            if(SecWebSocketKey1[i] == ' ') ++SpacesCount1;
> -            if(isdigit(SecWebSocketKey1[i])) Digits1 += SecWebSocketKey1[i];
> -         }
> -         Value1 = htonl(Digits1.convertUnsignedLong() / SpacesCount1);
> -         for(unsigned int i = 0; i < SecWebSocketKey2.size(); ++i)
> -         {
> -            if(SecWebSocketKey2[i] == ' ') ++SpacesCount2;
> -            if(isdigit(SecWebSocketKey2[i])) Digits2 += SecWebSocketKey2[i];
> -         }
> -         Value2 = htonl(Digits2.convertUnsignedLong() / SpacesCount2);
> -
> -         MD5Stream wsMD5Stream;
> -         char tmp[9] = { '\0' };
> -         memcpy(tmp, &Value1, 4);
> -         memcpy(&tmp[4], &Value2, 4);
> -         wsMD5Stream << tmp;
> -         if(unprocessedCharPtr < (mBuffer + mBufferPos + bytesRead))
> -         {
> -            unsigned int dataLen = (mBuffer + mBufferPos + bytesRead)
> - unprocessedCharPtr;
> -            Data content(unprocessedCharPtr, dataLen);
> -            wsMD5Stream << content;
> -         }
> -
> -         if(mMessage->exists(h_Origin))
> -         {
> -            wsResponse += "Sec-WebSocket-Origin: " +
> mMessage->const_header(h_Origin).value() + "\r\n";
> -         }
> -         if(mMessage->exists(h_Host))
> -         {
> -            wsResponse += Data("Sec-WebSocket-Location: ") +
> Data(transport()->transport() == resip::WSS ? "wss://" : "ws://") +
> mMessage->const_header(h_Host).value() + Data("/\r\n");
> -         }
> -         wsResponse += "\r\n" + wsMD5Stream.getBin();
> -         ErrLog(<<"WS client wants to use depracated protocol
> version, unsupported");
> -         delete mMessage;
> -         mMessage = 0;
> -         mBufferPos = 0;
> -         dropConnection = true;
> -         return false;
> +         mOutstandingSends.push_back(new SendData(
> +                  who(),
> +                  wsResponse,
> +                  Data::Empty,
> +                  Data::Empty,
> +                  true));
>        }
> -      else if(mMessage->exists(h_SecWebSocketKey))
> -      {
> -#ifdef USE_SSL
> -         SHA1Stream wsSha1Stream;
> -         wsSha1Stream <<
> (mMessage->const_header(h_SecWebSocketKey).value() +
> Data("258EAFA5-E914-47DA-95CA-C5AB0DC85B11"));
> -         Data wsAcceptKey = wsSha1Stream.getBin(160).base64encode();
> -         wsResponse += "Sec-WebSocket-Accept: "+ wsAcceptKey +"\r\n"
> -               "\r\n";
> -#endif
> -      }
>        else
>        {
> -         ErrLog(<<"No SecWebSocketKey header");
> +         dropConnection = true;
>           delete mMessage;
>           mMessage = 0;
>           mBufferPos = 0;
> -         dropConnection = true;
>           return false;
>        }
> -
> -      mOutstandingSends.push_back(new SendData(
> -            who(),
> -            wsResponse,
> -            Data::Empty,
> -            Data::Empty,
> -            true));
>     }
>     catch(resip::ParseException& e)
>     {
> @@ -661,7 +587,6 @@
>        delete mMessage;
>        mMessage = 0;
>        mBufferPos = 0;
> -      dropConnection = true;
>        return false;
>     }
> 
> @@ -672,7 +597,67 @@
>     return true;
>  }
> 
> +Data
> +ConnectionBase::makeWsHandshakeResponse()
> +{
> +   Data response = "HTTP/1.1 101 WebSocket Protocol Handshake\r\n"
> +      "Upgrade: WebSocket\r\n"
> +      "Connection: Upgrade\r\n"
> +      "Sec-WebSocket-Protocol: sip\r\n";
> +
> +   if(isUsingSecWebSocketKey())
> +   {
> +#ifdef USE_SSL
> +      SHA1Stream wsSha1Stream;
> +      wsSha1Stream <<
> (mMessage->const_header(h_SecWebSocketKey).value() +
> Data("258EAFA5-E914-47DA-95CA-C5AB0DC85B11"));
> +      Data wsAcceptKey = wsSha1Stream.getBin(160).base64encode();
> +      response += "Sec-WebSocket-Accept: " + wsAcceptKey + "\r\n\r\n";
> +#endif
> +      return response;
> +   }
> +   else if(isUsingDeprecatedSecWebSocketKeys())
> +   {
> +      ErrLog(<<"WS client wants to use depracated protocol version,
> unsupported");
> +   }
> +   else
> +   {
> +      ErrLog(<<"No SecWebSocketKey header");
> +   }
> +   return NULL;
> +}
> +
>  bool
> +ConnectionBase::scanMsgHeader(int bytesRead)
> +{
> +   mMsgHeaderScanner.prepareForMessage(mMessage);
> +   char *unprocessedCharPtr;
> +   MsgHeaderScanner::ScanChunkResult scanResult =
> mMsgHeaderScanner.scanChunk(mBuffer, mBufferPos + bytesRead,
> &unprocessedCharPtr);
> +   if (scanResult != MsgHeaderScanner::scrEnd)
> +   {
> +      if(scanResult != MsgHeaderScanner::scrNextChunk)
> +      {
> +         StackLog(<<"Failed to parse message, more bytes needed");
> +         StackLog(<< Data(mBuffer, bytesRead));
> +      }
> +      delete mMessage;
> +      mMessage=0;
> +      mBufferPos += bytesRead;
> +      return false;
> +   }
> +}
> +
> +bool ConnectionBase::isUsingDeprecatedSecWebSocketKeys()
> +{
> +   return mMessage->exists(h_SecWebSocketKey1) &&
> +      mMessage->exists(h_SecWebSocketKey2);
> +}
> +
> +bool ConnectionBase::isUsingSecWebSocketKey()
> +{
> +   return mMessage->exists(h_SecWebSocketKey);
> +}
> +
> +bool
>  ConnectionBase::wsProcessData(int bytesRead)
>  {
>     bool dropConnection = false;
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel@xxxxxxxxxxxxxxx
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel