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 619 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 HasPendingAsyncOperation();
+  bool HasUnhandledData();

[wtc, 2013/01/14 20:50:08]

It may be good to stress that this is unhandled received data
(as opposed to the data that we are trying to send).

Perhaps the word "unhandled" already implies received data.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Done.

+
  private:
   friend class base::RefCountedThreadSafe<Core>;
   ~Core();
 
   enum State {
     STATE_NONE,
     STATE_HANDSHAKE,
     STATE_GET_DOMAIN_BOUND_CERT_COMPLETE,
   };
 
@@ skipping 78 matching lines @@
   bool DoTransportIO();
   int BufferRecv();
   int BufferSend();
 
   void OnRecvComplete(int result);
   void OnSendComplete(int result);
 
   void DoConnectCallback(int result);
   void DoReadCallback(int result);
   void DoWriteCallback(int result);
+  void UpdateNSSStatus(int amount_in_read_buffer,
+                       bool operation_is_read,
+                       const base::Closure& callback);
 
   // Client channel ID handler.
   static SECStatus ClientChannelIDHandler(
       void* arg,
       PRFileDesc* socket,
       SECKEYPublicKey **out_public_key,
       SECKEYPrivateKey **out_private_key);
 
   // ImportChannelIDKeys is a helper function for turning a DER-encoded cert and
   // key into a SECKEYPublicKey and SECKEYPrivateKey. Returns OK upon success
@@ skipping 39 matching lines @@
   // Uses PostOrRunCallback and |weak_net_log_| to try and log a
   // SSL_CLIENT_CERT_PROVIDED event, with the indicated count.
   void AddCertProvidedEvent(int cert_count);
 
   // Sets the handshake state |channel_id_sent| flag and logs the
   // SSL_CHANNEL_ID_PROVIDED event.
   void SetChannelIDProvided();
 
   ////////////////////////////////////////////////////////////////////////////
   // Members that are ONLY accessed on the network task runner:
   ////////////////////////////////////////////////////////////////////////////

[Takashi Toyoshima, 2013/01/15 13:46:03]

From line 802 to 804.
This comment declares that following NSS task runner status can be accessed on
the network task runner. See also my comments at line 822.

 
   // True if the owning SSLClientSocketNSS has called Detach(). No further
   // callbacks will be invoked nor access to members owned by the network
   // task runner.
   bool detached_;
 
   // The underlying transport to use for network IO.
   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 status.

[wtc, 2013/01/14 20:50:08]

Nit: I think "status" can be removed from this comment without
losing much. Or the comment can say "The NSS task runner status".

It seems important to note that these members are accessed
on the network task runner.

[Takashi Toyoshima, 2013/01/15 13:46:03]

OK, I remove the last "status".

We have a comment at line 802 to 804 and I think it means that following members
(including these status variables) can be accessed on the network task runner.
So I omitted the comment and just placed them here.

+  int unhandled_buffer_size_;
+  bool waiting_read_;
+  bool waiting_write_;

[wtc, 2013/01/14 20:50:08]

It may be a good idea to name these members nss_waiting_read_
and nss_waiting_write_ to stress the fact that they represent
the NSS task runner status.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Done.

+
   ////////////////////////////////////////////////////////////////////////////
   // 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 60 matching lines @@
     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),
       domain_bound_cert_request_handle_(NULL),
+      unhandled_buffer_size_(0),
+      waiting_read_(false),
+      waiting_write_(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 192 matching lines @@
     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_);
+  DCHECK(!waiting_read_);
 
   user_read_buf_ = buf;
   user_read_buf_len_ = buf_len;
+  waiting_read_ = true;
 
   int rv = DoReadLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_read_callback_ = callback;

[wtc, 2013/01/14 20:50:08]

It may be clearer to first set waiting_read_ to false
(or assume it is false), and set waiting_read_ to true here.
This way, waiting_read_=true matches rv=ERR_IO_PENDING.

This comment also applies to the setting of waiting_write_
in SSLClientSocketNSS::Core::Write().

[Takashi Toyoshima, 2013/01/15 13:46:03]

Done.

   } else {
     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
+    waiting_read_ = false;
 
     if (!OnNetworkTaskRunner()) {
       PostOrRunCallback(FROM_HERE, base::Bind(callback, rv));
       return ERR_IO_PENDING;
     }
   }
 
   return rv;
 }
 
@@ skipping 12 matching lines @@
     return rv;
   }
 
   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_);
+  DCHECK(!waiting_write_);
 
   user_write_buf_ = buf;
   user_write_buf_len_ = buf_len;
+  waiting_write_ = true;
 
   int rv = DoWriteLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_write_callback_ = callback;
   } else {
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
+    waiting_write_ = false;
 
     if (!OnNetworkTaskRunner()) {
       PostOrRunCallback(FROM_HERE, base::Bind(callback, rv));
       return ERR_IO_PENDING;
     }
   }
 
   return rv;
 }
 
