Fix instability in SSL client/server sockets

net/socket/ssl_client_socket_nss.cc

 // Copyright (c) 2011 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 1170 matching lines @@
     LeaveFunction("");
     return;
   }
 
   // OnSendComplete may need to call DoPayloadRead while the renegotiation
   // handshake is in progress.
   int rv_read = ERR_IO_PENDING;
   int rv_write = ERR_IO_PENDING;
   bool network_moved;
   do {
-      if (user_read_buf_)
+      if (user_read_buf_ && rv_read == ERR_IO_PENDING)
           rv_read = DoPayloadRead();
-      if (user_write_buf_)
+      if (user_write_buf_ && rv_write == ERR_IO_PENDING)
           rv_write = DoPayloadWrite();
       network_moved = DoTransportIO();
-  } while (rv_read == ERR_IO_PENDING &&
-           rv_write == ERR_IO_PENDING &&
-           network_moved);
+  } while (network_moved);

[Wez, 2011/07/18 21:53:33]

This loop used to exit on error, or if one or other of the payload operations
succeeded.

If the loop continues after DoPayloadRead() or DoPayloadWrite() have returned
success, because |network_moved| is true, then they will end up writing the same
data to the channel again, or reading over the data that they just read; neither
function clears the buffer pointer on success.

Since it's also possible for *both* DoPayloadRead() and DoPayloadWrite() to
succeed (or fail), we may end up calling both DoReadCallback() *and*
DoWriteCallback(), which will cause a crash if the caller deletes the
SSLClientSocket in response to the DoReadCallback().

[Sergey Ulanov, 2011/07/19 19:26:56]

Actually the code below may crash even when only DoPayloadRead() succeeds: if
the socket is destroyed when we call DoReadCallback() then we may crash when
checking |user_write_buf_|. In any case, this should be dealt with in a separate
CL.

[wtc, 2011/07/21 00:42:30]

Wez, you are right on.  I recently noticed this problem
during a code review and filed http://crbug.com/88108.

 
   if (user_read_buf_ && rv_read != ERR_IO_PENDING)
       DoReadCallback(rv_read);
   if (user_write_buf_ && rv_write != ERR_IO_PENDING)
       DoWriteCallback(rv_write);
 
   LeaveFunction("");
 }
 
 void SSLClientSocketNSS::OnRecvComplete(int result) {
@@ skipping 101 matching lines @@
 
   if (!nss_bufs_) {
     LOG(DFATAL) << "!nss_bufs_";
     int rv = ERR_UNEXPECTED;
     net_log_.AddEvent(NetLog::TYPE_SSL_WRITE_ERROR,
                       make_scoped_refptr(new SSLErrorParams(rv, 0)));
     return rv;
   }
 
   bool network_moved;
-  int rv;
+  int rv = ERR_IO_PENDING;
   do {
-    rv = DoPayloadWrite();
+    if (rv == ERR_IO_PENDING)
+      rv = DoPayloadWrite();
     network_moved = DoTransportIO();
-  } while (rv == ERR_IO_PENDING && network_moved);
+  } while (network_moved);

[Wez, 2011/07/18 21:53:33]

This will continue to loop on PR_Write() errors.

[Sergey Ulanov, 2011/07/19 19:26:56]

Why can't we continue pushing data to the transport socket when PR_Write()
fails? What can cause PR_Write() to fail?

Anyway, I reverted this change.

 
   LeaveFunction("");
   return rv;
 }
 
 bool SSLClientSocketNSS::LoadSSLHostInfo() {
   const SSLHostInfo::State& state(ssl_host_info_->state());
 
   if (state.certs.empty())
     return false;
@@ skipping 400 matching lines @@
 }
 
 // Do network I/O between the given buffer and the given socket.
 // Return true if some I/O performed, false otherwise (error or ERR_IO_PENDING)
 bool SSLClientSocketNSS::DoTransportIO() {
   EnterFunction("");
   bool network_moved = false;
   if (nss_bufs_ != NULL) {
     int nsent = BufferSend();
     int nreceived = BufferRecv();
-    network_moved = (nsent > 0 || nreceived >= 0);

[Wez, 2011/07/18 21:53:33]

A zero return value from the underlying net::Socket::Read() indicates EOF. 
Changing the |network_moved| test to |nreceived| > 0 means that DoTransportIO()
will return false (no I/O performed) at EOF, which seems counter-intuitive.

If the underlying transport connection returns EOF, the SSLClientSocket should
try to ensure that if the SSL teardown handshake was processed then the caller
gets a zero result from the next (or pending) Read, otherwise they get an error.
 Do we care about that, though?

The result of DoTransportIO() should be consistent between SSLClientSocketNSS
and SSLServerSocketNSS.

[Sergey Ulanov, 2011/07/19 19:26:56]

The previous behavior may be counter-intuitive depending on how you look at it.
E.g. nreceived = 0 means that we haven't received anything, and we shouldn't try
reading again.
We pass nreceived = 0 to memio (see memio_PutReadResult() call in BufferRecv()),
and I think it should be properly propagated to the client layer.

+    network_moved = (nsent > 0 || nreceived > 0);
   }
   LeaveFunction(network_moved);
   return network_moved;
 }
 
 // Return 0 for EOF,
 // > 0 for bytes transferred immediately,
 // < 0 for error (or the non-error ERR_IO_PENDING).
 int SSLClientSocketNSS::BufferSend(void) {
   if (transport_send_busy_)
@@ skipping 513 matching lines @@
   valid_thread_id_ = base::PlatformThread::CurrentId();
 }
 
 bool SSLClientSocketNSS::CalledOnValidThread() const {
   EnsureThreadIdAssigned();
   base::AutoLock auto_lock(lock_);
   return valid_thread_id_ == base::PlatformThread::CurrentId();
 }
 
 }  // namespace net