Properly handle alerts from the peer in SSL_read.

net/socket/ssl_client_socket_openssl.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.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <errno.h>
@@ skipping 1545 matching lines @@
     // 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_;
     }
 
     if (client_auth_cert_needed_) {
       *next_result = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
-    } else if (*next_result < 0) {
+    } 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.

[Ryan Sleevi, 2015/03/17 01:01:28]

Thanks, I conformed that ssl3_get_record does indeed call OPENSSL_PUT_ERROR, but
causes a zero return. This zero is propogated through ssl3_read_bytes, which
then goes through the TLS_protocol_method and up to the caller of SSL_read.

       pending_read_ssl_error_ = SSL_get_error(ssl_, *next_result);
-      *next_result = MapOpenSSLErrorWithDetails(pending_read_ssl_error_,
-                                                err_tracer,
-                                                &pending_read_error_info_);
+      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_);

[davidben, 2015/03/11 21:47:18]

This logic is totally incomprehensible and insane. next_result switches from
type ssl_read_return_value_t* to net_return_value_t*. (And, of course,
ssl_read_return_value_t* is a distinct type from ssl_get_error_return_value_t*.)

I'll rewrite it in a follow-up.

+      }
 
       // 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) {
@@ skipping 462 matching lines @@
                                             ct::SCT_STATUS_LOG_UNKNOWN));
   }
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net