Bug 37682 – WebSocket crash when it receives bad header.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

If server sends back the following as websocket handshake, it crashes.

HTTP/1.1 101 Web Socket Protocol Handshake\r\n
Upgrade: WebSocket\r\n
Connection: Upgrade\r\n
¥:\r\n
\r\n

See http://code.google.com/p/chromium/issues/detail?id=41147 more detail.

[Fumitoshi Ukai]

Created attachment 53487 [details]
Patch

[Alexey Proskuryakov]

+static String encodeHeaderField(const char* data, size_t len)

This function needs a name that makes intended usage clear, something like encodeHeaderFieldForDisplay. Otherwise, one can't help but wonder why it does what it does.

But I'm also not sure how much it helps to display garbage headers encoded as JavaScript literal strings. For one thing, this could be natural for a JavaScript function, but HTTP is a distinct field. It's better to tell what is wrong about the header field name or value than dump a string a hex codes. An finally, this may not be an important enough case to to go into all this trouble.

+            return p;

Should this be "return 0" to indicate error?

[Fumitoshi Ukai]

(In reply to comment #2)
> +static String encodeHeaderField(const char* data, size_t len)
> 
> This function needs a name that makes intended usage clear, something like
> encodeHeaderFieldForDisplay. Otherwise, one can't help but wonder why it does
> what it does.

Ok. I'll rename it to encodeHeaderFieldForDisplay().

> 
> But I'm also not sure how much it helps to display garbage headers encoded as
> JavaScript literal strings. For one thing, this could be natural for a
> JavaScript function, but HTTP is a distinct field. It's better to tell what is
> wrong about the header field name or value than dump a string a hex codes. An
> finally, this may not be an important enough case to to go into all this
> trouble.

I see. Make it to dump a string a hex codes.
 
> +            return p;
> 
> Should this be "return 0" to indicate error?

Oops. I'll fix.

[Alexey Proskuryakov]

Comment on attachment 53487 [details]
Patch

> I see. Make it to dump a string a hex codes.

To re-iterate, I think that it's not great. Ideally, the text should explain what exactly is wrong with the header, and not just dump it in the face of the developer. But it's not a big deal.

Marking r- for the "return p" issue.

[Fumitoshi Ukai]

Created attachment 53494 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 53494 [details]
Patch

> CONSOLE MESSAGE: line 0: invalid header name: a5 

This looks even more confusing than before. What is invalid about the name "a5"?

[Fumitoshi Ukai]

(In reply to comment #6)
> (From update of attachment 53494 [details])
> > CONSOLE MESSAGE: line 0: invalid header name: a5 
> 
> This looks even more confusing than before. What is invalid about the name
> "a5"?

"a5" only is invalid UTF-8 sequence.

[Alexey Proskuryakov]

Yes, I understand this. But to a Web developer, this would look like an ASCII string "a5" - there is nothing to say or imply that this is hexadecimal. And most Web developers probably don't know how utf-8 works anyway.

[Fumitoshi Ukai]

(In reply to comment #8)
> Yes, I understand this. But to a Web developer, this would look like an ASCII
> string "a5" - there is nothing to say or imply that this is hexadecimal. And
> most Web developers probably don't know how utf-8 works anyway.

Ok, so changing console log to " invalid UTF-8 sequence in header name: 0xa5 (in hexadecimal)" is fine?

[Alexey Proskuryakov]

My preferred message would be just "invalid UTF-8 sequence in header name".

[Fumitoshi Ukai]

(In reply to comment #10)
> My preferred message would be just "invalid UTF-8 sequence in header name".

Does it mean we don't need to dump byte sequence of header name in hexadecimal?
IIRC, raw byte sequence might not be outputted correctly in some DRT, so we might need to rebaseline expectations.

[Alexey Proskuryakov]

I think that there is no need to dump the headers, yes.

[Fumitoshi Ukai]

(In reply to comment #12)
> I think that there is no need to dump the headers, yes.

Ok, so I'll change log message and remove dumping byte sequence.

[Fumitoshi Ukai]

Created attachment 53566 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 53566 [details]
Patch

r=me

[Fumitoshi Ukai]

Committed r57760: <http://trac.webkit.org/changeset/57760>