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

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 27d31b96cfa3a3a3d7f416d4caec7782a557962e..76ebb66c2a5e5bde369d52a88dddbc1e384d55d2 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -837,6 +837,15 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> {
   // 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_;
@@ -912,6 +921,8 @@ SSLClientSocketNSS::Core::Core(
       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),
@@ -1945,43 +1956,90 @@ int SSLClientSocketNSS::Core::DoPayloadRead() {
   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));
-
-  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)));
+  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 (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;
 }