Stop attempting to write to transport sockets in NSS on failure.

net/socket/ssl_client_socket_nss.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.
 
 // This file includes code SSLClientSocketNSS::DoVerifyCertComplete() derived
 // from AuthCertificateCallback() in
 // mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp.
 
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
@@ skipping 872 matching lines @@
   // True if NSS has False Started.
   bool false_started_;
   // True if NSS has called HandshakeCallback.
   bool handshake_callback_called_;
 
   HandshakeState nss_handshake_state_;
 
   bool transport_recv_busy_;
   bool transport_recv_eof_;
   bool transport_send_busy_;
+  int transport_send_error_;
 
   // Used by Read function.
   scoped_refptr<IOBuffer> user_read_buf_;
   int user_read_buf_len_;
 
   // Used by Write function.
   scoped_refptr<IOBuffer> user_write_buf_;
   int user_write_buf_len_;
 
   CompletionCallback user_connect_callback_;
@@ skipping 46 matching lines @@
       pending_read_nss_error_(0),
       next_handshake_state_(STATE_NONE),
       channel_id_xtn_negotiated_(false),
       channel_id_needed_(false),
       client_auth_cert_needed_(false),
       false_started_(false),
       handshake_callback_called_(false),
       transport_recv_busy_(false),
       transport_recv_eof_(false),
       transport_send_busy_(false),
+      transport_send_error_(0),

[wtc, 2014/06/16 19:57:21]

I assume transport_send_error_ is a net:: error code. If so, it should be
initialized to OK (same value but more descriptive).

Make the same change on line 2144.

[davidben, 2014/06/16 23:02:13]

Done.

       user_read_buf_len_(0),
       user_write_buf_len_(0),
       network_task_runner_(network_task_runner),
       nss_task_runner_(nss_task_runner),
       weak_net_log_(weak_net_log_factory_.GetWeakPtr()) {
 }
 
 SSLClientSocketNSS::Core::~Core() {
   // TODO(wtc): Send SSL close_notify alert.
   if (nss_fd_ != NULL) {
@@ skipping 1163 matching lines @@
 }
 
 // Return 0 if nss_bufs_ was empty,
 // > 0 for bytes transferred immediately,
 // < 0 for error (or the non-error ERR_IO_PENDING).
 int SSLClientSocketNSS::Core::BufferSend() {
   DCHECK(OnNSSTaskRunner());
 
   if (transport_send_busy_)
     return ERR_IO_PENDING;
+  if (transport_send_error_ != 0)
+    return transport_send_error_;

[Ryan Sleevi, 2014/06/16 22:07:07]

Why introduce another member, when we already have the memio_PutWriteResult
tracking the write error?

Ultimately, the exact error doesn't matter - you can see that the BufferSend rv
gets swallowed in DoTransportIO, so the need for the exact error isn't even
needed. Heck, ERR_ABORTED would be sufficient to break the loop.

This also seems to ensure greater consistency with the existing implementation
(eg: it avoids the need to special-note on 2711-2714), and also avoids the issue
Wan-Teh noted where you're not storing the result in case of sync failure in
DoBufferSend (since we handle that below, on line 2172)

[davidben, 2014/06/16 23:02:13]

Good point. That's much cleaner. Done.

 
   const char* buf1;
   const char* buf2;
   unsigned int len1, len2;
   memio_GetWriteParams(nss_bufs_, &buf1, &len1, &buf2, &len2);
   const unsigned int len = len1 + len2;
 
   int rv = 0;
   if (len) {
     scoped_refptr<IOBuffer> send_buffer(new IOBuffer(len));
@@ skipping 468 matching lines @@
   int rv = transport_->socket()->Write(
       send_buffer, len,
       base::Bind(&Core::BufferSendComplete,
                  base::Unretained(this)));
 
   if (!OnNSSTaskRunner() && rv != ERR_IO_PENDING) {
     nss_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&Core::BufferSendComplete, this, rv));
     return rv;
   }

[wtc, 2014/06/16 19:57:21]

IMPORTANT: I think we should also set transport_send_error_ here, if we are on
the NSS task runner:

  if (OnNSSTaskRunner() && rv != ERR_IO_PENDING && rv < 0) {
    transport_send_error_ = rv;
  }

We may be able to combine these two if statements (not sure if that'll be more
readable though).

Note: it may be better to do this is line 2172, next to the
memio_PutWriteResult(nss_bufs_, MapErrorToNSS(rv)) call.

[davidben, 2014/06/16 23:02:13]

Done. And then did Ryan's suggestion.

 
   return rv;
 }
 
 int SSLClientSocketNSS::Core::DoGetDomainBoundCert(const std::string& host) {
   DCHECK(OnNetworkTaskRunner());
 
   if (detached_)
     return ERR_FAILED;
 
@@ skipping 56 matching lines @@
 
     nss_task_runner_->PostTask(
         FROM_HERE, base::Bind(&Core::BufferSendComplete, this, result));
     return;
   }
 
   DCHECK(OnNSSTaskRunner());
 
   memio_PutWriteResult(nss_bufs_, MapErrorToNSS(result));
   transport_send_busy_ = false;
+  // If there was an error, save the result on the NSS task runner. Future calls
+  // to BufferSend will return the error synchronously rather than calling
+  // DoBufferSend on the network task runner. This is important to prevent
+  // spinning infinitely. See https://crbug.com/381160.

[wtc, 2014/06/16 19:57:21]

Nit: it may be better to move this comment (starting with the second sentence
"Future calls to BufferSend...") to SSLClientSocketNSS::Core::BufferSend (line
2144), where there is more context to understand this comment.

[davidben, 2014/06/16 23:02:13]

Done.

+  if (result < 0)
+    transport_send_error_ = result;
   OnSendComplete(result);
 }
 
 void SSLClientSocketNSS::Core::OnHandshakeIOComplete(int result) {
   if (!OnNSSTaskRunner()) {
     if (detached_)
       return;
 
     nss_task_runner_->PostTask(
         FROM_HERE, base::Bind(&Core::OnHandshakeIOComplete, this, result));
@@ skipping 897 matching lines @@
 scoped_refptr<X509Certificate>
 SSLClientSocketNSS::GetUnverifiedServerCertificateChain() const {
   return core_->state().server_cert.get();
 }
 
 ServerBoundCertService* SSLClientSocketNSS::GetServerBoundCertService() const {
   return server_bound_cert_service_;
 }
 
 }  // namespace net