SSLClientSocket::IsConnected should care for internal buffers

net/socket/ssl_client_socket_nss.cc

 // 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.
 
 // This file includes code SSLClientSocketNSS::DoVerifyCertComplete() derived
 // from AuthCertificateCallback() in
 // mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp.
 
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
@@ skipping 624 matching lines @@
   // any time.
   const HandshakeState& state() const { return network_handshake_state_; }
 
   // Called on the network task runner.
   // Read() and Write() mirror the net::Socket functions of the same name.
   // If ERR_IO_PENDING is returned, |callback| will be invoked on the network
   // task runner at a later point, unless the caller calls Detach().
   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();
 
   enum State {
     STATE_NONE,
     STATE_HANDSHAKE,
     STATE_GET_DOMAIN_BOUND_CERT_COMPLETE,
   };
 
+  enum Operation {
+    OPERATION_READ,
+    OPERATION_WRITE,
+    OPERATION_UPDATE,
+  };
+
   bool OnNSSTaskRunner() const;
   bool OnNetworkTaskRunner() const;
 
   ////////////////////////////////////////////////////////////////////////////
   // Methods that are ONLY called on the NSS task runner:
   ////////////////////////////////////////////////////////////////////////////
 
   // Called by NSS during full handshakes to allow the application to
   // verify the certificate. Instead of verifying the certificate in the midst
   // of the handshake, SECSuccess is always returned and the peer's certificate
@@ skipping 102 matching lines @@
   ////////////////////////////////////////////////////////////////////////////
   // Methods that are ONLY called on the network task runner:
   ////////////////////////////////////////////////////////////////////////////
   int DoBufferRecv(IOBuffer* buffer, int len);
   int DoBufferSend(IOBuffer* buffer, int len);
   int DoGetDomainBoundCert(const std::string& origin,
                            const std::vector<uint8>& requested_cert_types);
 
   void OnGetDomainBoundCertComplete(int result);
   void OnHandshakeStateUpdated(const HandshakeState& state);
+  void OnNSSTaskRunnerStateUpdated(int amount_in_read_buffer,

[Ryan Sleevi, 2013/01/23 01:46:20]

Naming: "OnNSSTaskRunnerStateUpdated" is perhaps a bit overkill. The
"NSSTaskRunner" doesn't have "state", it's the socket that lives on the NSS task
runner.

OnNSSBuffersUpdated() ?

[Takashi Toyoshima, 2013/01/23 06:53:02]

Agreed.
Done!

+                                   bool closed,
+                                   Operation operation,
+                                   const base::Closure& callback);
+  void DidNSSRead(int result);
+  void DidNSSWrite(int result);
 
   ////////////////////////////////////////////////////////////////////////////
   // Methods that are called on both the network task runner and the NSS
   // task runner.
   ////////////////////////////////////////////////////////////////////////////
   void OnHandshakeIOComplete(int result);
   void BufferRecvComplete(IOBuffer* buffer, int result);
   void BufferSendComplete(int result);
 
   // PostOrRunCallback is a helper function to ensure that |callback| is
@@ skipping 23 matching lines @@
   ClientSocketHandle* transport_;
   base::WeakPtrFactory<BoundNetLog> weak_net_log_factory_;
 
   // The current handshake state. Mirrors |nss_handshake_state_|.
   HandshakeState network_handshake_state_;
 
   // The service for retrieving Channel ID keys.  May be NULL.
   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:
   ////////////////////////////////////////////////////////////////////////////
   HostPortPair host_and_port_;
   SSLConfig ssl_config_;
 
   // NSS SSL socket.
   PRFileDesc* nss_fd_;
 
   // Buffers for the network end of the SSL state machine
@@ skipping 59 matching lines @@
     base::SequencedTaskRunner* nss_task_runner,
     ClientSocketHandle* transport,
     const HostPortPair& host_and_port,
     const SSLConfig& ssl_config,
     BoundNetLog* net_log,
     ServerBoundCertService* server_bound_cert_service)
     : detached_(false),
       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),
       nss_bufs_(NULL),
       next_handshake_state_(STATE_NONE),
       channel_id_xtn_negotiated_(false),
       channel_id_needed_(false),
       client_auth_cert_needed_(false),
       handshake_callback_called_(false),
       transport_recv_busy_(false),
