r137485 - trunk/src/net/socket

[w...@chromium.org]

Author: w...@chromium.org
Date: Wed May 16 12:36:31 2012
New Revision: 137485

Log:
Prevent the infinite loop inside SSLClientSocketNSS::OnSendComplete.

Two fixes are added. 1) We stay in the loop only if we will call
DoPayloadRead or DoPayloadWrite in the next iteration. 2) Don't call
BufferRecv again if BufferRecv has reported EOF before.

Each fix alone prevents the infinite loop. The second fix is less risky.
If necessary, we can go with just the second fix.

R=rsl...@chromium.org
BUG=127822
TEST=SSLServerSocketTest.WriteAfterPeerClose in net_unittests

Review URL: https://chromiumcodereview.appspot.com/10382186

Modified:
trunk/src/net/socket/ssl_client_socket_nss.cc
trunk/src/net/socket/ssl_client_socket_nss.h
trunk/src/net/socket/ssl_server_socket_unittest.cc

Modified: trunk/src/net/socket/ssl_client_socket_nss.cc
==============================================================================
--- trunk/src/net/socket/ssl_client_socket_nss.cc (original)
+++ trunk/src/net/socket/ssl_client_socket_nss.cc Wed May 16 12:36:31 2012
@@ -430,6 +430,7 @@
const SSLClientSocketContext& context)
: transport_send_busy_(false),
transport_recv_busy_(false),
+ transport_recv_eof_(false),
transport_(transport_socket),
host_and_port_(host_and_port),
ssl_config_(ssl_config),
@@ -633,6 +634,7 @@
user_write_callback_.Reset();
transport_send_busy_ = false;
transport_recv_busy_ = false;
+ transport_recv_eof_ = false;
user_read_buf_ = NULL;
user_read_buf_len_ = 0;
user_write_buf_ = NULL;
@@ -1219,6 +1221,7 @@
network_moved = DoTransportIO();
} while (rv_read == ERR_IO_PENDING &&
rv_write == ERR_IO_PENDING &&
+ (user_read_buf_ || user_write_buf_) &&
network_moved);

