Prevent the infinite loop inside SSLClientSocketNSS::OnSendComplete.

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
===================================================================
--- net/socket/ssl_client_socket_nss.cc	(revision 137212)
+++ net/socket/ssl_client_socket_nss.cc	(working copy)
@@ -430,6 +430,7 @@
                                        const SSLClientSocketContext& context)
     : transport_send_busy_(false),
       transport_recv_busy_(false),
+      transport_eof_(false),
       transport_(transport_socket),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
@@ -633,6 +634,7 @@
   user_write_callback_.Reset();
   transport_send_busy_   = false;
   transport_recv_busy_   = false;
+  transport_eof_         = false;
   user_read_buf_         = NULL;
   user_read_buf_len_     = 0;
   user_write_buf_        = NULL;
@@ -1219,6 +1221,7 @@
       network_moved = DoTransportIO();
   } while (rv_read == ERR_IO_PENDING &&
            rv_write == ERR_IO_PENDING &&
+           (user_read_buf_ || user_write_buf_) &&

[Ryan Sleevi, 2012/05/16 02:13:03]

Double checking my understanding here.

Pre-condition: The only time |user_read_buf_| and |user_write_buf_| will both be
NULL is during the Connect() phase, with the SSL handshake.

For SSL handshakes, those are handled above at line 1206.

DoPayloadRead() / DoPayloadWrite() simply push the two buffers from their
IOBuffers to the NSS memio functions. They do not invoke the respective
callbacks (That is, they do not call DoWriteCallback/DoReadCallback)

As such, user_read_buf_ / user_write_buf_ are never cleared/NULLed.

So it's not clear how this will ever fail, so I wasn't sure how this helps abort
the loop?

[wtc, 2012/05/16 03:04:29]

This turns out to be false.  While investigating this bug,
I was surprised to find out that SSLClientSocketNSS::Write()
returns immediately after writing the encrypted data to the
memio buffer, without waiting for the write to the transport
socket to be completed.

So |user_read_buf_| and |user_write_buf_| can also be both
NULL after a successful Write() call.

[Ryan Sleevi, 2012/05/16 03:44:11]

Ok, I think I've reasoned about this check. It still seems like it's not
strictly necessary, but I believe I've convinced myself it's not /harmful/
either :)

Below are my notes to make sure I understand (and if ever someone else comes
along wondering...)

1) SSLClientSocketNSS::Write is called.
2) Write calls DoWriteLoop(OK)
3) DoWriteLoop() calls DoPayloadWrite()
4) DoPayloadWrite() is able to PR_Write all of |user_write_buf_| into the NSS
memio. We'll say it's a 40 byte record.
5) DoWriteLoop() calls DoTransportIO()
6) DoTransportIO() calls BufferSend(), but the underlying transport socket queue
is full, returning ERR_IO_PENDING.

On completion of the underlying write, BufferSendComplete is called. Let's say
the OS only accepted 20 bytes. This is stored in the memio so that it will
advance it's circular buffer - leaving 20 bytes remaining to write.

BufferSendComplete calls OnSendComplete (1973).

Because this is not a handshake, both user_read_buf_ and user_write_buf_ are
NULL. Both rv_read and rv_write are initialized to ERR_IO_PENDING, and we enter
this loop.

Only DoTransportIO is called. DoTransportIO calls BufferSend() in a loop as long
as it keeps succeeding. If we write any bytes, |network_moved| is true (line
1923 - 1927). The only way to exit that loop is to run out of data to write (we
wrote it all successfully), to fill up the OS buffer (and thus we're pending and
OnSendComplete will be called at a later point), or to error out.

If there is an error, it's quietly silenced until the next caller to Write(), at
which point they'll receive the error when they call DoPayloadWrite() and it's
read from the NSS memio buffer.

In a purely send case, the first call to DoTransportIO() will return true, but
the second should return false (there's nothing to send and nothing to read). So
we should end up only making two total iterations - making this check
unnecessary?

The only way for |network_moved| to be true for the second or subsequent times,
after having drained the entire send buffer, is if there's continually new data
to be read from the socket into the NSS buffer. Since we only read during a
handshake or a call to Read(), and because we do store |user_read_buf_| during
deferred reads, we should not hit this, meaning this check is unnecessary?

I do understand now, at least with the original implementation always returning
true for read eof, how this would end up in an infinite loop for the second and
subsequent iterations, where both |user_read_buf_| and |user_send_buf_| were
NULL. I think your EOF check sufficiently covers this case, making this check
unnecessary, but like I said, I believe I've convinced myself it's not a bad
check either. We'll have always drained the send buffer, or we'll re-enter this
function later (if send was blocked), so we should never end up starving our
write side by early terminating when both (user_read_buf_, user_write_buf_) are
NULL.

[wtc, 2012/05/16 17:51:52]

You are right that the (user_read_buf_ || user_write_buf_)
check I added here is not strictly necessary.  This check
is the first fix in the CL's description, quoted here:

  Two fixes are added. 1) We stay in the loop only if we will call
  DoPayloadRead or DoPayloadWrite in the next iteration. 2) Don't call
  BufferRecv again if BufferRecv has reported EOF before.

  Each fix alone prevents the infinite loop. The second fix is less risky.
  If necessary, we can go with just the second fix.
This is correct.  I hope the CL's description I quoted above
describes this accurately.

            network_moved);
 
   if (user_read_buf_ && rv_read != ERR_IO_PENDING)
@@ -1922,7 +1925,9 @@
       if (rv > 0)
         network_moved = true;
     } while (rv > 0);
-    if (BufferRecv() >= 0)
+    bool reached_eof = transport_eof_;
+    rv = BufferRecv();

[Ryan Sleevi, 2012/05/16 02:13:03]

If we'd previously reached eof, why call BufferRecv? Isn't that an invalid state
for a socket?

I'm guessing the only way we can reach this state is when the peer has closed
down their send side but we're still flushing data on our send side (eg: a
close_notify, if we sent them)

It's probably fine, and no different than what we did today, but it seems worth
nothing.

[wtc, 2012/05/16 03:04:29]

Ah, you're right.  Not calling BufferRecv if we'd previously
reached eof is a better fix.
Yes.  The data on our send side can be close_notify or
actual encrypted data.
The idea is to prevent us from setting network_moved to
true repeatedly if BufferRecv() keeps returning 0.  If
network_moved is false, the do-while loop in OnSendComplete
will terminate.

+    if (rv > 0 || (!reached_eof && rv == 0))
       network_moved = true;
   }
   LeaveFunction(network_moved);
@@ -1990,8 +1995,11 @@
     if (rv == ERR_IO_PENDING) {
       transport_recv_busy_ = true;
     } else {
-      if (rv > 0)
+      if (rv > 0) {
         memcpy(buf, recv_buffer_->data(), rv);
+      } else if (rv == 0) {
+        transport_eof_ = true;
+      }
       memio_PutReadResult(nss_bufs_, MapErrorToNSS(rv));
       recv_buffer_ = NULL;
     }
@@ -2006,6 +2014,8 @@
     char* buf;
     memio_GetReadParams(nss_bufs_, &buf);
     memcpy(buf, recv_buffer_->data(), result);
+  } else if (result == 0) {
+    transport_eof_ = true;
   }
   recv_buffer_ = NULL;
   memio_PutReadResult(nss_bufs_, MapErrorToNSS(result));