+bool SSLClientSocketNSS::Core::HasPendingAsyncOperation() {
+  DCHECK(OnNetworkTaskRunner());
+  return waiting_read_ || waiting_write_;
+}
+
+bool SSLClientSocketNSS::Core::HasUnhandledData() {
+  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 979 matching lines @@
   unsigned int len1, len2;
   memio_GetWriteParams(nss_bufs_, &buf1, &len1, &buf2, &len2);
   const unsigned int len = len1 + len2;
 
   int rv = 0;
   if (len) {
     scoped_refptr<IOBuffer> send_buffer(new IOBuffer(len));
     memcpy(send_buffer->data(), buf1, len1);
     memcpy(send_buffer->data() + len1, buf2, len2);
 
     if (OnNetworkTaskRunner()) {

[Takashi Toyoshima, 2013/01/15 13:46:03]

This comment is not related to this CL.

At the head of this function, we use DCHECK(OnNSSTaskRunner()).
So, I guess following else is not needed.

Ditto on BufferRecv() function above.
If so, I'll upload another CL to remove following else.

[wtc, 2013/01/16 00:34:16]

The OnNetworkTaskRunner() test here means "if the network
task runner is the same as the NSS task runner". This
test should not be removed.

[Takashi Toyoshima, 2013/01/16 07:16:03]

Ah, sorry I mistakenly read this line 2199 as On*NSSTask*Runner().
You are right. Sorry for bothering.

       rv = DoBufferSend(send_buffer, len);
     } else {
       bool posted = network_task_runner_->PostTask(
           FROM_HERE,
           base::Bind(IgnoreResult(&Core::DoBufferSend), this, send_buffer,
                      len));
       rv = posted ? ERR_IO_PENDING : ERR_ABORTED;
     }
 
     if (rv == ERR_IO_PENDING) {
@@ skipping 71 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);
+  char* buf;
+  int amount_in_read_buffer = memio_GetReadParams(nss_bufs_, &buf);
+  // 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::UpdateNSSStatus,
+      this,
+      amount_in_read_buffer,
+      true,  // Operation is 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);
+  char* buf;
+  // Update buffer status because DoWriteLoop called DoTransportIO which may
+  // perform read operations.
+  int amount_in_read_buffer = memio_GetReadParams(nss_bufs_, &buf);
+  // 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::UpdateNSSStatus,
+      this,
+      amount_in_read_buffer,
+      false,  // Operation is not read, but write.
+      user_cb);
+  PostOrRunCallback(FROM_HERE, task);
+}
+
+void SSLClientSocketNSS::Core::UpdateNSSStatus(

[wtc, 2013/01/14 20:50:08]

Document this method either here or in the declaration.

I think this method could have a more precise name. I don't
have a good suggestion, but even "UpdateNSSTaskRunnerStatus"
would be clearer than "UpdateNSSStatus".

[Takashi Toyoshima, 2013/01/15 13:46:03]

OK. I use "UpdateNSSTaskRunnerStatus" for now.
I may change it again if I find better name.

+    int amount_in_read_buffer,
+    bool operation_is_read,
+    const base::Closure& callback) {
+  DCHECK(OnNetworkTaskRunner());
+  DCHECK(!callback.is_null());
+  DCHECK((operation_is_read && waiting_read_) ||
+         (!operation_is_read && waiting_write_));
+
+  unhandled_buffer_size_ = amount_in_read_buffer;
+  if (operation_is_read)
+    waiting_read_ = false;
+  else
+    waiting_write_ = false;
+  callback.Run();
 }
 
 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 602 matching lines @@
 }
 
 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;
+
+  if (!completed_handshake_)
+    ret = false;
+  else if (core_->HasPendingAsyncOperation())
+    ret = true;
+  else if (core_->HasUnhandledData())

[Ryan Sleevi, 2013/01/09 19:01:52]

BUG: I think there's a bug here. This fails to consider receiving a CloseNotify,
which causes a weird situation in which Read() will return EOF, but then
IsConnected() will continue returning true.

It's not enough to check that there's unhandled data. You need to check that
there's unhandled data and there hasn't been an exceptional return from (Read,
Write) that should cause such data to be ignored.

[Takashi Toyoshima, 2013/01/10 07:46:29]

OK, I'll restore ssl_recv_eof_, then remove above comments.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Try to fix this in the next CL.

+    ret = true;
+  else
+    ret = transport_->socket()->IsConnected();

[wtc, 2013/01/14 20:50:08]

I would probably write this as a single conditional expression,
or like this:
  if (!completed_handshake_) {
    ret = false;
  } else {
    ret = core_->HasPendingAsyncOperation() ||
          core_->HasUnhandledData() ||
          transport_->socket()->IsConnected();
  }

This comment also applies to the SSLClientSocketNSS::IsConnectedAndIdle()
method.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Done.

[Takashi Toyoshima, 2013/01/15 13:46:03]

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;
+
+  if (!completed_handshake_)
+    ret = false;
+  else if (core_->HasPendingAsyncOperation())
+    ret = true;  // This is probably wrong, but mirrors TCP.
+  else if (core_->HasUnhandledData())

[Ryan Sleevi, 2013/01/09 19:01:52]

Same here.

Design wise, you can probably simplify this:

if (!IsConnected())
  return false
if (core_->HasUnhandledData())
  return false;
return transport_->socket()->IsConnectedAndIdle();

[Takashi Toyoshima, 2013/01/10 07:46:29]

I tried to keep EnterFunction() and LeaveFunction() to work here. But can I
remove them? If so, I can simplify it as you suggested.

[Ryan Sleevi, 2013/01/10 08:01:23]

Just substitute the return for ret = ... & elseifs, like you have. Sorry for
being shorthand lazy here :)

[Takashi Toyoshima, 2013/01/10 08:17:14]

Ah, I see. You mean that I should reuse IsConnected() function here. I agreed.

[Takashi Toyoshima, 2013/01/15 13:46:03]

I applied Wan-Teh's suggestion after applying Ryan's idea.

+    ret = false;
+  else
+    ret = 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