Change the SSL Socket to be capable of having reads and...

Change the SSL Socket to be capable of having reads and
writes active concurrently.

BUG=none
TEST=none


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

[Mike Belshe, 2009-09-23 12:42:47]

Wan-Teh -

I'd like to add more tests with this change.  The current unit tests all pass,
but I worry about things like SSL renegotiation.  I'm not sure if it is being
adequately tested.

If you've got advice on how I can do some of these things, that would be super.

Mike

[wtc, 2009-09-28 21:22:16]

Mike, thanks for the CL and sorry it took me so long to
review it.

I suspect that we may have a less intrusive CL if we
split the current state machine into three:

1. next_handshake_state_ or next_connect_state_: for the connect/handshake
sequence

2. next_read_state_: for payload read, after handshake is completed

3. next_write_state: for payload write, after handshake is completed.

Another complication is the renegotiation handshake, which occurs during a
payload read.  We would need to jump to the handshake sequence, and suspend
payload read (easy) and
payload write (needs care) while renegotiation is in
progress.

http://codereview.chromium.org/225005/diff/1001/1003
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/225005/diff/1001/1003#newcode292
Line 292: #pragma warning(suppress: 4355)
Please remove this line, which is replaced by
ALLOW_THIS_IN_INITIALIZER_LIST.

