Implements new, more robust design for communicating between SSLConnectJobs.

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 <openssl/err.h>
@@ skipping 69 matching lines @@
       return SSL_CONNECTION_VERSION_TLS1;
     case 0x0302:
       return SSL_CONNECTION_VERSION_TLS1_1;
     case 0x0303:
       return SSL_CONNECTION_VERSION_TLS1_2;
     default:
       return SSL_CONNECTION_VERSION_UNKNOWN;
   }
 }
 
-// Compute a unique key string for the SSL session cache. |socket| is an
-// input socket object. Return a string.
-std::string GetSocketSessionCacheKey(const SSLClientSocketOpenSSL& socket) {
-  std::string result = socket.host_and_port().ToString();
-  result.append("/");
-  result.append(socket.ssl_session_cache_shard());
-  return result;
-}
-
 }  // namespace
 
 class SSLClientSocketOpenSSL::SSLContext {
  public:
   static SSLContext* GetInstance() { return Singleton<SSLContext>::get(); }
   SSL_CTX* ssl_ctx() { return ssl_ctx_.get(); }
   SSLSessionCacheOpenSSL* session_cache() { return &session_cache_; }
 
   SSLClientSocketOpenSSL* GetClientSocketFromSSL(const SSL* ssl) {
     DCHECK(ssl);
@@ skipping 23 matching lines @@
     // TODO(kristianm): Only select this if ssl_config_.next_proto is not empty.
     // It would be better if the callback were not a global setting,
     // but that is an OpenSSL issue.
     SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(), SelectNextProtoCallback,
                                      NULL);
   }
 
   static std::string GetSessionCacheKey(const SSL* ssl) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     DCHECK(socket);
-    return GetSocketSessionCacheKey(*socket);
+    return socket->GetSessionCacheKey();
   }
 
   static SSLSessionCacheOpenSSL::Config kDefaultSessionCacheConfig;
 
   static int ClientCertCallback(SSL* ssl, X509** x509, EVP_PKEY** pkey) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     CHECK(socket);
     return socket->ClientCertRequestCallback(ssl, x509, pkey);
   }
 
@@ skipping 200 matching lines @@
       transport_bio_(NULL),
       transport_(transport_socket.Pass()),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
       trying_cached_session_(false),
       next_handshake_state_(STATE_NONE),
       npn_status_(kNextProtoUnsupported),
       channel_id_request_return_value_(ERR_UNEXPECTED),
       channel_id_xtn_negotiated_(false),
-      net_log_(transport_->socket()->NetLog()) {}
+      net_log_(transport_->socket()->NetLog()) {
+}
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
   Disconnect();
 }
 
