When reading from an SSL socket, attempt to fully fill the caller's buffer

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 819 matching lines @@
   ////////////////////////////////////////////////////////////////////////////
   HostPortPair host_and_port_;
   SSLConfig ssl_config_;
 
   // NSS SSL socket.
   PRFileDesc* nss_fd_;
 
   // Buffers for the network end of the SSL state machine
   memio_Private* nss_bufs_;
 
+  // Used by DoPayloadRead() when attempting to completely fill the caller's
+  // buffer.

[wtc, 2013/02/13 22:06:51]

The goal should be: when data has been received, fill the
caller's buffer as much as possible *without blocking*.
"completely fill the caller's buffer" seems to imply that
we are willing to wait. I suggest adding "without blocking"
to the end of this comment. But perhaps "without blocking"
is obvious when people read the DoPayloadRead() code.

+  // If DoPayloadRead() encounters an error after having read some data, stores
+  // the results to return on the *next* call to DoPayloadRead(). A value > 0
+  // indicates there is no pending result (as 0 indicates EOF, < 0 indicates
+  // error).
+  int pending_read_result_;
+  PRErrorCode pending_read_prerror_;

[wtc, 2013/02/13 22:06:51]

prerror => nss_error

Document that this member is only meaningful when
pending_read_result_ is < 0.

+
   // The certificate chain, in DER form, that is expected to be received from
   // the server.
   std::vector<std::string> predicted_certs_;
 
   State next_handshake_state_;
 
   // True if channel ID extension was negotiated.
   bool channel_id_xtn_negotiated_;
   // True if the handshake state machine was interrupted for channel ID.
   bool channel_id_needed_;
@@ skipping 55 matching lines @@
       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),
+      pending_read_result_(1),
+      pending_read_prerror_(0),
       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),
       transport_recv_eof_(false),
       transport_send_busy_(false),
       user_read_buf_len_(0),
       user_write_buf_len_(0),
@@ skipping 1013 matching lines @@
   SetChannelIDProvided();
   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_);

[wtc, 2013/02/13 22:06:51]

Just wanted to verify that you meant to delete this
memio_GetReadableBufferSize call.

[Ryan Sleevi, 2013/02/13 22:55:48]

Nope. Rebase bug. Good catch.

-  PostOrRunCallback(
-      FROM_HERE,
-      base::Bind(&Core::OnNSSBufferUpdated, this, amount_in_read_buffer));
+  int rv;
+  // If a previous greedy read resulted in an error that was not consumed (eg:
+  // due to the caller having read some data successfully), then return that
+  // pending error now.
+  if (pending_read_result_ <= 0) {
+    rv = pending_read_result_;
+    PRErrorCode prerr = pending_read_prerror_;
+    pending_read_result_ = 1;
+    pending_read_prerror_ = 0;
 
-  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)));
+    if (rv != ERR_IO_PENDING) {

[wtc, 2013/02/13 22:06:51]

We should add && rv != 0. A graceful connection closure
should not cause a NetLog::TYPE_SSL_READ_ERROR event to be
logged. The current code logs a
NetLog::TYPE_SSL_SOCKET_BYTES_RECEIVED event if rv == 0.

+      PostOrRunCallback(
+          FROM_HERE,
+          base::Bind(&AddLogEventWithCallback, weak_net_log_,
+                     NetLog::TYPE_SSL_READ_ERROR,
+                     CreateNetLogSSLErrorCallback(rv, prerr)));
+    }
     return rv;
   }
+
+  // Perform a greedy read, attempting to read as much as the caller has
+  // requested. Normally, PR_Read will return exactly one SSL application data

[wtc, 2013/02/13 22:06:51]

Please add "in the current NSS implementation". We should
eventually change this NSS behavior.

It may be useful to mention the potential 1-byte
application data records if the server does 1/n-1 record
splitting.

Typo: noticably => noticeably

[Ryan Sleevi, 2013/02/13 22:55:48]

I'm not sure I agree, for reasons I explained previously, particularly as it
relates to renegotiation.

For example, if we greedily read to the point of re-entering a handshake state,
we lose the ability to process or access any handshake state (such as the
certificates) associated with the data received. Having the SSL library return
'record at a time' grants the application the control to access or respond
according to data received (whether server or client).

[wtc, 2013/02/15 23:30:04]

The techniques you're using here (calling PR_Read in a loop
and saving a pending read result in a data member) can also
be done inside NSS. This way all NSS users will benefit.
(This also applies to OpenSSL.)

NSS will need to take care to not do greedy read for
either DTLS or a blocking-mode SSL socket, but NSS has
access to internal state, which may enable it to do a better
job.

Re: renegotiation: SSL/TLS allows application data records
to be sent during a renegotiation, so an application needs
to use the NSS "handshake hook" to delineate data encrypted
using the old keys and data encrypted using the new keys.
In practice I don't think anyone is sending data during a
renegotiation, so an application can also use the NSS
"certificate auth hook" to delineate data encrypted using
different keys.

