diff options
author | toyoshim@chromium.org <toyoshim@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-01-25 07:52:55 +0000 |
---|---|---|
committer | toyoshim@chromium.org <toyoshim@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-01-25 07:52:55 +0000 |
commit | 32cbd36a062f9b40176dcb848537f60fd3fabce5 (patch) | |
tree | 60cdba5bb613877f4c6351f306d2043c8a4c63d2 | |
parent | b03766fafc23b0777826747dabdfd17d41bd36bf (diff) | |
download | chromium_src-32cbd36a062f9b40176dcb848537f60fd3fabce5.zip chromium_src-32cbd36a062f9b40176dcb848537f60fd3fabce5.tar.gz chromium_src-32cbd36a062f9b40176dcb848537f60fd3fabce5.tar.bz2 |
SSLClientSocket::IsConnected should care for internal buffers
SSLClientSocket::IsConnected() and SSLClientSocket::IsConnectedAndIdle()
may return false though it has buffered data. They should care for internally
processing buffer.
There are various implementation, for NSS, OpenSSL, and platform dependent
system libraries. This CL fix the issue on NSS. Fix for others will follow.
BUG=160033
TEST=browser_tests, net_unittests
Review URL: https://codereview.chromium.org/11366155
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@178775 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/net/websocket_browsertest.cc | 94 | ||||
-rw-r--r-- | chrome/chrome_tests.gypi | 1 | ||||
-rw-r--r-- | net/base/nss_memio.c | 8 | ||||
-rw-r--r-- | net/base/nss_memio.h | 5 | ||||
-rw-r--r-- | net/data/websocket/README | 13 | ||||
-rw-r--r-- | net/data/websocket/close-with-split-packet_wsh.py | 30 | ||||
-rw-r--r-- | net/data/websocket/split_packet_check.html | 37 | ||||
-rw-r--r-- | net/socket/ssl_client_socket_nss.cc | 165 |
8 files changed, 324 insertions, 29 deletions
diff --git a/chrome/browser/net/websocket_browsertest.cc b/chrome/browser/net/websocket_browsertest.cc new file mode 100644 index 0000000..4537e4d --- /dev/null +++ b/chrome/browser/net/websocket_browsertest.cc @@ -0,0 +1,94 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/string_util.h" +#include "base/utf_string_conversions.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_tabstrip.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" +#include "content/public/test/browser_test_utils.h" +#include "net/base/test_data_directory.h" +#include "net/test/test_server.h" + +namespace { + +class WebSocketBrowserTest : public InProcessBrowserTest { + public: + WebSocketBrowserTest() + : ws_server_(net::TestServer::TYPE_WS, + net::TestServer::kLocalhost, + net::GetWebSocketTestDataDirectory()), + wss_server_(net::TestServer::TYPE_WSS, + SSLOptions(SSLOptions::CERT_OK), + net::GetWebSocketTestDataDirectory()) { + } + + protected: + net::TestServer ws_server_; + net::TestServer wss_server_; + + private: + typedef net::TestServer::SSLOptions SSLOptions; + + DISALLOW_COPY_AND_ASSIGN(WebSocketBrowserTest); +}; + +// Test that the browser can handle a WebSocket frame split into multiple TCP +// segments. +IN_PROC_BROWSER_TEST_F(WebSocketBrowserTest, WebSocketSplitSegments) { + // Launch a WebSocket server. + ASSERT_TRUE(ws_server_.Start()); + + // Setup page title observer. + content::WebContents* tab = chrome::GetActiveWebContents(browser()); + content::TitleWatcher watcher(tab, ASCIIToUTF16("PASS")); + watcher.AlsoWaitForTitle(ASCIIToUTF16("FAIL")); + + // Visit a HTTP page for testing. + std::string scheme("http"); + GURL::Replacements replacements; + replacements.SetSchemeStr(scheme); + ui_test_utils::NavigateToURL( + browser(), + ws_server_.GetURL( + "split_packet_check.html").ReplaceComponents(replacements)); + + const string16 result = watcher.WaitAndGetTitle(); + EXPECT_TRUE(EqualsASCII(result, "PASS")); +} + +#if defined(USE_OPENSSL) +// TODO(toyoshim): Enable this test for OpenSSL once http://crbug.com/160033 +// is fixed against OpenSSL. +#define MAYBE_SecureWebSocketSplitRecords DISABLED_SecureWebSocketSplitRecords +#else +#define MAYBE_SecureWebSocketSplitRecords SecureWebSocketSplitRecords +#endif +// Test that the browser can handle a WebSocket frame split into multiple SSL +// records. +IN_PROC_BROWSER_TEST_F(WebSocketBrowserTest, + MAYBE_SecureWebSocketSplitRecords) { + // Launch a secure WebSocket server. + ASSERT_TRUE(wss_server_.Start()); + + // Setup page title observer. + content::WebContents* tab = chrome::GetActiveWebContents(browser()); + content::TitleWatcher watcher(tab, ASCIIToUTF16("PASS")); + watcher.AlsoWaitForTitle(ASCIIToUTF16("FAIL")); + + // Visit a HTTPS page for testing. + std::string scheme("https"); + GURL::Replacements replacements; + replacements.SetSchemeStr(scheme); + ui_test_utils::NavigateToURL( + browser(), + wss_server_.GetURL( + "split_packet_check.html").ReplaceComponents(replacements)); + + const string16 result = watcher.WaitAndGetTitle(); + EXPECT_TRUE(EqualsASCII(result, "PASS")); +} + +} // namespace diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index a50f187..69b3876 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1182,6 +1182,7 @@ 'browser/net/load_timing_observer_browsertest.cc', 'browser/net/predictor_browsertest.cc', 'browser/net/proxy_browsertest.cc', + 'browser/net/websocket_browsertest.cc', 'browser/page_cycler/page_cycler_browsertest.cc', 'browser/performance_monitor/performance_monitor_browsertest.cc', 'browser/policy/cloud_policy_browsertest.cc', diff --git a/net/base/nss_memio.c b/net/base/nss_memio.c index 4092c17..bd047106 100644 --- a/net/base/nss_memio.c +++ b/net/base/nss_memio.c @@ -402,6 +402,14 @@ int memio_GetReadParams(memio_Private *secret, char **buf) return memio_buffer_unused_contiguous(mb); } +int memio_GetReadableBufferSize(memio_Private *secret) +{ + struct memio_buffer* mb = &((PRFilePrivate *)secret)->readbuf; + PR_ASSERT(mb->bufsize); + + return memio_buffer_used_contiguous(mb); +} + void memio_PutReadResult(memio_Private *secret, int bytes_read) { struct memio_buffer* mb = &((PRFilePrivate *)secret)->readbuf; diff --git a/net/base/nss_memio.h b/net/base/nss_memio.h index a8bbc2c..8481d15 100644 --- a/net/base/nss_memio.h +++ b/net/base/nss_memio.h @@ -63,6 +63,11 @@ int memio_GetReadRequest(memio_Private *secret); */ int memio_GetReadParams(memio_Private *secret, char **buf); +/* Ask memio how many bytes are contained in the internal buffer. + * Returns bytes available to read, or 0 if none available. + */ +int memio_GetReadableBufferSize(memio_Private *secret); + /* Tell memio how many bytes were read from the network. * If bytes_read is 0, causes EOF to be reported to * NSS after it reads the last byte from the circular buffer. diff --git a/net/data/websocket/README b/net/data/websocket/README index 034a30d..9dab697 100644 --- a/net/data/websocket/README +++ b/net/data/websocket/README @@ -7,6 +7,13 @@ Multiple tests may share one resource, or URI handler. SSLUITest.TestWSSInvalidCertAndGoForward, SSLUITest.TestWSSClientCert, and SSLUITestIgnoreCertErrors.TestWSS. +- split_packet_check.html : A page for testing split packet handling. Here, + packets mean TCP segments for WebSocket, or SSL records for secure + WebSocket. This page changes the title to "PASS" to notify + content::TitleWatcher. + Used by WebSocketBrowserTest.WebSocketSplitSegments and + WebSocketBrowserTest.SecureWebSocketSplitRecords. + - websocket_shared_worker.html : A page provides simple WebSocket test in shared worker. This page changes page title to "PASS" to notify content::TitleWatcher. @@ -31,6 +38,12 @@ Multiple tests may share one resource, or URI handler. connection with arbitrary code and reason. Used by kinds of PPAPI tests for WebSocket. +- close-with-split-packet_wsh.py : A WebSocket URL handler for testing split + packet handling. Here, packets mean TCP segments for WebSocket, or SSL + records for secure WebSocket. + Used by WebSocketBrowserTest.WebSocketSplitSegments and + WebSocketBrowserTest.SecureWebSocketSplitRecords. + - protocol-test_wsh.py : A WebSocket URL handler for testing outgoing opening handshake protocol. Used by kinds of PPAPI tests for WebSocket. diff --git a/net/data/websocket/close-with-split-packet_wsh.py b/net/data/websocket/close-with-split-packet_wsh.py new file mode 100644 index 0000000..d5185c4 --- /dev/null +++ b/net/data/websocket/close-with-split-packet_wsh.py @@ -0,0 +1,30 @@ +# Copyright (c) 2012 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import struct + +from mod_pywebsocket import handshake +from mod_pywebsocket import stream + + +def web_socket_do_extra_handshake(_request): + pass + + +def web_socket_transfer_data(request): + line = request.ws_stream.receive_message() + if line is None: + return + if isinstance(line, unicode): + request.ws_stream.send_message(line, binary=False) + else: + request.ws_stream.send_message(line, binary=True) + + +def web_socket_passive_closing_handshake(request): + code = struct.pack('!H', 1000) + packet = stream.create_close_frame(code + 'split test'.encode('utf-8')) + request.connection.write(packet[:1]) + request.connection.write(packet[1:]) + raise handshake.AbortedByUserException('Abort the connection') diff --git a/net/data/websocket/split_packet_check.html b/net/data/websocket/split_packet_check.html new file mode 100644 index 0000000..8e273ec --- /dev/null +++ b/net/data/websocket/split_packet_check.html @@ -0,0 +1,37 @@ +<!DOCTYPE html> +<html> +<head> +<title>test ws split packet</title> +<script type="text/javascript"> + +var href = window.location.href; +var hostBegin = href.indexOf('/') + 2; +var hostEnd = href.lastIndexOf(':'); +var host = href.slice(hostBegin, hostEnd); +var portBegin = hostEnd + 1; +var portEnd = href.lastIndexOf('/'); +var port = href.slice(portBegin, portEnd); +var scheme = href.indexOf('https') >= 0 ? 'wss' : 'ws'; +var url = scheme + '://' + host + ':' + port + '/close-with-split-packet'; + +// Do connection test. +var ws = new WebSocket(url); + +ws.onopen = function() +{ + // Close WebSocket connection once it is established. + ws.close(); +} + +ws.onclose = function(event) +{ + // Check wasClean, then set proper title. + if (event.wasClean) + document.title = 'PASS'; + else + document.title = 'FAIL'; +} + +</script> +</head> +</html> diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc index 40ed798..1b33032 100644 --- a/net/socket/ssl_client_socket_nss.cc +++ b/net/socket/ssl_client_socket_nss.cc @@ -642,6 +642,11 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> { int Read(IOBuffer* buf, int buf_len, const CompletionCallback& callback); int Write(IOBuffer* buf, int buf_len, const CompletionCallback& callback); + // Called on the network task runner. + bool IsConnected(); + bool HasPendingAsyncOperation(); + bool HasUnhandledReceivedData(); + private: friend class base::RefCountedThreadSafe<Core>; ~Core(); @@ -774,6 +779,9 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> { void OnGetDomainBoundCertComplete(int result); void OnHandshakeStateUpdated(const HandshakeState& state); + void OnNSSBufferUpdated(int amount_in_read_buffer); + void DidNSSRead(int result); + void DidNSSWrite(int result); //////////////////////////////////////////////////////////////////////////// // Methods that are called on both the network task runner and the NSS @@ -817,6 +825,12 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> { ServerBoundCertService* server_bound_cert_service_; ServerBoundCertService::RequestHandle domain_bound_cert_request_handle_; + // The information about NSS task runner. + int unhandled_buffer_size_; + bool nss_waiting_read_; + bool nss_waiting_write_; + bool nss_is_closed_; + //////////////////////////////////////////////////////////////////////////// // Members that are ONLY accessed on the NSS task runner: //////////////////////////////////////////////////////////////////////////// @@ -896,6 +910,10 @@ SSLClientSocketNSS::Core::Core( transport_(transport), weak_net_log_factory_(net_log), server_bound_cert_service_(server_bound_cert_service), + unhandled_buffer_size_(0), + nss_waiting_read_(false), + nss_waiting_write_(false), + nss_is_closed_(false), host_and_port_(host_and_port), ssl_config_(ssl_config), nss_fd_(NULL), @@ -1089,11 +1107,17 @@ int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len, DCHECK(OnNetworkTaskRunner()); DCHECK(!detached_); DCHECK(transport_); + DCHECK(!nss_waiting_read_); + nss_waiting_read_ = true; bool posted = nss_task_runner_->PostTask( FROM_HERE, base::Bind(IgnoreResult(&Core::Read), this, make_scoped_refptr(buf), buf_len, callback)); + if (!posted) { + nss_is_closed_ = true; + nss_waiting_read_ = false; + } return posted ? ERR_IO_PENDING : ERR_ABORTED; } @@ -1110,14 +1134,21 @@ int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len, int rv = DoReadLoop(OK); if (rv == ERR_IO_PENDING) { + if (OnNetworkTaskRunner()) + nss_waiting_read_ = true; user_read_callback_ = callback; } else { user_read_buf_ = NULL; user_read_buf_len_ = 0; if (!OnNetworkTaskRunner()) { + PostOrRunCallback(FROM_HERE, base::Bind(&Core::DidNSSRead, this, rv)); PostOrRunCallback(FROM_HERE, base::Bind(callback, rv)); return ERR_IO_PENDING; + } else { + DCHECK(!nss_waiting_read_); + if (rv <= 0) + nss_is_closed_ = true; } } @@ -1130,13 +1161,18 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len, DCHECK(OnNetworkTaskRunner()); DCHECK(!detached_); DCHECK(transport_); + DCHECK(!nss_waiting_write_); + nss_waiting_write_ = true; bool posted = nss_task_runner_->PostTask( FROM_HERE, base::Bind(IgnoreResult(&Core::Write), this, make_scoped_refptr(buf), buf_len, callback)); - int rv = posted ? ERR_IO_PENDING : ERR_ABORTED; - return rv; + if (!posted) { + nss_is_closed_ = true; + nss_waiting_write_ = false; + } + return posted ? ERR_IO_PENDING : ERR_ABORTED; } DCHECK(OnNSSTaskRunner()); @@ -1152,20 +1188,42 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len, int rv = DoWriteLoop(OK); if (rv == ERR_IO_PENDING) { + if (OnNetworkTaskRunner()) + nss_waiting_write_ = true; user_write_callback_ = callback; } else { user_write_buf_ = NULL; user_write_buf_len_ = 0; if (!OnNetworkTaskRunner()) { + PostOrRunCallback(FROM_HERE, base::Bind(&Core::DidNSSWrite, this, rv)); PostOrRunCallback(FROM_HERE, base::Bind(callback, rv)); return ERR_IO_PENDING; + } else { + DCHECK(!nss_waiting_write_); + if (rv < 0) + nss_is_closed_ = true; } } return rv; } +bool SSLClientSocketNSS::Core::IsConnected() { + DCHECK(OnNetworkTaskRunner()); + return !nss_is_closed_; +} + +bool SSLClientSocketNSS::Core::HasPendingAsyncOperation() { + DCHECK(OnNetworkTaskRunner()); + return nss_waiting_read_ || nss_waiting_write_; +} + +bool SSLClientSocketNSS::Core::HasUnhandledReceivedData() { + DCHECK(OnNetworkTaskRunner()); + return unhandled_buffer_size_ != 0; +} + bool SSLClientSocketNSS::Core::OnNSSTaskRunner() const { return nss_task_runner_->RunsTasksOnCurrentThread(); } @@ -2044,6 +2102,13 @@ int SSLClientSocketNSS::Core::DoPayloadRead() { DCHECK_GT(user_read_buf_len_, 0); int rv = PR_Read(nss_fd_, user_read_buf_->data(), user_read_buf_len_); + // Update NSSTaskRunner status because PR_Read may consume |nss_bufs_|, then + // following |amount_in_read_buffer| may be changed here. + int amount_in_read_buffer = memio_GetReadableBufferSize(nss_bufs_); + PostOrRunCallback( + FROM_HERE, + base::Bind(&Core::OnNSSBufferUpdated, this, amount_in_read_buffer)); + if (client_auth_cert_needed_) { // We don't need to invalidate the non-client-authenticated SSL session // because the server will renegotiate anyway. @@ -2081,7 +2146,17 @@ int SSLClientSocketNSS::Core::DoPayloadWrite() { DCHECK(user_write_buf_); + int old_amount_in_read_buffer = memio_GetReadableBufferSize(nss_bufs_); int rv = PR_Write(nss_fd_, user_write_buf_->data(), user_write_buf_len_); + int new_amount_in_read_buffer = memio_GetReadableBufferSize(nss_bufs_); + // PR_Write could potentially consume the unhandled data in the memio read + // buffer if a renegotiation is in progress. If the buffer is consumed, + // notify the latest buffer size to NetworkRunner. + if (old_amount_in_read_buffer != new_amount_in_read_buffer) { + PostOrRunCallback( + FROM_HERE, + base::Bind(&Core::OnNSSBufferUpdated, this, new_amount_in_read_buffer)); + } if (rv >= 0) { PostOrRunCallback( FROM_HERE, @@ -2287,10 +2362,18 @@ void SSLClientSocketNSS::Core::DoReadCallback(int rv) { user_read_buf_ = NULL; user_read_buf_len_ = 0; - base::Closure c = base::Bind( - base::ResetAndReturn(&user_read_callback_), - rv); - PostOrRunCallback(FROM_HERE, c); + int amount_in_read_buffer = memio_GetReadableBufferSize(nss_bufs_); + // This is used to curry the |amount_int_read_buffer| and |user_cb| back to + // the network task runner. + PostOrRunCallback( + FROM_HERE, + base::Bind(&Core::OnNSSBufferUpdated, this, amount_in_read_buffer)); + PostOrRunCallback( + FROM_HERE, + base::Bind(&Core::DidNSSRead, this, rv)); + PostOrRunCallback( + FROM_HERE, + base::Bind(base::ResetAndReturn(&user_read_callback_), rv)); } void SSLClientSocketNSS::Core::DoWriteCallback(int rv) { @@ -2302,10 +2385,20 @@ void SSLClientSocketNSS::Core::DoWriteCallback(int rv) { // up front. user_write_buf_ = NULL; user_write_buf_len_ = 0; - base::Closure c = base::Bind( - base::ResetAndReturn(&user_write_callback_), - rv); - PostOrRunCallback(FROM_HERE, c); + // Update buffer status because DoWriteLoop called DoTransportIO which may + // perform read operations. + int amount_in_read_buffer = memio_GetReadableBufferSize(nss_bufs_); + // This is used to curry the |amount_int_read_buffer| and |user_cb| back to + // the network task runner. + PostOrRunCallback( + FROM_HERE, + base::Bind(&Core::OnNSSBufferUpdated, this, amount_in_read_buffer)); + PostOrRunCallback( + FROM_HERE, + base::Bind(&Core::DidNSSWrite, this, rv)); + PostOrRunCallback( + FROM_HERE, + base::Bind(base::ResetAndReturn(&user_write_callback_), rv)); } SECStatus SSLClientSocketNSS::Core::ClientChannelIDHandler( @@ -2565,9 +2658,9 @@ int SSLClientSocketNSS::Core::DoBufferSend(IOBuffer* send_buffer, int len) { return ERR_ABORTED; int rv = transport_->socket()->Write( - send_buffer, len, - base::Bind(&Core::BufferSendComplete, - base::Unretained(this))); + send_buffer, len, + base::Bind(&Core::BufferSendComplete, + base::Unretained(this))); if (!OnNSSTaskRunner() && rv != ERR_IO_PENDING) { nss_task_runner_->PostTask( @@ -2610,9 +2703,31 @@ int SSLClientSocketNSS::Core::DoGetDomainBoundCert( void SSLClientSocketNSS::Core::OnHandshakeStateUpdated( const HandshakeState& state) { + DCHECK(OnNetworkTaskRunner()); network_handshake_state_ = state; } +void SSLClientSocketNSS::Core::OnNSSBufferUpdated(int amount_in_read_buffer) { + DCHECK(OnNetworkTaskRunner()); + unhandled_buffer_size_ = amount_in_read_buffer; +} + +void SSLClientSocketNSS::Core::DidNSSRead(int result) { + DCHECK(OnNetworkTaskRunner()); + DCHECK(nss_waiting_read_); + nss_waiting_read_ = false; + if (result <= 0) + nss_is_closed_ = true; +} + +void SSLClientSocketNSS::Core::DidNSSWrite(int result) { + DCHECK(OnNetworkTaskRunner()); + DCHECK(nss_waiting_write_); + nss_waiting_write_ = false; + if (result < 0) + nss_is_closed_ = true; +} + void SSLClientSocketNSS::Core::BufferSendComplete(int result) { if (!OnNSSTaskRunner()) { if (detached_) @@ -2922,29 +3037,21 @@ void SSLClientSocketNSS::Disconnect() { } bool SSLClientSocketNSS::IsConnected() const { - // Ideally, we should also check if we have received the close_notify alert - // message from the server, and return false in that case. We're not doing - // that, so this function may return a false positive. Since the upper - // layer (HttpNetworkTransaction) needs to handle a persistent connection - // closed by the server when we send a request anyway, a false positive in - // exchange for simpler code is a good trade-off. EnterFunction(""); - bool ret = completed_handshake_ && transport_->socket()->IsConnected(); + bool ret = completed_handshake_ && + (core_->HasPendingAsyncOperation() || + (core_->IsConnected() && core_->HasUnhandledReceivedData()) || + transport_->socket()->IsConnected()); LeaveFunction(""); return ret; } bool SSLClientSocketNSS::IsConnectedAndIdle() const { - // Unlike IsConnected, this method doesn't return a false positive. - // - // Strictly speaking, we should check if we have received the close_notify - // alert message from the server, and return false in that case. Although - // the close_notify alert message means EOF in the SSL layer, it is just - // bytes to the transport layer below, so - // transport_->socket()->IsConnectedAndIdle() returns the desired false - // when we receive close_notify. EnterFunction(""); - bool ret = completed_handshake_ && transport_->socket()->IsConnectedAndIdle(); + bool ret = completed_handshake_ && + !core_->HasPendingAsyncOperation() && + !(core_->IsConnected() && core_->HasUnhandledReceivedData()) && + transport_->socket()->IsConnectedAndIdle(); LeaveFunction(""); return ret; } |