Bug 35572 – WebSocket handshake incompatible change in draft-hixie-thewebsocketprotocol-76

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

WebSocket protocol will introduce incompatible change in its handshake, 
http://lists.whatwg.org/pipermail/commit-watchers-whatwg.org/2010/003973.html

major changes seems the following:

new header in request:
 - Sec-WebSocket-*

change in response status line
  - HTTP/1.1 101 WebSocket Protocol Handshake

[Fumitoshi Ukai]

Announced on the hybi
http://www.ietf.org/mail-archive/web/hybi/current/msg01526.html

(In reply to comment #0)
> WebSocket protocol will introduce incompatible change in its handshake, 
> http://lists.whatwg.org/pipermail/commit-watchers-whatwg.org/2010/003973.html
> 
> major changes seems the following:
> 
> new header in request:
>  - Sec-WebSocket-*
> 
> change in response status line
>   - HTTP/1.1 101 WebSocket Protocol Handshake

[Alexey Proskuryakov]

<rdar://problem/7900015>

[Fumitoshi Ukai]

Created attachment 54262 [details]
Patch

[Early Warning System Bot]

Attachment 54262 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1815131

[WebKit Review Bot]

Attachment 54262 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1840104

[Fumitoshi Ukai]

Created attachment 54264 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 54264 [details]
Patch

+
+#include <algorithm>
+

No need for the empty lines. But you probably don't need this header file, see below.

+    for (p = header, lineLength = 0; p - header < len; p++, lineLength++) {

I think that it would be slightly better to zero out lineLength before the loop. Now it misleadingly looks like a loop variable.

+static String hostName(const KURL& url, bool secure)

Since this is a separate function now, perhaps we should ASSERT that secure flag matches url scheme here.

+    uint32_t space = static_cast<uint32_t>(WTF::randomNumber() * 12) + 1;

RandomNumber.h needs using directives - we do that for all WTF symbols that are exposed for use outside of WTF. This can be done in a later patch.

I didn't study the key generation algorithms in detail. How did you test them? Do examples from the spec pass?

+    String randomChars(randomCharacterInSecWebSocketKey);

It seems wasteful to construct this string from a buffer each time.

+    Vector<uint8_t, 16> digest = md5.checksum();

I don't understand why checksum() returns the result by value. It should probably take a reference to output (not something to do in this patch).

+    std::random_shuffle(fields.begin(), fields.end());

We prefer to have "using namespace std" at the top of the .cpp file. But also, I don't think that we should be randomizing the order here. This makes no sense to me, and the spec doesn't seem to require that (it says "Fields in the handshake are sent by the client in a random order; the order is not meaningful." - which I interpret as allowing arbitrary order).

+    // TODO(ukai): key1,2,3

We use FIXME, not TODO. I don't understand this comment, did you mean to address it before submitting the patch?

+        unsigned char m_challengeResponseExpected[16];

Should it be "m_expectedChallengeResponse" for better grammar?

+    for (size_t i = 0; i < fields.size(); i++)
+        builder.append(fields[i] + "\r\n");

Seems better to append() these separately - that's what StringBuilder is for.

+bool WebSocketHandshake::checkResponseHeaders()
 {
     ASSERT(m_mode == Normal);
     m_mode = Failed;
...
-    m_mode = Connected;
-    return;
+    return true;
 }

I don't understand how this function works - it now always sets m_mode to Failed, but it also returns a boolean. What is the relationship between these?

[Fumitoshi Ukai]

Created attachment 55209 [details]
Patch

[Fumitoshi Ukai]

Thanks for review.

(In reply to comment #7)
> (From update of attachment 54264 [details])
> +
> +#include <algorithm>
> +
> 
> No need for the empty lines. But you probably don't need this header file, see
> below.

Hmm, I see. we don't need it. Removed.


> +    uint32_t space = static_cast<uint32_t>(WTF::randomNumber() * 12) + 1;
> 
> RandomNumber.h needs using directives - we do that for all WTF symbols that are
> exposed for use outside of WTF. This can be done in a later patch.

Ok, I'll address this in another patch. just add FIXME now.
 
> I didn't study the key generation algorithms in detail. How did you test them?

This is straightforward implementation as spec describes.
I captured request/response and looked random string in request and correctly match response data with expected data.

> Do examples from the spec pass?

I think so.
> 
> +    String randomChars(randomCharacterInSecWebSocketKey);
> 
> It seems wasteful to construct this string from a buffer each time.

Make it DEFINE_STATIC_LOCAL.

 
> +    Vector<uint8_t, 16> digest = md5.checksum();
> 
> I don't understand why checksum() returns the result by value. It should
> probably take a reference to output (not something to do in this patch).

The interface was suggested by Maciej.
Just added FIXME to fix the interface to take a reference to output.

> 
> +    std::random_shuffle(fields.begin(), fields.end());
> 
> We prefer to have "using namespace std" at the top of the .cpp file. But also,
> I don't think that we should be randomizing the order here. This makes no sense
> to me, and the spec doesn't seem to require that (it says "Fields in the
> handshake are sent by the client in a random order; the order is not
> meaningful." - which I interpret as allowing arbitrary order).

I see.  Removed random_shuffle and add comment we don't need to shuffle here.

> 
> +    // TODO(ukai): key1,2,3
> 
> We use FIXME, not TODO. I don't understand this comment, did you mean to
> address it before submitting the patch?

Ah, I just wondered we need to pass m_secWebSocketKey{1,2}, m_key3 to WebSocketHandshakeReqeust.

 
> 
> +bool WebSocketHandshake::checkResponseHeaders()
>  {
>      ASSERT(m_mode == Normal);
>      m_mode = Failed;
> ...
> -    m_mode = Connected;
> -    return;
> +    return true;
>  }
> 
> I don't understand how this function works - it now always sets m_mode to
> Failed, but it also returns a boolean. What is the relationship between these?

I tried to change m_mode transition in readServerHandshake() and make checkResponseHeaders() only checks whether headers are valid or not and returns the result as boolean.

[Alexey Proskuryakov]

Comment on attachment 55209 [details]
Patch

+    // FIXME: using WTF in RandomNumber.h

This comment will be hard to understand for someone who didn't follow our discussion here. If you are going to address this soon, it would be best to just omit it.

It's not important enough to disrupt someone reading this code in the way a FIXME does.

+    // FIXME: md5.checksum() should take a reference to output instead of returning the result by value.

Also, this is probably not a good place to complain about MD5 methods - MD5.h would be such a place if we wanted to keep this unfixed for a long time.

+    DEFINE_STATIC_LOCAL(String, spaceChar, (" "));

There is no need to make a string for this - there is a version of insert() that takes a UChar.

+static void generateChallengeResponseExpected(uint32_t number1, uint32_t number2, unsigned char key3[8], unsigned char challengeResponseExpected[16])
+{
+    unsigned char challenge[16];
+    setChallengeNumber(&challenge[0], number1);
+    setChallengeNumber(&challenge[4], number2);
+    memcpy(&challenge[8], key3, 8);
+    MD5 md5;
+    md5.addBytes(challenge, sizeof(challenge));
+    // FIXME: md5.checksum() should take a reference to output instead of returning the result by value.
+    Vector<uint8_t, 16> digest = md5.checksum();
+    memcpy(challengeResponseExpected, digest.data(), 16);
+}

This also needs a grammar fix (responseExpected->expectedResponse).

r=me

[Fumitoshi Ukai]

Committed r59903: <http://trac.webkit.org/changeset/59903>