Fix instability in SSL client/server sockets

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index e827f398db65eb56a946e5c1ed286f455ccccb12..52e588b104c8c21bd682c97efc894e9dfd0bc8b2 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -1188,14 +1188,12 @@ void SSLClientSocketNSS::OnSendComplete(int result) {
   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);
@@ -1317,11 +1315,12 @@ int SSLClientSocketNSS::DoWriteLoop(int result) {
   }
 
   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;
@@ -1742,7 +1741,7 @@ bool SSLClientSocketNSS::DoTransportIO() {
   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;