@@ skipping 173 matching lines @@
 
   domain_bound_cert_request_handle_.Cancel();
 }
 
 int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len,
                                    const CompletionCallback& callback) {
   if (!OnNSSTaskRunner()) {
     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;
   }
 
   DCHECK(OnNSSTaskRunner());
   DCHECK(handshake_callback_called_);
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   DCHECK(user_read_callback_.is_null());
   DCHECK(user_connect_callback_.is_null());
   DCHECK(!user_read_buf_);
   DCHECK(nss_bufs_);
 
   user_read_buf_ = buf;
   user_read_buf_len_ = 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()) {
+      // Update |nss_is_closed_| and |nss_waiting_read_| from
+      // NetworkTaskRunner because it runs in multi-threaded mode.

[Ryan Sleevi, 2013/01/23 01:46:20]

nit: These comments are superflous

Same throughout the rest of the file

[Takashi Toyoshima, 2013/01/23 06:53:02]

Done.

+      PostOrRunCallback(FROM_HERE, base::Bind(&Core::DidNSSRead, this, rv));
+
       PostOrRunCallback(FROM_HERE, base::Bind(callback, rv));
       return ERR_IO_PENDING;
+    } else {
+      // Update |nss_is_closed_| directly because it runs in single-threaded
+      // mode. |nss_waiting_read_| is not set.

[Ryan Sleevi, 2013/01/23 01:46:20]

nit: These comments are superflous

Same throughout the rest of the file

[Takashi Toyoshima, 2013/01/23 06:53:02]

Done.

+      DCHECK(!nss_waiting_read_);
+      if (rv <= 0)
+        nss_is_closed_ = true;
     }
   }
 
   return rv;
 }
 
 int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
                                     const CompletionCallback& callback) {
   if (!OnNSSTaskRunner()) {
     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());
   DCHECK(handshake_callback_called_);
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   DCHECK(user_write_callback_.is_null());
   DCHECK(user_connect_callback_.is_null());
   DCHECK(!user_write_buf_);
   DCHECK(nss_bufs_);
 
   user_write_buf_ = buf;
   user_write_buf_len_ = 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()) {
+      // Update |nss_is_closed_| and |nss_waiting_write_| from
+      // NetworkTaskRunner because it runs in multi-threaded mode.
+      PostOrRunCallback(FROM_HERE, base::Bind(&Core::DidNSSWrite, this, rv));
+
       PostOrRunCallback(FROM_HERE, base::Bind(callback, rv));
       return ERR_IO_PENDING;
+    } else {
+      // Update |nss_is_closed_| directly because it runs in single-threaded
+      // mode. |nss_waiting_write_| is not set.
+      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();
 }
 
 bool SSLClientSocketNSS::Core::OnNetworkTaskRunner() const {
   return network_task_runner_->RunsTasksOnCurrentThread();
 }
 
 // static
 SECStatus SSLClientSocketNSS::Core::OwnAuthCertHandler(
@@ skipping 858 matching lines @@
   GotoState(STATE_HANDSHAKE);
   return OK;
 }
 
 int SSLClientSocketNSS::Core::DoPayloadRead() {
   DCHECK(OnNSSTaskRunner());
   DCHECK(user_read_buf_);
   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::OnNSSTaskRunnerStateUpdated,
+      this,
+      amount_in_read_buffer,
+      rv <= 0 && rv != ERR_IO_PENDING,
+      OPERATION_UPDATE,
+      base::Closure());

[Takashi Toyoshima, 2013/01/23 06:53:02]

Oops, I missed to post the created task... then DCHECK could not fire in tries.

+
   if (client_auth_cert_needed_) {
     // We don't need to invalidate the non-client-authenticated SSL session
     // because the server will renegotiate anyway.
     rv = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
     PostOrRunCallback(
         FROM_HERE,
         base::Bind(&AddLogEventWithCallback, weak_net_log_,
                    NetLog::TYPE_SSL_READ_ERROR,
                    CreateNetLogSSLErrorCallback(rv, 0)));
     return rv;
@@ skipping 17 matching lines @@
                  NetLog::TYPE_SSL_READ_ERROR,
                  CreateNetLogSSLErrorCallback(rv, prerr)));
   return rv;
 }
 
 int SSLClientSocketNSS::Core::DoPayloadWrite() {
   DCHECK(OnNSSTaskRunner());
 
   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::OnNSSTaskRunnerStateUpdated,
+        this,
+        new_amount_in_read_buffer,
+        false,
+        OPERATION_UPDATE,
+        base::Closure());
+  }
   if (rv >= 0) {
     PostOrRunCallback(
         FROM_HERE,
         base::Bind(&LogByteTransferEvent, weak_net_log_,
                    NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                    scoped_refptr<IOBuffer>(user_write_buf_)));
     return rv;
   }
   PRErrorCode prerr = PR_GetError();
   if (prerr == PR_WOULD_BLOCK_ERROR)