http://codereview.chromium.org/225005/diff/1001/1003#newcode605
Line 605: void SSLClientSocketWin::NotifyConnectComplete(int result) {
Let's name this function DoConnectCallback to be
consistent with the DoReadCallback and DoWriteCallback
functions in tcp_client_socket_win.{h,cc}.

http://codereview.chromium.org/225005/diff/1001/1003#newcode732
Line 732: return transport_->Read(transport_read_buf_, buf_len,
&read_callback_);
We should pass connect_callback_ instead of read_callback_
here.  ("connect" really means "handshake" for SSL.)
Then we won't need the special case for
next_state_ != STATE_HANDSHAKE_COMPLETE at the beginning
of OnReadComplete.

http://codereview.chromium.org/225005/diff/1001/1003#newcode885
Line 885: return transport_->Write(transport_write_buf_, buf_len,
&write_callback_);
Same comment: we should pass connect_callback_ instead of
write_callback_ here. Then we won't need the special case for
next_state_ != STATE_HANDSHAKE_COMPLETE at the beginning
of OnWriteComplete.

http://codereview.chromium.org/225005/diff/1001/1002
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/225005/diff/1001/1002#newcode56
Line 56: void OnConnectComplete(int result);
I believe that we can combine OnConnectComplete and
OnVerifyComplete into one function.  Not sure what the
function should be... OnHandshakeIOComplete?

http://codereview.chromium.org/225005/diff/1001/1002#newcode90
Line 90: // User function to callback when the connect completes.
We can just use user_read_callback_ for connect.
We can use read_callback_ for connect/verify.  This is
what we do in tcp_client_socket_win.{h,cc}.

http://codereview.chromium.org/225005/diff/1001/1002#newcode115
Line 115: STATE_HANDSHAKE_COMPLETE
We don't need the STATE_HANDSHAKE_COMPLETE state because
that state is represented by the completed_handshake_
member being true.

If you think it is clearer for next_state_ to be in this
state when the handshake is completed, let's rename this
state STATE_COMPLETED_HANDSHAKE so that a case-insensitive
search for "completed_handshake" will find both
STATE_COMPLETED_HANDSHAKE and completed_handshake_.

We can also remove the completed_handshake_ member and
replace it with next_state_ == STATE_COMPLETED_HANDSHAKE.

http://codereview.chromium.org/225005/diff/1001/1002#newcode170
Line 170: // True when the decoder needs more data in order to decode.
Nit: decoder => decrypter
     decode => decrypt

[Mike Belshe, 2009-09-28 22:22:08]

http://codereview.chromium.org/225005/diff/1001/1003
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/225005/diff/1001/1003#newcode292
Line 292: #pragma warning(suppress: 4355)
On 2009/09/28 21:22:16, wtc wrote:
> Please remove this line, which is replaced by
> ALLOW_THIS_IN_INITIALIZER_LIST.

Done.

http://codereview.chromium.org/225005/diff/1001/1003#newcode605
Line 605: void SSLClientSocketWin::NotifyConnectComplete(int result) {
Since it is only called from one place now, it is removed.

On 2009/09/28 21:22:16, wtc wrote:
> Let's name this function DoConnectCallback to be
> consistent with the DoReadCallback and DoWriteCallback
> functions in tcp_client_socket_win.{h,cc}.

http://codereview.chromium.org/225005/diff/1001/1003#newcode732
Line 732: return transport_->Read(transport_read_buf_, buf_len,
&read_callback_);
On 2009/09/28 21:22:16, wtc wrote:
> We should pass connect_callback_ instead of read_callback_
> here.  ("connect" really means "handshake" for SSL.)
> Then we won't need the special case for
> next_state_ != STATE_HANDSHAKE_COMPLETE at the beginning
> of OnReadComplete.

Done.

http://codereview.chromium.org/225005/diff/1001/1003#newcode885
Line 885: return transport_->Write(transport_write_buf_, buf_len,
&write_callback_);
On 2009/09/28 21:22:16, wtc wrote:
> Same comment: we should pass connect_callback_ instead of
> write_callback_ here. Then we won't need the special case for
> next_state_ != STATE_HANDSHAKE_COMPLETE at the beginning
> of OnWriteComplete.

Done.

http://codereview.chromium.org/225005/diff/1001/1002
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/225005/diff/1001/1002#newcode56
Line 56: void OnConnectComplete(int result);
OK - called it "ConnectIOComplete".

On 2009/09/28 21:22:16, wtc wrote:
> I believe that we can combine OnConnectComplete and
> OnVerifyComplete into one function.  Not sure what the
> function should be... OnHandshakeIOComplete?

http://codereview.chromium.org/225005/diff/1001/1002#newcode90
Line 90: // User function to callback when the connect completes.
I find that I like the separate fields - it seems more explicit?  It is true
that I waste 4 bytes, I guess.

On 2009/09/28 21:22:16, wtc wrote:
> We can just use user_read_callback_ for connect.
> We can use read_callback_ for connect/verify.  This is
> what we do in tcp_client_socket_win.{h,cc}.

http://codereview.chromium.org/225005/diff/1001/1002#newcode115
Line 115: STATE_HANDSHAKE_COMPLETE
Good idea.  Removed compelted_handshake_ in favor of completed_handshake(), and
used that everywhere.

On 2009/09/28 21:22:16, wtc wrote:
> We don't need the STATE_HANDSHAKE_COMPLETE state because
> that state is represented by the completed_handshake_
> member being true.
> 
> If you think it is clearer for next_state_ to be in this
> state when the handshake is completed, let's rename this
> state STATE_COMPLETED_HANDSHAKE so that a case-insensitive
> search for "completed_handshake" will find both
> STATE_COMPLETED_HANDSHAKE and completed_handshake_.
> 
> We can also remove the completed_handshake_ member and
> replace it with next_state_ == STATE_COMPLETED_HANDSHAKE.

http://codereview.chromium.org/225005/diff/1001/1002#newcode170
Line 170: // True when the decoder needs more data in order to decode.
On 2009/09/28 21:22:16, wtc wrote:
> Nit: decoder => decrypter
>      decode => decrypt

Done.

[wtc, 2009-10-02 17:58:30]

I made a pass through the CL.  I reviewed all the code
carefully except the DoPayloadRead() and
DoPayloadReadComplete() functions because I think we should
redo that part of the CL.

The most serious bug of the CL is that we don't go back to
reading payload data after a renegotiation handshake is
completed.  The next serious bug is that we don't keep
writing (encrypted) payload data after a partial write.
All the other issues are minor bugs or nits.

Because of the two serious bugs, I recommend that we
have two state machines (instead of the three that I
suggested last time):
1. The original state machine should be used for Connect()
and Read().  This allows us to handle renegotiation (a form
of Connect()) in the middle of a Read().
2. A new state machine should be used for Write().  This
allows us to handle partial writes.

http://codereview.chromium.org/225005/diff/5002/6002
File net/socket/ssl_client_socket_win.cc (left):

http://codereview.chromium.org/225005/diff/5002/6002#oldcode480
Line 480: completed_handshake_ = false;
This should be replaced by
  next_state_ = STATE_NONE;
because next_state_ stays in the STATE_COMPLETED_HANDSHAKE
state after the handshake now.

http://codereview.chromium.org/225005/diff/5002/6002#oldcode1176
Line 1176: void SSLClientSocketWin::DidCompleteRenegotiation(int result) {
This is a bug.  We can't simply replace
DidCompleteRenegotiation() "renegotiating_ = false" because
we are no longer going back to reading data after the
renegotiation is done.

http://codereview.chromium.org/225005/diff/5002/6002
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/225005/diff/5002/6002#newcode489
Line 489: // Shut down anything that may call us back through our callbacks.
Nit: we can remove "through our callbacks".

http://codereview.chromium.org/225005/diff/5002/6002#newcode508
Line 508: transport_read_buf_ = NULL;
I don't think we need to reset these members here.
In particular, user_read_buf_ and user_write_buf_ should
have been reset when Read and Write complete.
transport_read_buf_ and transport_write_buf_ could be
reused if this socket is ever reconnected.

If you really want to reset these members, please also
reset user_read_buf_len_ and user_write_buf_len_ to 0.

http://codereview.chromium.org/225005/diff/5002/6002#newcode565
Line 565: int rv = DoPayloadRead();
The Read and Write methods should have symmetry.  Now
Read calls only DoPayloadRead, but Write calls both
DoPayloadWrite and DoPayloadWriteComplete.  We should
eliminate this inconsistency.

http://codereview.chromium.org/225005/diff/5002/6002#newcode585
Line 585: rv = this->DoPayloadWrite();
Delete this->.

http://codereview.chromium.org/225005/diff/5002/6002#newcode590
Line 590: user_write_buf_ = NULL;
Let's also do
    user_write_buf_len_ = 0;

http://codereview.chromium.org/225005/diff/5002/6002#newcode605
Line 605: // After connect, SSL has some round trips.  It shouldn't
This sentence is a little misleading because SSLClientSocket
doesn't do (TCP) connect.  How about just
  SSL handshake has some roundtrips.

http://codereview.chromium.org/225005/diff/5002/6002#newcode607
Line 607: // then we've failed and need to notify the caller.
This comment isn't accurate.  rv can also be OK, which
means the handshake has completed successfully.

http://codereview.chromium.org/225005/diff/5002/6002#newcode619
Line 619: // Since we've completed a read, reset the need_more_data_ flag.
Lines 619-632 should be part of DoPayloadReadComplete().

http://codereview.chromium.org/225005/diff/5002/6002#newcode631
Line 631: result = OK;
These three lines don't look right.  In particular,
the final "result = OK" statement reduces lines 623-631 to
  if (results <= 0) {
    transport_read_buf_ = NULL;
    result = OK;

So we lose the error code.  That's clearly wrong.

http://codereview.chromium.org/225005/diff/5002/6002#newcode640
Line 640: user_read_buf_ = NULL;
Let's also do
    user_read_buf_len_ = 0;

http://codereview.chromium.org/225005/diff/5002/6002#newcode650
Line 650: DCHECK(user_connect_callback_);
We should DCHECK user_write_callback_ here.

I'm surprised that this DCHECK doesn't fail.

http://codereview.chromium.org/225005/diff/5002/6002#newcode653
Line 653: user_write_buf_ = NULL;
Let's also do
    user_write_buf_len_ = 0;

http://codereview.chromium.org/225005/diff/5002/6002#newcode683
Line 683: return DoVerifyCertComplete(rv);
We shouldn't need to make a special case for this state.

Let's eliminate the STATE_COMPLETED_HANDSHAKE state and
go back to using the completed_handshake_ bool member.

http://codereview.chromium.org/225005/diff/5002/6002#newcode930
Line 930: renegotiating_ = false;
We still need to go back to reading payload data...

http://codereview.chromium.org/225005/diff/5002/6002#newcode939
Line 939: int SSLClientSocketWin::DoPayloadRead() {
I find the mutual recursion between DoPayloadRead() and
DoPayloadReadComplete() a little hard to understand.
It's also a little surprising that DoPayloadRead() doesn't
call transport_->Read() under some conditions.

http://codereview.chromium.org/225005/diff/5002/6002#newcode957
Line 957: if (rv <= 0) {
Add:
    if (rv == 0) {
      // TODO(wtc): Unless we have received the close_notify alert, we need to
      // return an error code indicating that the SSL connection ended
      // uncleanly, a potential truncation attack.  See http://crbug.com/18586.
      if (bytes_received_ != 0)
        rv = ERR_SSL_PROTOCOL_ERROR;
    }

http://codereview.chromium.org/225005/diff/5002/6002#newcode964
Line 964: need_more_data_ = false;
Move this into DoPayloadReadComplete().

http://codereview.chromium.org/225005/diff/5002/6002#newcode970
Line 970: user_read_buf_ = NULL;
Let's also do
  user_read_buf_len_ = 0;

http://codereview.chromium.org/225005/diff/5002/6002#newcode1089
Line 1089: // We've alraedy copied data into the user buffer, so quit now.
Typo: alraedy

http://codereview.chromium.org/225005/diff/5002/6002#newcode1090
Line 1090: // TODO(mbelshe): We really should keep decoding as long as we can. 
This
So you added a while loop but didn't finish the work?
Not complaining, just wanted to make sure I understand the
code.

http://codereview.chromium.org/225005/diff/5002/6002#newcode1191
Line 1191: // Send the remaining bytes.
This is a bug.  The original code goes back to DoPayloadWrite
by setting next_state_ to STATE_PAYLOAD_WRITE.  This looping
is lost in the new code.

http://codereview.chromium.org/225005/diff/5002/6001
File net/socket/ssl_client_socket_win.h (left):

http://codereview.chromium.org/225005/diff/5002/6001#oldcode56
Line 56: void DoCallback(int result);
I wonder if we should have
  DoConnectCallback
  DoReadCallback
  DoWriteCallback

http://codereview.chromium.org/225005/diff/5002/6001
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/225005/diff/5002/6001#newcode85
Line 85: CompletionCallbackImpl<SSLClientSocketWin> write_callback_;
Nit: add a blank line below.

http://codereview.chromium.org/225005/diff/5002/6001#newcode117
Line 117: // in the STATE_COMPLETED_HANDSHAKE state.
Should we add "until it needs to do a renegotiation
handshake"?

[Mike Belshe, 2009-10-02 20:12:35]

http://codereview.chromium.org/225005/diff/5002/6002
File net/socket/ssl_client_socket_win.cc (left):

http://codereview.chromium.org/225005/diff/5002/6002#oldcode480
Line 480: completed_handshake_ = false;
On 2009/10/02 17:58:30, wtc wrote:
> This should be replaced by
>   next_state_ = STATE_NONE;
> because next_state_ stays in the STATE_COMPLETED_HANDSHAKE
> state after the handshake now.

Done.

http://codereview.chromium.org/225005/diff/5002/6002
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/225005/diff/5002/6002#newcode489
Line 489: // Shut down anything that may call us back through our callbacks.
On 2009/10/02 17:58:30, wtc wrote:
> Nit: we can remove "through our callbacks".

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode508
Line 508: transport_read_buf_ = NULL;
On 2009/10/02 17:58:30, wtc wrote:
> I don't think we need to reset these members here.
> In particular, user_read_buf_ and user_write_buf_ should
> have been reset when Read and Write complete.
> transport_read_buf_ and transport_write_buf_ could be
> reused if this socket is ever reconnected.
> 
> If you really want to reset these members, please also
> reset user_read_buf_len_ and user_write_buf_len_ to 0.

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode565
Line 565: int rv = DoPayloadRead();
On 2009/10/02 17:58:30, wtc wrote:
> The Read and Write methods should have symmetry.  Now
> Read calls only DoPayloadRead, but Write calls both
> DoPayloadWrite and DoPayloadWriteComplete.  We should
> eliminate this inconsistency.

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode585
Line 585: rv = this->DoPayloadWrite();
On 2009/10/02 17:58:30, wtc wrote:
> Delete this->.

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode590
Line 590: user_write_buf_ = NULL;
On 2009/10/02 17:58:30, wtc wrote:
> Let's also do
>     user_write_buf_len_ = 0;

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode605
Line 605: // After connect, SSL has some round trips.  It shouldn't
On 2009/10/02 17:58:30, wtc wrote:
> This sentence is a little misleading because SSLClientSocket
> doesn't do (TCP) connect.  How about just
>   SSL handshake has some roundtrips.

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode607
Line 607: // then we've failed and need to notify the caller.
reworded.

On 2009/10/02 17:58:30, wtc wrote:
> This comment isn't accurate.  rv can also be OK, which
> means the handshake has completed successfully.

http://codereview.chromium.org/225005/diff/5002/6002#newcode619
Line 619: // Since we've completed a read, reset the need_more_data_ flag.
On 2009/10/02 17:58:30, wtc wrote:
> Lines 619-632 should be part of DoPayloadReadComplete().

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode631
Line 631: result = OK;
good catch - it should be (I think)
  return ERR_SSL_PROTOCOL_ERROR;

The "close_notify alert" was unclear to me from reading the code- I have no idea
what it is.

On 2009/10/02 17:58:30, wtc wrote:
> These three lines don't look right.  In particular,
> the final "result = OK" statement reduces lines 623-631 to
>   if (results <= 0) {
>     transport_read_buf_ = NULL;
>     result = OK;
> 
> So we lose the error code.  That's clearly wrong.

http://codereview.chromium.org/225005/diff/5002/6002#newcode640
Line 640: user_read_buf_ = NULL;
On 2009/10/02 17:58:30, wtc wrote:
> Let's also do
>     user_read_buf_len_ = 0;

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode650
Line 650: DCHECK(user_connect_callback_);
On 2009/10/02 17:58:30, wtc wrote:
> We should DCHECK user_write_callback_ here.
> 
> I'm surprised that this DCHECK doesn't fail.

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode653
Line 653: user_write_buf_ = NULL;
On 2009/10/02 17:58:30, wtc wrote:
> Let's also do
>     user_write_buf_len_ = 0;

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode683
Line 683: return DoVerifyCertComplete(rv);
I don't like this idea.  That's just splitting the state across two members. 
When the state is STATE_NONE, the reader has no way of knowing, "oh, if the
completed_handshake_ flag is true, then the state is irrelevant".  I'd rather
figure out how to make the state machine complete than make it somehow fit a
particular while loop.


On 2009/10/02 17:58:30, wtc wrote:
> We shouldn't need to make a special case for this state.
> 
> Let's eliminate the STATE_COMPLETED_HANDSHAKE state and
> go back to using the completed_handshake_ bool member.

http://codereview.chromium.org/225005/diff/5002/6002#newcode930
Line 930: renegotiating_ = false;
On 2009/10/02 17:58:30, wtc wrote:
> We still need to go back to reading payload data...

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode939
Line 939: int SSLClientSocketWin::DoPayloadRead() {
DoPayloadRead does call transport_->Read(), so I don't quite understand the
concern?

On 2009/10/02 17:58:30, wtc wrote:
> I find the mutual recursion between DoPayloadRead() and
> DoPayloadReadComplete() a little hard to understand.
> It's also a little surprising that DoPayloadRead() doesn't
> call transport_->Read() under some conditions.

http://codereview.chromium.org/225005/diff/5002/6002#newcode957
Line 957: if (rv <= 0) {
On 2009/10/02 17:58:30, wtc wrote:
> Add:
>     if (rv == 0) {
>       // TODO(wtc): Unless we have received the close_notify alert, we need to
>       // return an error code indicating that the SSL connection ended
>       // uncleanly, a potential truncation attack.  See
http://crbug.com/18586.
>       if (bytes_received_ != 0)
>         rv = ERR_SSL_PROTOCOL_ERROR;
>     }

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode964
Line 964: need_more_data_ = false;
I don't like this.  Read completion occurs either when the read was satisfied
instantly (this case) or when the io callback is made.  Callers *should* call
DoPayloadReadComplete, but it seems like this flag is specifically about the IO
completing, not about DoPayloadReadComplete being called.

On 2009/10/02 17:58:30, wtc wrote:
> Move this into DoPayloadReadComplete().

http://codereview.chromium.org/225005/diff/5002/6002#newcode970
Line 970: user_read_buf_ = NULL;
On 2009/10/02 17:58:30, wtc wrote:
> Let's also do
>   user_read_buf_len_ = 0;

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode1089
Line 1089: // We've alraedy copied data into the user buffer, so quit now.
On 2009/10/02 17:58:30, wtc wrote:
> Typo: alraedy

Done.

http://codereview.chromium.org/225005/diff/5002/6002#newcode1090
Line 1090: // TODO(mbelshe): We really should keep decoding as long as we can. 
This
Yes, that's basically right; the while loop was part of the original
refactoring; the comment was done separately.  There is more work to do to make
the looping work right.

On 2009/10/02 17:58:30, wtc wrote:
> So you added a while loop but didn't finish the work?
> Not complaining, just wanted to make sure I understand the
> code.

http://codereview.chromium.org/225005/diff/5002/6002#newcode1191
Line 1191: // Send the remaining bytes.
On 2009/10/02 17:58:30, wtc wrote:
> This is a bug.  The original code goes back to DoPayloadWrite
> by setting next_state_ to STATE_PAYLOAD_WRITE.  This looping
> is lost in the new code.

Done.

http://codereview.chromium.org/225005/diff/5002/6001
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/225005/diff/5002/6001#newcode85
Line 85: CompletionCallbackImpl<SSLClientSocketWin> write_callback_;
On 2009/10/02 17:58:30, wtc wrote:
> Nit: add a blank line below.

Done.

http://codereview.chromium.org/225005/diff/5002/6001#newcode117
Line 117: // in the STATE_COMPLETED_HANDSHAKE state.
On 2009/10/02 17:58:30, wtc wrote:
> Should we add "until it needs to do a renegotiation
> handshake"?

Done.

[wtc, 2009-10-08 01:32:02]

You're now using mutual recursion for both Read and Write.
I still think mutual recursion is harder to understand and
could result in a long stack in rare cases (for example, if
we can only send a few bytes at a time), but let's give this
a try.

The jumps between Read and renegotiation are a little hard
to follow.  We seem to pay a high price in code clarify
for not merging Read into the handshake state loop.

http://codereview.chromium.org/225005/diff/11001/12002
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/225005/diff/11001/12002#newcode603
Line 603: // If there is no callback available to call, it had better
Nit: no callback => no connect callback.

http://codereview.chromium.org/225005/diff/11001/12002#newcode604
Line 604: // be because we are renegotiating.  We need to inform the
Add ", which occurs in the middle of a Read" after "renegotiating".

http://codereview.chromium.org/225005/diff/11001/12002#newcode605
Line 605: // caller of the SSL error, so we complete the read here.
Nit: SSL error => renegotiation error

Nit: capitalize Read to suggest it's the Read method.

http://codereview.chromium.org/225005/diff/11001/12002#newcode607
Line 607: DCHECK(renegotiating_);
Should we reset renegotiating_ to false?

http://codereview.chromium.org/225005/diff/11001/12002#newcode626
Line 626: if (!completed_handshake()) {
Is it better to test the renegotiating_ flag instead?

http://codereview.chromium.org/225005/diff/11001/12002#newcode627
Line 627: // If handshake renegotiation occurred, we need to go back
Nit: handshake renegotiation => renegotiation handshake

http://codereview.chromium.org/225005/diff/11001/12002#newcode629
Line 629: OnHandshakeIOComplete(result);
Should we DCHECK that result == OK?

http://codereview.chromium.org/225005/diff/11001/12002#newcode935
Line 935: DidCompleteRenegotiation(result);
DidCompleteRenegotiation sets next_state_ to
STATE_COMPLETED_RENOGOTIATION, but that is immediately
overwritten by line 938 below.  Should we add a return
statement here?

http://codereview.chromium.org/225005/diff/11001/12002#newcode937
Line 937: // The initial handshake, kicked off by a Connect, has completed.
The comment "initial handshake, kicked off by a Connect" is
no longer true in the new code because we can also get here
with a renegotiation.

http://codereview.chromium.org/225005/diff/11001/12002#newcode961
Line 961: if (rv <= 0) {
A negative or zero rv should be handled by DoPayloadReadComplete.  The reason is
that transport_->Read()
may complete asynchronously with a negative or zero result,
and in OnReadComplete you directly pass result to DoPayloadReadComplete.

If you still want to handle rv <= 0 here, lines 962- 971
should look like this:

  if (rv == 0 && bytes_received_ != 0) {
    // TODO(wtc): ...
    rv = ERR_SSL_PROTOCOL_ERROR;
  }
  if (rv != ERR_IO_PENDING)
    transport_read_buf_ = NULL;

http://codereview.chromium.org/225005/diff/11001/12002#newcode982
Line 982: if (rv != ERR_IO_PENDING) {
Move lines 982-985 to the end of Read() method as follows:

  if (rv == ERR_IO_PENDING) {
    user_read_callback_ = callback;
  } else {
    user_read_buf_ = NULL;
    user_read_buf_len_ = 0;
  }

http://codereview.chromium.org/225005/diff/11001/12002#newcode1183
Line 1183: user_write_buf_ = NULL;
Move these two lines to the end of Write(), like this:

  if (rv == ERR_IO_PENDING) {
    user_write_callback_ = callback;
  } else {
    user_write_buf_ = NULL;
    user_write_buf_len_ = 0;
  }

http://codereview.chromium.org/225005/diff/11001/12002#newcode1219
Line 1219: return DoPayloadReadComplete(result);
In the original code, we call either DoPayloadRead or
DoPayloadReadComplete depending on whether bytes_received_
is 0 or not (see SetNextStateForRead).

Now we always call DoPayloadReadComplete.  Is this correct?

http://codereview.chromium.org/225005/diff/11001/12002#newcode1254
Line 1254: void SSLClientSocketWin::DidCompleteRenegotiation(int result) {
You're not using the 'result' parameter.  Either delete it,
or there is a bug.

http://codereview.chromium.org/225005/diff/11001/12001
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/225005/diff/11001/12001#newcode76
Line 76: int DoRenegotiateComplete(int result);
DoRenegotiateComplete is easily confused with
DidCompleteRenegotiation.

Nit: Our convention is that STATE_FOO_BAR is associated with
the DoFooBar() method.  This method doesn't follow the
convention.

http://codereview.chromium.org/225005/diff/11001/12001#newcode85
Line 85: CompletionCallbackImpl<SSLClientSocketWin> connect_io_callback_;
Nit: rename this member handshake_io_callback_ to match
OnHandshakeIOComplete.

http://codereview.chromium.org/225005/diff/11001/12001#newcode93
Line 93: // User function to callback when the connect completes.
Nit: capitalize Connect to suggest it's the Connect method.
Same comment for Read on line 96 and Write on line 101 below.

http://codereview.chromium.org/225005/diff/11001/12001#newcode118
Line 118: STATE_COMPLETED_RENOGOTIATION,
Typo: "RENOG" => "RENEG" in STATE_COMPLETED_RENOGOTIATION

Do we really need to distinguish between the initial handshake
(STATE_COMPLETED_HANDSHAKE) and a renegotiation handshake
(STATE_COMPLETED_RENOGOTIATION)?

[Mike Belshe, 2009-10-08 21:03:23]

http://codereview.chromium.org/225005/diff/11001/12002
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/225005/diff/11001/12002#newcode603
Line 603: // If there is no callback available to call, it had better
On 2009/10/08 01:32:03, wtc wrote:
> Nit: no callback => no connect callback.

Done.

http://codereview.chromium.org/225005/diff/11001/12002#newcode604
Line 604: // be because we are renegotiating.  We need to inform the
On 2009/10/08 01:32:03, wtc wrote:
> Add ", which occurs in the middle of a Read" after "renegotiating".

Done.

http://codereview.chromium.org/225005/diff/11001/12002#newcode605
Line 605: // caller of the SSL error, so we complete the read here.
On 2009/10/08 01:32:03, wtc wrote:
> Nit: SSL error => renegotiation error
> 
> Nit: capitalize Read to suggest it's the Read method.

Done.

http://codereview.chromium.org/225005/diff/11001/12002#newcode607
Line 607: DCHECK(renegotiating_);
No - the DidCompleteRenegotiation() call takes care
of that now.

On 2009/10/08 01:32:03, wtc wrote:
> Should we reset renegotiating_ to false?

http://codereview.chromium.org/225005/diff/11001/12002#newcode627
Line 627: // If handshake renegotiation occurred, we need to go back
On 2009/10/08 01:32:03, wtc wrote:
> Nit: handshake renegotiation => renegotiation handshake

Done.

http://codereview.chromium.org/225005/diff/11001/12002#newcode629
Line 629: OnHandshakeIOComplete(result);
On 2009/10/08 01:32:03, wtc wrote:
> Should we DCHECK that result == OK?

Done.

http://codereview.chromium.org/225005/diff/11001/12002#newcode935
Line 935: DidCompleteRenegotiation(result);
Yes.  I wonder why this didn't break.

On 2009/10/08 01:32:03, wtc wrote:
> DidCompleteRenegotiation sets next_state_ to
> STATE_COMPLETED_RENOGOTIATION, but that is immediately
> overwritten by line 938 below.  Should we add a return
> statement here?

http://codereview.chromium.org/225005/diff/11001/12002#newcode937
Line 937: // The initial handshake, kicked off by a Connect, has completed.
On 2009/10/08 01:32:03, wtc wrote:
> The comment "initial handshake, kicked off by a Connect" is
> no longer true in the new code because we can also get here
> with a renegotiation.

Done.

http://codereview.chromium.org/225005/diff/11001/12002#newcode1219
Line 1219: return DoPayloadReadComplete(result);
It's ok.

DoPayloadReadComplete handles the case where there is not enough data (at the
end if len==0, it calls DoPayloadRead()).

On 2009/10/08 01:32:03, wtc wrote:
> In the original code, we call either DoPayloadRead or
> DoPayloadReadComplete depending on whether bytes_received_
> is 0 or not (see SetNextStateForRead).
> 
> Now we always call DoPayloadReadComplete.  Is this correct?

http://codereview.chromium.org/225005/diff/11001/12002#newcode1254
Line 1254: void SSLClientSocketWin::DidCompleteRenegotiation(int result) {
Deleted.
On 2009/10/08 01:32:03, wtc wrote:
> You're not using the 'result' parameter.  Either delete it,
> or there is a bug.

http://codereview.chromium.org/225005/diff/11001/12001
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/225005/diff/11001/12001#newcode76
Line 76: int DoRenegotiateComplete(int result);
Done.

note:  this convention was not recognized by me.  Do others recognize it without
comments that it exists? :-)

On 2009/10/08 01:32:03, wtc wrote:
> DoRenegotiateComplete is easily confused with
> DidCompleteRenegotiation.
> 
> Nit: Our convention is that STATE_FOO_BAR is associated with
> the DoFooBar() method.  This method doesn't follow the
> convention.

http://codereview.chromium.org/225005/diff/11001/12001#newcode85
Line 85: CompletionCallbackImpl<SSLClientSocketWin> connect_io_callback_;
On 2009/10/08 01:32:03, wtc wrote:
> Nit: rename this member handshake_io_callback_ to match
> OnHandshakeIOComplete.

Done.

http://codereview.chromium.org/225005/diff/11001/12001#newcode93
Line 93: // User function to callback when the connect completes.
On 2009/10/08 01:32:03, wtc wrote:
> Nit: capitalize Connect to suggest it's the Connect method.
> Same comment for Read on line 96 and Write on line 101 below.

Done.

http://codereview.chromium.org/225005/diff/11001/12001#newcode118
Line 118: STATE_COMPLETED_RENOGOTIATION,
On 2009/10/08 01:32:03, wtc wrote:
> Typo: "RENOG" => "RENEG" in STATE_COMPLETED_RENOGOTIATION
> 
> Do we really need to distinguish between the initial handshake
> (STATE_COMPLETED_HANDSHAKE) and a renegotiation handshake
> (STATE_COMPLETED_RENOGOTIATION)?

Done.

[wtc, 2009-10-09 19:23:47]

LGTM.  This CL can't get stuck in code reviews forever.
I think we should check it in and deal with the fallouts.

Breaking DoPayloadDecrypt() out of DoPayloadReadComplete()
is a great idea.  Unfortunately, we now have a three-way
mutual recursion.

The transition between Read and renegotiation is also
hard to understand.  With a caffeine boost, I still couldn't
check it in one setting.  Sorry.

http://codereview.chromium.org/225005/diff/17004/15008
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/225005/diff/17004/15008#newcode294
Line 294: &SSLClientSocketWin::OnHandshakeIOComplete)),
Nit: indentation of this line needs updating.

http://codereview.chromium.org/225005/diff/17004/15008#newcode565
Line 565: user_read_callback_ = callback;
Why don't we need an "else" block that resets user_read_buf_
and user_read_buf_len_?

The reason I keep pressing you on this issue is that this
part of Read() (when the Read completes synchronously) should
be identical to the part of OnReadComplete() right before
the c->Run() call (when the Read completes asynchronously).
But they're different.

http://codereview.chromium.org/225005/diff/17004/15008#newcode584
Line 584: user_write_callback_ = callback;
Why don't we need an "else" block that resets user_write_buf_
and user_write_buf_len_?

This part of Write() (when the Write completes synchronously)
should be identical to the part of OnWriteComplete() right
before the c->Run() call (when the Write completes
asynchronously).

http://codereview.chromium.org/225005/diff/17004/15008#newcode608
Line 608: CompletionCallback* c = user_read_callback_;
This part (lines 608-610) should be identical to lines
627-631 in OnReadComplete, because both are notifying the
caller of an asynchronous Read completion.

http://codereview.chromium.org/225005/diff/17004/15008#newcode623
Line 623: if (result >= 0)
Do we still need to decrypt the payload if result is 0?

http://codereview.chromium.org/225005/diff/17004/15008#newcode955
Line 955: if (rv < 0)
Do we still need to decrypt the payload if rv is 0?

http://codereview.chromium.org/225005/diff/17004/15008#newcode963
Line 963: user_read_buf_ = NULL;
I would just do this at the end of Read() and OnReadCompelte().

http://codereview.chromium.org/225005/diff/17004/15008#newcode976
Line 976: if (result == ERR_IO_PENDING)
This check violates an invariant of the original code:
we never pass ERR_IO_PENDING to a DoFooComplete() function,
because by definition the STATE_FOO_COMPLETE state means
the FOO operation has completed, so the result cannot be
ERR_IO_PENDING.

So we need to fix the code at line 954 to call
DoPayloadReadComplete() only if (rv != ERR_IO_PENDING).
See how you do that in DoPayloadWrite() at line 1190.

http://codereview.chromium.org/225005/diff/17004/15008#newcode989
Line 989: return ERR_SSL_PROTOCOL_ERROR;
Don't we also need to reset ransport_read_buf_ before
returning ERR_SSL_PROTOCOL_ERROR?

You may want to reset transport_read_buf_ first.

http://codereview.chromium.org/225005/diff/17004/15008#newcode1106
Line 1106: OnHandshakeIOComplete(OK);
I suggest that we just do
  return DoLoop(OK);
here.

http://codereview.chromium.org/225005/diff/17004/15008#newcode1192
Line 1192: user_write_buf_ = NULL;
I would just do this at the end of Write() and OnWriteCompelte().

http://codereview.chromium.org/225005/diff/17004/15007
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/225005/diff/17004/15007#newcode70
Line 70: int DoVerifyCertComplete(int result);
Nit: let's add a blank line here to suggest that the following
DoXXX functions are no longer part of the state machine.

http://codereview.chromium.org/225005/diff/17004/15007#newcode71
Line 71: int DoPayloadDecrypt();
Nit: list DoPayloadDecrypt after DoPayloadReadComplete,
which matches the order in which these functions are called.

[Mike Belshe, 2009-10-09 22:03:13]

http://codereview.chromium.org/225005/diff/17004/15008
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/225005/diff/17004/15008#newcode294
Line 294: &SSLClientSocketWin::OnHandshakeIOComplete)),
On 2009/10/09 19:23:47, wtc wrote:
> Nit: indentation of this line needs updating.

Done.

http://codereview.chromium.org/225005/diff/17004/15008#newcode565
Line 565: user_read_callback_ = callback;
On 2009/10/09 19:23:47, wtc wrote:
> Why don't we need an "else" block that resets user_read_buf_
> and user_read_buf_len_?

This is already done inside of DoPayloadRead().  If the result was not
ERR_IO_PENDING, DoPayloadRead() already zeroed these two.

As per discussion, I have done this.
> 
> The reason I keep pressing you on this issue is that this
> part of Read() (when the Read completes synchronously) should
> be identical to the part of OnReadComplete() right before
> the c->Run() call (when the Read completes asynchronously).
> But they're different.

http://codereview.chromium.org/225005/diff/17004/15008#newcode584
Line 584: user_write_callback_ = callback;
On 2009/10/09 19:23:47, wtc wrote:
> Why don't we need an "else" block that resets user_write_buf_
> and user_write_buf_len_?
> 
> This part of Write() (when the Write completes synchronously)
> should be identical to the part of OnWriteComplete() right
> before the c->Run() call (when the Write completes
> asynchronously).

Done.

http://codereview.chromium.org/225005/diff/17004/15008#newcode608
Line 608: CompletionCallback* c = user_read_callback_;
On 2009/10/09 19:23:47, wtc wrote:
> This part (lines 608-610) should be identical to lines
> 627-631 in OnReadComplete, because both are notifying the
> caller of an asynchronous Read completion.

Done.

http://codereview.chromium.org/225005/diff/17004/15008#newcode623
Line 623: if (result >= 0)
No, we don't. fixed.

On 2009/10/09 19:23:47, wtc wrote:
> Do we still need to decrypt the payload if result is 0?

http://codereview.chromium.org/225005/diff/17004/15008#newcode955
Line 955: if (rv < 0)
On 2009/10/09 19:23:47, wtc wrote:
> Do we still need to decrypt the payload if rv is 0?

Done.

http://codereview.chromium.org/225005/diff/17004/15008#newcode963
Line 963: user_read_buf_ = NULL;
On 2009/10/09 19:23:47, wtc wrote:
> I would just do this at the end of Read() and OnReadCompelte().

Done.

http://codereview.chromium.org/225005/diff/17004/15008#newcode976
Line 976: if (result == ERR_IO_PENDING)
Moved the check to the caller.

On 2009/10/09 19:23:47, wtc wrote:
> This check violates an invariant of the original code:
> we never pass ERR_IO_PENDING to a DoFooComplete() function,
> because by definition the STATE_FOO_COMPLETE state means
> the FOO operation has completed, so the result cannot be
> ERR_IO_PENDING.
> 
> So we need to fix the code at line 954 to call
> DoPayloadReadComplete() only if (rv != ERR_IO_PENDING).
> See how you do that in DoPayloadWrite() at line 1190.

http://codereview.chromium.org/225005/diff/17004/15008#newcode989
Line 989: return ERR_SSL_PROTOCOL_ERROR;
On 2009/10/09 19:23:47, wtc wrote:
> Don't we also need to reset ransport_read_buf_ before
> returning ERR_SSL_PROTOCOL_ERROR?
> 
> You may want to reset transport_read_buf_ first.

Done.

http://codereview.chromium.org/225005/diff/17004/15008#newcode1106
Line 1106: OnHandshakeIOComplete(OK);
On 2009/10/09 19:23:47, wtc wrote:
> I suggest that we just do
>   return DoLoop(OK);
> here.

Done.

http://codereview.chromium.org/225005/diff/17004/15008#newcode1192
Line 1192: user_write_buf_ = NULL;
On 2009/10/09 19:23:47, wtc wrote:
> I would just do this at the end of Write() and OnWriteCompelte().

Done.

http://codereview.chromium.org/225005/diff/17004/15007
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/225005/diff/17004/15007#newcode70
Line 70: int DoVerifyCertComplete(int result);
On 2009/10/09 19:23:47, wtc wrote:
> Nit: let's add a blank line here to suggest that the following
> DoXXX functions are no longer part of the state machine.

Done.

http://codereview.chromium.org/225005/diff/17004/15007#newcode71
Line 71: int DoPayloadDecrypt();
Ok - I was trying to match the ordering of the read functions.  I don't really
care - but the 'style' is not explicit.

On 2009/10/09 19:23:47, wtc wrote:
> Nit: list DoPayloadDecrypt after DoPayloadReadComplete,
> which matches the order in which these functions are called.