[Webkit-unassigned] [Bug 51253] WebSockets: unbounded buffer growth when server sends bad data

Mon Jan 10 01:20:30 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=51253

Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78377|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |

--- Comment #13 from Alexey Proskuryakov <ap at webkit.org>  2011-01-10 01:20:29 PST ---
(From update of attachment 78377)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.