Skip to content

Commit

Permalink
Close SSLClientSocketOpenSSL cleanly if the transport was closed.
Browse files Browse the repository at this point in the history
Servers do not reliably send close_notify. This regressed in
https://codereview.chromium.org/367963007 which had a side effect of making us
sensitive to this.

BUG=422246

Review URL: https://codereview.chromium.org/655813003

Cr-Commit-Position: refs/heads/master@{#300308}
  • Loading branch information
davidben authored and Commit bot committed Oct 20, 2014
1 parent ed46091 commit be6ce7e
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 2 deletions.
9 changes: 9 additions & 0 deletions net/socket/ssl_client_socket_openssl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1363,6 +1363,15 @@ int SSLClientSocketOpenSSL::DoPayloadRead() {
} else if (*next_result < 0) {
int err = SSL_get_error(ssl_, *next_result);
*next_result = MapOpenSSLError(err, err_tracer);

// Many servers do not reliably send a close_notify alert when shutting
// down a connection, and instead terminate the TCP connection. This is
// reported as ERR_CONNECTION_CLOSED. Because of this, map the unclean
// shutdown to a graceful EOF, instead of treating it as an error as it
// should be.
if (*next_result == ERR_CONNECTION_CLOSED)
*next_result = 0;

if (rv > 0 && *next_result == ERR_IO_PENDING) {
// If at least some data was read from SSL_read(), do not treat
// insufficient data as an error to return in the next call to
Expand Down
76 changes: 74 additions & 2 deletions net/socket/ssl_client_socket_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ class SynchronousErrorStreamSocket : public WrappedStreamSocket {
// If there is already a pending asynchronous read, the configured error
// will not be returned until that asynchronous read has completed and Read()
// is called again.
void SetNextReadError(Error error) {
void SetNextReadError(int error) {
DCHECK_GE(0, error);
have_read_error_ = true;
pending_read_error_ = error;
Expand All @@ -286,7 +286,7 @@ class SynchronousErrorStreamSocket : public WrappedStreamSocket {
// If there is already a pending asynchronous write, the configured error
// will not be returned until that asynchronous write has completed and
// Write() is called again.
void SetNextWriteError(Error error) {
void SetNextWriteError(int error) {
DCHECK_GE(0, error);
have_write_error_ = true;
pending_write_error_ = error;
Expand Down Expand Up @@ -1813,6 +1813,78 @@ TEST_F(SSLClientSocketTest, Read_WithWriteError) {
#endif
}

// Tests that SSLClientSocket fails the handshake if the underlying
// transport is cleanly closed.
TEST_F(SSLClientSocketTest, Connect_WithZeroReturn) {
SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
SpawnedTestServer::kLocalhost,
base::FilePath());
ASSERT_TRUE(test_server.Start());

AddressList addr;
ASSERT_TRUE(test_server.GetAddressList(&addr));

TestCompletionCallback callback;
scoped_ptr<StreamSocket> real_transport(
new TCPClientSocket(addr, NULL, NetLog::Source()));
scoped_ptr<SynchronousErrorStreamSocket> transport(
new SynchronousErrorStreamSocket(real_transport.Pass()));
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_EQ(OK, rv);

SynchronousErrorStreamSocket* raw_transport = transport.get();
scoped_ptr<SSLClientSocket> sock(
CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
test_server.host_port_pair(),
kDefaultSSLConfig));

raw_transport->SetNextReadError(0);

rv = callback.GetResult(sock->Connect(callback.callback()));
EXPECT_EQ(ERR_CONNECTION_CLOSED, rv);
EXPECT_FALSE(sock->IsConnected());
}

// Tests that SSLClientSocket cleanly returns a Read of size 0 if the
// underlying socket is cleanly closed.
// This is a regression test for https://crbug.com/422246
TEST_F(SSLClientSocketTest, Read_WithZeroReturn) {
SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
SpawnedTestServer::kLocalhost,
base::FilePath());
ASSERT_TRUE(test_server.Start());

AddressList addr;
ASSERT_TRUE(test_server.GetAddressList(&addr));

TestCompletionCallback callback;
scoped_ptr<StreamSocket> real_transport(
new TCPClientSocket(addr, NULL, NetLog::Source()));
scoped_ptr<SynchronousErrorStreamSocket> transport(
new SynchronousErrorStreamSocket(real_transport.Pass()));
int rv = callback.GetResult(transport->Connect(callback.callback()));
EXPECT_EQ(OK, rv);

// Disable TLS False Start to ensure the handshake has completed.
SSLConfig ssl_config;
ssl_config.false_start_enabled = false;

SynchronousErrorStreamSocket* raw_transport = transport.get();
scoped_ptr<SSLClientSocket> sock(
CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
test_server.host_port_pair(),
ssl_config));

rv = callback.GetResult(sock->Connect(callback.callback()));
EXPECT_EQ(OK, rv);
EXPECT_TRUE(sock->IsConnected());

raw_transport->SetNextReadError(0);
scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
EXPECT_EQ(0, rv);
}

TEST_F(SSLClientSocketTest, Read_SmallChunks) {
SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
SpawnedTestServer::kLocalhost,
Expand Down

0 comments on commit be6ce7e

Please sign in to comment.