SSLClientSocket::IsConnected should care for internal buffers

net/socket/ssl_client_socket_nss.cc

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 4a26b270430862ac38bf387cd940b56bb442290e..736453c6a35e527bf2a89f8c7938102f8376fb9c 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -2677,6 +2677,7 @@ SSLClientSocketNSS::SSLClientSocketNSS(
       server_bound_cert_service_(context.server_bound_cert_service),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
       completed_handshake_(false),
+      core_recv_eof_(false),
       next_handshake_state_(STATE_NONE),
       nss_fd_(NULL),
       net_log_(transport_socket->socket()->NetLog()),
@@ -2870,7 +2871,9 @@ bool SSLClientSocketNSS::IsConnected() const {
   // 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_recv_eof_;

[tyoshino (SeeGerritForStatus), 2012/12/14 15:13:54]

why &?

[Takashi Toyoshima, 2012/12/21 06:39:03]

Oops...

+  if (ret)
+    ret = transport_->socket()->IsConnected();

[wtc, 2012/12/14 22:55:14]

1. Please define |ret| using a single expression:

  bool ret = completed_handshake_ && !core_recv_eof_ &&
             transport_->socket()->IsConnected();

2. Make the same change to IsConnectedAndIdle().

IMPORTANT: I think this function is the root of the problem.

I believe that whenever there is a Read() that is still in
progress, IsConnected() must return true without calling
transport_->socket()->IsConnected(). The reason is that
there may be received but not yet processed data in flight.

Unfortunately I don't see a way to detect whether there is
a pending Read(). Only the Core object knows that, indicated
by a non-null Core::user_read_buf_ member.

It seems that we need to add a 'bool waiting_read_' member
to SSLClientSocketNSS. Then we can use a fix similar to your
fix to TCPClientSocketWin::IsConnected.

By the way, do you know why TCPClientSocketLibevent::IsConnected
doesn't need this fix? Is it because TCPClientSocketLibevent
does non-blocking socket I/O as opposed to async socket I/O?
I'm sorry if I have asked this question before.

[Ryan Sleevi, 2012/12/15 01:03:04]

I discussed with Wan-Teh and would propose an alternative solution here.

With SSL sockets, we have two buffers at play - we have the underlying transport
socket's buffers (which may be operated by the kernel) and we have the NSS
socket's buffers (reflected in the NSS MemIO, which is owned by the ::Core and
can only be accessed on the NSS task runner)

The current IsConnected/IsConnectedAndIdle TCP implementation uses MSG_PEEK,
which allows it to determine if the OS buffers contain any data. If they're
empty / socket is disconnected, it returns true.

There is a legitimate bug here, in that we should be considering the state of
the NSS internal buffers before communicating with the underlying socket.

SSL::IsConnected() {
  if (!completed_handshake_)
    return false;
  if (waiting_read_ || waiting_write)
    return true;  // An async read/write is still pending.
  if (ssl_buffers_have_data())
    return true;  // It may still be possible to read data

  // if (received_close_notify_)
  //   return true;
  return transport_->socket()->IsConnected();
}

SSL::IsConnectedAndIdle() {
  if (!completed_handshake_)
    return false;
  if (waiting_read_ || waiting_write_)
    return true;  // This is probably wrong, but mirrors TCP

  if (ssl_buffers_have_data())
    return false;  // If there is data buffered, we are NOT idle

  // if (received_close_notify_)
  //   return false;
  return transport_->socket()->IsConnectedAndIdle();
}


The trick here is how to implement "ssl_buffers_have_data()" call, since as I
mentioned, memio lives on the NSS task runner, not the socket's task runner.

The NSS memio only receives data in response to a Read() or Write() call [or
technically a Connect() call]. In order to satisfy a Read/Write call, the Core
will post back to the Network Task Runner (via PostOrRunCallback) to run the
Read callback.

The way to do this is to have
::Core::DoReadCallback/::Core::DoWriteCallback/::Core::DoConnectCallback curry
the state of the NSS memio buffers back to the Network Task Runner, and then
have the Network Task Runner execute the actual callback

Instead of

base::Closure c = base::Bind(
    base::ResetAndReturn(&some_callback_here_),
    rv);
PostOrRunCallback(FROM_HERE, c);

It would be
DCHECK(OnNSSTaskRunner());
int amount_in_read_buffer = ...
// This will invoke the user callback with |rv| as result
base::Closure user_cb = base::Bind(
    base::ResetAndReturn(&...), rv);
// This is used to curry the amount_in_read_buffer and |post_or_run_cb| back to
the network task runner
base::Closure task = base::Bind(
    &Core::UpdateBufferState, this, amount_in_read_buffer,
    user_cb);
PostOrRunCallback(FROM_HERE, task);


void ...::Core::UpdateBufferState(int pending_data, const base::Closure& task) {
  DCHECK(OnNetworkTaskRunner());
  available_read_data_ = pending_data;
  task.Run();
}

   LeaveFunction("");
   return ret;
 }
@@ -2885,7 +2888,9 @@ bool SSLClientSocketNSS::IsConnectedAndIdle() const {
   // transport_->socket()->IsConnectedAndIdle() returns the desired false
   // when we receive close_notify.
   EnterFunction("");
-  bool ret = completed_handshake_ && transport_->socket()->IsConnectedAndIdle();
+  bool ret = completed_handshake_ & !core_recv_eof_;

[tyoshino (SeeGerritForStatus), 2012/12/14 15:13:54]

ditto

[Takashi Toyoshima, 2012/12/21 06:39:03]

Done.

+  if (ret)
+    ret = transport_->socket()->IsConnectedAndIdle();
   LeaveFunction("");
   return ret;
 }
@@ -2957,6 +2962,8 @@ int SSLClientSocketNSS::Read(IOBuffer* buf, int buf_len,
 
   EnterFunction(buf_len);
   int rv = core_->Read(buf, buf_len, callback);
+  if (rv == 0)
+    core_recv_eof_ = true;
   LeaveFunction(rv);
 
   return rv;