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 1092 matching lines @@
   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) {
     user_read_callback_ = callback;

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

Note that there are three cases. This is case 1, and this will
be handled by the code I suggested in Core::DoReadCallback().

Case 2 is lines 1119-1120. This still needs to be handled.

Case 3 is line 1124. It is handled by your new code on
lines 2965-2966 in SSLClientSocketNSS::Read().

   } else {
     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
 
     if (!OnNetworkTaskRunner()) {
       PostOrRunCallback(FROM_HERE, base::Bind(callback, rv));
       return ERR_IO_PENDING;
     }
   }
 
@@ skipping 1100 matching lines @@
 
 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);

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

Core::Read() may be completed asynchronously and report
the read result |rv| using user_read_callback_ here.
If |rv| is 0, we should also set core_recv_eof_ to true,
in a thread-safe way.

   PostOrRunCallback(FROM_HERE, c);
 }
 
 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.
@@ skipping 366 matching lines @@
                               scoped_refptr<IOBuffer>(read_buffer), result));
     return;
   }
 
   DCHECK(OnNSSTaskRunner());
 
   if (result > 0) {
     char* buf;
     int nb = memio_GetReadParams(nss_bufs_, &buf);
     CHECK_GE(nb, result);
     memcpy(buf, read_buffer->data(), result);

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

IMPORTANT: here we copy data received from transport_socket()
to the nss_bufs_ memio buffer. Then we call OnRecvComplete,
which will only process the data in nss_bufs_ if there is a
pending Read().

So, if there is unprocessed received data in nss_bufs_,
SSLClientSocketNSS::IsConnected() also needs to return true
without calling transport_socket()->IsConnected().

   } else if (result == 0) {
     transport_recv_eof_ = true;
   }
 
   memio_PutReadResult(nss_bufs_, MapErrorToNSS(result));
   transport_recv_busy_ = false;
   OnRecvComplete(result);
 }
 
 void SSLClientSocketNSS::Core::PostOrRunCallback(
@@ skipping 38 matching lines @@
     const SSLConfig& ssl_config,
     const SSLClientSocketContext& context)
     : nss_task_runner_(nss_task_runner),
       transport_(transport_socket),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       cert_verifier_(context.cert_verifier),
       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()),
       transport_security_state_(context.transport_security_state),
       valid_thread_id_(base::kInvalidThreadId) {
   EnterFunction("");
   InitCore();
   LeaveFunction("");
 }
 
@@ skipping 167 matching lines @@
   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

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

Note this comment. I believe your CL is trying to fix this
false positive in one case: the server has sent the
close_notify alert, we have processed it (so core_recv_eof_
is true), but the server has not closed the TCP connection
(so transport_->socket()->IsConnected() will return true).

   // 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_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;
 }
 
 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 = 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;
 }
 
 int SSLClientSocketNSS::GetPeerAddress(IPEndPoint* address) const {
   return transport_->socket()->GetPeerAddress(address);
 }
 
 int SSLClientSocketNSS::GetLocalAddress(IPEndPoint* address) const {
   return transport_->socket()->GetLocalAddress(address);
@@ skipping 51 matching lines @@
   return base::TimeDelta::FromMicroseconds(-1);
 }
 
 int SSLClientSocketNSS::Read(IOBuffer* buf, int buf_len,
                              const CompletionCallback& callback) {
   DCHECK(core_);
   DCHECK(!callback.is_null());
 
   EnterFunction(buf_len);
   int rv = core_->Read(buf, buf_len, callback);
+  if (rv == 0)
+    core_recv_eof_ = true;
   LeaveFunction(rv);
 
   return rv;
 }
 
 int SSLClientSocketNSS::Write(IOBuffer* buf, int buf_len,
                               const CompletionCallback& callback) {
   DCHECK(core_);
   DCHECK(!callback.is_null());
 
@@ skipping 462 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