Prevent the infinite loop inside SSLClientSocketNSS::OnSendComplete.

Prevent the infinite loop inside SSLClientSocketNSS::OnSendComplete.

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.

R=rsleevi@chromium.org
BUG=127822
TEST=SSLServerSocketTest.WriteAfterPeerClose in net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137485

[wtc, 2012-05-15 23:24:24]

rsleevi: please review the entire CL.

hclam: please review just the changes to the FakeDataChannel
class in ssl_server_socket_unittest.cc because you are the
original author of that class.

jnd: just FYI.  I believe SSLClientSocketOpenSSL needs the
same fix.

[wtc, 2012-05-16 00:36:53]

jnd: I found that SSLServerSocketOpenSSL isn't implemented,
so I can't run my regression test with the use_openssl=1
build.  I ported my fix to SSLClientSocketOpenSSL.  Please
review.

[Ryan Sleevi, 2012-05-16 02:13:02]

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1224: (user_read_buf_ || user_write_buf_) &&
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?

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1929: rv = BufferRecv();
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.

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.h:217: bool transport_eof_;
nit: Call this transport_recv_eof_, since |transport_| is full duplex, but we're
only checking read eof here.

[Ryan Sleevi, 2012-05-16 02:16:46]

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:479: TEST_F(SSLServerSocketTest,
WriteAfterPeerClose) {
Just to make sure - this is really testing the behaviour of the SSLClientSocket,
but it's here as an SSLServerSocketTest because the ability to avoid network IO
(the FakeDataChannel) while still having packets successfully encrypt/decrypt
with SSL (which is why we can't use the SocketDataProvider)

I agree with the approach - avoiding the test server = great - but am just
concerned about the naming/documentation, since it's not really testing the
server impl.

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

rsleevi: please review patch set 4.  Thank you for your
comments.  I made all the changes you suggested.  I answer
your questions below.

jnd: I decided to fix SSLClientSocketOpenSSL separately
(including porting http://codereview.chromium.org/7399025)
after the SSLClientSocketNSS bug fix is final.

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1224: (user_read_buf_ || user_write_buf_) &&

On 2012/05/16 02:13:03, Ryan Sleevi wrote:
> 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.

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.

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1929: rv = BufferRecv();

On 2012/05/16 02:13:03, Ryan Sleevi wrote:
> If we'd previously reached eof, why call BufferRecv? Isn't that an invalid
state
> for a socket?

Ah, you're right.  Not calling BufferRecv if we'd previously
reached eof is a better fix.

> 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)

Yes.  The data on our send side can be close_notify or
actual encrypted data.

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

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.

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:479: TEST_F(SSLServerSocketTest,
WriteAfterPeerClose) {

On 2012/05/16 02:16:46, Ryan Sleevi wrote:
> Just to make sure - this is really testing the behaviour of the
SSLClientSocket,
> but it's here as an SSLServerSocketTest because the ability to avoid network
IO
> (the FakeDataChannel) while still having packets successfully encrypt/decrypt
> with SSL (which is why we can't use the SocketDataProvider)

Yes.  I added a comment to clarify this.

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

lgtm

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1224: (user_read_buf_ || user_write_buf_) &&
On 2012/05/16 03:04:29, wtc wrote:
> 
> On 2012/05/16 02:13:03, Ryan Sleevi wrote:
> > 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.
> 
> 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.

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]

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10382186/diff/1009/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1224: (user_read_buf_ || user_write_buf_) &&

On 2012/05/16 03:44:11, Ryan Sleevi wrote:
> 
> 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 :)

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.

> 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.

This is correct.  I hope the CL's description I quoted above
describes this accurately.

[commit-bot: I haz the power, 2012-05-16 18:00:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/10382186/10006

[Alpha Left Google, 2012-05-16 19:31:10]

LGTM on the test.

[commit-bot: I haz the power, 2012-05-16 19:36:33]

Change committed as 137485