Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(771)

Unified Diff: net/socket/ssl_client_socket_nss.cc

Issue 11366155: SSLClientSocket::IsConnected should care for internal buffers (Closed) Base URL: http://git.chromium.org/chromium/src.git@master
Patch Set: review #30 Created 7 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « net/socket/ssl_client_socket_nss.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: net/socket/ssl_client_socket_nss.cc
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index 809dbc5beb28ce36b36e1e76c8ef4141a24eb715..b4fe95add31a21b75cc6741212a33542c6ef60f6 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 IsClosed();
+ bool HasPendingAsyncOperation();
+ bool HasUnhandledReceivedData();
+
private:
friend class base::RefCountedThreadSafe<Core>;
~Core();
@@ -652,6 +657,12 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> {
STATE_GET_DOMAIN_BOUND_CERT_COMPLETE,
};
+ enum Operation {
+ OPERATION_READ,
+ OPERATION_WRITE,
+ OPERATION_UPDATE,
+ };
+
bool OnNSSTaskRunner() const;
bool OnNetworkTaskRunner() const;
@@ -740,6 +751,10 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> {
void DoConnectCallback(int result);
void DoReadCallback(int result);
void DoWriteCallback(int result);
+ void UpdateNSSTaskRunnerStatus(int amount_in_read_buffer,
+ bool closed,
+ Operation operation,
+ const base::Closure& callback);
// Client channel ID handler.
static SECStatus ClientChannelIDHandler(
@@ -817,6 +832,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 +917,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),
@@ -1104,6 +1129,7 @@ int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len,
DCHECK(user_connect_callback_.is_null());
DCHECK(!user_read_buf_);
DCHECK(nss_bufs_);
+ DCHECK(!nss_waiting_read_);
user_read_buf_ = buf;
user_read_buf_len_ = buf_len;
@@ -1111,6 +1137,7 @@ int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len,
int rv = DoReadLoop(OK);
if (rv == ERR_IO_PENDING) {
user_read_callback_ = callback;
+ nss_waiting_read_ = true;
} else {
user_read_buf_ = NULL;
user_read_buf_len_ = 0;
@@ -1146,6 +1173,7 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
DCHECK(user_connect_callback_.is_null());
DCHECK(!user_write_buf_);
DCHECK(nss_bufs_);
+ DCHECK(!nss_waiting_write_);
user_write_buf_ = buf;
user_write_buf_len_ = buf_len;
@@ -1153,6 +1181,7 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
int rv = DoWriteLoop(OK);
if (rv == ERR_IO_PENDING) {
user_write_callback_ = callback;
+ nss_waiting_write_ = true;
} else {
user_write_buf_ = NULL;
user_write_buf_len_ = 0;
@@ -1166,6 +1195,20 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
return rv;
}
+bool SSLClientSocketNSS::Core::IsClosed() {
+ 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 +2087,17 @@ 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_);
+ base::Closure task = base::Bind(
+ &Core::UpdateNSSTaskRunnerStatus,
+ this,
+ amount_in_read_buffer,
+ rv <= 0 && rv != ERR_IO_PENDING,
+ OPERATION_UPDATE,
+ base::Closure());
+
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 +2135,21 @@ 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) {
+ base::Closure task = base::Bind(
+ &Core::UpdateNSSTaskRunnerStatus,
+ this,
+ new_amount_in_read_buffer,
+ false,
+ OPERATION_UPDATE,
+ base::Closure());
+ }
if (rv >= 0) {
PostOrRunCallback(
FROM_HERE,
@@ -2287,10 +2355,20 @@ 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 will invoke the user callback with |rv| as result.
+ base::Closure user_cb = base::Bind(
+ base::ResetAndReturn(&user_read_callback_), rv);
+ // This is used to curry the |amount_int_read_buffer| and |user_cb| back to
+ // the network task runner.
+ base::Closure task = base::Bind(
+ &Core::UpdateNSSTaskRunnerStatus,
+ this,
+ amount_in_read_buffer,
+ rv <= 0, // EOF or errors.
+ OPERATION_READ,
+ user_cb);
+ PostOrRunCallback(FROM_HERE, task);
}
void SSLClientSocketNSS::Core::DoWriteCallback(int rv) {
@@ -2302,10 +2380,45 @@ 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 will invoke the user callback with |rv| as result.
+ base::Closure user_cb = base::Bind(
+ base::ResetAndReturn(&user_write_callback_), rv);
+ // This is used to curry the |amount_int_read_buffer| and |user_cb| back to
+ // the network task runner.
+ base::Closure task = base::Bind(
+ &Core::UpdateNSSTaskRunnerStatus,
+ this,
+ amount_in_read_buffer,
+ rv < 0, // Errors.
+ OPERATION_WRITE,
+ user_cb);
+ PostOrRunCallback(FROM_HERE, task);
+}
+
+void SSLClientSocketNSS::Core::UpdateNSSTaskRunnerStatus(
+ int amount_in_read_buffer,
+ bool closed,
+ Operation operation,
+ const base::Closure& callback) {
+ DCHECK(OnNetworkTaskRunner());
+ DCHECK(!callback.is_null());
Ryan Sleevi 2013/01/18 00:03:24 Have you actually run this on a debug build on Lin
Takashi Toyoshima 2013/01/21 14:51:17 Thanks. I didn't check it in debug build this time
+ DCHECK((operation == OPERATION_READ && nss_waiting_read_) ||
+ (operation == OPERATION_WRITE && nss_waiting_write_) ||
+ operation == OPERATION_UPDATE);
+
+ unhandled_buffer_size_ = amount_in_read_buffer;
+ if (closed)
+ nss_is_closed_ = true;
+ if (operation == OPERATION_READ)
+ nss_waiting_read_ = false;
+ else if (operation == OPERATION_WRITE)
+ nss_waiting_write_ = false;
+
+ if (operation != OPERATION_UPDATE)
+ callback.Run();
}
SECStatus SSLClientSocketNSS::Core::ClientChannelIDHandler(
@@ -2734,6 +2847,7 @@ SSLClientSocketNSS::SSLClientSocketNSS(
server_bound_cert_service_(context.server_bound_cert_service),
ssl_session_cache_shard_(context.ssl_session_cache_shard),
completed_handshake_(false),
+ ssl_is_closed_(false),
next_handshake_state_(STATE_NONE),
nss_fd_(NULL),
net_log_(transport_socket->socket()->NetLog()),
@@ -2922,29 +3036,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_ &&
+ !ssl_is_closed_ &&
+ (core_->HasPendingAsyncOperation() ||
+ (!core_->IsClosed() && core_->HasUnhandledReceivedData()) ||
+ transport_->socket()->IsConnected());
Ryan Sleevi 2013/01/18 00:03:24 indents are wrong here
Takashi Toyoshima 2013/01/21 14:51:17 Done.
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 = IsConnected() &&
+ (core_->IsClosed() || !core_->HasUnhandledReceivedData()) &&
+ transport_->socket()->IsConnectedAndIdle();
LeaveFunction("");
return ret;
}
@@ -3016,6 +3122,8 @@ int SSLClientSocketNSS::Read(IOBuffer* buf, int buf_len,
EnterFunction(buf_len);
int rv = core_->Read(buf, buf_len, callback);
+ if (rv <= 0 && rv != ERR_IO_PENDING)
+ ssl_is_closed_ = true;
LeaveFunction(rv);
return rv;
@@ -3028,6 +3136,8 @@ int SSLClientSocketNSS::Write(IOBuffer* buf, int buf_len,
EnterFunction(buf_len);
int rv = core_->Write(buf, buf_len, callback);
+ if (rv < 0 && rv != ERR_IO_PENDING)
+ ssl_is_closed_ = true;
LeaveFunction(rv);
return rv;
« no previous file with comments | « net/socket/ssl_client_socket_nss.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698