if (user_read_buf_ && rv_read != ERR_IO_PENDING)
@@ -1922,7 +1925,7 @@
if (rv > 0)
network_moved = true;
} while (rv > 0);
- if (BufferRecv() >= 0)
+ if (!transport_recv_eof_ && BufferRecv() >= 0)
network_moved = true;
}
LeaveFunction(network_moved);
@@ -1932,7 +1935,7 @@
// Return 0 for EOF,
// > 0 for bytes transferred immediately,
// < 0 for error (or the non-error ERR_IO_PENDING).
-int SSLClientSocketNSS::BufferSend(void) {
+int SSLClientSocketNSS::BufferSend() {
if (transport_send_busy_)
return ERR_IO_PENDING;

@@ -1971,7 +1974,7 @@
LeaveFunction("");
}

-int SSLClientSocketNSS::BufferRecv(void) {
+int SSLClientSocketNSS::BufferRecv() {
if (transport_recv_busy_) return ERR_IO_PENDING;

char* buf;
@@ -1990,8 +1993,11 @@
if (rv == ERR_IO_PENDING) {
transport_recv_busy_ = true;
} else {
- if (rv > 0)
+ if (rv > 0) {
memcpy(buf, recv_buffer_->data(), rv);
+ } else if (rv == 0) {
+ transport_recv_eof_ = true;
+ }
memio_PutReadResult(nss_bufs_, MapErrorToNSS(rv));
recv_buffer_ = NULL;
}
@@ -2006,6 +2012,8 @@
char* buf;
memio_GetReadParams(nss_bufs_, &buf);
memcpy(buf, recv_buffer_->data(), result);
+ } else if (result == 0) {
+ transport_recv_eof_ = true;
}
recv_buffer_ = NULL;
memio_PutReadResult(nss_bufs_, MapErrorToNSS(result));

Modified: trunk/src/net/socket/ssl_client_socket_nss.h
==============================================================================
--- trunk/src/net/socket/ssl_client_socket_nss.h (original)
+++ trunk/src/net/socket/ssl_client_socket_nss.h Wed May 16 12:36:31 2012
@@ -149,9 +149,9 @@
void SaveSSLHostInfo();

bool DoTransportIO();
- int BufferSend(void);
+ int BufferSend();
void BufferSendComplete(int result);
- int BufferRecv(void);
+ int BufferRecv();
void BufferRecvComplete(int result);

// Handles an NSS error generated while handshaking or performing IO.
@@ -214,6 +214,7 @@

bool transport_send_busy_;
bool transport_recv_busy_;
+ bool transport_recv_eof_;
scoped_refptr<IOBuffer> recv_buffer_;

scoped_ptr<ClientSocketHandle> transport_;

Modified: trunk/src/net/socket/ssl_server_socket_unittest.cc
==============================================================================
--- trunk/src/net/socket/ssl_server_socket_unittest.cc (original)
+++ trunk/src/net/socket/ssl_server_socket_unittest.cc Wed May 16 12:36:31 2012
@@ -54,10 +54,14 @@
public:
FakeDataChannel()
: read_buf_len_(0),
- ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+ ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
+ closed_(false),
+ write_called_after_close_(false) {
}

int Read(IOBuffer* buf, int buf_len, const CompletionCallback& callback) {
+ if (closed_)
+ return 0;
if (data_.empty()) {
read_callback_ = callback;
read_buf_ = buf;
@@ -68,6 +72,16 @@
}

int Write(IOBuffer* buf, int buf_len, const CompletionCallback& callback) {
+ if (closed_) {
+ if (write_called_after_close_)
+ return net::ERR_CONNECTION_RESET;
+ write_called_after_close_ = true;
+ write_callback_ = callback;
+ MessageLoop::current()->PostTask(
+ FROM_HERE, base::Bind(&FakeDataChannel::DoWriteCallback,
+ weak_factory_.GetWeakPtr()));
+ return net::ERR_IO_PENDING;
+ }
data_.push(new net::DrainableIOBuffer(buf, buf_len));
MessageLoop::current()->PostTask(
FROM_HERE, base::Bind(&FakeDataChannel::DoReadCallback,
@@ -75,6 +89,14 @@
return buf_len;
}

+ // Closes the FakeDataChannel. After Close() is called, Read() returns 0,
+ // indicating EOF, and Write() fails with ERR_CONNECTION_RESET. Note that
+ // after the FakeDataChannel is closed, the first Write() call completes
+ // asynchronously, which is necessary to reproduce bug 127822.
+ void Close() {
+ closed_ = true;
+ }
+
private:
void DoReadCallback() {
if (read_callback_.is_null() || data_.empty())
@@ -88,6 +110,15 @@
callback.Run(copied);
}

+ void DoWriteCallback() {
+ if (write_callback_.is_null())
+ return;
+
+ CompletionCallback callback = write_callback_;
+ write_callback_.Reset();
+ callback.Run(net::ERR_CONNECTION_RESET);
+ }
+
int PropogateData(scoped_refptr<net::IOBuffer> read_buf, int read_buf_len) {
scoped_refptr<net::DrainableIOBuffer> buf = data_.front();
int copied = std::min(buf->BytesRemaining(), read_buf_len);
@@ -103,10 +134,20 @@
scoped_refptr<net::IOBuffer> read_buf_;
int read_buf_len_;

+ CompletionCallback write_callback_;
+
std::queue<scoped_refptr<net::DrainableIOBuffer> > data_;

base::WeakPtrFactory<FakeDataChannel> weak_factory_;

+ // True if Close() has been called.
+ bool closed_;
+
+ // Controls the completion of Write() after the FakeDataChannel is closed.
+ // After the FakeDataChannel is closed, the first Write() call completes
+ // asynchronously.
+ bool write_called_after_close_;
+
DISALLOW_COPY_AND_ASSIGN(FakeDataChannel);
};

@@ -147,7 +188,10 @@
return net::OK;
}

- virtual void Disconnect() OVERRIDE {}
+ virtual void Disconnect() OVERRIDE {
+ incoming_->Close();
+ outgoing_->Close();
+ }

virtual bool IsConnected() const OVERRIDE {
return true;
@@ -429,6 +473,60 @@
EXPECT_EQ(0, memcmp(write_buf->data(), read_buf->data(), write_buf->size()));
}

+// A regression test for bug 127822 (http://crbug.com/127822).
+// If the server closes the connection after the handshake is finished,
+// the client's Write() call should not cause an infinite loop.
+// NOTE: this is a test for SSLClientSocket rather than SSLServerSocket.
+TEST_F(SSLServerSocketTest, ClientWriteAfterServerClose) {
+ Initialize();
+
+ TestCompletionCallback connect_callback;
+ TestCompletionCallback handshake_callback;
+
+ // Establish connection.
+ int client_ret = client_socket_->Connect(connect_callback.callback());
+ ASSERT_TRUE(client_ret == net::OK || client_ret == net::ERR_IO_PENDING);
+
+ int server_ret = server_socket_->Handshake(handshake_callback.callback());
+ ASSERT_TRUE(server_ret == net::OK || server_ret == net::ERR_IO_PENDING);
+
+ client_ret = connect_callback.GetResult(client_ret);
+ ASSERT_EQ(net::OK, client_ret);
+ server_ret = handshake_callback.GetResult(server_ret);
+ ASSERT_EQ(net::OK, server_ret);
+
+ scoped_refptr<net::StringIOBuffer> write_buf =
+ new net::StringIOBuffer("testing123");
+
+ // The server closes the connection. The server needs to write some
+ // data first so that the client's Read() calls from the transport
+ // socket won't return ERR_IO_PENDING. This ensures that the client
+ // will call Read() on the transport socket again.
+ TestCompletionCallback write_callback;
+
+ server_ret = server_socket_->Write(write_buf, write_buf->size(),
+ write_callback.callback());
+ EXPECT_TRUE(server_ret > 0 || server_ret == net::ERR_IO_PENDING);
+
+ server_ret = write_callback.GetResult(server_ret);
+ EXPECT_GT(server_ret, 0);
+
+ server_socket_->Disconnect();
+
+ // The client writes some data. This should not cause an infinite loop.
+ client_ret = client_socket_->Write(write_buf, write_buf->size(),
+ write_callback.callback());
+ EXPECT_TRUE(client_ret > 0 || client_ret == net::ERR_IO_PENDING);
+
+ client_ret = write_callback.GetResult(client_ret);
+ EXPECT_GT(client_ret, 0);
+
+ MessageLoop::current()->PostDelayedTask(
+ FROM_HERE, MessageLoop::QuitClosure(),
+ base::TimeDelta::FromMilliseconds(10));
+ MessageLoop::current()->Run();
+}
+
// This test executes ExportKeyingMaterial() on the client and server sockets,
// after connecting them, and verifies that the results match.
// This test will fail if False Start is enabled (see crbug.com/90208).