Fix instability in SSL client/server sockets

net/socket/ssl_server_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.
 
 #include "net/socket/ssl_server_socket_nss.h"
 
 #if defined(OS_WIN)
 #include <winsock2.h>
 #endif
 
@@ skipping 407 matching lines @@
 
   if (!completed_handshake_)
     return;
 
   if (user_write_buf_) {
     int rv = DoWriteLoop(result);
     if (rv != ERR_IO_PENDING)
       DoWriteCallback(rv);
   } else {
     // Ensure that any queued ciphertext is flushed.
     DoTransportIO();

[wtc, 2011/07/21 00:27:12]

IMPORTANT: We may get here via this call stack:
    BufferSendComplete > OnSendComplete > DoTransportIO

This DoTransportIO call does call Write() again to send
the rest of the data.  So I believe your diagnosis of the
bug is not entirely correct.  If the SSLServerSocketNSS
object stays around, it will flush the rest of the data.
So I believe the bug occurs only if we destroy the
SSLServerSocketNSS object when we think the SSL Write()
has succeeded.

[Sergey Ulanov, 2011/07/21 00:33:28]

BufferSendComplete() is called from Write callback, but the write callback is
never called when write completes synchronously.

   }
 }
 
 void SSLServerSocketNSS::OnRecvComplete(int result) {
   if (next_handshake_state_ == STATE_HANDSHAKE) {
     // In handshake phase.
     OnHandshakeIOComplete(result);
     return;
   }
 
@@ skipping 82 matching lines @@
     char *buf;
     memio_GetReadParams(nss_bufs_, &buf);
     memcpy(buf, recv_buffer_->data(), result);
   }
   recv_buffer_ = NULL;
   memio_PutReadResult(nss_bufs_, MapErrorToNSS(result));
   transport_recv_busy_ = false;
   OnRecvComplete(result);
 }
 
-// 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)
+// Do as much as possible network I/O between the buffer and the

[wtc, 2011/07/21 00:27:12]

as much as possible network I/O => as much network I/O as possible

Please make the same change to ssl_client_socket_nss.cc.

[Sergey Ulanov, 2011/07/21 00:51:15]

Done.

+// transport socket. Return true if some I/O performed, false
+// otherwise (error or ERR_IO_PENDING).
 bool SSLServerSocketNSS::DoTransportIO() {
   bool network_moved = false;
   if (nss_bufs_ != NULL) {
-    int nsent = BufferSend();
-    int nreceived = BufferRecv();
-    network_moved = (nsent > 0 || nreceived >= 0);
+    int rv;
+    // Read and write as much data as we can. Loops are neccessary
+    // because Read() and Write() may return synchronously.
+    do {
+      rv = BufferSend();
+      if (rv > 0)
+        network_moved = true;
+    } while (rv > 0);

[wtc, 2011/07/21 00:27:12]

IMPORTANT: Unless this BufferSend loop empties the
nss_bufs_, it won't fix the bug completely.  I believe the
last SSLServerSocket::Write() call before the
SSLServerSocket is destroyed is still vulnerable to this
bug.

The real fix is for SSLServerSocket::Write to not complete
until the nss_bufs_ has been emptied.

Do you agree?

[Sergey Ulanov, 2011/07/21 00:33:28]

This loop can exit in 3 cases:
 1. There is no pending data in nss_bufs_ - that's what we want,
 2. BufferSend() returned ERR_IO_PENDING - in that case BufferSendComplete will
be called later, which will call DoTransportIO() again.
 3. BufferSend() returned another error - in that case we don't care about
writing rest of the data.

[wtc, 2011/07/21 00:56:45]

It is case 2 that I am worried about.

The caller may destroy this SSLServerSocketNSS object
before the transport socket Write() completes asynchronously.
Then the peer of the SSL connection won't receive all
the data.  But now I know case 1 will fix the bug you
encountered.

[Sergey Ulanov, 2011/07/21 01:09:51]

Yes, write callback doesn't mean that the data was delivered to the other end,
but it's also true for TCP sockets: if write() completes for a TCP socket it
doesn't mean that the data was actually sent, it just gives you signal that you
may try to send next chunk of data. There isn't much we can do about it without
affecting performance.

+    do {
+      rv = BufferRecv();
+      if (rv >= 0)
+        network_moved = true;
+    } while (rv > 0);

[wtc, 2011/07/21 00:27:12]

The loop around BufferRecv should not be necessary.

[Sergey Ulanov, 2011/07/21 00:51:15]

Yes, agree, this loop isn't neccessary, but it may make reading more efficient,
don't think it matters much though. Removed it.

   }
   return network_moved;
 }
 
 int SSLServerSocketNSS::DoPayloadRead() {
   DCHECK(user_read_buf_);
   DCHECK_GT(user_read_buf_len_, 0);
   int rv = PR_Read(nss_fd_, user_read_buf_->data(), user_read_buf_len_);
   if (rv >= 0)
     return rv;
@@ skipping 192 matching lines @@
   // We must call EnsureOCSPInit() here, on the IO thread, to get the IO loop
   // by MessageLoopForIO::current().
   // X509Certificate::Verify() runs on a worker thread of CertVerifier.
   EnsureOCSPInit();
 #endif
 
   return OK;
 }
 
 }  // namespace net