@@ skipping 185 matching lines @@
   PostOrRunCallback(FROM_HERE, c);
 }
 
 void SSLClientSocketNSS::Core::DoReadCallback(int rv) {
   DCHECK(OnNSSTaskRunner());
   DCHECK_NE(ERR_IO_PENDING, rv);
   DCHECK(!user_read_callback_.is_null());
 
   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::OnNSSTaskRunnerStateUpdated,
+      this,
+      amount_in_read_buffer,
+      rv <= 0,  // EOF or errors.
+      OPERATION_READ,
+      user_cb);
+  PostOrRunCallback(FROM_HERE, task);
 }
 
 void SSLClientSocketNSS::Core::DoWriteCallback(int rv) {
   DCHECK(OnNSSTaskRunner());
   DCHECK_NE(ERR_IO_PENDING, rv);
   DCHECK(!user_write_callback_.is_null());
 
   // Since Run may result in Write being called, clear |user_write_callback_|
   // 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::OnNSSTaskRunnerStateUpdated,
+      this,
+      amount_in_read_buffer,
+      rv < 0,  // Errors.
+      OPERATION_WRITE,
+      user_cb);
+  PostOrRunCallback(FROM_HERE, task);
 }
 
 SECStatus SSLClientSocketNSS::Core::ClientChannelIDHandler(
     void* arg,
     PRFileDesc* socket,
     SECKEYPublicKey **out_public_key,
     SECKEYPrivateKey **out_private_key) {
   Core* core = reinterpret_cast<Core*>(arg);
   DCHECK(core->OnNSSTaskRunner());
 
@@ skipping 239 matching lines @@
 }
 
 int SSLClientSocketNSS::Core::DoBufferSend(IOBuffer* send_buffer, int len) {
   DCHECK(OnNetworkTaskRunner());
   DCHECK_GT(len, 0);
 
   if (detached_)
     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(
         FROM_HERE,
         base::Bind(&Core::BufferSendComplete, this, rv));
     return rv;
   }
 
   return rv;
 }
@@ skipping 22 matching lines @@
         FROM_HERE,
         base::Bind(&Core::OnHandshakeIOComplete, this, rv));
     return ERR_IO_PENDING;
   }
 
   return rv;
 }
 
 void SSLClientSocketNSS::Core::OnHandshakeStateUpdated(
     const HandshakeState& state) {
+  DCHECK(OnNetworkTaskRunner());
   network_handshake_state_ = state;
 }
 
+void SSLClientSocketNSS::Core::OnNSSTaskRunnerStateUpdated(

[Ryan Sleevi, 2013/01/23 01:46:20]

I find it weird to have both this function and DidNSSRead / DidNSSWrite, and
understanding the distinction between them, given that we update the
NSSTaskRunnerState whenever we read or write.

Should we remove the operation & bool closed from this, and have the existing
functions use DidNSSRead (with extra params) and DidNSSWrite?

+    int amount_in_read_buffer,
+    bool closed,
+    Operation operation,
+    const base::Closure& callback) {
+  DCHECK(OnNetworkTaskRunner());
+  DCHECK(operation == OPERATION_UPDATE || !callback.is_null());
+  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();
+}
+
+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_)
       return;
 
     nss_task_runner_->PostTask(
         FROM_HERE, base::Bind(&Core::BufferSendComplete, this, result));
     return;
   }
 
@@ skipping 289 matching lines @@
   user_connect_callback_.Reset();
   server_cert_verify_result_.Reset();
   completed_handshake_   = false;
   start_cert_verification_time_ = base::TimeTicks();
   InitCore();
 
   LeaveFunction("");
 }
 
 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 = IsConnected() &&
+             !(core_->IsConnected() && core_->HasUnhandledReceivedData()) &&

[Ryan Sleevi, 2013/01/23 01:46:20]

In thinking through this logic more, perhaps it should be separate.

What is the expected return if core_->HasPendingAsyncOperation?

It's true that it's connected, but it's false that it's idle (because there's a
pending operation)

[Takashi Toyoshima, 2013/01/23 06:53:02]

Agreed.
I give up to reuse IsConnected() here.

+             transport_->socket()->IsConnectedAndIdle();
   LeaveFunction("");
   return ret;
 }
 
 int SSLClientSocketNSS::GetPeerAddress(IPEndPoint* address) const {
   return transport_->socket()->GetPeerAddress(address);
 }
 
 int SSLClientSocketNSS::GetLocalAddress(IPEndPoint* address) const {
   return transport_->socket()->GetLocalAddress(address);
@@ skipping 533 matching lines @@
   EnsureThreadIdAssigned();
   base::AutoLock auto_lock(lock_);
   return valid_thread_id_ == base::PlatformThread::CurrentId();
 }
 
 ServerBoundCertService* SSLClientSocketNSS::GetServerBoundCertService() const {
   return server_bound_cert_service_;
 }
 
 }  // namespace net