+bool SSLClientSocketOpenSSL::InSessionCache() const {
+  SSLContext* context = SSLContext::GetInstance();
+  std::string cache_key = GetSessionCacheKey();
+  return context->session_cache()->SSLSessionIsInCache(cache_key);
+}
+
+void SSLClientSocketOpenSSL::SetHandshakeCompletionCallback(
+    const base::Closure& callback) {
+  handshake_completion_callback_ = callback;
+  SSLContext* context = SSLContext::GetInstance();
+  context->session_cache()->SetSessionAddedCallback(
+      ssl_,
+      base::Bind(&SSLClientSocketOpenSSL::OnHandshakeCompletion,
+                 base::Unretained(this)));
+}
+
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_;
   cert_request_info->cert_authorities = cert_authorities_;
   cert_request_info->cert_key_types = cert_key_types_;
 }
 
 SSLClientSocket::NextProtoStatus SSLClientSocketOpenSSL::GetNextProto(
     std::string* proto, std::string* server_protos) {
   *proto = npn_proto_;
@@ skipping 44 matching lines @@
 
   // Set SSL to client mode. Handshake happens in the loop below.
   SSL_set_connect_state(ssl_);
 
   GotoState(STATE_HANDSHAKE);
   rv = DoHandshakeLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_connect_callback_ = callback;
   } else {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
+    if (rv < OK)
+      OnHandshakeCompletion();
   }
 
   return rv > OK ? OK : rv;
 }
 
 void SSLClientSocketOpenSSL::Disconnect() {
+  OnHandshakeCompletion();

[Ryan Sleevi, 2014/07/25 01:36:36]

This surprises me, since we traditionally don't call callbacks during
Disconnect() ( it's actually documented like that in
http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/socket.h /
http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/stream_socket.h )

[mshelley, 2014/07/26 00:58:27]

The idea here was that we want to ensure that if a socket is randomly deleted,
its messenger will be notified so that it can delete any pending jobs dependent
upon that socket's connection.

Would it be more appropriate to make this call before Disconnect in the
destructor? 

[Ryan Sleevi, 2014/07/26 02:22:32]

Right, I got that part.
I think it's OK to call here, but it means you need to beef up the documentation
for this class (and setting the handshake callback), so that you're explicitly
documenting in the reader the 'edge' cases that can come up (similar to the
previous comments regarding ordering of callback execution), and what is or is
not 'acceptable' behaviour to do in such a callback.

   if (ssl_) {
+    SSLContext* context = SSLContext::GetInstance();
+    context->session_cache()->RemoveSessionAddedCallback(ssl_);
     // Calling SSL_shutdown prevents the session from being marked as
     // unresumable.
     SSL_shutdown(ssl_);
     SSL_free(ssl_);
     ssl_ = NULL;
   }
   if (transport_bio_) {
     BIO_free_all(transport_bio_);
     transport_bio_ = NULL;
   }
@@ skipping 149 matching lines @@
                                  int buf_len,
                                  const CompletionCallback& callback) {
   user_read_buf_ = buf;
   user_read_buf_len_ = buf_len;
 
   int rv = DoReadLoop(OK);
 
   if (rv == ERR_IO_PENDING) {
     user_read_callback_ = callback;
   } else {
-    if (rv > 0)
+    if (rv > 0) {
       was_ever_used_ = true;
+    } else if (rv < 0) {
+      // Failure of a read attempt may indicate a failed false start
+      // connection.
+      OnHandshakeCompletion();
+    }

[Ryan Sleevi, 2014/07/25 01:36:36]

I would actually move this to line 632

if (rv != ERR_IO_PENDING && rv < 0)
  OnHandshakeCompletion()

In this case, we shouldn't call the callback until the state updates on line
629/630 and 654/655 have happened (in part, this matches lines 786/787 and
798/799 in maintaining the ordering of variable assignments before callbacks)

[mshelley, 2014/07/26 00:58:27]

Done.

     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
   }
 
   return rv;
 }
 
 int SSLClientSocketOpenSSL::Write(IOBuffer* buf,
                                   int buf_len,
                                   const CompletionCallback& callback) {
   user_write_buf_ = buf;
   user_write_buf_len_ = buf_len;
 
   int rv = DoWriteLoop(OK);
 
   if (rv == ERR_IO_PENDING) {
     user_write_callback_ = callback;
   } else {
-    if (rv > 0)
+    if (rv > 0) {
       was_ever_used_ = true;
+    } else {
+      // Failure of a write attempt may indicate a failed false start
+      // connection.
+      OnHandshakeCompletion();
+    }
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
   }
 
   return rv;
 }
 
 int SSLClientSocketOpenSSL::SetReceiveBufferSize(int32 size) {
   return transport_->socket()->SetReceiveBufferSize(size);
 }
@@ skipping 10 matching lines @@
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
 
   ssl_ = SSL_new(context->ssl_ctx());
   if (!ssl_ || !context->SetClientSocketForSSL(ssl_, this))
     return ERR_UNEXPECTED;
 
   if (!SSL_set_tlsext_host_name(ssl_, host_and_port_.host().c_str()))
     return ERR_UNEXPECTED;
 
   trying_cached_session_ = context->session_cache()->SetSSLSessionWithKey(
-      ssl_, GetSocketSessionCacheKey(*this));
+      ssl_, GetSessionCacheKey());
 
   BIO* ssl_bio = NULL;
   // 0 => use default buffer sizes.
   if (!BIO_new_bio_pair(&ssl_bio, 0, &transport_bio_, 0))
     return ERR_UNEXPECTED;
   DCHECK(ssl_bio);
   DCHECK(transport_bio_);
 
   SSL_set_bio(ssl_, ssl_bio, ssl_bio);
 
