[webkit-reviews] review granted: [Bug 51253] WebSockets: unbounded buffer growth when server sends bad data : [Attachment 78377] fix chromium build failure

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 10 01:20:29 PST 2011


Alexey Proskuryakov <ap at webkit.org> has granted Joe Mason <jmason at rim.com>'s
request for review:
Bug 51253: WebSockets: unbounded buffer growth when server sends bad data
https://bugs.webkit.org/show_bug.cgi?id=51253

Attachment 78377: fix chromium build failure
https://bugs.webkit.org/attachment.cgi?id=78377&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78377&action=review

I'm going to say r+, but please address the comments before landing.

If you'd like to ask for another quick review pass after addressing the
comments, please feel free to do so.

> Source/WebCore/websockets/WebSocketHandshake.cpp:3
> + * Copyright (C) Research In Motion Limited 2011. All rights reserved.

Should it be "Copyright (C) 2011"?

> Source/WebCore/websockets/WebSocketHandshake.cpp:433
> +    static const int maxLength = 1024;

We don't abbreviate when we can avoid it. I suggest something like
"maximumStatusLineLength".

> Source/WebCore/websockets/WebSocketHandshake.cpp:454
> +	       // The caller will search for the ending newline with strnstr,
> +	       // which does not scan past nulls.  So a null in the stream will

> +	       // cause it to loop forever, reading more data and then
repeating
> +	       // the search only on the data before the null, unless we abort
> +	       // immediately.

This explains why the caller will do a wrong thing, but doesn't explain why we
shouldn't just fix the caller. Do we really want to detect an error here?

Assuming that I understand the situation correctly, this may be more clear:
"The caller isn't prepared to deal with null bytes in status line. WebSockets
specification doesn't prohibit this, but HTTP does, so we'll just treat this as
an error".

> Source/WebCore/websockets/WebSocketHandshake.cpp:456
> +	       return p + 1 - header;

Shouldn't we set m_mode = Failed?

> Source/WebCore/websockets/WebSocketHandshake.cpp:466
> -	   return INT_MAX;
> +	   return maxLength;

Ditto, shouldn't we set m_mode = Failed?

>
LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength-expected.txt
:1
> +CONSOLE MESSAGE: line 0: Status line is too long:
...............................................................................
.................................................…

Hmm, but this isn't 1024 bytes? Why?

Please consider not dumping the bad string - if it's over 1024 bytes, it's
likely garbage, and can obstruct reading other text in console. The developer
can using tcpdump or Wireshark to debug the problem.

> LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength.html:13
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<html>
> +<head>
> +<link rel="stylesheet" href="../../../js-test-resources/js-test-style.css">
> +<script src="../../../js-test-resources/js-test-pre.js"></script>
> +<script src="../../../js-test-resources/js-test-post-function.js"></script>
> +</head>
> +<body>
> +<div id="description"></div>
> +<div id="console"></div>
> +<script src="script-tests/handshake-fail-by-maxlength.js"></script>
> +</body>
> +</html>

Please don't split tests into .html and .js parts. This is a anti-pattern that
can be seen in many existing tests, but it should be avoided for new tests.

> LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength_wsh.py:1
> +# Copyright (C) Research In Motion Limited 2011. All rights reserved.

Copyright (C) 2011?

> LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength_wsh.py:15
> +#	 * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.

<http://webkit.org/coding/bsd-license.html> is two clause. Or is this copied
from a Google originated file, so you decided to keep their license?

I don't really understand why this needs a license block, and other new files
don't.


More information about the webkit-reviews mailing list