Tidy up SSL_read error handling.

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index d81b24b5177cf4618fd98c7938ae245527b5d87c..1db73d74ea33de33e23c343d82d68b8616707b32 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -1534,71 +1534,61 @@ int SSLClientSocketOpenSSL::DoPayloadRead() {
   }
 
   int total_bytes_read = 0;
+  int ssl_ret;
   do {
-    rv = SSL_read(ssl_, 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_) {
-    rv = total_bytes_read;
-  } else {
-    // Otherwise, an error occurred (rv <= 0). The error needs to be handled
-    // immediately, while the OpenSSL 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.
+    ssl_ret = SSL_read(ssl_, user_read_buf_->data() + total_bytes_read,
+                       user_read_buf_len_ - total_bytes_read);
+    if (ssl_ret > 0)
+      total_bytes_read += ssl_ret;
+  } while (total_bytes_read < user_read_buf_len_ && ssl_ret > 0);
+
+  // Although only the final SSL_read call may have failed, the failure needs to
+  // processed immediately, while the information still available in OpenSSL's
+  // error queue.
+  if (client_auth_cert_needed_) {
+    pending_read_error_ = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
+  } else if (ssl_ret <= 0) {

[Ryan Sleevi, 2015/03/17 00:33:14]

Why is this change correct?

Previously, this code only executed for cases where total_bytes_read !=
user_read_buf_len (per line 1544)

[davidben, 2015/03/20 20:46:04]

Claim: Exactly one of ssl_ret <= 0 and total_bytes_read == user_read_buf_len_
are true.


In English:

ssl_ret <= 0 means the last SSL_read call failed. total_bytes_read ==
user_read_buf_len_ means we managed to read everything.

If the last SSL_read call failed, that means we hadn't read everything, tried to
read more, and didn't manage. Thus, total_bytes_read < user_read_buf_len_.

If we managed to read everything, that means the last SSL_read call must have
succeeded and we wouldn't have bothered trying to read more. This means ssl_ret
> 0.


In mindless symbol-manipulation:

Precondition: user_read_buf_len_ > 0. (See comment below.)

The for loop has exited so:
  ssl_ret <= 0 or total_bytes_read >= user_read_buf_len_.

SSL_read never returns anything larger than user_read_buf_len_ -
total_bytes_read, so total_bytes_read <= user_read_buf_len_. Thus we know:
  ssl_ret <= 0 or total_bytes_read == user_read_buf_len_.

So we know at least one is true. We need to show at most one is.

Assume both are:
  ssl_ret <= 0 and total_bytes_read == user_read_buf_len_

Thus, in the final iteration of the loop, ssl_ret <= 0 and line 1542 did not
run. So total_bytes_read and user_read_buf_len_ did not change.

That means that total_bytes_read == user_read_buf_len_ was true at the start of
the loop iteration. Thus the loop must have ran only once, otherwise we'd have
aborted the loop after the previous iteration,

If the loop ran only once and that iteration had ssl_ret <= 0, that means
total_bytes_read never changed, so user_read_buf_len_ = total_bytes_read = 0.
But we're assuming user_read_buf_len_ > 0. Contradiction.

----

Slightly less formally, this is just cleaning up the logic so that it's:

1. Convert the error code, if any, into a net error code with associated stuff.

2. If there's any data to return, return that first and defer the error.
Otherwise, return the error immediately.

+    // A zero return from SSL_read may mean any of:
+    // - The underlying BIO_read returned 0.
+    // - The peer sent a close_notify.
+    // - Any arbitrary error. https://crbug.com/466303
     //
-    // If no data was read, |*next_result| will point to the return value of
-    // this function. If at least some data was read, |*next_result| will point
-    // to |pending_read_error_|, to be returned in a future call to
-    // DoPayloadRead() (e.g.: after the current data is handled).
-    int *next_result = &rv;
-    if (total_bytes_read > 0) {
-      pending_read_error_ = rv;
-      rv = total_bytes_read;
-      next_result = &pending_read_error_;
+    // TransportReadComplete converts the first to an ERR_CONNECTION_CLOSED
+    // error, so it does not occur. The second and third are distinguished by
+    // SSL_ERROR_ZERO_RETURN.
+    pending_read_ssl_error_ = SSL_get_error(ssl_, ssl_ret);
+    if (pending_read_ssl_error_ == SSL_ERROR_ZERO_RETURN) {
+      pending_read_error_ = 0;
+    } else {
+      pending_read_error_ = MapOpenSSLErrorWithDetails(
+          pending_read_ssl_error_, err_tracer, &pending_read_error_info_);
     }
 
-    if (client_auth_cert_needed_) {
-      *next_result = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
-    } else if (*next_result <= 0) {
-      // A zero return from SSL_read may mean any of:
-      // - The underlying BIO_read returned 0.
-      // - The peer sent a close_notify.
-      // - Any arbitrary error. https://crbug.com/466303
-      //
-      // TransportReadComplete converts the first to an ERR_CONNECTION_CLOSED
-      // error, so it does not occur. The second and third are distinguished by
-      // SSL_ERROR_ZERO_RETURN.
-      pending_read_ssl_error_ = SSL_get_error(ssl_, *next_result);
-      if (pending_read_ssl_error_ == SSL_ERROR_ZERO_RETURN) {
-        *next_result = 0;
-      } else {
-        *next_result = MapOpenSSLErrorWithDetails(
-            pending_read_ssl_error_, err_tracer, &pending_read_error_info_);
-      }
+    // Many servers do not reliably send a close_notify alert when shutting down
+    // a connection, and instead terminate the TCP connection. This is reported
+    // as ERR_CONNECTION_CLOSED. Because of this, map the unclean shutdown to a
+    // graceful EOF, instead of treating it as an error as it should be.
+    if (pending_read_error_ == ERR_CONNECTION_CLOSED)
+      pending_read_error_ = 0;
+  }
 
-      // Many servers do not reliably send a close_notify alert when shutting
-      // down a connection, and instead terminate the TCP connection. This is
-      // reported as ERR_CONNECTION_CLOSED. Because of this, map the unclean
-      // shutdown to a graceful EOF, instead of treating it as an error as it
-      // should be.
-      if (*next_result == ERR_CONNECTION_CLOSED)
-        *next_result = 0;
-
-      if (rv > 0 && *next_result == ERR_IO_PENDING) {
-          // If at least some data was read from SSL_read(), do not treat
-          // insufficient data as an error to return in the next call to
-          // DoPayloadRead() - instead, let the call fall through to check
-          // SSL_read() again. This is because DoTransportIO() may complete
-          // in between the next call to DoPayloadRead(), and thus it is
-          // important to check SSL_read() on subsequent invocations to see
-          // if a complete record may now be read.
-        *next_result = kNoPendingReadResult;
-      }
-    }
+  if (total_bytes_read > 0 || user_read_buf_len_ == 0) {

[Ryan Sleevi, 2015/03/17 00:33:14]

How is it possible for user_read_buf_len to equal 0?

[davidben, 2015/03/20 20:46:04]

I noticed that we don't actually have any checks that you don't try to Read()
with zero length. Though, tracing through BoringSSL, it looks like SSL_read
behaves weird too. So neither the old logic nor the new would really work.

I've just added a DCHECK for now. Seems SocketLibevent::Read also has one.

+    // Return any bytes read to the caller. The error will be deferred to the
+    // next call of DoPayloadRead.
+    rv = total_bytes_read;
+
+    // Do not treat insufficient data as an error to return in the next call to
+    // DoPayloadRead() - instead, let the call fall through to check SSL_read()
+    // again. This is because DoTransportIO() may complete in between the next
+    // call to DoPayloadRead(), and thus it is important to check SSL_read() on
+    // subsequent invocations to see if a complete record may now be read.
+    if (pending_read_error_ == ERR_IO_PENDING)
+      pending_read_error_ = kNoPendingReadResult;
+  } else {
+    // No bytes were returned. Return the pending read error immediately.
+    DCHECK_NE(kNoPendingReadResult, pending_read_error_);
+    rv = pending_read_error_;
+    pending_read_error_ = kNoPendingReadResult;
   }
 
   if (rv >= 0) {