Fix instability in SSL client/server sockets

net/socket/ssl_server_socket_nss.cc

Index: net/socket/ssl_server_socket_nss.cc
diff --git a/net/socket/ssl_server_socket_nss.cc b/net/socket/ssl_server_socket_nss.cc
index bdcff2c1002aab343b04c8327ea970b7ec844524..00054ef56085f252738a6046b948edae55a94574 100644
--- a/net/socket/ssl_server_socket_nss.cc
+++ b/net/socket/ssl_server_socket_nss.cc
@@ -528,14 +528,25 @@ void SSLServerSocketNSS::BufferRecvComplete(int result) {
   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;
 }