Fix a crash in net::SSLClientSocketWin::OnIOComplete(int) when

an HttpNetworkTransaction is destroyed while its SSLClientSocket
is verifying a certificate.

Document that ClientSocket::Disconnect aborts any pending IO and
prevents completion callback from running, like a cancel method.

Change the verifier_ member of SSLClientSocket to a scoped_ptr so
that we can destroy the CertVerifier object in the Disconnect method.
(CertVerifier doesn't have a cancel method, so we cancel pending
certificate verification by destroying the CertVerifier object.)

R=willchan
BUG=http://crbug.com/13981
TEST=none.  I can only reproduce this crash by modifying the
source code to create that condition.
Review URL: http://codereview.chromium.org/147159

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@19307 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 34 insertions, 14 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index a343151..8d2821b 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -399,8 +399,8 @@ uint64 HttpNetworkTransaction::GetUploadProgress() const {
 }
 
 HttpNetworkTransaction::~HttpNetworkTransaction() {
-  // If we still have an open socket, then make sure to disconnect it so we
-  // don't try to reuse it later on.
+  // If we still have an open socket, then make sure to disconnect it so it
+  // won't call us back and we don't try to reuse it later on.
   if (connection_.is_initialized())
     connection_.socket()->Disconnect();
 
diff --git a/net/socket/client_socket.h b/net/socket/client_socket.h
index 54866b8..d406fc7 100644
--- a/net/socket/client_socket.h
+++ b/net/socket/client_socket.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2009 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.
 
@@ -33,9 +33,13 @@ class ClientSocket : public Socket {
   //
   virtual int Connect(CompletionCallback* callback) = 0;
 
-  // Called to disconnect a connected socket.  Does nothing if the socket is
-  // already disconnected.  After calling Disconnect it is possible to call
-  // Connect again to establish a new connection.
+  // Called to disconnect a socket.  Does nothing if the socket is already
+  // disconnected.  After calling Disconnect it is possible to call Connect
+  // again to establish a new connection.
+  //
+  // If IO (Connect, Read, or Write) is pending when the socket is
+  // disconnected, the pending IO is cancelled, and the completion callback
+  // will not be called.
   virtual void Disconnect() = 0;
 
   // Called to test if the connection is still alive.  Returns false if a
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index fc2fed6..4ee0f0a 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -66,6 +66,7 @@
 #include "base/logging.h"
 #include "base/nss_init.h"
 #include "base/string_util.h"
+#include "net/base/cert_verifier.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/base/ssl_info.h"
@@ -366,6 +367,9 @@ void SSLClientSocketNSS::Disconnect() {
     nss_fd_ = NULL;
   }
 
+  // Shut down anything that may call us back (through buffer_send_callback_,
+  // buffer_recv_callback, or _io_callback_).
+  verifier_.reset();
   transport_->Disconnect();
 
   // Reset object state
@@ -721,13 +725,17 @@ int SSLClientSocketNSS::DoVerifyCert(int result) {
     flags |= X509Certificate::VERIFY_REV_CHECKING_ENABLED;
   if (ssl_config_.verify_ev_cert)
     flags |= X509Certificate::VERIFY_EV_CERT;
-  return verifier_.Verify(server_cert_, hostname_, flags,
-                          &server_cert_verify_result_, &io_callback_);
+  verifier_.reset(new CertVerifier);
+  return verifier_->Verify(server_cert_, hostname_, flags,
+                           &server_cert_verify_result_, &io_callback_);
 }
 
 // Derived from AuthCertificateCallback() in
 // mozilla/source/security/manager/ssl/src/nsNSSCallbacks.cpp.
 int SSLClientSocketNSS::DoVerifyCertComplete(int result) {
+  DCHECK(verifier_.get());
+  verifier_.reset();
+
   if (result == OK) {
     // Remember the intermediate CA certs if the server sends them to us.
     CERTCertList* cert_list = CERT_GetCertChainFromCert(
diff --git a/net/socket/ssl_client_socket_nss.h b/net/socket/ssl_client_socket_nss.h
index f53e53a..2454966 100644
--- a/net/socket/ssl_client_socket_nss.h
+++ b/net/socket/ssl_client_socket_nss.h
@@ -15,7 +15,6 @@
 #include <string>
 
 #include "base/scoped_ptr.h"
-#include "net/base/cert_verifier.h"
 #include "net/base/cert_verify_result.h"
 #include "net/base/completion_callback.h"
 #include "net/base/nss_memio.h"
@@ -24,6 +23,7 @@
 
 namespace net {
 
+class CertVerifier;
 class X509Certificate;
 
 // An SSL client socket implemented with Mozilla NSS.
@@ -99,7 +99,7 @@ class SSLClientSocketNSS : public SSLClientSocket {
   scoped_refptr<X509Certificate> server_cert_;
   CertVerifyResult server_cert_verify_result_;
 
-  CertVerifier verifier_;
+  scoped_ptr<CertVerifier> verifier_;
 
   bool completed_handshake_;
 
diff --git a/net/socket/ssl_client_socket_win.cc b/net/socket/ssl_client_socket_win.cc
index f461094..f40fad4 100644
--- a/net/socket/ssl_client_socket_win.cc
+++ b/net/socket/ssl_client_socket_win.cc
@@ -10,6 +10,7 @@
 #include "base/singleton.h"
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
+#include "net/base/cert_verifier.h"
 #include "net/base/connection_type_histograms.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -477,6 +478,8 @@ int SSLClientSocketWin::Connect(CompletionCallback* callback) {
 void SSLClientSocketWin::Disconnect() {
   // TODO(wtc): Send SSL close_notify alert.
   completed_handshake_ = false;
+  // Shut down anything that may call us back through io_callback_.
+  verifier_.reset();
   transport_->Disconnect();
 
   if (send_buffer_.pvBuffer)
@@ -853,11 +856,15 @@ int SSLClientSocketWin::DoVerifyCert() {
     flags |= X509Certificate::VERIFY_REV_CHECKING_ENABLED;
   if (ssl_config_.verify_ev_cert)
     flags |= X509Certificate::VERIFY_EV_CERT;
-  return verifier_.Verify(server_cert_, hostname_, flags,
-                          &server_cert_verify_result_, &io_callback_);
+  verifier_.reset(new CertVerifier);
+  return verifier_->Verify(server_cert_, hostname_, flags,
+                           &server_cert_verify_result_, &io_callback_);
 }
 
 int SSLClientSocketWin::DoVerifyCertComplete(int result) {
+  DCHECK(verifier_.get());
+  verifier_.reset();
+
   // If we have been explicitly told to accept this certificate, override the
   // result of verifier_.Verify.
   // Eventually, we should cache the cert verification results so that we don't
diff --git a/net/socket/ssl_client_socket_win.h b/net/socket/ssl_client_socket_win.h
index 6e03309..461047c 100644
--- a/net/socket/ssl_client_socket_win.h
+++ b/net/socket/ssl_client_socket_win.h
@@ -14,7 +14,6 @@
 #include <string>
 
 #include "base/scoped_ptr.h"
-#include "net/base/cert_verifier.h"
 #include "net/base/cert_verify_result.h"
 #include "net/base/completion_callback.h"
 #include "net/base/ssl_config_service.h"
@@ -22,6 +21,8 @@
 
 namespace net {
 
+class CertVerifier;
+
 // An SSL client socket implemented with the Windows Schannel.
 class SSLClientSocketWin : public SSLClientSocket {
  public:
@@ -104,7 +105,7 @@ class SSLClientSocketWin : public SSLClientSocket {
 
   SecPkgContext_StreamSizes stream_sizes_;
   scoped_refptr<X509Certificate> server_cert_;
-  CertVerifier verifier_;
+  scoped_ptr<CertVerifier> verifier_;
   CertVerifyResult server_cert_verify_result_;
 
   CredHandle* creds_;