@@ skipping 83 matching lines @@
   return OK;
 }
 
 void SSLClientSocketOpenSSL::DoReadCallback(int rv) {
   // Since Run may result in Read being called, clear |user_read_callback_|
   // up front.
   if (rv > 0)
     was_ever_used_ = true;
   user_read_buf_ = NULL;
   user_read_buf_len_ = 0;
+  if (rv < 0)
+    OnHandshakeCompletion();
   base::ResetAndReturn(&user_read_callback_).Run(rv);
 }
 
 void SSLClientSocketOpenSSL::DoWriteCallback(int rv) {
   // Since Run may result in Write being called, clear |user_write_callback_|
   // up front.
   if (rv > 0)
     was_ever_used_ = true;
   user_write_buf_ = NULL;
   user_write_buf_len_ = 0;
+  if (rv < 0)
+    OnHandshakeCompletion();
   base::ResetAndReturn(&user_write_callback_).Run(rv);
 }
 
+std::string SSLClientSocketOpenSSL::GetSessionCacheKey() const {
+  return CreateSessionCacheKey(host_and_port_, ssl_session_cache_shard_);
+}
+
+void SSLClientSocketOpenSSL::OnHandshakeCompletion() {
+  if (!handshake_completion_callback_.is_null())
+    base::ResetAndReturn(&handshake_completion_callback_).Run();
+}
+
 bool SSLClientSocketOpenSSL::DoTransportIO() {
   bool network_moved = false;
   int rv;
   // Read and write as much data as possible. The loop is necessary because
   // Write() may return synchronously.
   do {
     rv = BufferSend();
     if (rv != ERR_IO_PENDING && rv != 0)
       network_moved = true;
   } while (rv > 0);
@@ skipping 112 matching lines @@
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   return result;
 }
 
 void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
   if (!user_connect_callback_.is_null()) {
     CompletionCallback c = user_connect_callback_;
     user_connect_callback_.Reset();
     c.Run(rv > OK ? OK : rv);
   }
+  if (rv < OK)
+    OnHandshakeCompletion();

[Ryan Sleevi, 2014/07/25 01:36:36]

This surprises me, as it's inverted from your order of events on line 788/799
and 800/801, in which you call your callback than the user's callback.

All the more reason for the expanded comment/documentation on the exact
sequencing of events.

[mshelley, 2014/07/26 00:58:27]

Ah yes, this is wrong; it should be called before user_connect_callback_ is run.

 }
 
 X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
   server_cert_chain_->Reset(SSL_get_peer_cert_chain(ssl_));
   server_cert_ = server_cert_chain_->AsOSChain();
 
   if (!server_cert_chain_->IsValid())
     DVLOG(1) << "UpdateServerCert received invalid certificate chain from peer";
-
   return server_cert_.get();
 }
 
 void SSLClientSocketOpenSSL::OnHandshakeIOComplete(int result) {
   int rv = DoHandshakeLoop(result);
   if (rv != ERR_IO_PENDING) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     DoConnectCallback(rv);
   }
 }
@@ skipping 79 matching lines @@
     }
 
     bool network_moved = DoTransportIO();
     if (network_moved && next_handshake_state_ == STATE_HANDSHAKE) {
       // In general we exit the loop if rv is ERR_IO_PENDING.  In this
       // special case we keep looping even if rv is ERR_IO_PENDING because
       // the transport IO may allow DoHandshake to make progress.
       rv = OK;  // This causes us to stay in the loop.
     }
   } while (rv != ERR_IO_PENDING && next_handshake_state_ != STATE_NONE);
+
   return rv;
 }
 
 int SSLClientSocketOpenSSL::DoReadLoop(int result) {
   if (result < 0)
     return result;
 
   bool network_moved;
   int rv;
   do {
@@ skipping 81 matching lines @@
   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_RECEIVED, rv,
                                   user_read_buf_->data());
   }
   return rv;
 }
 
 int SSLClientSocketOpenSSL::DoPayloadWrite() {
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
   int rv = SSL_write(ssl_, user_write_buf_->data(), user_write_buf_len_);
-
   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                                   user_write_buf_->data());
     return rv;
   }
 
   int err = SSL_get_error(ssl_, rv);
   return MapOpenSSLError(err, err_tracer);
 }
 
@@ skipping 300 matching lines @@
   DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_;
   return SSL_TLSEXT_ERR_OK;
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net