If NSS does the greedy read itself, it can also take care
to not return data encrypted with different keys in the
same PR_Read call. NSS knows when a renegotiation is done,
so it may be able to delineate the data more easily.

+  // record's worth of data per invocation. The record size is dictated by the
+  // server, and may be noticably smaller than the caller's buffer.
+  //
+  // However, this greedy read may result in renegotiations/re-handshakes
+  // happening or may lead to some data being read, followed by an EOF (such as
+  // a TLS close-notify). If at least some data was read, then the error should
+  // be deferred until the next call to DoPayloadRead(). Otherwise, the error
+  // should be returned immediately.

[wtc, 2013/02/13 22:06:51]

I believe that if renegotiation happens, PR_Read will return
-1 with PR_WOULD_BLOCK_ERROR. So it should be transparent to
us.

EOF (zero return from PR_Read) should not be referred to as
an error.

[Ryan Sleevi, 2013/02/13 22:55:48]

My focus was in highlighting that SSL is not always "call and response" (eg:
WebSockets), so things like renegotiation cause writes to happen in read and
would prevent us from reading 'all' the data in the buffer.

+  int total_bytes_read = 0;
+  do {
+    rv = PR_Read(nss_fd_, user_read_buf_->data() + total_bytes_read,
+                 user_read_buf_len_ - total_bytes_read);
+    if (rv > 0)
+      total_bytes_read += rv;
+  } while (total_bytes_read < user_read_buf_len_ && rv > 0);
+
+  if (total_bytes_read == user_read_buf_len_) {
+    // The caller's entire request was satisfied without error. No further
+    // processing needed.
+    rv = total_bytes_read;
+  } else {
+    // Otherwise, an error occurred (rv <= 0). The error needs to be handled
+    // immediately, while the NSPR/NSS errors are still available in
+    // thread-local storage. However, the handled/remapped error code should
+    // only be returned if no application data was already read; if it was, the
+    // error code should be deferred until the next call of DoPayloadRead.
+    int* next_result = &rv;

[wtc, 2013/02/13 22:06:51]

Just a commentary: the use of next_result makes this code a
little hard to understand.

+    if (total_bytes_read > 0) {
+      pending_read_result_ = rv;

[Ryan Hamilton, 2013/02/13 17:23:18]

same comment about using a const kNoPendingReadResult = 1.

+      rv = total_bytes_read;
+      next_result = &pending_read_result_;
+    }
+
+    if (client_auth_cert_needed_) {
+      *next_result = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;

[wtc, 2013/02/13 22:06:51]

We should set pending_read_prerror_ to 0 in this case,
especially if next_result is  &pending_read_prerror_.

+    } else if (*next_result < 0) {
+      // If *next_result == 0, then that indicates EOF, and no special error
+      // handling is needed.
+      pending_read_prerror_ = PR_GetError();
+      if (pending_read_prerror_ == PR_WOULD_BLOCK_ERROR) {
+        *next_result = ERR_IO_PENDING;
+        pending_read_prerror_ = 0;

[wtc, 2013/02/13 22:06:51]

It seems OK for pending_read_prerror_ to be
PR_WOULD_BLOCK_ERROR in this case.

IMPORTANT: I think it is better to set *next_result to 1 if next_result
points to &pending_read_result_. Otherwise we will waste
the next DoPayloadRead() call doing that (see lines 1963-1977).

[Ryan Sleevi, 2013/02/13 22:55:48]

I'm not sure I fully follow, but what I understand you to be suggesting, I think
would be buggy.

The goal here is to make sure that control is returned to the caller to indicate
ERR_IO_PENDING - that is, there is an operation pending. This is handled on line
1976.

If we set pending_read_result_ = kNoPendingReadResult, then we would not return
early - thus calling PR_Read again, before control of the underlying transport
had returned any (new) data for PR_Read to use. Thus we'd still end up in
pending, but we'd have spent even more time in the function.

+      } else {
+        *next_result = HandleNSSError(pending_read_prerror_, false);
+      }
+    }
+  }
+
   if (rv >= 0) {
     PostOrRunCallback(
         FROM_HERE,
         base::Bind(&LogByteTransferEvent, weak_net_log_,
                    NetLog::TYPE_SSL_SOCKET_BYTES_RECEIVED, rv,
                    scoped_refptr<IOBuffer>(user_read_buf_)));
-    return rv;
+  } else if (rv != ERR_IO_PENDING) {
+    PostOrRunCallback(
+        FROM_HERE,
+        base::Bind(&AddLogEventWithCallback, weak_net_log_,
+                   NetLog::TYPE_SSL_READ_ERROR,
+                   CreateNetLogSSLErrorCallback(rv, pending_read_prerror_)));
+    pending_read_prerror_ = 0;
   }
-  PRErrorCode prerr = PR_GetError();
-  if (prerr == PR_WOULD_BLOCK_ERROR)
-    return ERR_IO_PENDING;
-
-  rv = HandleNSSError(prerr, false);
-  PostOrRunCallback(
-      FROM_HERE,
-      base::Bind(&AddLogEventWithCallback, weak_net_log_,
-                 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_);
@@ skipping 1444 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